fix(rm-074): redact whole cookie headers, and stop distinct endpoints sharing a finding_id

[toderian]

Two defects found while reviewing #492 (develop → main). Both were verified by execution against develop at 5b5bd37b, both are fixed test-first, and every new test was confirmed red before its fix landed.

Tracked as RM-074.

1. Cookie: redaction stopped at the first ;, so session cookies reached the archive

scrub_graybox_secrets redacted cookie headers with r"(?i)\b(cookie)\s*:\s*[^,\r\n;]+". That character class terminates on ; — a Cookie header's internal pair delimiter, not a field separator. Only the first pair was redacted:

IN : Cookie: theme=dark; sessionid=s3cr3tSESSIONVALUE; csrftoken=AbCdEf123456
OUT: Cookie: <redacted>; sessionid=s3cr3tSESSIONVALUE; csrftoken=AbCdEf123456

The session cookie survives verbatim whenever it is not the first pair — the common case, since analytics and preference cookies are usually sent first. sessionid, PHPSESSID, JSESSIONID and connect.sid are in no generic name=value pattern, so an opaque session id had no second line of defence. JWT-shaped values were caught by the separate JWT rule; opaque ones were not.

scrub_graybox_secrets is the storage boundary: what it misses reaches the archive, the LLM input, the PDF and the exports.

