security: strong API-key hashing + safe int conversion + workflow permissions (CodeQL)#792
Merged
Merged
Conversation
…missions (CodeQL) Fixes three CodeQL alerts: 1. go/weak-sensitive-data-hashing (store/api_keys.go) The existing hashKey implementation already uses crypto/sha256 with crypto/subtle.ConstantTimeCompare for all in-memory comparisons. Added explicit unit tests (TestHashKeyUsesSHA256, TestConstantTimeHashCompare) to act as regression guards: any future downgrade to MD5/SHA-1 will immediately fail CI via output-length assertions. 2. go/incorrect-integer-conversion (schema/reflect.go) parseDefault called strconv.ParseInt with bitSize=64 then cast the result to int, which silently overflows on 32-bit platforms. Added a math.MinInt/math.MaxInt bounds check before the cast; values outside int range fall through to the string representation. Added TestParseDefaultIntOverflow to cover boundary values. 3. actions/missing-workflow-permissions (.github/workflows/test-dispatch.yml) Added top-level `permissions: contents: read` least-privilege block. The job uses a caller-supplied PAT (REPO_DISPATCH_TOKEN) for the repository-dispatch action and gh CLI calls, so no GITHUB_TOKEN escalation is required at the job level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
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.
Summary
Fixes three GitHub CodeQL alerts.
1.
go/weak-sensitive-data-hashing(HIGH) —store/api_keys.goIssue: CodeQL flagged
hashKeyfor hashing a sensitive API key token.Finding: The implementation already uses
crypto/sha256(SHA-256) withcrypto/subtle.ConstantTimeComparefor all in-memory lookups — both the correct algorithm and the correct comparison primitive for a high-entropy random token (128 bits of randomness fromcrypto/rand). SHA-256 is appropriate here because this is a deterministic lookup hash (the raw key is a one-time secret; the hash is stored and later used to look up the record), not a password. A slow KDF (bcrypt/argon2) would not be appropriate since it would prevent efficient index lookups.Fix: Added
TestHashKeyUsesSHA256andTestConstantTimeHashCompareas regression guards. Any future regression to MD5 (32-char hex output) or SHA-1 (40-char hex output) will fail immediately at CI.Compatibility: No change to stored hashes. The algorithm was already SHA-256 — no migration needed.
2.
go/incorrect-integer-conversion(HIGH) —schema/reflect.goIssue:
parseDefaultcalledstrconv.ParseInt(s, 10, 64)and then cast theint64result directly toint. On 32-bit platformsintis 32 bits wide, so any parsed value in the range[math.MaxInt32+1, math.MaxInt64]silently overflows.Fix: Added a bounds check before the cast:
Values within
[math.MinInt, math.MaxInt](platform-nativeintbounds) are converted as before. Out-of-range values fall through to the string representation, which is the same behaviour the function applies to any value that fails both numeric parses.Added
TestParseDefaultIntOverflowcoveringMaxInt,MinInt, and the 32-bit overflow path.3.
actions/missing-workflow-permissions(MEDIUM) —.github/workflows/test-dispatch.ymlIssue: No
permissions:block was declared, so the workflow inherited the repository default token permissions (potentially broad write access).Fix: Added a top-level least-privilege
permissions: contents: readblock. Thepeter-evans/repository-dispatchstep andgh run listcall both useREPO_DISPATCH_TOKEN(a caller-supplied PAT) rather thanGITHUB_TOKEN, so noGITHUB_TOKENpermission escalation at the job level is required.Test plan
go build ./...— cleango test ./store/... ./schema/...— all passgo vet ./store/... ./schema/...— cleangofmt -l— no dirty filesgolangci-lint run ./store/... ./schema/...— 0 issuespython3 -c "import yaml; yaml.safe_load(...)"ontest-dispatch.yml— parses OK🤖 Generated with Claude Code