fix(video): re-anchor transcript log-redaction so PII/credential maskers can't reopen the leak (#12150 P1 follow-up) - #12503
Merged
Conversation
…ail payload so PII/credential maskers can't reopen the transcript leak
Owner
Author
|
Merging: the two red checks are inherited base-red (tracked in #12518), not from this diff. "No new ESLint warnings" fails on the CodeQL ratchet (13 open alerts > baseline 11: js/incomplete-url-substring-sanitization ×4, js/weak-cryptographic-algorithm ×3, js/incomplete-sanitization ×2, js/insufficient-password-hash ×2, js/stack-trace-exposure ×1) — none introduced by this change (a pure re-anchor helper + a param thread; the lint metric itself is eslintWarnings=0/eslintErrors=0). The same check is red on unrelated PRs (#12428, #12421). "Fast Quality Gates" is the aggregate wrapper over the same ratchet. |
diegosouzapw
added a commit
that referenced
this pull request
Sep 3, 2026
#12350 fixed the LOCAL_ONLY half of the checker (prefixes + patterns + imported consts). isAlwaysProtectedPath() is two-armed the same way: ALWAYS_PROTECTED_API_PATHS.some(...) || ALWAYS_PROTECTED_API_PATTERNS.some(...) but the checker still read only the path array, so the four credential routes gated by the GHSA-5926-2w35-7h4q pattern (#12600) — /api/providers/{id}/{claude,codex}-auth/{export,apply-local} — reported as 'has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS', asking for the removal of a CORRECT annotation on a credential-export route. Verified with the real predicate: all four isAlwaysProtectedPath() → true; control /api/providers/{id}/models → false. tests/unit/openapi-security-tiers.test.ts already checks BOTH arrays (#12600 updated the test but not the gate script) and stays green — this commit makes the gate agree with the test and with the runtime. Also carries the file-size rebaseline for four caps grown by merged PRs (chat.ts +10 from #12427/#12503; stream.ts / accountFallback.ts / codex.ts +17 from #12179), rationale recorded in the baseline file. Refs #12581
diegosouzapw
added a commit
that referenced
this pull request
Sep 4, 2026
…too (+ file-size rebaseline) (#12605) * fix(ci): mirror isLocalOnlyPath in the security-tier gate and rebaseline four merged-growth file caps Two base-reds on release/v3.8.51 (#12581), both drained at the source. 1) check:openapi-security-tiers reported six CORRECTLY annotated routes as unprotected and demanded the removal of their x-loopback-only annotation — pushing the fix in the unsafe direction. The gate re-reads routeGuard.ts as text (it cannot import the module: routeGuard pulls the server runtime and the gate runs on plain node), but it only read the FIRST half of isLocalOnlyPath(): LOCAL_ONLY_API_PREFIXES.some(...) || LOCAL_ONLY_API_PATTERNS.some(...) so every route gated by a regex (/api/providers/volcengine-plan/connect/*) or by an imported constant (VNC_ROUTE_PREFIX, which the text parse turned into the literal string "VNC_ROUTE_PREFIX") looked open. Proven with isLocalOnlyPath() at runtime: all six return true; the control /api/providers/{id}/refresh stays false. New scripts/check/routeGuardConstants.mjs reads BOTH arrays, resolves imported identifiers by following the import, and THROWS on an unresolvable token instead of silently degrading it into a literal. Its array scanner is hand-rolled because regex literals carry the brackets and commas a \[([^\]]+)\] capture plus a naive comma split break on ([^/] and {1,3}). The reverse pass (missing-annotation warnings) now uses the same predicate. 2) check:file-size: four frozen files grew past their cap through merged PRs — chat.ts +10 (#12427/#12503 video-transcript redaction, derived from the post-guardrail payload at the single dispatch point) and stream.ts / accountFallback.ts / codex.ts +17 total (#12179 hot-path regex hoisting, bounded caches, quadratic-buffering fix). All cohesive at existing chokepoints; rebaselined with the rationale recorded in the baseline file. Refs #12581 * fix(ci): security-tier gate must honor ALWAYS_PROTECTED_API_PATTERNS too #12350 fixed the LOCAL_ONLY half of the checker (prefixes + patterns + imported consts). isAlwaysProtectedPath() is two-armed the same way: ALWAYS_PROTECTED_API_PATHS.some(...) || ALWAYS_PROTECTED_API_PATTERNS.some(...) but the checker still read only the path array, so the four credential routes gated by the GHSA-5926-2w35-7h4q pattern (#12600) — /api/providers/{id}/{claude,codex}-auth/{export,apply-local} — reported as 'has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS', asking for the removal of a CORRECT annotation on a credential-export route. Verified with the real predicate: all four isAlwaysProtectedPath() → true; control /api/providers/{id}/models → false. tests/unit/openapi-security-tiers.test.ts already checks BOTH arrays (#12600 updated the test but not the gate script) and stays green — this commit makes the gate agree with the test and with the runtime. Also carries the file-size rebaseline for four caps grown by merged PRs (chat.ts +10 from #12427/#12503; stream.ts / accountFallback.ts / codex.ts +17 from #12179), rationale recorded in the baseline file. Refs #12581 * fix(ci): re-anchor the zcodeProtocol public-creds allowlist entry (302 -> 313) The check:public-creds allowlist pins each frozen literal by FILE:LINE, so #12179 (hot-path regex hoisting in the same file) shifted the ZCode handshake id from L302 to L313 and broke the gate twice over: the old entry went stale ('a violação foi corrigida; REMOVA a entrada') while the literal itself, now at L313, was no longer covered. The literal is unchanged and still not a credential: `omniroute-${process.pid}` is a per-process handshake id for the local ZCode app-server, already audited and frozen with that justification. Only the anchor moves. Refs #12581 * test(ci): re-anchor the ZCode allowlist test to L313 alongside the gate entry The allowlist key is file:LINE:value, so the synthetic source in this test pads to the exact line the entry pins. Re-anchoring the entry 302 -> 313 (previous commit) without moving the padding left the test asserting the old line — caught by Unit Tests fast-path (4/4) on #12605. Both halves now sit at 313, and the test still proves the allowlist does NOT weaken detection: swapping the value for 'upstream-client-' is still flagged. Refs #12581 * docs(ci): changelog fragment for #12605 * chore(ci): trim #12605 to the one fix the base still needs The base drained fast while this PR was open. Re-verified on 008da6d and dropped everything already covered there: - check-public-creds.mjs: the base already re-anchors the ZCode entry to L313 (my commit only added a comment on top) -> reverted to the base version. - file-size-baseline.json: the base rebaselined chat.ts/codex.ts/ accountFallback.ts to HIGHER caps than mine, and stream.ts measures 3064 against the base cap of 3072 — my 3078 bump would have loosened a cap for no reason -> reverted to the base version. What the base still does NOT have, verified on its current tip: node scripts/check/check-openapi-security-tiers.mjs -> EXIT=1, 4 mismatches so the ALWAYS_PROTECTED_API_PATTERNS half stays, plus its changelog entry. Refs #12581
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…ers can't reopen the leak (diegosouzapw#12150 P1 follow-up) (diegosouzapw#12503) * fix(video): re-anchor log-redaction fullText from the finished guardrail payload so PII/credential maskers can't reopen the transcript leak * docs(video): clarify P1 transcript-retention scope and re-anchor; list P2 surfaces (diegosouzapw#12430)
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…too (+ file-size rebaseline) (diegosouzapw#12605) * fix(ci): mirror isLocalOnlyPath in the security-tier gate and rebaseline four merged-growth file caps Two base-reds on release/v3.8.51 (diegosouzapw#12581), both drained at the source. 1) check:openapi-security-tiers reported six CORRECTLY annotated routes as unprotected and demanded the removal of their x-loopback-only annotation — pushing the fix in the unsafe direction. The gate re-reads routeGuard.ts as text (it cannot import the module: routeGuard pulls the server runtime and the gate runs on plain node), but it only read the FIRST half of isLocalOnlyPath(): LOCAL_ONLY_API_PREFIXES.some(...) || LOCAL_ONLY_API_PATTERNS.some(...) so every route gated by a regex (/api/providers/volcengine-plan/connect/*) or by an imported constant (VNC_ROUTE_PREFIX, which the text parse turned into the literal string "VNC_ROUTE_PREFIX") looked open. Proven with isLocalOnlyPath() at runtime: all six return true; the control /api/providers/{id}/refresh stays false. New scripts/check/routeGuardConstants.mjs reads BOTH arrays, resolves imported identifiers by following the import, and THROWS on an unresolvable token instead of silently degrading it into a literal. Its array scanner is hand-rolled because regex literals carry the brackets and commas a \[([^\]]+)\] capture plus a naive comma split break on ([^/] and {1,3}). The reverse pass (missing-annotation warnings) now uses the same predicate. 2) check:file-size: four frozen files grew past their cap through merged PRs — chat.ts +10 (diegosouzapw#12427/diegosouzapw#12503 video-transcript redaction, derived from the post-guardrail payload at the single dispatch point) and stream.ts / accountFallback.ts / codex.ts +17 total (diegosouzapw#12179 hot-path regex hoisting, bounded caches, quadratic-buffering fix). All cohesive at existing chokepoints; rebaselined with the rationale recorded in the baseline file. Refs diegosouzapw#12581 * fix(ci): security-tier gate must honor ALWAYS_PROTECTED_API_PATTERNS too diegosouzapw#12350 fixed the LOCAL_ONLY half of the checker (prefixes + patterns + imported consts). isAlwaysProtectedPath() is two-armed the same way: ALWAYS_PROTECTED_API_PATHS.some(...) || ALWAYS_PROTECTED_API_PATTERNS.some(...) but the checker still read only the path array, so the four credential routes gated by the GHSA-5926-2w35-7h4q pattern (diegosouzapw#12600) — /api/providers/{id}/{claude,codex}-auth/{export,apply-local} — reported as 'has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS', asking for the removal of a CORRECT annotation on a credential-export route. Verified with the real predicate: all four isAlwaysProtectedPath() → true; control /api/providers/{id}/models → false. tests/unit/openapi-security-tiers.test.ts already checks BOTH arrays (diegosouzapw#12600 updated the test but not the gate script) and stays green — this commit makes the gate agree with the test and with the runtime. Also carries the file-size rebaseline for four caps grown by merged PRs (chat.ts +10 from diegosouzapw#12427/diegosouzapw#12503; stream.ts / accountFallback.ts / codex.ts +17 from diegosouzapw#12179), rationale recorded in the baseline file. Refs diegosouzapw#12581 * fix(ci): re-anchor the zcodeProtocol public-creds allowlist entry (302 -> 313) The check:public-creds allowlist pins each frozen literal by FILE:LINE, so diegosouzapw#12179 (hot-path regex hoisting in the same file) shifted the ZCode handshake id from L302 to L313 and broke the gate twice over: the old entry went stale ('a violação foi corrigida; REMOVA a entrada') while the literal itself, now at L313, was no longer covered. The literal is unchanged and still not a credential: `omniroute-${process.pid}` is a per-process handshake id for the local ZCode app-server, already audited and frozen with that justification. Only the anchor moves. Refs diegosouzapw#12581 * test(ci): re-anchor the ZCode allowlist test to L313 alongside the gate entry The allowlist key is file:LINE:value, so the synthetic source in this test pads to the exact line the entry pins. Re-anchoring the entry 302 -> 313 (previous commit) without moving the padding left the test asserting the old line — caught by Unit Tests fast-path (4/4) on diegosouzapw#12605. Both halves now sit at 313, and the test still proves the allowlist does NOT weaken detection: swapping the value for 'upstream-client-' is still flagged. Refs diegosouzapw#12581 * docs(ci): changelog fragment for diegosouzapw#12605 * chore(ci): trim diegosouzapw#12605 to the one fix the base still needs The base drained fast while this PR was open. Re-verified on 9960e46 and dropped everything already covered there: - check-public-creds.mjs: the base already re-anchors the ZCode entry to L313 (my commit only added a comment on top) -> reverted to the base version. - file-size-baseline.json: the base rebaselined chat.ts/codex.ts/ accountFallback.ts to HIGHER caps than mine, and stream.ts measures 3064 against the base cap of 3072 — my 3078 bump would have loosened a cap for no reason -> reverted to the base version. What the base still does NOT have, verified on its current tip: node scripts/check/check-openapi-security-tiers.mjs -> EXIT=1, 4 mismatches so the ALWAYS_PROTECTED_API_PATTERNS half stays, plus its changelog entry. Refs diegosouzapw#12581
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
1 participant
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.
Why
Follow-up to the merged #12427 (#12150 P1). The P1 whole-branch review found an Important fail-open that merged before the fix could land: the video-bridge guardrail runs at priority 7, but the PII masker (10) and credential masker (95) rewrite the same chained payload afterward, in place. When either masking flag is on, the flattened video description text changes, so the log-redaction content-match (
part.text === fullText) misses and the transcript persists in the call log (only PII-masked). This hits exactly the privacy-maximal operators who enable both features. (Default path is unaffected — PII masking is off by default, Hard Rule #20.)Fix
reanchorVideoBridgeRedaction(new, pure, unit-tested insrc/lib/guardrails/videoBridge.ts) re-reads each redaction entry'sfullTextfrom the finished pre-call guardrail payload at the advisory(container, messageIndex, partIndex)— valid because chain guardrails rewrite text in place, never splice the message array.deriveVideoBridgeLog(src/sse/handlers/chat.ts) now takes the finalbodyand applies it, so the sink match sees the post-masker text.redactedTextis unchanged (rendered from structured cues). New entries are returned; the shared guardrailmetais never mutated.Validation
TDD: two
reanchorVideoBridgeRedactionunit tests (re-reads post-masker text; falls back to original when the advisory index no longer resolves) — RED before, GREEN after. Focused suites green:guardrails/videoBridge+video-bridge-log-redaction+video-bridge-memory-suppression(38/38).typecheck:core,prettier --check(touched files),check:docs-allclean. Base is green.Also clarifies the GUARDRAILS.md retention note and lists the remaining P2 surfaces. Refs #12150, #12430; follows up #12427.