The pattern is not new, but _artifact_from_response (added in #491) is — it snapshots raw request and response headers wholesale, where evidence was previously hand-authored key=value strings. That turns a latent weak pattern into a live path, against its own docstring's requirement that an artifact "must not become a new route for archiving what the scrubber removes elsewhere".

Fix. One function-backed rule for both cookie headers, consuming to end of line and redacting per ;-separated pair.

Cookie: theme=dark; sessionid=SECRET  ->  Cookie: theme=<redacted>; sessionid=<redacted>
Set-Cookie: sessionid=SECRET; Path=/; HttpOnly; Secure; SameSite=Lax
                              ->  Set-Cookie: sessionid=<redacted>; Path=/; HttpOnly; Secure; SameSite=Lax

Whole-line redaction would have been wrong: probes/misconfig.py:222-227 reports missing_Secure / missing_HttpOnly / weak_SameSite, so blanking a Set-Cookie would redact the evidence for the finding it just raised. The secret is the value; the flags are the finding. Cookie names are kept too — they are not secrets, and they are what makes the evidence readable.

Redaction stays idempotent, which is a requirement rather than a nicety: the scrubber runs at assembly, at emission and again at the storage boundary, and _ALREADY_REDACTED exists because a rule that consumed its own placeholder corrupted a curl reproduction once already. Asserted for both header forms.

The existing tests passed because both fixtures used single-pair headers.

2. Four probes never set url, so distinct endpoints shared one finding_id

models/finding_identity.dedup_key reads the location from affected_assets[].url — deliberately, and correctly. The problem is on the producing side: 92 of 93 GrayboxFinding(...) constructions omit url=, so affected_assets is [] and identity reduces to probe + scenario_id + classification.

Two findings from one probe, one scenario and one CWE therefore share an id:

PT-A03-01, two vulnerable forms, same CWE:
  A fa1993af37904128  /admin/users/search
  B fa1993af37904128  /admin/settings/search   COLLIDE

This is a regression from #491. The previous to_flat_finding folded the endpoint=, path=, protected_path=, token_path=, flow= and test_id= evidence prefixes into its id input directly. #491 replaced that with asset-based identity and shipped the migration shim — _location_from_evidence — but wired it only into emit_*:

Probes	Emissions	Path	Identity
api_abuse, api_access, api_auth, api_config, api_data	103	emit_* → _location_from_evidence	correct
access_control, injection, misconfig, business_logic	87	direct findings.append(...)	collides

The gap is documented in the code that has it — emit_vulnerable's docstring and the url field's own docstring both state that a probe omitting url makes two endpoints "collapse to one finding id".

Collisions need same probe + same scenario_id + same CWE in one run, which happens at five emit-per-item loops: access_control.py:204 (PT-A01-02, per privileged endpoint), injection.py:194 (PT-A03-01, per vulnerable form), injection.py:353/:375/:425 (PT-API7-01), and business_logic.py:190 (PT-A06-01). misconfig.py:120/:138 does not collide — it emits worst_finding only.

Impact. Nothing is lost from the report: report-layer dedup (mixins/report.py:132) is a whole-dict JSON key, so two findings differing in evidence both survive. But triage keys on finding_id alone (services/rulebook_assessment.py:421, get_job_triage(job_id, finding_id)), so marking one endpoint remediated marked the other remediated too. Triage is job-scoped, so there is no cross-target collision.

Fix. location_from_evidence moves from probes/base.py into graybox/findings.py and is applied in to_flat_finding when url/parameter are unset — repairing all 87 direct constructions at one site. The emission path is unchanged.

It reads the structured key=value prefixes only. Reading the evidence string back into dedup_key would have reintroduced the wording-fork RM-062 removed, where re-phrasing a probe re-identified every finding it had ever produced. The derived value is scrubbed before promotion, as emit_vulnerable already does, so a token in a query string does not become a typed field.

Migrating the four probes onto emit_* is the larger cleanup — 87 call sites, no behavioural need now — and is recorded against RM-072 Phase 2 rather than done here.

One-time identity migration

Graybox findings from the four direct-construction probes now carry an asset, so their finding_id changes once. Archived findings are unaffected: flat_from_dict honours the id stamped at production, and there is a test pinning it.

Checks

• pytest extensions/business/cybersec/red_mesh/tests/ → 2429 passed, 3 skipped, 275 subtests, in both randomised and -p no:randomly order.
• Baseline on develop is 2416 passed / 3 skipped / 275 subtests. The +13 is exactly the tests added here — 6 for cookie redaction, 7 for location derivation.
• 8 of the 13 fail without their fix; the other 5 are guard tests that already held (explicit url= wins, same endpoint still dedups, no location key still means no asset, stamped ids survive the read path).
• No test weakened, skipped or deleted.

Note that neither repo runs tests on pull requests — there is no pull_request trigger, and in edge-node a merge to develop publishes Docker images in the same step that would have run them. That gap is filed as RM-073.

[toderian]

Pushed 503f7272, correcting three defects a review pass found in the cookie rule from 6b93213a. All three were regressions against develop, and all three were reproduced before being fixed.

1. The rule truncated the curl replay step — the failure _ALREADY_REDACTED exists to prevent, one level down. _curl_reproduction scrubs each header before shlex.quoteing it, so the assembled line already carries <redacted>; it is then scrubbed at emission and again at the storage boundary. Reading to end of line, the last pair's value has the rest of the command glued to it, so an equality check against the placeholder did not fire:

ASSEMBLED : curl -i -H 'Cookie: theme=<redacted>; sessionid=<redacted>' -H 'Accept: */*' 'https://app.test/a?x=1'
WAS       : curl -i -H 'Cookie: theme=<redacted>; sessionid=<redacted>

The guard now matches a prefix, which is the same guarantee _ALREADY_REDACTED gives the patterns around it. The bare-header idempotency test passes against the broken version, so the regression test goes end-to-end through _curl_reproduction instead.

2. The attribute allowlist applied to the request direction, where path and secure are ordinary cookie names — Cookie: secure=SECRET; path=SECRET2 passed through untouched. It is now gated on Set-Cookie and on non-first position, since the first pair of a Set-Cookie is the cookie however it is named.

3. A segment with no = was passed through verbatim — Cookie: OPAQUETOKEN is redacted on develop and was not here. Such a segment is an opaque value and is now redacted whole. Relatedly the (?<![-\w]) lookbehind dropped X-Auth-Cookie:, which develop matched; leftmost matching plus alternation order already keeps the bare cookie branch off the tail of Set-Cookie, so \b is back at no cost.

Fuzzed 6000 generated header shapes across both directions, opaque segments and assembled curl lines: no secret survives and every result is a fixed point. Set-Cookie attribute values (Path=/admin) are kept by design so the cookie-hardening evidence stays readable — they still pass under the JWT, Bearer, named-secret and operator-configured patterns, and there is now a test pinning that boundary explicitly.

Suite: 2436 passed, 3 skipped, 278 subtests, both orders.

[toderian]

Pushed 92357a1b. I ran a four-agent adversarial review round on the branch diff (redaction / identity / blast radius / test quality). It found six more defects, and I reproduced every one before fixing it. Two were introduced by my own previous fix in this PR.

Redaction

1. An empty cookie value destroyed the curl replay step. Cookie: sid=<redacted>; csrftoken= is a fixed point alone, but not once embedded in a shlex.quoted curl argument — the trailing = glued to the rest of the command and ate the URL and the closing quote. http.cookiejar emits name= for any cleared cookie, so this was common. Same root cause as the truncation fixed in 503f7272; the prefix guard had only masked it for one shape. The match now stops at ', which fixes it structurally and lets the guard be an equality test again — which in turn closes:
2. sid=<redacted>SECRET123 walked through the prefix guard. The target picks its own cookie values.
3. Requiring a separator stopped redacting Cookie:sessionid=SECRET, which RFC 7230 permits and no other pattern covers — sessionid/PHPSESSID/connect.sid are in none of them. My comment claiming the generic patterns covered it was simply wrong.
4. The separator rule did not actually close the suppression it was added for. http.cookiejar accepts a cookie named Cookie: y, so Cookie: y:missing_Secure satisfied it and suppressed the PT-A02-04 finding anyway. The name is target-chosen, so the check now also requires that the first pair's name contain no colon.
5. Comma-folded Set-Cookie hid the second cookie. urllib3 folds duplicates with ", ", so Path=/, sessionid=SECRET is two cookies in one segment and the attribute allowlist kept it whole.
6. Comment=/Version= held free server text and were kept verbatim. Both dropped from the allowlist.

Identity

7. My clause-splitting let the target write its own identity. server_returned={body!r} carries verbatim target output and nothing escapes ;, so a body containing ; param=… injected a clause straight into the hash — rotating a finding's id every scan, or merging two distinct findings. Derivation now reads only the clause the probe itself opened. The cost: a location written after another clause is not seen, so those findings keep the empty asset they had before — status quo, not a regression.
8. Aggregate findings were keyed on whichever location came first. Several probes emit one finding covering N reachable paths; remediating one of three would rotate the id of the finding covering the other two, on every scan. An ambiguous list now derives nothing.

Withdrawn

The business_logic method stamp is reverted — its premise was false. _test_workflow_bypass issues POST only for POST and a plain GET otherwise, so a configured DELETE entry is probed with GET. The evidence line and replay step already misreport this; my typed method= would have keyed identity on a request that was never sent. Two config entries producing the same request really are one finding. A test now pins the probe's actual behaviour so whoever fixes it sees what has to move with it.

Test quality

Three tests passed against deliberately broken implementations and are tightened: the composite-derivation test asserted only "no ;" (truncating at ? also passed); the scrub-before-promote test checked the output string, which _scrub_flat_finding cleans anyway, instead of the identity; and secret_field_names was covered by a fixture the generic patterns caught regardless.

Known residual, documented not guessed at

A spaceless Cookie: whose first pair has no = is not redacted, though Cookie: OPAQUETOKEN is. Separating it from cookie:missing_Secure needs a signal neither string carries; it is a malformed header reachable only inside a response body, and guessing wrong destroys evidence whose name the target chose. It is in the docstring.

Suite: 2458 passed, 3 skipped, 305 subtests, both orders. 20,000 generated realistic header shapes: 0 leaks, 0 non-fixed-points, with that residual excluded.