Skip to content

fix: secret scan missed every prefixed secret name (db_password, STRIPE_SECRET) - #2

Merged
aoc81 merged 2 commits into
mainfrom
fix/secret-scan-word-boundary
Sep 12, 2026
Merged

aoc81 merged 2 commits into
mainfrom
fix/secret-scan-word-boundary

Conversation

@aoc81

@aoc81 aoc81 commented Sep 12, 2026 •

Copy link
Copy Markdown
Owner

The bug

The Hardcoded secret assignment rule in plugin/scripts/scan_secrets.py wrapped
its keyword alternation in \b. _ is a word character, so \b never fires
between an underscore and a letter
— every prefixed spelling walked past a
blocking guard:

HIT   password = "..."          MISS  db_password = "..."
HIT   secret = "..."            MISS  my_api_key = "..."
HIT   api_key = "..."           MISS  STRIPE_SECRET = "..."
HIT   client_secret: "..."      MISS  aws_secret_access_key = "..."

The MISS column is the dominant real-world spelling — snake_case and
SCREAMING_SNAKE in Python, YAML and .env content, which is precisely what gets
pasted into a learning. client_secret hit only because client[_-]?secret is
spelled out as its own alternative; that coincidence disguised the gap, since a
spot check of the obvious names passes.

The prefix-anchored rules (ghp_, sk-ant-, AKIA…, JWT) were never affected.
The hole was in the one rule covering credentials with no recognisable format.

Found by planting an aws_secret_access_key canary in the store immediately
after installing the pre-push hook — the scan exited 0 and reported clean.

The fix

Replace the \b anchors with an explicit non-alphanumeric left boundary plus
flat affix character classes. Flat classes rather than nested quantifiers, so
backtracking stays linear: a pathological 800-character line searches in ~1ms.
The value remains capture group 1, which _is_placeholder depends on.

False-positive check

Widening the name must not cost the placeholder filter its job — the main risk,
since this gates a push. Verified still clean:

  • db_password = changeme → filtered as placeholder
  • my_api_key = "your_key_here" → filtered as placeholder
  • password: x, secret_name = db → no match (value too short / not secret-like)

Tests

Three new cases in tests/test_scan_secrets.py covering the prefixed hits, the
prefixed placeholders, and the intended non-hits. Full suite: 162 tests, OK.

Also: this repo now uses a personal store

The second commit replaces a blunt learnings/ gitignore with personalStoreDir
in .lore.json — the feature built for exactly this. Notes about developing the
plugin stay private, while learnings/ goes back to un-ignored so the plugin's
own repo does not contradict the default every user gets (team store, committed
and shared).

With no published store here, the local guards had nothing left to protect, so
the pre-push hook, the .lore/ script copies and .github/workflows/lore.yml
are dropped. Verified both scripts exit 0 with a message when the store is
absent, so that workflow would have been permanently green without checking
anything — and the .lore/ copies duplicated plugin/scripts/ two directories
away, drifting silently whenever a fix landed in one and not the other (which
already happened once in this branch).

aoc81 and others added 2 commits September 12, 2026 17:06
…PE_SECRET)

The "Hardcoded secret assignment" rule wrapped its keyword alternation in `\b`.
`_` is a word character, so `\b` never fires between an underscore and a letter
and every prefixed spelling slipped past a BLOCKING guard:

    HIT   password = "..."          MISS  db_password = "..."
    HIT   api_key = "..."           MISS  my_api_key = "..."
    HIT   client_secret: "..."      MISS  STRIPE_SECRET = "..."
                                    MISS  aws_secret_access_key = "..."

The MISS column is the dominant real-world spelling — snake_case and
SCREAMING_SNAKE in Python, YAML and .env content, which is exactly what gets
pasted into a learning. `client_secret` hit only because `client[_-]?secret` is
spelled out as its own alternative, which disguised the gap: a spot check of the
obvious names passes.

Found by planting an `aws_secret_access_key` canary in the store right after
installing the pre-push hook; the scan exited 0.

Replace the `\b` anchors with an explicit non-alphanumeric left boundary plus
flat affix character classes (not nested quantifiers, so backtracking stays
linear — a pathological 800-char line searches in ~1ms). The value stays capture
group 1, which `_is_placeholder` depends on.

Verified the widening does not cost the placeholder filter its job:
`db_password = changeme` and `my_api_key = "your_key_here"` are still clean, and
short/non-secret values (`password: x`, `secret_name = db`) still do not match.

Also gitignore this repo's own `learnings/`: they are local dev notes, not
content shipped to users of the plugin.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit ignored `learnings/` outright. That got this repo's notes out
of the published tree, but by overriding the default the plugin gives everyone
else — the team store is committed and shared by default, and that default should
stay visible in the plugin's own repo.

Use the feature built for this instead: `personalStoreDir` in `.lore.json`, a
private second store recall reads alongside the team one (entries surface flagged
`[personal]`). `learnings/` goes back to un-ignored; `.lore/` is ignored, and the
committed `.lore.json` makes the convention explicit for anyone else working here.

Drop the local guards along with it: the pre-push hook, the `.lore/` script
copies, and `.github/workflows/lore.yml`. They exist to stop a secret reaching a
*published* store, and this repo no longer has one — verified both scripts exit 0
with a message when the store is absent, so the workflow would have been green
without checking anything. The script copies were also a duplicate of
`plugin/scripts/` two directories away, silently drifting whenever a fix landed in
one and not the other, which had already happened once in this branch.

Also drop the scaffolded `learnings/` tree: its README and template were copies of
`plugin/templates/`, which is in this same repo.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@aoc81
aoc81 merged commit f50dca8 into main Sep 12, 2026
6 checks passed
@aoc81 aoc81 mentioned this pull request Sep 12, 2026
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.

1 participant