ci(pre-commit): pinned gosec hook on changed packages (closes #1374) - #1376
Conversation
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pre-commit configuration now delegates gosec scanning to a script that filters staged Go files, resolves owning modules and packages, ensures gosec v2.28.0 is available, and scans each affected module. A Terraform resource also receives a formatting-only change. ChangesChanged-package gosec scanning
Terraform formatting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PreCommit
participant GosecHook
participant GoModules
participant Gosec
PreCommit->>GosecHook: Pass staged filenames
GosecHook->>Gosec: Ensure pinned cached binary
GosecHook->>GoModules: Resolve owning module packages
GosecHook->>Gosec: Scan affected packages per module
Gosec-->>PreCommit: Return aggregated status
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Action performedReview finished.
|
|
PR #1382 bumped the CI pin to gosec v2.28.0. When this PR lands, please align the pin in |
|
Aligned in ea75569: the hook pin in |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/gosec-hook.sh`:
- Around line 40-42: Update the MODULE_DIRS declaration in scripts/gosec-hook.sh
to include tests/e2e as a recognized module root, preserving longest-prefix
ordering so staged files there are scanned relative to their standalone Go
module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 840851e4-c6c2-4081-8bb1-87ce8ec036a7
📒 Files selected for processing (2)
.pre-commit-config.yamlscripts/gosec-hook.sh
Address the Major CR finding on #1376 and the red pre-commit CI job. - scripts/gosec-hook.sh: add tests/e2e to MODULE_DIRS. It is a standalone Go module (tests/e2e/go.mod) but the hook only knew providers/* and pkg, so staged files there were scanned from the repo root as ./tests/e2e, breaking module/package resolution. Now mirrors the per-module loop in ci.yml (root, pkg, providers/{aws,azure,gcp}, tests/e2e). - terraform/environments/azure/ci-cd-permissions/sp.tf: strip the extra trailing blank line so the file ends with a single newline. The end-of-file-fixer pre-commit hook runs --all-files in CI and flagged this pre-existing whitespace on every PR. The pre-commit gocyclo failure is pre-existing repo-wide debt (four AWS service-client functions at complexity 11, below golangci's threshold of 15 but above the hook's -over 10). Not touched by this PR; tracked in the follow-up issue.
|
Pushed 21a58d5 addressing the red pre-commit CI job:
Also fixed the Major CR finding (tests/e2e module registration) - see thread reply. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Address the Major CR finding on #1376 and the red pre-commit CI job. - scripts/gosec-hook.sh: add tests/e2e to MODULE_DIRS. It is a standalone Go module (tests/e2e/go.mod) but the hook only knew providers/* and pkg, so staged files there were scanned from the repo root as ./tests/e2e, breaking module/package resolution. Now mirrors the per-module loop in ci.yml (root, pkg, providers/{aws,azure,gcp}, tests/e2e). - terraform/environments/azure/ci-cd-permissions/sp.tf: strip the extra trailing blank line so the file ends with a single newline. The end-of-file-fixer pre-commit hook runs --all-files in CI and flagged this pre-existing whitespace on every PR. The pre-commit gocyclo failure is pre-existing repo-wide debt (four AWS service-client functions at complexity 11, below golangci's threshold of 15 but above the hook's -over 10). Not touched by this PR; tracked in the follow-up issue.
21a58d5 to
5302dea
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Replace the whole-repo gosec invocation with a per-module, per-changed-package script (scripts/gosec-hook.sh) that: - Pins gosec v2.26.1 (matches securego/gosec SHA in ci.yml); auto-installs to ~/.cache/pre-commit-gosec/v2.26.1/gosec on first use via go install, detected via go version -m so the "dev" string from go install does not fool the version check. - Accepts staged .go file paths from pre-commit (pass_filenames: true), resolves each to its owning Go module (root / pkg / providers/aws / providers/azure / providers/gcp), then runs gosec once per affected module scanning only the packages that contain changed files. - Skips deleted files and testdata/ paths cleanly. - Same -exclude= rule list as the previous hook; keeps local and CI pre-commit verdicts aligned. - Exits nonzero on any finding; exits 0 when no live .go files are staged. Tested: clean pass on cmd/ + providers/aws/ in ~1.4 s; deliberate G501 (crypto/md5 import) correctly fails with exit 1; clean pass restored after removing the scratch file.
PR #1384 bumps the CI Security Scanning gosec pin to v2.28.0; align the pre-commit hook so local and CI verdicts stay in agreement. Updates the GOSEC_VERSION pin in scripts/gosec-hook.sh and the doc mentions in the hook config comment. Re-verified with v2.28.0: auto-install to the versioned cache dir works; clean pass on cmd/ + pkg/ + providers/aws in ~1.6 s warm; deliberate G501 (crypto/md5) scratch file fails with exit 1; clean pass restored after removal.
Address the Major CR finding on #1376 and the red pre-commit CI job. - scripts/gosec-hook.sh: add tests/e2e to MODULE_DIRS. It is a standalone Go module (tests/e2e/go.mod) but the hook only knew providers/* and pkg, so staged files there were scanned from the repo root as ./tests/e2e, breaking module/package resolution. Now mirrors the per-module loop in ci.yml (root, pkg, providers/{aws,azure,gcp}, tests/e2e). - terraform/environments/azure/ci-cd-permissions/sp.tf: strip the extra trailing blank line so the file ends with a single newline. The end-of-file-fixer pre-commit hook runs --all-files in CI and flagged this pre-existing whitespace on every PR. The pre-commit gocyclo failure is pre-existing repo-wide debt (four AWS service-client functions at complexity 11, below golangci's threshold of 15 but above the hook's -over 10). Not touched by this PR; tracked in the follow-up issue.
5302dea to
aabd957
Compare
|
Rebased onto main (post-#1364, Lint now green on main); should be fully green on this head. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Merged to main (closes #1374): a pinned (gosec v2.28.0), per-changed-package, module-aware pre-commit hook that surfaces gosec findings at commit time instead of first failing in CI. Verified it blocks a commit on a deliberate G501 finding and passes clean otherwise. First fully-green PR post-#1364 (all jobs green, no debt exception). Note: the hook currently applies a -exclude rule set that CI does not; aligning both to one shared source is tracked in #1394. |
The latest securego/gosec release is v2.28.0. Update the self-install in the Security Scanning job so CI runs the current version. PR #1376 (pre-commit gosec hook) is still open; a comment has been posted there asking to align its pin to v2.28.0 when it lands.
…#1382) (#1384) * ci: bump gosec pin from v2.26.1 to v2.28.0 The latest securego/gosec release is v2.28.0. Update the self-install in the Security Scanning job so CI runs the current version. PR #1376 (pre-commit gosec hook) is still open; a comment has been posted there asking to align its pin to v2.28.0 when it lands. * ci: retire 10 #nosec annotations obsolete in gosec v2.28.0 Tested each annotation by temporarily removing it and running gosec v2.28.0 on the owning module. Findings that no longer appear indicate the rule was removed or no longer triggers on the pattern. Retired (finding gone in v2.28.0): - G201 x6: postgres_analytics.go (x4) and analytics_postgres.go (x2) -- gosec v2.28.0 removed the G201 SQL-format-string rule entirely - G101 x2: email/templates.go -- gosec no longer flags email copy text containing the word "password" as a hardcoded credential - G101 x1: credentials/resolver.go:27 (azure_client_secret constant) -- gosec no longer flags this string; sibling GCP constants still flagged - G122 x1 (partial): deploy/frontend.go annotation updated from G304,G122 to G304 -- G122 is not a recognized rule in v2.28.0 Kept (finding still returned after removing the annotation): - G302,G304: pkg/common/audit.go -- 0644 file permission + path variable - G404: pkg/retry/exponential.go and providers/aws/recommendations/ratelimiter.go - G115, G101, G704, G703, G304, G204, G117, G505, G706, G104, G703, G705: all remaining annotations (verified per-module) Full verification: gosec v2.28.0 on all 6 modules -> 0 findings; go build ./... and go vet ./... clean; tests pass on all touched packages.
What / Why
Replaces the single whole-repo `gosec ./...` invocation in the pre-commit hook with a script (`scripts/gosec-hook.sh`) that:
Fail-test proof
Gate results
Notes
PR #1363 (in-flight) changes the CI `security-scan` job to loop all modules with gosec v2.26.1. This hook is already aligned: same version pin, same per-module approach, same exclusion list.
Summary by CodeRabbit