Repository navigation
Conversation
…ng files locateSpan resumed the scan at the finding's span, but a finding's col points at the capture group, which sits after the start of the whole match. For an ^-anchored pattern (env.kv) the only position ^ can match is the line start, already behind the resume point, so exec found nothing and locateSpan returned -1. redactFindingSpans treats -1 as "cannot mask" and returns null, which its contract requires the caller to read as "drop the whole payload" (garrytan#2912). Any file carrying an env-style assignment was therefore withheld in full, including files whose match was a genuine secret rather than the reported false positive. Resume the scan at the line start so an anchored match is reachable, and keep requiring the span to land exactly at the reported offset, so the fail-closed contract is unchanged. Adds test/locate-span-anchored.test.ts: five cases, all failing before this change and passing after. test/redact-engine.test.ts stays at 98/98. Refs garrytan#2912 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
* fix(settings): preserve symlinked settings targets Resolve the selected target for locking, mutation, backup, and rollback; refuse target changes and preserve private modes. Addresses #2830. * fix(redact): bind masking to original detected spans Inspired by #2929's anchored-span diagnosis; independently implemented using normalization offsets. Addresses #2930 and the relocation portion of #2912 without changing detection sensitivity. * fix(evals): exclude operator credentials from prefix admission Adapts the credential-suffix screen proposed in #2636, with real launched-child regression coverage and deliberate provider-auth exceptions. * fix(artifacts): retain custom allowlist rules on reinitialization Preserve the exact user-owned suffix and publish only a successfully assembled replacement. Independently implements the repair reported in #2907. * test(cso): verify exact masked reads and unmaskable payload refusal * fix(cso): preserve exact filesystem identities through lease recovery Preserve 64-bit device/inode identity and nanosecond race checks. Add native NTFS lifecycle coverage for #2927; retain ambiguous legacy-state refusal without claiming Windows PID-reuse recovery is resolved. * fix(redact): bind pre-push scans to destination and preserve seam context Uses #2935 (bd07318) as source evidence for push-target range and slice-overlap defects. Independently implemented; no cherry-pick or release metadata adoption. * test(ci): gate native agent ownership and settings links on macOS * fix(browse): bind agent lifetimes and cleanup to owned generations Uses #2931 by Chris Hutton / Claude Fable 5.1 as attributed design input; independently implemented without broad sweeps or copied code. Keep uncertain children and locks rather than deleting foreign state. * test(ci): include concurrent shutdown controls in the native macOS gate * v1.88.1.0 fix: harden credential boundaries and owned state * fix(redact): preserve target provenance and scan boundary semantics * test(artifacts): read managed rules from atomic allowlist assembly * fix: preserve native exit observations and fixture prerequisites * fix: preserve UTF-16 offsets through redaction normalization
|
Thank you @aclansley for isolating the anchored-match relocation failure and proposing the exact-position check. Your diagnosis and anchored-span approach are explicitly credited in the maintainer integration, #2942, merged as The shipped implementation binds masking to the original spans captured during scanning, rather than searching the raw text again. It also preserves normalization offsets, overlapping matches, and fail-closed behavior. See Closing this PR as superseded by that integration, not as a merge or wholesale adoption of this branch. As you correctly separated in your proposal, detector false positives are a different problem; #2912 remains open for that and its outstanding size-limit concerns. Thanks for keeping the original fix focused and fail-closed. |
What
locateSpan()cannot relocate an^-anchored pattern, soredactFindingSpans()returnsnulland its caller drops the whole payload. This is the mechanism behind #2912.Why it happens
A finding's
colpoints at the reported span — the capture group when the pattern has one — which sits after the start of the whole match.locateSpanresumed the scan two characters before that span:env.kvis^-anchored:regex: /^[ \t]*(?:export[ \t]+)?[A-Z][A-Z0-9_]*(?:KEY|TOKEN|SECRET|...)[ \t]*=[ \t]*['"]?([^\s'"]{8,})['"]?/The only position its
^can match is the line start, and that is already behindlastIndex.execscans forward, never matches the line the finding came from, andlocateSpanreturns-1.redactFindingSpansthen returnsnull— which, per its own contract, the caller must read as "drop the whole payload".The scope is wider than #2912 reports
#2912 frames this as a false positive on lines that read a key from the environment. The trigger is actually any
env.kvmatch at all, so a file is dropped just as readily when the match is a genuine secret:That is the fourth case in the added test file. The practical effect is that the files most likely to contain credentials are the ones most likely to be withheld.
The fix
Resume the scan at the line start so an anchored match is reachable, then walk matches and accept only the one whose span lands exactly at the reported offset.
Fail-closed behaviour is unchanged: when no match lands at that offset,
locateSpanstill returns-1and the payload is still dropped. The change makes a correct relocation possible; it does not make masking more permissive.Verification
test/locate-span-anchored.test.ts(new)test/redact-engine.test.tsVerified against
mainat v1.87.5.0.test/gstack-redact-cli.test.ts,test/telemetry.test.tsandtest/redact-audit-log.test.tsreport an identical 124 pass / 37 fail both with and without this change — those 37 are pre-existing failures in my sandbox (they shell out throughexecSync) and are unrelated to it.Deliberately not addressed
The other half of #2912 —
env.kvfiring onos.environ[in the first place — is left alone. Tightening that validator trades against false negatives on real secrets, which felt like a maintainer's call rather than mine. This change stops the file loss either way, and the two are independent.🤖 Generated with Claude Code