Repository navigation
Conversation
…secrets only Closes #819. assertArtifactPublicationsContainNoSecrets fails closed and deliberately does not rewrite bytes, so a false positive destroys a run after the agent work is already paid for. Its speculative heuristics cannot carry that weight: - Shannon entropy does not separate the populations. A versioned contract method scores 4.62 and a GitHub PAT 4.25, either side of the 4.3 cutoff, so strings that read identically to a human land on opposite sides. - Unlabeled 40-hex matches every git commit hash, including the pinned dependency commits a setup document is expected to list. - The key-name assignment rule fires on the English sentence "For every token:". Introduce SecretScanMode. "all" is the default and every existing caller keeps its behavior; publication passes "positive-only", which runs exact configured values, all vendor patterns, BIP39 mnemonics, context-labeled private keys, URL credentials and Bearer tokens, and skips the three speculative rules. The other callers are unchanged deliberately: attempt-ledger, state and config/redaction rewrite text for display rather than failing, and modal/public-bundle publishes genuinely outward, so over-redaction there is cheap and appropriate. Residual exposure, stated plainly: an opaque secret in no recognized format, which is also not a configured credential value, is no longer caught in a published artifact. That is a real reduction. The existing fixture asserting exactly that case is updated rather than deleted, so the tradeoff is visible in the test rather than implied. Adds regression coverage for the five prose shapes that each killed a run, and asserts that nine credential formats plus exact configured values are still rejected.
…by ctime 'a runtime document read rejects an atomic path replacement during its snapshot' fails about half the time on main, in isolation as well as in suite. It is not a slow test: it is reporting a real gap. readRegularFileSnapshot compares fstat on the open descriptor before and after reading. A rename over the path leaves that descriptor on the original inode, so dev, ino and size are unchanged and the only remaining evidence is the ctime bump from the unlink. The kernel records ctime at timestamp granularity, not instruction granularity, and open/read/rename/fstat routinely complete inside a single tick on an NVMe disk -- so the replacement went undetected and the read returned content for a path that no longer referred to that inode. Compare nlink as well. The rename unlinks the original inode, dropping its link count to zero, which is exact and independent of the clock. Deterministic afterwards: 10/10 in isolation, 316/316 across three suite runs. VALIDATOR_BUILD_IDENTITY covers the compiled validator modules, so touching schema-registry.ts moves it by design; both pinned copies are updated. The assertion is a tripwire that a validator change must be deliberate, and it still is.
The Bearer rule is case-insensitive, so it matches the English word bearer. OpenZeppelin's IAccessControl documents 'the role bearer (i.e. `account`)', and the rule redacted the following prose -- enough to fail an artifact gate that cannot rewrite bytes. Require the captured token to reach MIN_EXACT_SECRET_VALUE_LENGTH and to be made of token characters, which '(i.e.' is not. A real 'Authorization: Bearer <token>' is unaffected.
|
Added a third commit: the The captured token must now reach Worth noting this one survived the |
aviggiano
left a comment
There was a problem hiding this comment.
We want to avoid manually scaning secrets, we should use a third party library
…tlint (#865) 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>
|
Superseded by #865, which lands the requested third-party migration (secretlint 13.0.4): the publication gate keeps this PR's positive-only semantics and exact-value matching, the speculative heuristics stay retired from the gate, and this PR's regression cases (role-bearer prose, the docs-example AKIA rejection, the token-shape Bearer fix) are ported as tests. |
Closes #819.
What changes
assertArtifactPublicationsContainNoSecretsnow scans in"positive-only"mode.sk-,sk-ant-,ghp_,github_pat_,glpat-,hf_,xox*-,AKIA/ASIA,AIza,npm_,ya29., JWT, PEM, alchemy/infura URLs)Bearertokenstoken:/secret =/ …)SecretScanModedefaults to"all", so every existing caller is unchanged. Only the publication gate opts into"positive-only".The other callers are left alone deliberately —
attempt-ledger.ts,state.tsandconfig/redaction.tsrewrite text for display rather than failing, andmodal/public-bundle.tspublishes genuinely outward. Over-redaction is cheap there. It is not cheap in the gate, which fails closed and refuses to rewrite bytes, so a false positive destroys a run.Why these three rules cannot stay
Shannon entropy does not separate the populations:
The cutoff is 4.3. A GitHub PAT (4.25) and a 32-byte hex key (3.90) score below the worst identifier (4.62) — there is no threshold that separates them, so this is not tunable.
It also is not what catches real credentials: that GitHub PAT scores below the cutoff and is still detected, because the vendor patterns run first. For every format the codebase can name, the speculative rules contribute nothing.
The other two are plainly miscalibrated for technical prose: unlabeled 40-hex matches every git commit hash, and the key-name rule fires on
For every token:.Residual exposure
An opaque secret in no recognized format, which is also not a configured credential value, is no longer caught in a published artifact. That is a real reduction, not a free win.
The existing fixture asserting exactly that case is updated rather than deleted, so the tradeoff is visible in the test suite instead of implied. It is a good trade because artifacts are local run output, configured credentials remain byte-matched, and the current behavior destroyed five consecutive runs on false positives.
Second commit — a flaky test that was reporting a real bug
a runtime document read rejects an atomic path replacement during its snapshotfails ~50% of the time onmain, in isolation as well as in suite. It is not slow, it is correct only when it wins a race.readRegularFileSnapshotcomparesfstaton the open descriptor before and after reading. A rename over the path leaves that descriptor on the original inode, sodev,inoandsizeare unchanged and the only remaining evidence is the ctime bump from the unlink — which the kernel records at timestamp granularity. Open/read/rename/fstat routinely complete inside one tick on NVMe, and the replacement then goes undetected: the read returns content for a path that no longer refers to that inode.Comparing
nlinkfixes it exactly — the rename drops the original inode's link count to zero, independent of the clock. 10/10 deterministic in isolation afterwards.VALIDATOR_BUILD_IDENTITYcovers the compiled validator modules, so touchingschema-registry.tsmoves it by design; both pinned copies are updated. The tripwire still trips on the next deliberate change.Verification
pnpm -w lintclean,pnpm -w format:checkcleanAlternatives tried first
Teaching the entropy exemption successive shapes of source notation — dotted chains, single tokens,
/and::separators — and applyingMIN_EXACT_SECRET_VALUE_LENGTHto the key-name rule. Each unblocked one run and the next failed on a new shape, five times. TheFor every token:case still trips with the length floor applied, because in Markdown the captured value is the following word rather than an escape.Greptile Summary
The PR introduces a positive-only secret-scanning mode for fail-closed artifact publication and strengthens runtime-document snapshot replacement detection.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Maintained artifact bytes] --> B[Exact configured-value scan] B -->|Match| X[Reject publication] B -->|No match| C[Decode text] C --> D[Positive-only recognized-secret scan] D -->|Recognized secret| X D -->|No recognized secret| E[Publish original bytes]Reviews (2): Last reviewed commit: "fix(security): require a token-shaped va..." | Re-trigger Greptile
Context used (3)