Repository navigation
fix(agent-os): redact secrets glued to a following suffix - #3853
MohammadHaroonAbuomar merged 3 commits into
Conversation
CredentialRedactor left AWS access key, GitHub token, Google API key and Stripe secret key completely unredacted when a valid secret was immediately followed by an annotation like _old or _rotated. Each of those patterns has a fixed length or a value class that excludes underscore, so a trailing \b found no shorter match to fall back on and the whole pattern failed rather than truncating. Basic auth secret had the same gap on its left edge, since it still used \b there after every other prefix anchored pattern had already moved to a lookbehind. Replace the trailing \b with the mirror lookahead (?![A-Za-z0-9]) on the four affected patterns, and anchor Basic auth secret on both sides. Patterns whose value class already includes underscore (OpenAI, Bearer, JWT) are unaffected since they already absorb the suffix. Fixes microsoft#3494 Signed-off-by: qubeena07 <qubeena7@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
Loaded credential_redactor.py from main, from this branch, and from #3855 side by side in one
process and ran the same inputs through all three, because #3855 fixes the same bug and the
difference between the two is what decides which one should land.
The bug is real and the explanation in your comment is the correct one. For a pattern whose value
class excludes _ or whose length is fixed, a trailing \b after a suffix like _old has no
shorter alternative to back off to, so the whole match fails and the complete secret passes through
unredacted rather than being truncated. That is a fail-open, and it is the worst failure mode this
module has.
Measured, contains_credentials on each tree:
| input | main | #3853 | #3855 |
|---|---|---|---|
AKIAIOSFODNN7EXAMPLE_old |
miss | HIT | HIT |
ghp_<24>_old |
miss | HIT | miss |
ghs_<24>_deprecated |
miss | HIT | miss |
AIza<35>_v2 |
miss | HIT | HIT |
sk_live_FakeTestKey0000_rotated |
miss | HIT | HIT |
Basic <b64>_old |
miss | HIT | HIT |
auth_Basic <b64> |
miss | HIT | HIT |
url_https://alice:password@example.com/resource |
miss | HIT | HIT |
And the negatives, which is the half that matters for a widening change. AKIAIOSFODNN7EXAMPLEX,
xAKIAIOSFODNN7EXAMPLE_old, AIza<35>9, sk_live_short and a plain https://example.com/resource
are all still unmatched on this branch, identically to main. The mirror assertion is exactly as
strict about what may follow as the fixed length or value class already was, so nothing widens.
The line that separates this PR from #3855 is the GitHub token one. #3855 leaves that pattern's
(?![A-Za-z0-9_]) in place, so on that branch:
redact("ghp_AAAAAAAAAAAAAAAAAAAAAAAA_old")
main -> ghp_AAAAAAAAAAAAAAAAAAAAAAAA_old
#3853 -> [REDACTED]_old
#3855 -> ghp_AAAAAAAAAAAAAAAAAAAAAAAA_old
A live GitHub PAT annotated _old, _backup or _rotated is exactly the shape a real secret takes
in a config file or a rotation note, and it stays fully in the clear under #3855. This branch is a
strict superset on every case I tried, and it is the one I would merge.
Two things I checked and want on the record. Removing f"{_fake_github_token('ghs')}_" from the
negative list is correct and not a weakened test: that string is now a real detection, so keeping it
as a negative would have asserted the bug. And the new ReDoS tests are the right instinct, since the
added lookahead is a single fixed-width assertion rather than a repeated class, so linear-time
behaviour is preserved.
Nothing blocking. Credit to #3855 for finding the same class independently; the base64-padding case
it tests (auth_Basic YWJjZGVmZ2hpaw==) already passes on this branch, so nothing is lost by
landing this one.
|
MohammadHaroonAbuomar liamcrumm flagging for priority. This closes a fail-open in #3855 fixes the same class from another contributor but leaves the GitHub token pattern untouched, so |
Carlos Hernandez (carloshvp)
left a comment
Carlos Hernandez (carloshvp)
left a comment
There was a problem hiding this comment.
Reviewed head 756773999ad0663e70e6a5836c14e8a195b68cde. I ran all 93 credential-redactor tests on Python 3.11; restoring the parent source while retaining these tests produces 11 failures. An independent 98-case matrix covered all five short GitHub token prefixes at multiple lengths, underscore annotations, AWS/Google/Stripe/Basic credentials, and negative boundaries. I also checked padded Basic auth.
I compared this head with #3855 at 721429a8. Both fix the AWS/Google/Stripe/Basic cases I exercised, but #3855 leaves underscore-suffixed GitHub tokens unredacted. This PR covers that gap. My recommendation is to land #3853 as the broader fix for #3494; this supersedes my earlier preference implied by approving #3855. Credit to both contributors for the overlapping work.
No blocking finding. Source lint passes; the test import-order warning also occurs on the parent. I verified the signoff and a conflict-free merge simulation against main 359a2332.
, microsoft#3916, microsoft#3924, microsoft#3853, advance watermarks
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- Superset rule violated for the Google API key pattern: the new trailing lookahead
(?![A-Za-z0-9])misses a valid 39-char key whose 35th value char is-when an alphanumeric follows, which the old\bdetected. The full key then leaves the MCP gateway unredacted under both BLOCK and SANITIZE policies, where origin/main blocked or redacted it. This is the same glued-suffix bug class the PR claims to fix. File: agent-governance-python/agent-os/src/agent_os/credential_redactor.py:153.
Review on microsoft#3934 caught a real narrowing: the previous plain \b treated hyphen as an automatic boundary on its own, since hyphen is not a word character, so a 35 character key ending in a hyphen and glued directly to more text used to be redacted. The new lookahead treats that the same as every other "one more alphanumeric character right there" case already in this file, on the same reasoning already applied to the AWS pattern, and leaves it unredacted. That reasoning is sound, but I had not written it down or tested it, so it looked like an accident rather than a choice. Added a comment and a test pinning it. Also confirmed the Python port (PR microsoft#3853, agent-os credential_redactor.py) has the identical gap in its Google API key pattern, and will pin it the same way there. Signed-off-by: qubeena07 <qubeena7@gmail.com>
… test Review on the dotnet port of this fix, PR 3934, caught a real narrowing that also applies here: a trailing \b would have treated hyphen as an automatic boundary on its own, since hyphen is not a word character, so a 35 character key ending in a hyphen and glued directly to more text used to be redacted before the fix in this branch. The mirror assertion treats that the same as every other "one more alphanumeric character right there" case already covered, on the same reasoning already applied to AWS access key. That reasoning is sound, but it was not written down or tested, so it looked like an accident rather than a choice. Added a comment and a test pinning it. 94 passed, tests/test_credential_redactor.py. Signed-off-by: qubeena07 <qubeena7@gmail.com>
|
Follow-up: review on the dotnet port of this same fix, PR #3934, caught a real narrowing in the Google API key pattern that applies here too. A trailing \b would have treated hyphen as an automatic boundary on its own, since hyphen is not a word character, so a 35 character key ending in a hyphen and glued directly to more text used to be redacted before this fix, and the mirror assertion now treats that the same as the other "one more alphanumeric character" cases already covered here, on the same reasoning already applied to AWS access key. That reasoning holds, but it was not written down or tested, so pushed a comment and a dedicated test pinning it (test_does_not_widen_google_api_key_match_when_key_ends_in_hyphen) rather than leave it as an unexamined side effect. 94 passed locally. |
…to more text Review on this PR pointed out that a detector change must never lose a shape the previous version caught. The mirror assertion alone loses one: a trailing \b treats hyphen as an automatic boundary on its own, since hyphen is not a word character, so a 35 character key ending in a hyphen and glued straight to more text was redacted before this branch, and the mirror assertion by itself does not see that, since it only looks at what follows, not at what was actually consumed. The pattern now also accepts whenever the character actually consumed at that position is a hyphen, on top of the existing "not alphanumeric" check, restoring the original behavior for that one shape while keeping the "one more alphanumeric character means a different token" guard for every other ending. Verified against the previous pattern across a grid of 35th characters and following characters: the only three cases that change are exactly the ones this is meant to fix, nothing else widens. Flips the pin test added for this same gap into a positive regression test, since the gap is now closed rather than documented as accepted. 94 passed, tests/test_credential_redactor.py. Reverting only the source change and keeping the flipped test: it fails, confirming the test exercises this fix. Signed-off-by: qubeena07 <qubeena7@gmail.com>
|
Agreed on the strict resolution, a detector change should never lose a shape the base caught. Pushed the exact change requested: the Google tail is now (?:(?![A-Za-z0-9])|(?<=-)), and the pin test is flipped to assert the hyphen-ending case is redacted rather than documenting it as an accepted gap. Verified locally against a grid of every 35th character (alnum, hyphen, underscore) crossed with every plausible following character, comparing this change against the previously pushed pattern: the only three cases that change are the hyphen-ending-glued-to-alnum ones, nothing else widens. 94 passed in tests/test_credential_redactor.py, and reverting only the source change while keeping the flipped test fails it, confirming it exercises the fix. Also applied the identical change to the dotnet port, PR #3934, for consistency. |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
Verified at c14a10d: the Google tail is now the strict-superset form and the pin test is flipped to a positive regression test. A 567k-text base-vs-head probe shows zero lost shapes across the five changed patterns and no lookalike over-redaction. All three commits signed.
…3853) * fix(agent-os): redact secrets glued to a following suffix CredentialRedactor left AWS access key, GitHub token, Google API key and Stripe secret key completely unredacted when a valid secret was immediately followed by an annotation like _old or _rotated. Each of those patterns has a fixed length or a value class that excludes underscore, so a trailing \b found no shorter match to fall back on and the whole pattern failed rather than truncating. Basic auth secret had the same gap on its left edge, since it still used \b there after every other prefix anchored pattern had already moved to a lookbehind. Replace the trailing \b with the mirror lookahead (?![A-Za-z0-9]) on the four affected patterns, and anchor Basic auth secret on both sides. Patterns whose value class already includes underscore (OpenAI, Bearer, JWT) are unaffected since they already absorb the suffix. Fixes microsoft#3494 Signed-off-by: qubeena07 <qubeena7@gmail.com> * fix(agent-os): pin the Google API key hyphen boundary tradeoff with a test Review on the dotnet port of this fix, PR 3934, caught a real narrowing that also applies here: a trailing \b would have treated hyphen as an automatic boundary on its own, since hyphen is not a word character, so a 35 character key ending in a hyphen and glued directly to more text used to be redacted before the fix in this branch. The mirror assertion treats that the same as every other "one more alphanumeric character right there" case already covered, on the same reasoning already applied to AWS access key. That reasoning is sound, but it was not written down or tested, so it looked like an accident rather than a choice. Added a comment and a test pinning it. 94 passed, tests/test_credential_redactor.py. Signed-off-by: qubeena07 <qubeena7@gmail.com> * fix(agent-os): redact a Google API key ending in a hyphen when glued to more text Review on this PR pointed out that a detector change must never lose a shape the previous version caught. The mirror assertion alone loses one: a trailing \b treats hyphen as an automatic boundary on its own, since hyphen is not a word character, so a 35 character key ending in a hyphen and glued straight to more text was redacted before this branch, and the mirror assertion by itself does not see that, since it only looks at what follows, not at what was actually consumed. The pattern now also accepts whenever the character actually consumed at that position is a hyphen, on top of the existing "not alphanumeric" check, restoring the original behavior for that one shape while keeping the "one more alphanumeric character means a different token" guard for every other ending. Verified against the previous pattern across a grid of 35th characters and following characters: the only three cases that change are exactly the ones this is meant to fix, nothing else widens. Flips the pin test added for this same gap into a positive regression test, since the gap is now closed rather than documented as accepted. 94 passed, tests/test_credential_redactor.py. Reverting only the source change and keeping the flipped test: it fails, confirming the test exercises this fix. Signed-off-by: qubeena07 <qubeena7@gmail.com> --------- Signed-off-by: qubeena07 <qubeena7@gmail.com> Signed-off-by: yuvrajsingh2428 <offcyuvi2428@gmail.com>
4 participants
Related Issue
Fixes #3494
Timeline
None
Type of Change
Package(s) Affected
Core & runtime:
Testing
Unit Testing
Added
test_redacts_secret_glued_to_a_following_word_character, covering AWS access key, all five GitHub token prefixes, Google API key, Stripe secret key and Basic auth secret with a suffix such as_oldor_rotatedglued directly to the secret. Addedtest_trailing_anchor_does_not_widen_the_matchto confirm the new lookahead does not loosen the boundary: one extra character after a fixed length key, a Stripe key under the length floor, a short Basic value and a Basic auth URL with no credentials all still fail to match. Added two glued left edge cases (auth_Basic ...andurl_https://user:pass@...) to the existingtest_detects_secret_glued_to_preceding_word_characterparametrization, since Basic auth secret still had the left edge\bbug that every other prefix anchored pattern had already moved away from. Removed one case fromtest_github_token_boundaries_and_lengths_avoid_false_positivesthat encoded the bug itself as expected behavior (a GitHub token followed by a bare underscore was asserted to produce no match at all). Addedtest_trailing_lookahead_patterns_handle_adversarial_input_quicklyto confirm the new lookahead does not introduce backtracking on 100k character adversarial input.pytest agent-governance-python/agent-os/tests/test_credential_redactor.py: 93 passed. Reverting only the source change while keeping the new tests: 11 of those fail, confirming they exercise the fix rather than just describing it.Manual Testing
Ran
AKIAIOSFODNN7EXAMPLE_oldthroughCredentialRedactor.redactandcontains_credentialsdirectly before and after the change: before, the input passed through completely unredacted and detection reported nothing; after, it redacts to[REDACTED]_oldand detection reportsAWS access key. Repeated for the GitHub, Google, Stripe and Basic auth cases described in the linked issue.Checklist
Attribution & Prior Art
Prior art / related projects: none. Note for reviewers: issue #3494 previously had a fix proposed in #3495, which went through review and was closed on 2026-08-05 for a reason unrelated to the code ("this repository is not accepting submissions from this account"). This PR reaches the same diagnosis independently by reading the current pattern table in
credential_redactor.pyagainst the issue's own description, and was not copied from #3495.AI Assistance
An AI coding assistant running in my terminal was used for the regex analysis, the test authoring and this description. I reviewed, ran and verified every change before opening this PR.
IP, Patents, and Licensing