From 84aca120555914d1e497474ad040e0b9182c2087 Mon Sep 17 00:00:00 2001 From: ConnorYoh <40631091+ConnorYoh@users.noreply.github.com> Date: Wed, 10 Jun 2026 10:08:43 +0100 Subject: [PATCH] PR-S4: shadow-mode hardening (review follow-ups) (#6523) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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. --- .../payg/filter/PaygChargeInterceptor.java | 68 +++++++++++++-- .../payg/filter/PaygFilterProperties.java | 10 ++- .../saas/payg/filter/PaygOutputExtractor.java | 86 ++++++++++++++----- .../payg/filter/PaygResponseBodyWrapper.java | 10 ++- .../filter/PaygChargeInterceptorTest.java | 63 ++++++++++++++ .../payg/filter/PaygOutputExtractorTest.java | 9 ++ testing/compose/payg/saas-seed.sql | 11 ++- 7 files changed, 224 insertions(+), 33 deletions(-) diff --git a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygChargeInterceptor.java b/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygChargeInterceptor.java index febcbeb78..66aaf5ca7 100644 --- a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygChargeInterceptor.java +++ b/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). + * + *
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);
+ }
}
diff --git a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygFilterProperties.java b/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygFilterProperties.java
index 0df2bf01b..89a24e388 100644
--- a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygFilterProperties.java
+++ b/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;
}
}
diff --git a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygOutputExtractor.java b/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygOutputExtractor.java
index 86b5cb602..e413d6c73 100644
--- a/app/saas/src/main/java/stirling/software/saas/payg/filter/PaygOutputExtractor.java
+++ b/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
- // 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 to recover
- // lineage capture.
- boolean missingOrGeneric =
- mediaType == null || OCTET_STREAM_CONTENT_TYPE.equalsIgnoreCase(mediaType);
- boolean isPdf = PDF_CONTENT_TYPE.equalsIgnoreCase(mediaType);
- 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 (PDF_CONTENT_TYPE.equalsIgnoreCase(mediaType)) {
+ // Wrapper-owned path. Don't claim ownership. Magic-byte check protects against tools
+ // that emit application/pdf for non-PDF payloads (mirrors the ZIP-entry path below).
+ if (!isPdfMagic(bodyPath)) {
+ log.debug(
+ "Response advertised application/pdf but content does not start with"
+ + " %PDF- magic bytes; skipping OUTPUT recording. body={}",
+ bodyPath);
+ return List.of();
}
- }
-
- 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