Skip to content

chore(security): drop inert .gitleaksignore and detect-private-key exclusions - #1311

Merged
IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-299
Oct 25, 2026
Merged

IAmJSD merged 2 commits into
LeanerCloud:mainfrom
IAmJSD:fix/issue-299

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Root cause

  • .gitleaksignore doesn't do anything. It holds one bare path, internal/secrets/aws_resolver_httptest_test.go, under two comment lines. gitleaks expects finding fingerprints and rejects a bare path as an invalid entry. More to the point, nothing in this repo has ever run gitleaks: no workflow, pre-commit hook, Makefile target or script. git grep -i gitleaks finds only a runbook to-do (.github/runbooks/credential-compromise.md:128) and the 2026-09-02 audit, and git log --all -S gitleaks finds only the commits that added those two documents. So the file suppresses nothing, yet reads as if it did. The issue counted two stale entries; on main there is one.
  • git-secrets is the real secret gate. It runs in the pre-commit workflow at 1.3.0, pinned by SHA, and scripts/setup-git-secrets.sh calls git secrets --register-aws. The built-in allow-list of that command already covers the AWS documentation example keys (AKIAIOSFODNN7EXAMPLE / wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY) in that test file. Deleting .gitleaksignore therefore changes no scan result.
  • The detect-private-key exclusions in .pre-commit-config.yaml covered nothing real. The hook excluded (_test\.go|frontend/src/index\.html)$, but no tracked file contains a private-key header. The exclusions exempted no existing file and would have let a key pasted into any Go test fixture through (audit A14-036).

Fix

  • Delete .gitleaksignore. The issue's acceptance criteria name this as the option when gitleaks isn't adopted.
  • Remove the exclude: from the detect-private-key hook, and replace the outdated comment with one that explains why there is no exclusion. It also says what a future test should do: build the PEM at runtime, as scripts/test-git-secrets-allowlist.sh already does, or add a narrow per-file exclude with a written reason.

No Go code, workflow or frontend/ file is touched. Removing the frontend/src/index.html pattern changes only the config file.

Closes #299

Verification

This is config and fixture evidence only. No Go code changed, so there are no Go tests.

  1. The real hook over every tracked file, after the change, with pre-commit 4.7.0, pre-commit-hooks v6.0.0:
    pre-commit run detect-private-key --all-files
    Detect private keys......................................................Passed
    
    It passed on unmodified main too, before the change. I ran this hook, not the offline grep alternative; I also ran the grep as a cross-check (item 3).
  2. Fail-before/pass-after for the exclusion, with a fixture in a throwaway mktemp -d git repo, deleted afterwards. The fixture is internal/x/fixture_test.go, which contains a -----BEGIN PRIVATE KEY----- block. I ran pre-commit run detect-private-key --all-files against the real config files:
    • main's .pre-commit-config.yaml (git show HEAD:.pre-commit-config.yaml): Passed, exit 0. The key in the test file got through.
    • this branch's .pre-commit-config.yaml: Failed with Private key found: internal/x/fixture_test.go, exit 1.
  3. Offline cross-check: git ls-files -z | xargs -0 grep -lE 'BEGIN (RSA |DSA |EC |OPENSSH |PGP |ENCRYPTED )?PRIVATE KEY|PuTTY-User-Key-File-2|BEGIN OpenVPN Static key V1' printed nothing.
  4. bash scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL, exit 0. I ran it with two git-secrets builds:
    • the CI-pinned 1.3.0 tag, cloned into a temp dir, with HEAD checked to equal GIT_SECRETS_SHA ad82d68ee924906a0401dfd48de5057731a9bc84 from pre-commit.yml;
    • a local checkout at 7d6b970c.
  5. No remaining references: git grep -n -F gitleaksignore -- ':!docs/audits' finds nothing, so no script, test or workflow reads the file.
  6. Config sanity: pre-commit run check-yaml --files .pre-commit-config.yaml and pre-commit run trailing-whitespace --files .pre-commit-config.yaml both passed.

Gaps: the full pre-commit run --all-files and the CI workflows were not run on this branch.

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ b56afb2b = origin/main a14dc8f2 plus these heads, merged in this order:

Branch Tested head
fix/issue-204 76f85a4
fix/issue-438 78fe011
fix/issue-838 120b07f
fix/issue-143 aef1256
fix/issue-349 cfbb551
fix/issue-80 63405f9
fix/issue-199 36b9f8f
fix/issue-41 96fc237
fix/issue-299 6af14ed
fix/issue-324 3ab6729

fix/issue-324 and fix/issue-204 both edit .github/workflows/ci.yml next to the azure-kv-access-policy job. The tree kept #324's new "Run Function URL reader self-tests" step and #204's updated job comment; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers. The full suite ran on ed0c7174, which differs from b56afb2b only by one .gitallowed line on fix/issue-324:

  • Passed: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
  • Static checks: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. gocyclo -over 10 is silent on the changed non-test files.
  • Integration tests, every changed package that has them, one package at a time (-tags integration -count=1, 60-minute local timeout): internal/api, internal/purchase, internal/scheduler and internal/server all pass.
  • Secrets: the first run caught a bare placeholder account ID in fix/issue-324's new tftest, now allowlisted with an anchored .gitallowed line. Re-run at b56afb2b: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
  • Nothing was applied to any cloud.

