Skip to content

fix: prevent redaction false positives for token-like values - #8

Merged
kjanat merged 2 commits into
masterfrom
fix/redact-false-positives
Aug 2, 2026
Merged

fix: prevent redaction false positives for token-like values#8
kjanat merged 2 commits into
masterfrom
fix/redact-false-positives

Conversation

@kjanat

@kjanat kjanat commented Aug 2, 2026

Copy link
Copy Markdown
Owner

This pull request improves the detection and redaction logic for secrets and tokens, making it both stricter and more accurate. It ensures that only valid tokens are redacted (not just by their prefixes), improves JWT shape validation, and expands the list of trivial values that should not be redacted. Several new and updated tests demonstrate the improved handling of common prefix collisions and edge cases.

Detection logic improvements:

  • Token redaction now requires both a known prefix and a sufficiently long, valid body in the correct alphabet, preventing accidental redaction of variables that merely share a prefix (e.g., npm_command or npm_lifecycle_event are no longer redacted, but real tokens like npm_Pw6... are).
  • JWT detection is stricter: a value must have three dotted segments, and the first two must be valid base64url-encoded JSON objects (start with ey) to be considered a JWT, preventing false positives on ordinary identifier chains [1] [2].

Handling of trivial and non-secret values:

  • The list of trivial words that are never redacted now includes read and write, covering common permission scopes.
  • Documentation and test cases clarify that values like id-token: write and short numerics are not secrets and remain visible [1] [2].

Test suite enhancements:

  • Added new test cases to verify that only true tokens are redacted and that variables with common prefixes are left untouched [1] [2] [3].
  • Updated documentation to explain the improved detection rules and rationale behind them.

These changes make the redaction process more robust, reducing false positives and ensuring that only actual secrets are masked.

kjanat added 2 commits August 2, 2026 22:36
`known_token_prefix` matched on the marker alone, so `npm_` — the npm
registry token prefix — claimed every variable npm and Bun inject into a
script's environment. Piping `bun run <script>` output through
`envctl redact` blanked npm_command, npm_config_*, npm_lifecycle_*, and
npm_package_* wholesale, because each line was masked as a bare secret
token before its `KEY=VALUE` shape was ever considered.

Pair every marker with the body its issuer puts after it: a minimum run
length and the alphabet that run stays inside. npm issues base62(uuid),
36 characters with no separator, so a 24-character base62 floor keeps
real tokens masked while `npm_config_local_prefix` — separator at
character 7 — falls out. The other markers gain the same guard, which
also tightens the loose ones: `AKIA` now wants AWS's 16 upper-case
characters, `SG.` a 24-character dotted body, and the vendor-neutral
`sk-` fallback trades its total-length test for a body run.

The run is measured from the marker, not required to reach the end of
the value, so a token quoted in prose or embedded in a URL still masks.
Two more ways a config file read as credentials.

`is_jwt_shape` accepted any three dot-separated base64url segments over 20
characters, which is also what an ordinary identifier chain looks like:
`steps.publish.outputs` is three segments and 21 characters. A workflow
line carrying one inside `${{ }}` tripped the deep scan, and because the
whole `key: value` line is the value at that point, the line came back as
`outputs: <redacted>` with everything gone.

What a JWT adds to the shape is that its first two segments are a
base64url JSON object — the header and the claims set — so both open with
`ey`, the encoding of `{` and the byte after it. Requiring that is the
same anchor other scanners use, and every JWT in the suite already
carries it.

`id-token: write` masked because the key holds a TOKEN segment and
`write` was not a trivial value. It is a GitHub Actions permission
scope, so `read` and `write` join the trivial words next to `on`, `off`,
and `none` — a strong secret name no longer masks them.
@kjanat kjanat self-assigned this Aug 2, 2026
@kjanat kjanat added the cr:review Allow CodeRabbit review label Aug 2, 2026
@gitguardian

This comment was marked as spam.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Improve secret and token detection accuracy.

Require recognised token prefixes, valid character alphabets, and issuer-specific minimum lengths before redaction. Tighten JWT detection to require three segments with base64url-encoded JSON headers and claims. Preserve read, write, and common npm variable values. Add documentation and regression tests for prefix collisions, JWTs, permission scopes, and embedded tokens.

Walkthrough

The redaction engine now validates known token prefixes against minimum lengths and issuer-specific alphabets. It recognises sk- tokens, requires JWT header and claims segments to begin with ey, and treats read and write as trivial values. New fixtures cover CI configuration, npm environment variables, token-prefix collisions, JWTs, bearer tokens, and unchanged token-like values. The README documents these rules.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • kjanat/envctl#2: Related to environment-derived redaction behaviour in src/redact.c.
  • kjanat/envctl#4: Directly related to token-prefix detection and overlapping fixture coverage.
  • kjanat/envctl#5: Related to secret-value detection changes in src/redact.c.

Poem

Ahoy, false positives, walk the plank,
ey marks JWTs; short tokens sank.
read and write now pass the gate,
True secrets meet their masked fate.
Cleaner redaction sails ahead.

🚥 Pre-merge checks | ✅ 6 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Changelog Update ⚠️ Warning The PR modifies source code in src/redact.c, but CHANGELOG.md is absent and therefore was not updated under Unreleased. Add CHANGELOG.md with an entry under ## [Unreleased] describing the token, JWT, and trivial-value detection changes.
Semver Version Bump Validation ⚠️ Warning src/redact.c contains functional changes, but no recognised version file exists in either base or PR, so no SemVer version bump is present. Add a recognised version file or update the project’s recognised version field with a SemVer PATCH increment for these bug fixes.
✅ Passed checks (6 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title Check ✅ Passed Title check skipped as CodeRabbit has written the PR title.
Agents.Md Documentation Updated ✅ Passed No AGENTS.md file exists in the repository, so the documentation-update requirement is not applicable.
Description check ✅ Passed The description directly matches the changes to token validation, JWT detection, trivial values, documentation, and tests.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot changed the title todo fix: prevent redaction false positives for token-like values Aug 2, 2026
@kjanat
kjanat merged commit a43b0e1 into master Aug 2, 2026
10 checks passed
@kjanat
kjanat deleted the fix/redact-false-positives branch August 2, 2026 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cr:review Allow CodeRabbit review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant