ci: pin actions to SHAs, add Dependabot, and gate on a gitleaks scan - #8
Merged
Conversation
`actions/checkout@v4` and `actions/setup-node@v4` tracked mutable tags. A tag can be repointed by whoever controls the upstream repo, so a compromised action would have executed in our runner. Both are now full commit SHAs with the version in a trailing comment. Both `v4` floating tags currently resolve to v4.4.0, so these pins are a no-op against what was already running — this changes the trust model, not the build: actions/checkout 11d5960a326750d5838078e36cf38b85af677262 v4.4.0 actions/setup-node 49933ea5288caeca8642d1e84afbd3f7d6820020 v4.4.0 Dependabot lands in the same commit because it is what makes pinning an improvement rather than a regression. An unpinned tag at least drifts forward; a pin with nothing bumping it just ages silently until you are a year behind and no longer noticing. Grouped so the actions arrive as one PR. Not added here: the npm ecosystem for the three project directories. They carry devDependencies only, but esbuild is among them and it produces the committed *.user.js that CI requires to be byte-identical to source — so every esbuild bump PR would arrive red on the drift check and need a manual rebuild commit to go green. That is a review routine worth adopting deliberately rather than switching on as a side effect of this issue. Refs #6 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WSt6ZYkUkUyDc28cHgwhDj
This repo is public and nothing checked content before now — the pre-commit hook only bumps @Version in staged *.user.js. Auto Login sharpens the generic risk. Per docs/adr/0003 credentials are stored and shared in recoverable plaintext, and an export is `AL1:` + base64url(deflate-raw(JSON)) — an encoding, not encryption. Pasted into a fixture, README, or issue body that is a full credential dump that reads as ordinary text, and no generic rule fires on it. Hence the custom rule, written against the actual encoder in auto-login/src/share.ts rather than a guess: it covers the `AL1U:` uncompressed fallback too, and its length floor keeps it off the `SHARE_PREFIX` constants in source and in the built bundle. Verified to fire rather than merely to pass — a planted share string of each form, a non-example AKIA key, and a GitHub PAT all produce findings and a non-zero exit, in both working-tree and git-history mode. The current tree and all 62 commits of history are clean, so no allowlist was needed and the check does not arrive red. Scans full history rather than a base..head range. History being clean makes that affordable (~1s) and it removes the failure mode that matters most here: a mis-computed range scans nothing and still exits 0, which is a check that cannot fail. In practice it only ever trips on what a diff adds. gitleaks is installed as a version-pinned, SHA256-verified release binary rather than a marketplace action — one less third-party action in the supply chain and nothing extra to keep pinned. Refs #7 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WSt6ZYkUkUyDc28cHgwhDj
Review findings on the gitleaks job. The `\b` anchor in the share-string rule was a false negative, not a safety margin: `AL1U?:` is already distinctive, and requiring a word boundary let `share_AL1:<body>` through untouched. Verified before and after — the prefixed form now fires, the bare form still does. The full-history comment claimed the job "only ever fails on what a diff adds", which is wrong in the one case that matters. A hit anywhere in history fails every branch, and deleting the secret later does not clear it because the blob stays reachable. That behaviour is deliberate and matches the repo's git-workflow standard, but nobody should meet it for the first time on a red merge queue, so the comment now states it and points at --baseline-path / .gitleaksignore as the deferral hatch. Also: `detect` is deprecated in 8.30.1 in favour of `git`, so use the supported form now rather than discovering it at v9; --retry-all-errors so curl retries connection failures, not just HTTP statuses; and the commit count says 64, which is what the scan actually reports. Documented rather than fixed: the *stored* form of the same credentials. Review proposed a rule keyed on "password"/"username"/"updatedAt", but that is not the shape — per src/types.ts a credential is `value` on a `fill` step, with no key naming it and nothing distinguishing a filled password from a filled username. A rule keyed on the real shape (pagePattern + value) hits the fixtures in auto-login/test/ — runner.test.ts alone has `value: 'hunter2'` — so it would need an allowlist over that directory, which would blind the scan to a real share string committed into a fixture. That is one of the leak paths issue #7 named, so the gap is now written down in .gitleaks.toml instead of papered over. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WSt6ZYkUkUyDc28cHgwhDj
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
1 participant
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 #6. Closes #7.
Both follow-ups from #3 in one branch — they're the same concern (what enters the repo and what the runner executes) and neither is big enough to justify its own review.
#6 — SHA pins + Dependabot
actions/checkout11d5960a326750d5838078e36cf38b85af677262actions/setup-node49933ea5288caeca8642d1e84afbd3f7d6820020Verified both ways:
v4.4.0and the floatingv4currently resolve to exactly these commits, so the pins are a behavioural no-op against what was already running — this changes the trust model without changing the build. (The versions suggested in #6, v4.2.2 / v4.1.0, were stale; pinning those would have been a silent downgrade.).github/dependabot.ymladds thegithub-actionsecosystem, grouped, weekly. Pinning without this is arguably worse than tracking a tag — a SHA doesn't rot loudly, it just quietly ages.npm deliberately excluded. All three projects carry
esbuild, which produces the committed*.user.jsthat #3's drift check requires to be byte-identical. An esbuild bump changes those bytes, so every such Dependabot PR would arrive red and need a manual rebuild commit. That's a review routine worth adopting on purpose, not switching on as a side effect.#7 — secret scan
gitleaks 8.30.1 as a checksum-verified release binary rather than a marketplace action: one less third-party action in the supply chain and nothing extra to keep pinned. Runs as a
secret-scanjob inci.yml(same triggers/permissions, so a separate workflow would duplicate the concurrency block for nothing)..gitleaks.tomlextends the upstream default ruleset and adds one repo-specific rule for the Auto Login share string. This is the part a generic scanner cannot catch: perdocs/adr/0003, a share string isAL1:/AL1U:+ base64url of every stored credential — encoding, not encryption — so it reads as ordinary text. The rule was written against the real encoder inauto-login/src/share.ts, not a guess at the format.Scans full history, not a
base..headrange (a deviation from #7's suggestion). History is clean, so it doesn't arrive red; computing a diff range is the part that breaks quietly, since a mis-computed base scans nothing and still exits 0. At ~1s for this repo, whole-history buys immunity to that for free.Verification
The last PR shipped a check that silently could not fail, so each claim here was tested rather than assumed:
AL1U:+ base64url of a real credential object). Scan exits 1. Removed afterwards.SHARE_PREFIX = 'AL1:'inshare.tsand in the built bundle do not trip it; the length floor handles that, confirmed by the clean scan.sha256matches what's in the workflow.🤖 Generated with Claude Code
https://claude.ai/code/session_01WSt6ZYkUkUyDc28cHgwhDj