Review manually / alternatives

  • Maintainer's alternative: adopt gitleaks for real. That means a pinned, checksum-verified gitleaks install in .github/workflows/pre-commit.yml and a pre-commit hook. Then rebuild .gitleaksignore from gitleaks detect --report-format json fingerprints, or use a .gitleaks.toml allowlist for the AWS example keys. It would add a second secret scanner next to git-secrets, so it's a policy call. This PR takes the "delete the inert file" option from the issue's acceptance criteria instead.
  • Removing the _test.go exclusion means a future PEM test fixture needs a per-file exclusion, or must build the PEM at runtime as scripts/test-git-secrets-allowlist.sh does. Today no tracked file needs one, as verified above.
  • The issue counted two stale .gitleaksignore entries; on main there is one bare-path line under two comment lines.

Out of scope

  • docs/audits/codebase-audit-2026-09-02.md still mentions .gitleaksignore: in A14-036, and at line 10351, which lists it among scanner suppression files that "carry per-entry written justifications". The audit is a dated record, so I left it alone.
  • The runbook's to-do "Enable gitleaks pre-commit hook" (.github/runbooks/credential-compromise.md:128) is unchanged. It belongs with the adopt-gitleaks decision above.

Maintainer: please mirror the issue's triage labels onto this PR: bug, impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/xs.

🤖 Generated with Claude Code

…clusions

.gitleaksignore held a single bare path, which gitleaks does not accept as
an ignore entry (it expects finding fingerprints), and no workflow, hook,
Makefile target or script in this repo has ever run gitleaks. The file
suppressed nothing while reading as an active suppression. git-secrets is
the secret gate, and its --register-aws allow-list already covers the AWS
documentation example keys in internal/secrets/aws_resolver_httptest_test.go.

The detect-private-key hook excluded every _test.go file and
frontend/src/index.html. No tracked file contains a private-key header, so
both exclusions exempted nothing while letting a key pasted into any Go
test fixture past the gate. Remove them.

Closes LeanerCloud#299

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added impact/internal Team-internal only type/chore Maintenance / non-user-visible triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline effort/xs Trivial / one-liner labels Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 5bdd7fc5feab9e039df3353058f9d813d8e98f4c

Blocking:

  • none

Non-blocking:

  • With the _test\.go exclusion gone and CI's pre-commit job running pre-commit run --all-files (pre-commit.yml:300), any future PR that adds a PEM literal to a Go test fixture will fail CI. That is the intent of sec: gitleaks .gitleaksignore has invalid fingerprints (suppression not effective) [platform part] #299. Today no tracked file and no open PR diff contains such a literal (checked below), so nothing in flight breaks.
  • .github/runbooks/credential-compromise.md:128 still lists "Enable gitleaks pre-commit hook" as a to-do. That's fine to leave with the adopt-gitleaks decision, as the body says.

Attacked:

  • Scope against sec: gitleaks .gitleaksignore has invalid fingerprints (suppression not effective) [platform part] #299's acceptance criteria. The PR takes the "delete the inert .gitleaksignore" option and decides the _test\.go exclusion by removing it, so Closes #299 is justified. git grep -i gitleaks shows nothing reads the file: no workflow, hook, Makefile or script, only the runbook to-do and the dated audit.
  • Breakage: grepped every tracked file for each pattern the detect-private-key hook checks (RSA, DSA, EC, OPENSSH, PGP, ENCRYPTED, SSH2 and plain PRIVATE KEY headers, PuTTY, OpenVPN): no matches. Also scanned the gh pr diff of every open platform PR for added lines with those markers: no hits.
  • Fail-before / pass-after: in a throwaway git repo I added internal/x/fixture_test.go containing a BEGIN RSA PRIVATE KEY block. pre-commit run -c <origin/main config> detect-private-key --all-files gave (no files to check) Skipped, so the key got through. With this PR's config the same command gave Failed with Private key found: internal/x/fixture_test.go. The temp repo was deleted afterwards.
  • Ran: pre-commit run detect-private-key --all-files on the branch (Passed) and bash scripts/test-git-secrets-allowlist.sh (137 PASS, 0 FAIL). No Go, workflow or frontend files changed.

Body-stale:

  • none

Reviewer: rB6. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

This was referenced Oct 25, 2026
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 5bdd7fc5feab9e039df3353058f9d813d8e98f4c merged onto main (5aa3df5b), together with #1155, #1219, #1221, #1223, #1224, #1225, #1226, #1227, #1256, #1262, #1266, #1270, #1272, #1275, #1283, #1286, #1293, #1297, #1299, #1300, #1301, #1304, #1307, #1311, #1313, #1325 in that order (combined tree f3a3331f, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 25, 2026 20:32
@IAmJSD
IAmJSD merged commit 890e59d into LeanerCloud:main Oct 25, 2026
34 checks passed
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 5bdd7fc5, green CI, and the combined-tree verification above. Squash-merged at exactly 5bdd7fc5feab9e039df3353058f9d813d8e98f4c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sec: gitleaks .gitleaksignore has invalid fingerprints (suppression not effective) [platform part]

1 participant