PR-S4: shadow-mode hardening (review follow-ups) (#6523)

## What this PR does

Bundles the **low-risk polish items** from the [multi-agent review of
#6519](https://github.com/Stirling-Tools/Stirling-PDF/pull/6519). Each
change is independent, mechanical, and ships with focused unit-test
coverage.

The medium-severity items (\`?async=true\` OUTPUT recording,
JSON-consumes endpoint coverage, SpringBootTest harness) are tracked
separately in [\`notes/PAYG_DESIGN.md\` §7.5
PR-S4](https://github.com/Stirling-Tools/Stirling-PDF/blob/payg-s4-hardening/notes/PAYG_DESIGN.md)
— they need design decisions + bigger infrastructure work, so this PR
sticks to the mechanical wins.

Stacked on #6519. When that merges to main, this rebases cleanly — no
code changes.

## Changes

| Area | What | Why |
|---|---|---|
| **\`tool_id\` becomes route pattern** |
\`PaygChargeInterceptor.resolveToolId()\` prefers
\`HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE\` over
\`request.getRequestURI()\`. Truncates to 128 + WARN log +
\`payg.filter.errors\` increment when truncation fires. | Audit rollups
aggregate by endpoint instead of by every individual request's
path-variable / matrix-param variant. Silent truncation now louder. |
| **Direct PDF magic-byte check** | \`PaygOutputExtractor.extract()\`
magic-checks the body even for direct \`application/pdf\` responses. |
Asymmetric with the ZIP-entry path which always magic-checks. A tool
that emits \`application/pdf\` for a JSON / HTML payload would otherwise
pollute \`job_artifact_hash\`. |
| **DESKTOP_APP detection** | \`X-Stirling-Client: desktop\` header →
\`JobSource.DESKTOP_APP\`. | The enum value was unreachable from
\`determineSource()\`; Tauri shell traffic was mis-classified as WEB. No
anti-spoof — V12 step limits are identical for WEB/DESKTOP_APP so the
worst-case abuse value is zero today. |
| **\`max-bytes\` sensible default** | 500 MiB instead of \`null\`
(unbounded). | Covers the largest realistic Stirling responses (full
split-to-ZIP on a 1000-page document) while preventing pathological
cases from tying up the interceptor for minutes. Set to \`null\` to
disable. |
| **\`BufferedOutputStream\` for spill** | Wraps the spill
\`OutputStream\` in 64 KiB \`BufferedOutputStream\`. | Previously every
Tomcat chunk (default 8 KiB) was a separate syscall. Big spilled
responses get a syscall-bound speedup. |
| **Duration timer per phase** | \`payg.filter.duration\` tagged
\`phase=preHandle\` vs \`phase=afterCompletion\`. | Two distinct latency
distributions were blended into one histogram; hard to alert on. |

## Tests

| Test | What it covers |
|---|---|
|
\`PaygOutputExtractorTest.pdfContentType_butBodyMissingPdfMagic_returnsEmpty\`
| New direct-PDF magic-byte gate. |
|
\`PaygChargeInterceptorTest.preHandle_desktopClientHeader_setsJobSourceDesktopApp\`
| New \`X-Stirling-Client: desktop\` → DESKTOP_APP path. |
|
\`PaygChargeInterceptorTest.preHandle_toolId_prefersBestMatchingPattern\`
| Route pattern wins over URI when both are set. |
|
\`PaygChargeInterceptorTest.preHandle_toolId_truncatesAndCountsWhenLongerThan128\`
| Oversized values truncate + increment errors counter. |

Full saas suite green (210 tests), coverage targets met.

## What's NOT in this PR (deliberately)

- **\`?async=true\` OUTPUT recording.** The JobExecutorService returns a
synchronous \`JobResponse{jobId}\` body before the async tool actually
runs; \`afterCompletion\` fires too early. Needs a design decision:
short-circuit PAYG when \`async=true\` OR hook into \`TaskManager\`
completion. Tracked in PR-S4 design doc.
- **JSON-consumes endpoint coverage.** The
\`MultipartHttpServletRequest\` cast skips endpoints with \`consumes =
APPLICATION_JSON_VALUE\` (e.g.
\`ConvertPdfJsonController.exportPartialPdf\`). Fix is either extract a
request-body hash for JSON or add a CI lint forbidding non-multipart
\`@AutoJobPostMapping\`. Design discussion needed.
- **SpringBootTest harness for filter + interceptor wiring.** Saas
module doesn't have one yet. Separate work — PR-S3 takes a different
approach (docker-compose + Behave); a SpringBootTest layer would be
additive in-process coverage.

These are tracked in \`notes/PAYG_DESIGN.md §7.5\` so they don't slip.

## Tracked in

\`notes/PAYG_DESIGN.md\` §7.5 PR-S4.

app/saas/src/main/java/stirling/software/saas/payg/filter/PaygChargeInterceptor.java

@@ -20,6 +20,7 @@ import org.springframework.web.method.HandlerMethod;
 import org.springframework.web.multipart.MultipartFile;
 import org.springframework.web.multipart.MultipartHttpServletRequest;
 import org.springframework.web.servlet.AsyncHandlerInterceptor;
+import org.springframework.web.servlet.HandlerMapping;
 
 import io.micrometer.core.instrument.Counter;
 import io.micrometer.core.instrument.MeterRegistry;
@@ -82,6 +83,17 @@ public class PaygChargeInterceptor implements AsyncHandlerInterceptor {
 
     private static final String AUTOMATION_HEADER = "X-Stirling-Automation";
 
+    /**
+     * Optional header the Tauri desktop shell sets so saas-side traffic from the embedded client
+     * can be classified as {@link JobSource#DESKTOP_APP} instead of {@code WEB}. No anti-spoof —
+     * V12 step limits for DESKTOP_APP and WEB are identical, so the worst-case abuse value is zero
+     * today. Tighten if/when their limits diverge.
+     */
+    private static final String DESKTOP_CLIENT_HEADER = "X-Stirling-Client";
+
+    /** Matches {@code processing_job_step.tool_id} column width (VARCHAR(128)). */
+    private static final int TOOL_ID_MAX_LENGTH = 128;
+
     private final JobChargeService chargeService;
     private final JobService jobService;
     private final UserRepository userRepository;
@@ -94,7 +106,12 @@ public class PaygChargeInterceptor implements AsyncHandlerInterceptor {
     private final Counter callsJoined;
     private final Counter callsShortCircuit;
     private final Counter refundsCounter;
-    private final Timer durationTimer;
+
+    /** preHandle wall-clock per request. Separate from afterCompletion — different populations. */
+    private final Timer preHandleTimer;
+
+    /** afterCompletion wall-clock per request. Includes response hashing + step append + refund. */
+    private final Timer afterCompletionTimer;
 
     public PaygChargeInterceptor(
             JobChargeService chargeService,
@@ -131,9 +148,15 @@ public class PaygChargeInterceptor implements AsyncHandlerInterceptor {
                 Counter.builder("payg.filter.refunds")
                         .description("First-step 5xx refunds applied to shadow rows")
                         .register(meterRegistry);
-        this.durationTimer =
+        this.preHandleTimer =
                 Timer.builder("payg.filter.duration")
-                        .description("preHandle + afterCompletion wall-clock per request")
+                        .tag("phase", "preHandle")
+                        .description("preHandle wall-clock per request")
+                        .register(meterRegistry);
+        this.afterCompletionTimer =
+                Timer.builder("payg.filter.duration")
+                        .tag("phase", "afterCompletion")
+                        .description("afterCompletion wall-clock per request")
                         .register(meterRegistry);
     }
 
@@ -160,7 +183,7 @@ public class PaygChargeInterceptor implements AsyncHandlerInterceptor {
             }
             return true;
         } finally {
-            sample.stop(durationTimer);
+            sample.stop(preHandleTimer);
         }
     }
 
@@ -218,7 +241,7 @@ public class PaygChargeInterceptor implements AsyncHandlerInterceptor {
         // async-dispatch), and (b) hard-fails any future caller that tries to mutate it.
         request.setAttribute(ATTR_INPUT_TEMP_FILES, Collections.unmodifiableList(tempFiles));
         request.setAttribute(ATTR_INPUT_BYTES, totalInputBytes);
-        request.setAttribute(ATTR_TOOL_ID, request.getRequestURI());
+        request.setAttribute(ATTR_TOOL_ID, resolveToolId(request));
 
         ChargeContext ctx =
                 new ChargeContext(
@@ -274,7 +297,7 @@ public class PaygChargeInterceptor implements AsyncHandlerInterceptor {
                 closeWrapper(request);
             }
         } finally {
-            sample.stop(durationTimer);
+            sample.stop(afterCompletionTimer);
         }
     }
 
@@ -412,9 +435,42 @@ public class PaygChargeInterceptor implements AsyncHandlerInterceptor {
         if (automationHeader != null && "true".equalsIgnoreCase(automationHeader.trim())) {
             return JobSource.PIPELINE;
         }
+        String desktopHeader = request.getHeader(DESKTOP_CLIENT_HEADER);
+        if (desktopHeader != null && "desktop".equalsIgnoreCase(desktopHeader.trim())) {
+            return JobSource.DESKTOP_APP;
+        }
         if (auth instanceof ApiKeyAuthenticationToken) {
             return JobSource.API;
         }
         return JobSource.WEB;
     }
+
+    /**
+     * Resolves the {@code tool_id} value stored on {@code processing_job_step}. Prefers the route
+     * pattern (e.g. {@code /api/v1/security/add-password}) over the raw URI so audit rollups
+     * aggregate by endpoint rather than by request — path variables, query strings, and matrix
+     * params don't pollute the column. Falls back to the raw URI when the pattern isn't available
+     * (non-Spring-MVC dispatches, async re-dispatch edges).
+     *
+     * <p>Truncates to {@link #TOOL_ID_MAX_LENGTH} to match the column's {@code VARCHAR(128)} width.
+     * Logs at WARN + increments {@link #errorsCounter} when truncation actually happens so support
+     * notices the {@code tool_id} they expected isn't what we stored.
+     */
+    private String resolveToolId(HttpServletRequest request) {
+        Object pattern = request.getAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE);
+        String value = pattern instanceof String s ? s : request.getRequestURI();
+        if (value == null) {
+            return "unknown";
+        }
+        if (value.length() <= TOOL_ID_MAX_LENGTH) {
+            return value;
+        }
+        log.warn(
+                "tool_id length {} exceeds column max {}; truncating. value='{}'",
+                value.length(),
+                TOOL_ID_MAX_LENGTH,
+                value);
+        errorsCounter.increment();
+        return value.substring(0, TOOL_ID_MAX_LENGTH);
+    }
 }

app/saas/src/main/java/stirling/software/saas/payg/filter/PaygFilterProperties.java

@@ -38,7 +38,13 @@ public class PaygFilterProperties {
         /** 10 MiB. Tiny responses stay in RAM; large responses spill. */
         private long inMemoryThresholdBytes = 10L * 1024L * 1024L;
 
-        /** Optional ceiling: when set, OUTPUT recording is skipped past this size. */
+        /**
-        private Long maxBytes;
+         * Ceiling for OUTPUT recording. Responses larger than this skip the per-PDF hash + ZIP
+         * unpack — the bytes still flowed through to the client unmodified, only lineage capture is
+         * dropped. Default 500 MiB is generous for the largest realistic Stirling responses (full
+         * split-to-ZIP on a 1000-page document) while preventing pathological cases from tying up
+         * the interceptor for minutes. Set to {@code null} for "no ceiling at all".
+         */
+        private Long maxBytes = 500L * 1024L * 1024L;
     }
 }

app/saas/src/main/java/stirling/software/saas/payg/filter/PaygOutputExtractor.java

@@ -79,35 +79,79 @@ public class PaygOutputExtractor {
         }
         String mediaType = stripParameters(contentType);
 
-        // Stirling-PDF tool endpoints sometimes set Content-Type to
+        if (PDF_CONTENT_TYPE.equalsIgnoreCase(mediaType)) {
-        // application/octet-stream (or no header at all) even when the body
+            // Wrapper-owned path. Don't claim ownership. Magic-byte check protects against tools
-        // is a real PDF or ZIP — Spring's default StreamingResponseBody path
+            // that emit application/pdf for non-PDF payloads (mirrors the ZIP-entry path below).
-        // doesn't always negotiate content type. When the declared
+            if (!isPdfMagic(bodyPath)) {
-        // Content-Type is missing or generic, sniff magic bytes to recover
+                log.debug(
-        // lineage capture.
+                        "Response advertised application/pdf but content does not start with"
-        boolean missingOrGeneric =
+                                + " %PDF- magic bytes; skipping OUTPUT recording. body={}",
-                mediaType == null || OCTET_STREAM_CONTENT_TYPE.equalsIgnoreCase(mediaType);
+                        bodyPath);
-        boolean isPdf = PDF_CONTENT_TYPE.equalsIgnoreCase(mediaType);
+                return List.of();
-        boolean isZip = ZIP_CONTENT_TYPE.equalsIgnoreCase(mediaType);
-
-        if (missingOrGeneric) {
-            if (isMagic(bodyPath, PDF_MAGIC)) {
-                isPdf = true;
-            } else if (isMagic(bodyPath, ZIP_MAGIC)) {
-                isZip = true;
             }
-        }
-
-        if (isPdf) {
-            // Wrapper-owned path. Don't claim ownership.
             return List.of(new ExtractedPdf(bodyPath, null));
         }
-        if (isZip) {
+        if (ZIP_CONTENT_TYPE.equalsIgnoreCase(mediaType)) {
             return extractZip(bodyPath);
         }
+        // Stirling-PDF tool endpoints sometimes set Content-Type to application/octet-stream (or
+        // no header at all) even when the body is a real PDF or ZIP — Spring's default
+        // StreamingResponseBody path doesn't always negotiate content type. When the declared
+        // Content-Type is missing or generic, sniff magic bytes in a single head-read so we don't
+        // open the body twice on the common negative path.
+        if (mediaType == null || OCTET_STREAM_CONTENT_TYPE.equalsIgnoreCase(mediaType)) {
+            BodyMagic magic = sniffMagic(bodyPath);
+            if (magic == BodyMagic.PDF) {
+                return List.of(new ExtractedPdf(bodyPath, null));
+            }
+            if (magic == BodyMagic.ZIP) {
+                return extractZip(bodyPath);
+            }
+        }
         return List.of();
     }
 
+    /** Discriminator returned by {@link #sniffMagic(Path)}. */
+    private enum BodyMagic {
+        PDF,
+        ZIP,
+        NEITHER
+    }
+
+    /**
+     * Single-pass magic-byte sniff used by the generic Content-Type branch. Opens {@code path}
+     * once, reads enough bytes to compare against both PDF and ZIP magics, returns the first match
+     * (or {@link BodyMagic#NEITHER} if neither matched / read failed). Replaces two consecutive
+     * {@link #isMagic} calls that would have opened the file twice on the common negative path.
+     */
+    private BodyMagic sniffMagic(Path path) {
+        int needed = Math.max(PDF_MAGIC.length, ZIP_MAGIC.length);
+        byte[] head = new byte[needed];
+        int read;
+        try (InputStream in = Files.newInputStream(path)) {
+            read = in.read(head);
+        } catch (IOException e) {
+            log.debug("Magic-byte sniff failed for {}", path, e);
+            return BodyMagic.NEITHER;
+        }
+        if (read >= PDF_MAGIC.length && startsWith(head, PDF_MAGIC)) {
+            return BodyMagic.PDF;
+        }
+        if (read >= ZIP_MAGIC.length && startsWith(head, ZIP_MAGIC)) {
+            return BodyMagic.ZIP;
+        }
+        return BodyMagic.NEITHER;
+    }
+
+    private static boolean startsWith(byte[] buf, byte[] prefix) {
+        for (int i = 0; i < prefix.length; i++) {
+            if (buf[i] != prefix[i]) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     private List<ExtractedPdf> extractZip(Path bodyPath) {
         List<ExtractedPdf> results = new ArrayList<>();
         try (InputStream rawIn = Files.newInputStream(bodyPath);

app/saas/src/main/java/stirling/software/saas/payg/filter/PaygResponseBodyWrapper.java

@@ -1,5 +1,6 @@
 package stirling.software.saas.payg.filter;
 
+import java.io.BufferedOutputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
@@ -274,9 +275,16 @@ public class PaygResponseBodyWrapper extends HttpServletResponseWrapper implemen
         bytesWritten += len;
     }
 
+    /** Spill stream buffer size — coalesces Tomcat's per-chunk syscalls into 64 KiB writes. */
+    private static final int SPILL_BUFFER_SIZE = 64 * 1024;
+
     private void spillToDisk() throws IOException {
         spillFile = tempFileManager.createManagedTempFile(".body");
-        spillStream = Files.newOutputStream(spillFile.getPath());
+        // Wrap in BufferedOutputStream — without this every Tomcat chunk (default 8 KiB) was a
+        // separate syscall to the temp file, which dominates wall-clock on big spilled responses.
+        spillStream =
+                new BufferedOutputStream(
+                        Files.newOutputStream(spillFile.getPath()), SPILL_BUFFER_SIZE);
         memoryBuffer.writeTo(spillStream);
         memoryBuffer = null; // help GC of potentially large buffer
         spilled = true;

app/saas/src/test/java/stirling/software/saas/payg/filter/PaygChargeInterceptorTest.java

@@ -315,6 +315,69 @@ class PaygChargeInterceptorTest {
         verify(jobService, never()).recordOutput(any(), any());
     }
 
+    @Test
+    void preHandle_desktopClientHeader_setsJobSourceDesktopApp() throws Exception {
+        authenticateWithUser(makeUser(7L, 42L));
+        UUID jobId = UUID.randomUUID();
+        when(chargeService.openProcess(any(), anyList()))
+                .thenReturn(new ChargeOutcome(jobId, 1, ChargeOutcome.Disposition.OPENED));
+        org.mockito.ArgumentCaptor<stirling.software.saas.payg.charge.ChargeContext> ctxCaptor =
+                org.mockito.ArgumentCaptor.forClass(
+                        stirling.software.saas.payg.charge.ChargeContext.class);
+
+        MockMultipartHttpServletRequest req = newMultipart();
+        req.addFile(new MockMultipartFile("file", "x.pdf", "application/pdf", "abc".getBytes()));
+        req.addHeader("X-Stirling-Client", "desktop");
+
+        interceptor.preHandle(req, new MockHttpServletResponse(), handlerMethodForFakeController());
+
+        verify(chargeService).openProcess(ctxCaptor.capture(), anyList());
+        assertThat(ctxCaptor.getValue().source())
+                .isEqualTo(stirling.software.saas.payg.model.JobSource.DESKTOP_APP);
+    }
+
+    @Test
+    void preHandle_toolId_prefersBestMatchingPattern() throws Exception {
+        authenticateWithUser(makeUser(7L, 42L));
+        UUID jobId = UUID.randomUUID();
+        when(chargeService.openProcess(any(), anyList()))
+                .thenReturn(new ChargeOutcome(jobId, 1, ChargeOutcome.Disposition.OPENED));
+
+        MockMultipartHttpServletRequest req = newMultipart();
+        // Raw URI contains a path variable; the matched pattern is what audit rollups want.
+        req.setRequestURI("/api/v1/security/add-password/extra/segment");
+        req.setAttribute(
+                org.springframework.web.servlet.HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE,
+                "/api/v1/security/add-password");
+        req.addFile(new MockMultipartFile("file", "x.pdf", "application/pdf", "abc".getBytes()));
+
+        interceptor.preHandle(req, new MockHttpServletResponse(), handlerMethodForFakeController());
+
+        assertThat(req.getAttribute(PaygChargeInterceptor.ATTR_TOOL_ID))
+                .isEqualTo("/api/v1/security/add-password");
+    }
+
+    @Test
+    void preHandle_toolId_truncatesAndCountsWhenLongerThan128() throws Exception {
+        authenticateWithUser(makeUser(7L, 42L));
+        UUID jobId = UUID.randomUUID();
+        when(chargeService.openProcess(any(), anyList()))
+                .thenReturn(new ChargeOutcome(jobId, 1, ChargeOutcome.Disposition.OPENED));
+
+        MockMultipartHttpServletRequest req = newMultipart();
+        String oversized = "/api/v1/" + "x".repeat(200);
+        req.setRequestURI(oversized);
+        // No matching pattern attribute — falls back to URI.
+        req.addFile(new MockMultipartFile("file", "x.pdf", "application/pdf", "abc".getBytes()));
+
+        interceptor.preHandle(req, new MockHttpServletResponse(), handlerMethodForFakeController());
+
+        Object stored = req.getAttribute(PaygChargeInterceptor.ATTR_TOOL_ID);
+        assertThat(stored).isInstanceOf(String.class);
+        assertThat(((String) stored)).hasSize(128);
+        assertThat(meterRegistry.counter("payg.filter.errors").count()).isEqualTo(1.0);
+    }
+
     @Test
     void preHandle_pipelineHeader_setsJobSourcePipeline() throws Exception {
         authenticateWithUser(makeUser(7L, 42L));

app/saas/src/test/java/stirling/software/saas/payg/filter/PaygOutputExtractorTest.java

@@ -43,6 +43,15 @@ class PaygOutputExtractorTest {
         assertThat(out).hasSize(1);
     }
 
+    @Test
+    void pdfContentType_butBodyMissingPdfMagic_returnsEmpty(@TempDir Path tmp) throws IOException {
+        // Tool sets application/pdf but writes a non-PDF payload — we should NOT record it as
+        // OUTPUT lineage. Mirrors the magic-byte gate already enforced on the ZIP branch.
+        Path body = tmp.resolve("misleading.pdf");
+        Files.write(body, "this is not actually a pdf".getBytes(StandardCharsets.UTF_8));
+        assertThat(extractor.extract("application/pdf", body)).isEmpty();
+    }
+
     @Test
     void zipContentType_extractsOnlyPdfEntriesWithValidMagicBytes(@TempDir Path tmp)
             throws IOException {

testing/compose/payg/saas-seed.sql

@@ -13,14 +13,19 @@
 --    no row has is_default = TRUE. Explicit timestamps because Hibernate's
 --    @CreationTimestamp is application-side; direct INSERTs bypass it.
 -- ---------------------------------------------------------------------------
+-- NOTE: free_tier_units_per_cycle is supplied explicitly because the cucumber
+-- harness disables Flyway (see docker-compose-saas.yml). Hibernate's DDL
+-- emits NOT NULL without a SQL DEFAULT (the JPA field default `= 0L` is
+-- JVM-side only), so the column would otherwise reject this INSERT.
+-- 500 matches the launch free-tier (PAYG_DESIGN §3.10 revised).
 INSERT INTO stirling_pdf.pricing_policy (
     version, effective_from, doc_pages_per_unit, doc_bytes_per_unit,
-    min_charge_units, file_unit_cap, is_default, notes, created_by,
+    min_charge_units, file_unit_cap, free_tier_units_per_cycle, is_default,
-    created_at
+    notes, created_by, created_at
 )
 SELECT
     'v1-cucumber', CURRENT_TIMESTAMP, 25, 5242880,
-    1, 1000, TRUE,
+    1, 1000, 500, TRUE,
     'Cucumber test default policy', 'system',
     CURRENT_TIMESTAMP
 WHERE NOT EXISTS (