fix(pdf): fail closed when the outline / AcroForm walks hit their node cap

README.md

@@ -100,7 +100,7 @@ python3.12 -m venv .venv && source .venv/bin/activate
 # Install dependencies (Lambda + local-service + test-only deps)
 pip install -r src/requirements.txt -r src/requirements-local.txt -r src/requirements-dev.txt
 
-# Run the full test suite (450 tests: 400 CDR Lambda + 50 local variant)
+# Run the full test suite (454 tests: 404 CDR Lambda + 50 local variant)
 cd src && pytest test_cdr.py test_cdr_local.py -v
 
 # Run one class or test

docs/claude/checklist-and-invariants.md

@@ -57,7 +57,7 @@ Run this checklist before considering any change complete. Each item maps to a c
 
 ### Test invariants — verify coverage was not regressed
 
-- [ ] **450 tests pass** (all passed) — `cd src && pytest test_cdr.py test_cdr_local.py -v` shows no failures.
+- [ ] **454 tests pass** (all passed) — `cd src && pytest test_cdr.py test_cdr_local.py -v` shows no failures.
 - [ ] **No routing/disarm logic duplicated into a front-end** — `handler` and `app.py` both delegate to `cdr_dispatch`; a security decision must live in exactly one place (pitfall #41).
 - [ ] **Every new CDR path has a test** — if you add a new strip rule, add a fixture that carries that threat and assert it is removed.
 - [ ] **Every new try/except warn-and-continue block has a failure-path test** — prove the success path completes even when the wrapped operation throws.

docs/claude/pitfalls.md

@@ -94,6 +94,7 @@ it is listed once, under the group whose code you would be editing.
 - [#58 — A MIME parameter defeats a `+xml` suffix test — and the OPC container-layer sweep that found nothing else](#58-a-mime-parameter-defeats-a-xml-suffix-test-and-the-opc-container-layer-sweep-that-found-nothing-else)
 - [#59 — A resource cap that `return`s hands the attacker the sweep's coverage — bound the work, but fail closed](#59-a-resource-cap-that-returns-hands-the-attacker-the-sweeps-coverage-bound-the-work-but-fail-closed)
 - [#60 — Auditing every cap's failure direction, and making "fail closed" a CI guard instead of a habit](#60-auditing-every-caps-failure-direction-and-making-fail-closed-a-ci-guard-instead-of-a-habit)
+- [#61 — The #60 audit enumerated caps it already knew about — `_MAX_WALK_NODES` was never on the list, and truncated in two sweeps](#61-the-60-audit-enumerated-caps-it-already-knew-about-_max_walk_nodes-was-never-on-the-list-and-truncated-in-two-sweeps)
 
 **Multi-bug audit batches**
 
@@ -386,6 +387,7 @@ The direct follow-up to #59: if one resource cap silently truncated, the others
 | Cap | On exceeding | Correct? |
 |---|---|---|
 | `_PDF_WALK_MAX_NODES` | `CdrReject` | fixed in #59 |
+| `_MAX_WALK_NODES` (outlines, AcroForm) | `continue` / `break` → truncate | **NO — missed by this audit, fixed in #61** |
 | `_EXTERNAL_REF_SCAN_MAX_NODES` | `return True` (= "is external", so the subtree is removed) | yes — closed by design |
 | `_DecompressionBudget` / `_MAX_TOTAL_ENTRY_BYTES` | `CdrReject` | yes |
 | `_MAX_ZIP_ENTRIES` | `_validate_zip_structure` hard-fail verdict | yes |
@@ -400,3 +402,12 @@ That last row is why this guard is mutation-based rather than a grep for `return
 **Two things the guard needed in order not to become the very defect it checks for.** (a) **A baseline green run before any mutant means anything** — without it a broken checkout reports every mutant "caught" and the guard passes vacuously (#57). This fired immediately and usefully: the first version exported `SANITISED_BUCKET`/`QUARANTINE_BUCKET`, which the test module defaults with `os.environ.setdefault` — that *yields* to the environment, so five tests asserting the literal `test-sanitised`/`test-quarantine` names failed. The documented "run pytest BARE" rule, rediscovered by writing a tool that broke it. (b) **A negative control on the guard itself**: a deliberately unpinned `raise CdrReject` was injected and the guard correctly named it by line. A checker never shown failing is an assumption (#57 again).
 
 **General rule: when a review finds a bug class rather than a bug, the deliverable is the executable check, not the fixed instance.** This repo already had that instinct for docs (`check_test_count.py`, `check_cap_defaults.py`, `check_pitfalls_index.py`, `check_iac_parity.py`); security invariants deserve it more, because a doc drifting is embarrassing and a fail-open shipping is not. Related: #59 (the bug), #57 (instrument discipline), #53 (docstring vs behaviour).
+
+### 61. The #60 audit enumerated caps it already knew about — `_MAX_WALK_NODES` was never on the list, and truncated in two sweeps
+An external review (Codex, 2026-08-17) found the exact bug class #59 documents and #60 claimed to have swept clean, in a constant the #60 table never mentions.
+
+`_strip_pdf_outlines` ended its walk with `continue` and `_strip_acroform_fields` with `break` on reaching `_MAX_WALK_NODES` (100,000). Both then returned normally and `cdr_pdf` shipped the file as sanitised. Reproduced end to end: a **12.2 MiB** PDF with a 100,002-node outline chain and a **10.6 MiB** PDF with a 100,002-node `/Kids` chain each came back "sanitised" carrying live actions — well under the 100 MB `_MAX_FILE_BYTES`, so no size guard is in the way. Fixed to `raise CdrReject`, matching `_walk_pdf_nodes`. Pinned by `test_outline_walk_cap_rejects_rather_than_truncating` / `test_acroform_walk_cap_rejects_rather_than_truncating` (exception) and the two `_payload_never_ships` tests (outcome), so any silent-completion replacement still fails.
+
+**Why #60 missed it, and the lesson that is not "audit harder".** #60's table is a *hand-written enumeration* — ten constants someone remembered. `_MAX_WALK_NODES` is a *shared* cap used by two functions and named nothing like `_PDF_WALK_MAX_NODES`, so it fell out of recall. `check_fail_closed.py` could not catch it either: that guard mutates rejections that **exist** and asks whether a test notices; it is structurally blind to a cap that never raises in the first place. **A guard that verifies existing invariants cannot find a missing one.** The complement to `check_fail_closed.py` is enumeration from the source, not from memory: `grep -n '_MAX_[A-Z_]*\|_BUDGET' src/lambda_function.py` yields the real list, and every hit must be traced to an explicit failure direction before the audit counts as complete.
+
+**General rule: when a review concludes "no second instance", state how the instances were enumerated.** "I checked the caps" and "I listed every `_MAX_`/budget constant in the module and traced each" are different claims with the same wording, and only the second one is falsifiable. Related: #59 (the bug class), #60 (the audit that missed this), #57 (a check that cannot fail is not a check).

docs/claude/testing.md

@@ -21,7 +21,7 @@
 > produce, since it never attempts the fetch. The manual pass therefore stands at **14 of 14 with
 > no engine unverified**. See `docs/viewer-validation/CHECKLIST.md`.
 
-Tests (400 in `test_cdr.py` + 50 in `test_cdr_local.py` = **450 total**) construct malicious fixtures entirely in-memory — no fixture files on disk. S3/SNS calls are patched with `unittest.mock`. Required env vars are set automatically via `os.environ.setdefault`. `src/test_cdr.py` covers the CDR Lambda; `src/test_cdr_local.py` covers the pure `cdr_dispatch` core and the local FastAPI service (`app.py`), reusing the same in-memory fixtures. Run: `cd src && pytest test_cdr.py test_cdr_local.py -v`.
+Tests (404 in `test_cdr.py` + 50 in `test_cdr_local.py` = **454 total**) construct malicious fixtures entirely in-memory — no fixture files on disk. S3/SNS calls are patched with `unittest.mock`. Required env vars are set automatically via `os.environ.setdefault`. `src/test_cdr.py` covers the CDR Lambda; `src/test_cdr_local.py` covers the pure `cdr_dispatch` core and the local FastAPI service (`app.py`), reusing the same in-memory fixtures. Run: `cd src && pytest test_cdr.py test_cdr_local.py -v`.
 
 > **Run it bare — do not export AWS env vars around it.** `src/conftest.py` sets the
 > credentials/region and `test_cdr.py` sets the bucket and topic names, both via

docs/deployment-runbook.md

@@ -242,12 +242,12 @@ aws events list-targets-by-rule \
   --query 'Targets[].Arn'
 ```
 
-**OpenTofu / Terraform** — the EventBridge rule is named `cdr-s3-object-created`:
+**OpenTofu / Terraform** — the EventBridge rule is named `${resource_prefix}-s3-object-created`, i.e. `$PREFIX-s3-object-created` (it is *not* the fixed string `cdr-s3-object-created` unless you deployed with the default prefix):
 ```bash
 ( cd terraform && tofu state list )          # resource inventory
 
 aws events list-targets-by-rule \
-  --rule cdr-s3-object-created \
+  --rule $PREFIX-s3-object-created \
   --query 'Targets[].Arn'
 ```
 
@@ -356,25 +356,33 @@ aws cloudwatch get-metric-statistics \
   --extended-statistics p50 p99 \
   --query 'Datapoints[0].{p50:ExtendedStatistics.p50,p99:ExtendedStatistics.p99}'
 
-# Max memory used
-aws cloudwatch get-metric-statistics \
-  --namespace AWS/Lambda \
-  --metric-name MaxMemoryUsed \
-  --dimensions Name=FunctionName,Value=$PREFIX-lambda \
-  --start-time $START \
-  --end-time   $END \
-  --period 600 \
-  --statistics Maximum \
-  --query 'Datapoints[0].Maximum'
+# Max memory used — NOT a CloudWatch metric. `MaxMemoryUsed` does not exist in the
+# AWS/Lambda namespace; querying it returns an empty Datapoints list, which reads as
+# "no memory pressure" when it actually means "no such metric". The number lives only
+# in the Lambda REPORT log line, so pull it from Logs Insights (this is what
+# docs/benchmark.py does):
+START_EPOCH=$(python3 -c "from datetime import datetime,timezone,timedelta; print(int((datetime.now(timezone.utc)-timedelta(minutes=10)).timestamp()))")
+END_EPOCH=$(python3   -c "from datetime import datetime,timezone; print(int(datetime.now(timezone.utc).timestamp()))")
+QID=$(aws logs start-query \
+  --log-group-name /aws/lambda/$PREFIX-lambda \
+  --start-time $START_EPOCH \
+  --end-time   $END_EPOCH \
+  --query-string 'filter @message like /REPORT/ | parse @message "Max Memory Used: * MB" as mem | stats max(mem) as max_mem' \
+  --query queryId --output text)
+sleep 5
+aws logs get-query-results --query-id "$QID" --query 'results[0][0].value'
 ```
 
+Or skip the manual queries entirely and run `python docs/benchmark.py --bucket <source-bucket>`,
+which reports p50/p99 duration, throttles and max memory in one pass.
+
 ### Tuning thresholds
 
 | Metric | Current setting | Action if exceeded |
 |---|---|---|
 | p99 Duration > 250 s | Timeout = 300 s | Increase `Timeout` in `template.yaml`; re-deploy |
 | p99 Duration > 200 s on PDFs | 1024 MB memory | Increase `MemorySize` (Lambda CPU scales with RAM) |
-| MaxMemoryUsed > 900 MB | 1024 MB memory | Increase `MemorySize` to 2048 MB |
+| Max Memory Used > 900 MB (REPORT log line) | 1024 MB memory | Increase `MemorySize` to 2048 MB |
 | Throttles > 0 | `ReservedConcurrentExecutions: 20` | Increase reservation if throughput SLA demands it |
 | DLQ depth > 0 | — | Inspect DLQ messages: `aws sqs receive-message --queue-url <DLQ_URL>` |
 
@@ -499,7 +507,7 @@ RULE_NAME=$(aws cloudformation describe-stack-resource \
 aws events describe-rule --name "$RULE_NAME"             --query 'State'
 
 # OpenTofu / Terraform:
-aws events describe-rule --name cdr-s3-object-created    --query 'State'
+aws events describe-rule --name $PREFIX-s3-object-created --query 'State'
 ```
 If disabled, enable it with `aws events enable-rule --name <rule>`.
 

src/lambda_function.py

@@ -1930,7 +1930,17 @@ def _strip_pdf_outlines(catalog) -> list[str]:
 
     while stack:
         node = stack.pop()
-        if node is None or visited_count >= _MAX_WALK_NODES:
+        if visited_count >= _MAX_WALK_NODES:
+            # FAIL CLOSED (pitfall #59/#60): `continue`-ing here drains the stack without
+            # examining it, so outline items beyond the cap keep their /A /AA actions and
+            # cdr_pdf still returns a "sanitised" file. Node count is attacker-controlled
+            # — a 100_002-node outline tree fits well under _MAX_FILE_BYTES.
+            logger.warning("PDF outline walk hit the %d-node cap; rejecting",
+                           _MAX_WALK_NODES)
+            raise CdrReject(
+                f"PDF outline tree exceeds the {_MAX_WALK_NODES}-node walk cap; "
+                "cannot prove all outline actions were stripped")
+        if node is None:
             continue
         try:
             ident = node.objgen
@@ -1974,7 +1984,14 @@ def _strip_acroform_fields(fields) -> list[str]:
     while stack:
         field = stack.pop()
         if visited_count >= _MAX_WALK_NODES:
-            break
+            # FAIL CLOSED (pitfall #59/#60): `break`ing here abandons the rest of the
+            # field tree, leaving deeper /JS /A /AA live in a file that is then reported
+            # sanitised. See the identical bound in _walk_pdf_nodes.
+            logger.warning("PDF AcroForm walk hit the %d-node cap; rejecting",
+                           _MAX_WALK_NODES)
+            raise CdrReject(
+                f"PDF AcroForm field tree exceeds the {_MAX_WALK_NODES}-node walk cap; "
+                "cannot prove all field actions were stripped")
         try:
             try:
                 ident = field.objgen

src/test_cdr.py

@@ -1801,6 +1801,45 @@ def test_acroform_deep_noncyclic_kids_fully_swept(self):
         assert len(removed) == depth, \
             f"only {len(removed)}/{depth} fields swept — deep chain silently truncated"
 
+    def _kids_chain_pdf(self, depth):
+        pdf = pikepdf.Pdf.new()
+        pdf.add_blank_page()
+        prev = None
+        for i in range(depth):
+            field = pdf.make_indirect(pikepdf.Dictionary(
+                T=pikepdf.String(f"f{i}"),
+                FT=pikepdf.Name("/Tx"),
+                JS=pikepdf.String("app.alert('CAPTAIL');"),
+            ))
+            if prev is not None:
+                field["/Kids"] = pikepdf.Array([prev])
+            prev = field
+        pdf.Root["/AcroForm"] = pikepdf.Dictionary(Fields=pikepdf.Array([prev]))
+        buf = io.BytesIO()
+        pdf.save(buf, compress_streams=False)
+        return buf.getvalue()
+
+    # ── Audit fix: the field-tree cap `break`ed, abandoning the rest of the /Kids chain
+    #    while cdr_pdf still reported the file sanitised. Same failure direction as the
+    #    outline cap and _walk_pdf_nodes — bound the work, but fail closed (pitfall #59) ──
+    def test_acroform_walk_cap_rejects_rather_than_truncating(self, monkeypatch):
+        monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10)
+        raw = self._kids_chain_pdf(40)
+        assert b"CAPTAIL" in raw  # precondition: payload really is in the input
+        with pytest.raises(cdr.CdrReject, match="walk cap"):
+            cdr.cdr_pdf(raw)
+
+    def test_acroform_walk_cap_payload_never_ships(self, monkeypatch):
+        """Pins the outcome, not the exception."""
+        monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10)
+        raw = self._kids_chain_pdf(40)
+        try:
+            clean, _ = cdr.cdr_pdf(raw)
+        except cdr.CdrReject:
+            return
+        assert b"CAPTAIL" not in clean, \
+            "over-cap AcroForm field actions survived into the sanitised output"
+
 
 class TestPdfNamesEmbeddedFiles:
     """PDF /Names./EmbeddedFiles is removed."""
@@ -3064,6 +3103,51 @@ def test_pdf_deep_noncyclic_outline_chain_fully_swept(self):
         assert len(removed) == depth, \
             f"only {len(removed)}/{depth} outline nodes swept — deep chain silently truncated"
 
+    def _outline_chain_pdf(self, depth):
+        pdf = pikepdf.Pdf.new()
+        pdf.add_blank_page()
+        first = None
+        prev = None
+        for i in range(depth):
+            item = pdf.make_indirect(pikepdf.Dictionary(
+                Title=pikepdf.String(f"item{i}"),
+                A=pikepdf.Dictionary(S=pikepdf.Name("/JavaScript"),
+                                     JS=pikepdf.String("app.alert('CAPTAIL')")),
+            ))
+            if prev is not None:
+                prev["/Next"] = item
+            else:
+                first = item
+            prev = item
+        pdf.Root["/Outlines"] = pikepdf.Dictionary(
+            Type=pikepdf.Name("/Outlines"), First=first, Last=prev, Count=depth)
+        buf = io.BytesIO()
+        pdf.save(buf, compress_streams=False)
+        return buf.getvalue()
+
+    # ── Audit fix: the outline cap `continue`d, draining the stack without examining it,
+    #    so outline items past the cap kept their /A /AA and cdr_pdf still returned a
+    #    "sanitised" file. Node count is attacker-controlled (a 100_002-node outline tree
+    #    is ~12 MB, far under _MAX_FILE_BYTES) — the cap must fail closed (pitfall #59) ──
+    def test_outline_walk_cap_rejects_rather_than_truncating(self, monkeypatch):
+        monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10)
+        raw = self._outline_chain_pdf(40)
+        assert b"CAPTAIL" in raw  # precondition: payload really is in the input
+        with pytest.raises(cdr.CdrReject, match="walk cap"):
+            cdr.cdr_pdf(raw)
+
+    def test_outline_walk_cap_payload_never_ships(self, monkeypatch):
+        """Pins the outcome, not the exception: an over-cap outline tree must never reach
+        the sanitised bucket with live actions, whatever path replaces CdrReject."""
+        monkeypatch.setattr(cdr, "_MAX_WALK_NODES", 10)
+        raw = self._outline_chain_pdf(40)
+        try:
+            clean, _ = cdr.cdr_pdf(raw)
+        except cdr.CdrReject:
+            return
+        assert b"CAPTAIL" not in clean, \
+            "over-cap outline actions survived into the sanitised output"
+
     def test_pdf_goToE_annotation_action_stripped(self):
         """/GoToE (re-reaches embedded files) was not in the old denylist; the new
         unconditional /A deletion catches it (M4)."""