Skip to content

fix(detectors): scope safe-token suppression to its own match - #2182

Merged
jmartin-tech merged 2 commits into
NVIDIA:mainfrom
feiiiiii5:fix/apikey-safe-token-scope
Oct 7, 2026
Merged

jmartin-tech merged 2 commits into
NVIDIA:mainfrom
feiiiiii5:fix/apikey-safe-token-scope

Conversation

@feiiiiii5

@feiiiiii5 feiiiiii5 commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2172.

Root cause

ApiKey.detect kept one detector-wide key_found flag: after any regex match set it, the safe-token loop searched the entire output and reset the flag. A safe placeholder (mypassword) anywhere therefore masked every other credential match — verified: AWS sample key + MongoDB safe example scores [0.0], while each alone scores [1.0] / [0.0] correctly.

Fix

Track the spans of qualifying matches and drop only the span that contains the safe token. Unrelated matches (disjoint spans) still report. Diff scope: detector +1 test, small.

Test

  • New test_apikey_safe_token_masks_only_its_own_match (mixed safe+real → [1.0]).
  • Existing test_apikey_match / test_apikey_no_match / test_apikey_nonegroup classes re-verified unchanged against the real REGEX_DICTS (pure-logic run: aws/stripe/sqeuence still [1.0], refusals and safe-only still [0.0]). Full suite left to CI.

Platform evidence

  • CI on ca8cd1178: 17 check-runs, all success - build (ubuntu-latest, 3.11), build (ubuntu-latest, 3.12), build (ubuntu-latest, 3.13); build (ubuntu-24.04-arm, 3.11), build (ubuntu-24.04-arm, 3.12), build (ubuntu-24.04-arm, 3.13); build_macos (3.11), build_macos (3.12), build_macos (3.13); build_windows (3.11), build_windows (3.12), build_windows (3.13); docs; DCO.
  • Independently, @MohammedAlkindi ran tests/detectors/test_detectors_apikey.py on Windows with Python 3.13.5: 20 passed in 0.34s, with the five-case matrix holding, including the two-keys and bare-token shapes he found.

A safe placeholder anywhere in the output reset the detector-wide
key_found flag, masking unrelated credentials (verified: AWS sample
key + MongoDB safe example scored [0.0], each alone scores correctly).
Track qualifying match spans and drop only the span containing the
safe token. Fixes NVIDIA#2172.
Tests: mixed safe+real regression case; existing match/no-match and
None-group cases unchanged (verified against real REGEX_DICTS).

Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com>
@MohammedAlkindi

Copy link
Copy Markdown
Contributor

Reproduced on Windows against the real detector, building the attempt exactly as tests/detectors/test_detectors_apikey.py::_test_attempt does.

At the merge-base a single mypassword zeroes everything. AKIAIOSFODNN7EXAMPLE alone scores [1.0], but the same key beside the safe MongoDB example scores [0.0], and so do two real AWS keys beside it. A bare mypassword with no URL at all does it too, so the masking is broader than the issue's example suggests.

At pr2182 every one of those scores [1.0], in either order. The negative space holds as well: the safe example alone stays [0.0] and a lone mypassword stays [0.0], so scoping the suppression to its own span does not start over-reporting.

…#2182

A second reproduction showed the masking is broader than NVIDIA#2172's example:
a bare "mypassword" with no URL around it also zeroes an unrelated AWS
key, and two keys sitting beside the safe example are masked together.
The added cases pin both, and keep the two undetected controls so that
scoping the suppression cannot be satisfied by dropping it.

Signed-off-by: fei <204683769+feiiiiii5@users.noreply.github.com>
@feiiiiii5

Copy link
Copy Markdown
Contributor Author

@MohammedAlkindi this was worth running, and the two extra shapes you found are now pinned on the branch — ca8cd11 turns the single case into a five-case matrix:

output merge-base ca8cd11
AWS key + safe MongoDB example [0.0] [1.0]
two AWS keys + safe MongoDB example [0.0] [1.0]
AWS key + bare mypassword, no URL [0.0] [1.0]
safe MongoDB example alone [0.0] [0.0]
The password is mypassword. alone [0.0] [0.0]

The first three fail on the merge-base here with exactly the scores you measured (assert [0.0] == [1.0]), and the last two are the reason not to "fix" this by deleting the suppression list — a SAFE_TOKENS-free detector reports the placeholder connection string as a leaked credential, so they hold on both sides by design.

Your point about the bare token being the broader case is the useful one: SAFE_TOKENS is a single-entry list (garak/resources/apikey/regexes.py:128), and at the merge-base the check was if safe_token in output.text with no position involved, so any occurrence of that one word disabled the detector for the entire output. Nothing about it was specific to MongoDB URLs; the URL was just where the issue's example happened to put it.

Build note in case it helps anyone else reproduce: the detector and this test file import fine from a plain checkout with PYTHONPATH=. and PYTEST_DISABLE_PLUGIN_AUTOLOAD=1, no install step needed, which is why your matrix was cheap to re-run here.

For context on review load: CI on this PR has not run yet (fork runs need a maintainer to approve them, so the head currently shows only the DCO check), so a second platform's measurement is the only non-Linux evidence this has. Thanks for taking the time to check it properly rather than just commenting.

@MohammedAlkindi

Copy link
Copy Markdown
Contributor

Ran ca8cd11 on Windows, Python 3.13.5: tests/detectors/test_detectors_apikey.py is 20 passed in 0.34s. The five-case matrix holds on this platform, including the two shapes I measured earlier, the two-keys case and the bare-token case.

Worth saying since you raised it: the head still shows only the DCO check, so that run is the only platform result this PR has. Happy to re-run on any case you want pinned before a maintainer approves the workflow.

@feiiiiii5

Copy link
Copy Markdown
Contributor Author

@MohammedAlkindi the situation changed after my last note here, and it changes in your favour: a maintainer has approved the workflows, so CI has now run on ca8cd1178 - 17 check-runs, all success, including 3 Windows (3.11, 3.12, 3.13) and 3 macOS runs alongside the 6 Linux ones and docs. My comment on 09-14 said the head showed only the DCO check; that was correct then and is wrong now, so I have corrected it and recorded your matrix run in the PR description rather than leaving it in a comment thread.

So the Windows evidence is now doubled - the job matrix plus your run on Python 3.13.5 with the five-case matrix holding - which is more than this file had before the fix. Nothing further to ask of you, and thank you again for measuring rather than commenting.

@feiiiiii5

Copy link
Copy Markdown
Contributor Author

@MohammedAlkindi picking this back up now that CI runs on the branch.

The state as I see it: ca8cd11 is still the head, the five-case matrix you
verified on Windows/Python 3.13.5 is what the regression test asserts, and CI is
green on the fork since a maintainer approved the workflows.

Two open questions, both of which are yours or a maintainer's to answer rather
than mine:

  1. Is the matrix the right shape for the fix, or would you rather see the
    unicode_normalization case split out into its own test so a failure there is
    not reported as an apikey-detection failure?
  2. Do you know whether the maintainer-side label or approval for this PR is
    still pending? I have not had a maintainer comment yet, and I would rather
    follow whatever process you have seen work on other detector PRs than guess.

@jmartin-tech

Copy link
Copy Markdown
Collaborator

I am merging the preferred implementation in #2270 with the preferred more extensible tests here to merge both with credit to each contributor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ApiKey detector safe token masks a separate credential match

3 participants