feat: harden CodeQL findings gate + fix cookie-suppression comment placement - #1218
Merged
Wikid82 merged 7 commits intoAug 4, 2026
Merged
Conversation
…and backup handlers CodeQL's codeql[rule-id] inline suppression syntax requires a standalone comment line positioned exactly one line before the flagged statement. The go/cookie-secure-not-set comment in auth_handler.go was a same-line trailing comment on c.SetCookie(, and four go/log-injection comments in crowdsec_handler.go/backup_handler.go had their tagged line two lines above the flagged statement instead of one. Reposition all five to standalone lines at the correct offset; fold the auth_handler.go truth- table justification into setSecureCookie's own doc comment so it no longer cites the rotating docs/plans/current_spec.md plan file. No behavior/logic change anywhere - comment placement only, verified by the existing ~24-test auth_handler_test.go suite passing unmodified. A fresh SARIF scan after the placement fix still shows no suppressions key on the cookie finding (true for every result in the file, not just this one) - a local CodeQL CLI/query-pack limitation for this call shape, not a placement error. Registered as a dated, reviewable exception in .github/codeql/codeql-suppressions.yml instead. Closes out docs/issues/codeql-cookie-suppression-not-honored.md. Claude-Session: https://claude.ai/code/session_01GAy1VnK6QW7U19vMmDFZgZ
…ented exceptions Flip .github/security-severity-policy.yml's codeql.blocking_levels from [error] to [error, warning, note] so every CodeQL finding blocks by default, with a documented exceptions mechanism (.github/codeql/codeql-suppressions.yml or a native in-source suppression) as the only way out. Wire both scripts/pre-commit-hooks/codeql-check-findings.sh and .github/workflows/codeql.yml to call the shared scripts/security/codeql-findings-gate.sh instead of their own independently-duplicated inline jq, and add a parity assertion in scripts/ci/check-codeql-parity.sh so local and CI can't drift back apart. Also fixes a real bug found while verifying this end-to-end: local scans (--source-root=backend/frontend) produce module-relative SARIF paths while CI produces repo-root-relative paths matching the ignore-list's own convention, so a valid, non-expired exception that correctly matched CI never matched the identical finding locally. codeql-findings-gate.sh now normalizes SARIF paths to the repo-root-relative form before matching or printing, with a new bats fixture covering the exact scenario. Claude-Session: https://claude.ai/code/session_01GAy1VnK6QW7U19vMmDFZgZ
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
Contributor
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
…ecs and fixtures CodeQL's js/unused-local-variable and js/automatic-semicolon-insertion queries flagged dead imports, unused destructured variables, and a missing statement-terminating semicolon across the core/, integration/, fixtures/, and utils/ E2E test files. Removes genuinely dead code and fixes the ASI hazard with an explicit semicolon; a few spots (proxy toggle state, SSL indicator check) got real assertions instead of being silently unused. Claude-Session: https://claude.ai/code/session_01GAy1VnK6QW7U19vMmDFZgZ
…E specs Applies the same js/unused-local-variable cleanup to security-enforcement/ and security/ specs: dead auth/fixture imports, unused SELECTORS/mock objects that were never referenced, and unused destructured resource IDs from testData helper calls. A handful of spots got real assertions instead of a silent unused read (WAF toggle state actually flips, accessible-name checks are enforced, emergency-token status is verified unchanged). Claude-Session: https://claude.ai/code/session_01GAy1VnK6QW7U19vMmDFZgZ
…sks E2E specs Rounds out the js/unused-local-variable cleanup across settings/, tasks/, and the remaining misc specs (modal-dropdown-triage, uptime-monitoring, theme). Removes dead imports and unused destructured locals; a few spots gained real assertions where the unused read was clearly meant to check something (password-strength indicator, tab-order role check, WAF rule toggle, dropdown z-index result, scroll-container sizing) rather than just being deleted outright. Claude-Session: https://claude.ai/code/session_01GAy1VnK6QW7U19vMmDFZgZ
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
2 participants
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
go/cookie-secure-not-setfinding atauth_handler.go:191(already tracked indocs/issues/codeql-cookie-suppression-not-honored.md) was surfacing as a live warning because its// codeql[go/cookie-secure-not-set]suppression comment was placed as a trailing same-line comment — CodeQL's suppression syntax requires a standalone comment exactly one line before the alert. Repositioned at this site plus 3 more with the identical bug (crowdsec_handler.go×3,backup_handler.go×1). Comment-only change, zero behavior change; the underlying cookie logic (secure=false only for local/private-network origins over plain HTTP) was independently re-verified sound.suppressionsfield for this call shape — disclosed transparently rather than assumed fixed..github/codeql/codeql-suppressions.yml— the same pattern this repo already uses for Trivy via.trivyignore. This closes the gap that let the cookie finding ride silently tomainas a "non-blocking warning."scripts/security/codeql-findings-gate.sh.Test plan
batstests pass (scripts/security/tests/codeql-findings-gate.bats), including a regression case proving warning-level findings now correctly fail, and a path-normalization case for local-vs-CI SARIF path conventionslefthook run codeql(Go + JS): end-to-end pass, cookie finding shown asSUPPRESSEDwith dated justification (review by 2026-11-04); zero unsuppressed findingsNEW FINDING (no exception on file)) — proving this is a real behavior change, not just documentationscripts/ci/check-codeql-parity.shpasses, including a new assertion that local and CI reference the same shared gate scriptgo build ./...,go test ./internal/api/handlers/...passlefthook run pre-commit(shellcheck, semgrep, etc.) clean