Repository navigation
Batch A dev candidate: RV-197 security stack (redaction + fail-closed redactor + authority parity) - #118
Merged
Conversation
…ctor (F1); authority parser-differential + userinfo guard (F2) Post-merge review RV-183 found two SECURITY defects in the merged rag_api producer (#114/#115, main aa66a3a) that the PRs' own tests did not catch. Fix-forward, red-first-then-green permanent tests for each. F1 (R2 redaction did not cover every metadata exit; redactor failed open): - GET /documents (get_documents_by_ids) returned stored Document.metadata verbatim, so a LEGACY row whose quarantined_link is still a raw string leaked credentials/path/token. Apply _redacted_metadata at that exit, after the entitlement filter, without mutating shared ORM state. - _redacted_metadata redacted only a str; a non-string non-canonical marker ({"url": raw}, [raw]) passed through on every route. Now redact anything that is not EXACTLY the canonical {scheme,host,refusal_reason,sha256} object (fail-closed). Hardens /query* too. F2 (host-allowlist parser differential): _reject_ungoverned_link matched on Python urlparse().hostname, which disagrees with WHATWG (browsers, Core's isGovernedUrl) on backslash, and stores userinfo secrets. When the allowlist is configured, refuse-before-the-host-check any link whose authority contains a backslash, ASCII control char, whitespace, or %-encoded delimiter, and refuse userinfo outright (typed reason link_malformed_authority). Scheme-first ordering preserved; RAG_GOVERNED_LINK_HOSTS UNSET stays scheme-only (no new rejections). Permanent tests: tests/utils/test_get_documents_metadata_redaction.py (new), additions + two behavior updates in tests/test_embed_link_host_allowlist.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…+ N1/N4/N5/N6/N8 Second fix-forward on the same PR, addressing RV-197 re-review findings on top of 1c8e690. New commit (not an amend). F2 residual (BLOCKING SECURITY, T16-T19/T36): a tab/LF/CR inside the "://" separator made _raw_authority return '' (link.find("://") == -1) so the guard saw no authority, while urlsplit strips the control char and reads the allowed host -> admitted 200 and STORED raw; WHATWG reads evil.com. Two independent defenses in _reject_malformed_authority: (a) refuse any C0 control char anywhere in the link before parsing; (b) require _raw_authority(link) == parsed.netloc. Scheme-first ordering and the UNSET early-return are unchanged. N1/N7 (real leak): _is_canonical_redacted_marker checked only the key set, so a dict with the canonical keys but RAW VALUES rode the wire on all 4 metadata routes. Now validate value shapes (scheme/host/sha256/refusal_reason regexes; scheme/host may be None); a raw value or extra key -> redact (fail closed). N4 (non-ASCII / punycode): the governed-host list is ASCII and stdlib IDNA admits invalid xn-- labels WHATWG rejects. Refuse any non-ASCII authority and any xn-- label (conservative tightening; T05 and T20 now refused). N5 (typed refusal): urlparse ValueError on an NFKC-unsafe / bad-IPv6 authority was an untyped 500; now a typed 422 link_malformed_authority (nothing stored, no echo). N6: restore the malformed-authority LOG never-echo assertion (was dropped with no repo replacement). N8: add @-free %-only and whitespace-only cases so those branches are independently mutation-killed (MF2d/MF2e). All guards gated behind the configured allowlist except the unparseable-URL refusal; RAG_GOVERNED_LINK_HOSTS UNSET stays scheme-only for admitted links. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ority + N11 marker values Stacks on #116 (agent/files-rag-rv197-security, head b7b2784). Three items, all reusing the #116 F1 redactor (_redacted_metadata) and F2 authority guard (_reject_malformed_authority): item (5) — GET /records and GET /records/all did a raw SELECT * including cmetadata and returned it UNREDACTED (mounted only under DEBUG_RAG_API, entitlement-filtered, but not redacted), so a legacy raw quarantined_link leaks the raw URL/credentials/path/token exactly as GET /documents did before F1. Apply the #116 fail-closed redactor to each served row's cmetadata AFTER the entitlement filter (reused via a new pgvector_routes._redact_row_cmetadata seam; a clean/canonical row passes through unchanged, container type preserved). N10 — the authority guard now refuses shapes Python stores but WHATWG cannot parse: an out-of-range/non-numeric port (parsed.port accessed in a try -> typed 422 link_malformed_authority, never an untyped 500) and a host with non-host chars (< ^ | ...) or an IPvFuture literal [v1...] (strict host-character allow-list, or a valid IP for a bracketed authority). Only reached when the allowlist is configured; UNSET stays scheme-only with zero new rejections. N11 — _is_canonical_redacted_marker now checks VALUES tightly, not just shape: scheme strict lower-case, host a strict hostname (no ':'/userinfo, no whitespace/newline), sha256 exactly 64 hex, refusal_reason EXACTLY one of the typed reasons (exact case). `\Z` (not `$`) closes the trailing- newline bypass. Anything failing -> non-canonical -> redacted (fail-closed); the genuine canonical object still passes untouched (retrieved == stored). Permanent red-first regression coverage added for all three (reproduced at b7b2784, then green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
One merge path, not three. This PR is the linear union of #116 ( Merge one of:
CI is green on Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A Release Assembly (Lane 7) development candidate that composes the RV-197 security stack into a single, dependency-complete branch off
main, for one-action review/landing by the release owner.It is the linear union of two existing Files-lane PRs (their branches are left untouched):
agent/files-rag-rv197-security@b7b2784397d85136ce2042494d9eba6617d20c6e— GET /documents redaction + fail-closed redactor (F1); authority parser-differential + userinfo guard (F2).agent/files-rag-records-redaction@653a29d364ac9b11be0c45633259df0fc9e0c555— fail-closed /records redaction + N10 authority + N11 marker-value tightening (stacked on RV-197 SECURITY fix-forward: GET /documents redaction + fail-closed redactor (F1); authority parser-differential + userinfo guard (F2) #116).Candidate head
653a29d=main(aa66a3ab) + 3 commits (pure fast-forward).mainhas not advanced past #116's base, so the candidate tree is byte-identical to #117's head.Evidence (each rung reported separately)
653a29dare 3/3 SUCCESS —build-and-test,Shipped runtime (Dockerfile),Shipped runtime (Dockerfile.lite). (Repo tests run only in the prepared image per the repo's own notes; the Actionsbuild-and-testgreen is the standing evidence.)653a29d. Verdict: PASS — no blocker, no major. It verified: every metadata-serving read exit is redacted after the entitlement filter; the redactor fails closed on every non-canonical shape (only scheme/host + typed reason + one-way sha256 survive); RV-197D security follow-up: fail-closed /records redaction + N10 authority + N11 marker values #117's\Zanchor + host-class tightening close real leak vectors that RV-197 SECURITY fix-forward: GET /documents redaction + fail-closed redactor (F1); authority parser-differential + userinfo guard (F2) #116 alone left open; WHATWG/urlparse authority parity holds for every admitted link; and the added tests exercise real code paths (not SQL doubles) including denied/cross-entity/never-echo negatives.clean.Minor follow-ups recorded (none is a leak/bypass; all fail closed — do not block this candidate)
quarantined_linkthat makesurlparseraise would 500 the whole batch (fail-closed, ungraceful, untested).source/file_directoryinternal paths still ride the wire on/documentsand/records*(out of these PRs' scope; renderfilename).test_get_documents_cross_tenant_stays_404varies entity, not tenant (the/documentsfilter isuser_id-only — pre-existing D-KSPT-1).Safety
mainonly); this PR runsci.yml(test-only). No schema migration, no new env var, no config change.🤖 Generated with Claude Code
https://claude.ai/code/session_01YZDr6AXUNNvccRXwWZNLNc
Generated by Claude Code