Repository navigation
Replace hand-rolled vendor secret patterns with secretlint - #865
Merged
Merged
Conversation
…tlint Maintainer instruction from the review comments on #820 ("We want to avoid manually scanning secrets, we should use a third party library") and #823 ("This sensitive redaction is giving us too much headache, we want preferably to use a 3rd party library instead of reinventing the wheel"). This PR supersedes both. Root cause across #819 and #822: a hand-rolled pattern set has to be recalibrated shape by shape every time credential text collides with ordinary technical prose, and each recalibration (entropy exemptions, Bearer token shape, the ak-/as- suffix class) only unblocked one run before the next prose shape failed the fail-closed publication gate. Detection now has three tiers: - Exact configured credential values stay matched byte-for-byte; the library cannot know the run's configuration, so that tier remains app knowledge. - Vendor-format detection is delegated to secretlint 13.0.4 (@secretlint/core plus @secretlint/secretlint-rule-preset-recommend, MIT, pure JS), run fully in-process on a dedicated worker thread behind a synchronous shared-memory bridge so every existing sync caller, including the generated Smithers verifier, keeps its API. A documented supplemental pattern remains only for each concrete true positive the preset cannot see (short/truncated PEM blocks, non-canonical sk-ant-/sk- prefixes, AKIA/ASIA IDs, AIza keys, Modal ak-/as- keys with the #822 hyphen fix, ya29 tokens, JWTs, Alchemy/Infura URLs, Bearer tokens with the #820 token-shape fix), plus the BIP39 mnemonic and context-labeled private-key checks, which no secretlint rule covers. - The speculative heuristics (high entropy, unlabeled 40-hex, key-name assignment) are retired from the publication gate, which now scans positive-only; they remain for default display-redaction callers, where over-redaction is cheap. The preset's filter-comments rule is excluded: scanned content is agent-controlled, and in-content secretlint-disable comments must not be able to suppress the gate's findings (covered by a regression test). The gate keeps failing closed: a hit still rejects publication without rewriting bytes, exact values plus every positively identified format from the #819/#820/#822/#823 examples are still rejected (asserted one by one in the artifacts suite), and the five prose shapes that each killed a campaign run are asserted publishable. Closes #819 Closes #822 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
This was referenced Aug 25, 2026
Closed
Closed
aviggiano
added a commit
that referenced
this pull request
Aug 25, 2026
The three fail-on-hit secret gates in packages/modal — recovery lifecycle, public eval diagnostics, and public benchmark bundle files — ran redactSecretsInText in default "all" mode, so the speculative high-entropy pass flagged the pipeline's own identifiers (ci-<run_id>-… logical run ids, ultrafuzz-ci-… volume names) as secrets. Inside the sandbox that kills the worker as dependency-unreachable before any node runs; on the runner it crashes collect with "Modal recovery lifecycle contains secret-like content". Every benchmark lane has been broken this way since the gates landed in #558, unexercised while the workflow was disabled. Align them with the artifacts secret gate (#865, #819): keep the exact injected-secret-value check byte-for-byte and run the generic scan as containsSensitiveSecrets(text, [], "positive-only"). All positive detections — secretlint findings, supplemental vendor formats, mnemonics, labeled private keys, URL and Bearer credentials — still fail the gates; existing ghp_/sk- test fixtures still throw. Rewriting sanitizers stay in "all" mode. Closes #883 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #819. Closes #822. Supersedes #820 and #823, applying the maintainer instruction from their review comments: "We want to avoid manually scanning secrets, we should use a third party library" (#820) and "This sensitive redaction is giving us too much headache, we want preferably to use a 3rd party library instead of reinventing the wheel" (#823).
Library chosen
secretlint 13.0.4 —
@secretlint/core+@secretlint/secretlint-rule-preset-recommend(pinned exact, MIT, pure JS, no Go binary). Its programmatic API returns findings with match ranges, which the redaction callers need to rewrite text. The API is Promise-based while every caller of this module is synchronous — including the generated Smithers workflow verifier, whose emitted code is pinned by contract tests — so the library runs fully in-process on a dedicated worker thread behind a synchronous shared-memory bridge (Atomics.wait+ length-prefixed payloads in a growableSharedArrayBuffer). Steady-state cost is ~0.2ms per scan, flat over thousands of calls; an earlyMessagePort-based bridge degraded linearly with message count (95ms/scan after 2000 messages) and was replaced, which is also why the modal/evals suites now run faster than before.The preset's
filter-commentsrule is deliberately excluded: scanned content is agent-controlled, and an in-contentsecretlint-disablecomment must not be able to suppress the fail-closed gate's findings (regression-tested).The tier model
SecretScanModedefaults to"all", so every existing redaction caller is unchanged; only the fail-closed publication gate opts into"positive-only".True-positive proof table
Every credential format the old hand-rolled patterns could name is asserted still rejected by the gate (
packages/artifacts/test/artifacts.test.ts, k01–k15):ghp_…(36),github_pat_…(82),gho_…githubglpat-…gitlabxoxb-…, Slack webhook URLsslacknpm_…(36)npmhf_…(34)huggingfaceprivatekeyhttps://user:pass@hostbasicauthpostgres://user:pass@host/dbdatabase-connection-stringsk-ant-…short/legacy (preset needs 90–128 chars endingAA)sk-…(OpenRouter/DeepSeek; preset needs theT3BlbkFJmarker)AKIA/ASIAIDs (preset's ID scan is opt-in, bundles an account-ID keyword heuristic of exactly the #819-retired class, and builtin-ignores the docs example key)AIza…,ya29.…, Modalak-/as-, JWTs, Alchemy/Infura URL keys,Bearer <token>localhost) and non-HTTP schemesEach supplemental pattern carries a comment naming the concrete gap it fills — documented gap-fillers, not a parallel scanner.
False positives eliminated (ported as regression tests)
…-treated-as-private-…,…-marked-ak-…) — clean in both scan modes (theak-/as-suffix class no longer admits-; opaqueak-/as-tokens incl. underscores still detected).Authorization: Bearer <token>still does.verify:goal-plan(and with it the class-goals/threat-goals lanes) no longer read as credentials.Test updates with justification
token=keyword heuristic now use a positively identifiedghp_credential.Residual exposure, stated plainly: an opaque secret in no recognized format that is also not a configured value no longer fails publication. Same trade #820 made: artifacts are local run output, configured credentials remain byte-matched, and the speculative rules destroyed five consecutive runs on false positives while contributing nothing for any nameable format.
Dependency policy-gate evidence
node scripts/validate-audit-profile-package.mjs— passpnpm -w security:dependency-advisories— "0 High/Critical production advisories; 0 active exceptions"pnpm -w test:ci-scripts— 109/109 (bun policy-script suite)packages/security:@secretlint/core@13.0.4,@secretlint/secretlint-rule-preset-recommend@13.0.4(+@secretlint/types@13.0.4dev). All MIT; transitive additions are@secretlint/{types,profiler},debug,ms,structured-source,boundary. Runs fully offline, in-process.Verification
a runtime document read rejects an atomic path replacement during its snapshot, untouched here) · modal 629/629 · evals 426/426 · config 72/72 · runtime secret-behavior files (generated-workflow-verifier, verified-output, workspace-handoff) 223/223pnpm -w typecheck,pnpm lint,pnpm format:checkcleanVALIDATOR_BUILD_IDENTITYmodules are untouched.🤖 Generated with Claude Code
Greptile Summary
The PR replaces hand-maintained vendor credential patterns with a pinned secretlint scanner while retaining supplemental checks for unsupported formats. It also introduces a synchronous worker-thread bridge and separates positive-only publication gating from broader display redaction.
Confidence Score: 5/5
The PR appears safe to merge, with no concrete changed-code failure remaining after reviewing the scanner bridge, packaging assumptions, publication gate, and redaction behavior.
The supported Node runtime provides the required worker and shared-memory features, the worker source is included in the package build, configured credentials remain byte-matched, and no realistic recognized credential was shown to bypass the revised detection paths.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Text or artifact bytes] --> B[Exact configured-value check] B --> C[Synchronous scanner API] C --> D[SharedArrayBuffer request] D --> E[Secretlint worker] E --> F[Finding ranges] F --> G[Supplemental positive-format checks] G --> H{Caller mode} H -->|positive-only| I[Publication gate accepts or rejects unchanged bytes] H -->|all| J[Apply speculative heuristics] J --> K[Redacted display or persisted diagnostic]Reviews (1): Last reviewed commit: "feat(security): replace hand-rolled vend..." | Re-trigger Greptile
Context used (3)