chore(security): suppress validated false positives in code scanning#541
Merged
Conversation
Full triage of all 63 open GitHub code scanning alerts (CodeQL + gosec) found zero true positives in shipped code. This change makes the suppressions durable: - Convert //nolint:gosec annotations to #nosec GXXX form at 14 sites: standalone gosec (the CI SARIF job) ignores //nolint and only honors #nosec, which is why already-justified findings kept reappearing in the Security tab. golangci-lint's gosec honors #nosec too, so lint coverage is unchanged. - gosec CI job: exclude test/ and wstest/ (separate Go modules, never compiled into the server binary) so the Security tab stays scoped to shipped code. - CodeQL: path-ignore .tmi-clients/** (CI checkout of the generated client repo; its Debug-gated request dump is third-party generated code) and refresh the known-FP documentation for GORM map-based queries with the validated safe sub-patterns (ColumnMap/OrderByCol literal keys, static-SQL clause.Expr) plus a new note on the slogging clear-text-logging sink FPs. The 33 CodeQL false-positive alerts are being dismissed in GitHub with per-group justifications. Hardening follow-up for the attribute-only log-redaction gap is tracked in #540. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014VXEwai1f6hYn1Kd7BHepL
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Log redaction in internal/slogging is attribute-based: redactionHandler
matches slog.Attr keys and SanitizeLogMessage only strips control
characters, so a secret interpolated directly into a format string
(e.g. logger.Info("key=%s", cfg.LLMAPIKey)) would bypass redaction.
Add scripts/check-sensitive-log-args.py: scans non-test Go files under
api/, auth/, cmd/, internal/, and pkg/ for logger-style calls whose
argument span (multi-line aware, string-literal stripped) references a
sensitive config/credential field (Password, LLMAPIKey, ClientSecret,
etc.). len(x.Secret) is allowed as a safe derived value. Wired into
make lint (new check-sensitive-log-args target) and the Security &
Quality CI lint job.
Fixes #540
gosec associates an end-of-line comment on an `if ... {` line with the
block contents, not the if node where the violation is reported, so the
4 directives placed after `{` (G306 in genconfig/genconfigdocs, G115 in
ooxml_common) did not suppress and resurfaced as new alerts on the PR.
Directives on plain statements attach correctly and were unaffected.
Verified with standalone gosec v2.22.5 using the exact CI invocation:
0 issues repo-wide.
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
Full triage of all 63 open GitHub code scanning alerts (CodeQL + gosec) — every alert was validated against the actual code. Zero true positives in shipped code. This PR makes the suppressions durable so validated false positives stop reappearing in the Security tab, and closes the one latent gap the triage surfaced (#540):
//nolint:gosecto// #nosec GXXX -- <justification>. Standalone gosec (the CI SARIF job) ignores//nolintand only honors#nosec— which is why already-justified findings kept resurfacing. golangci-lint's gosec honors#nosectoo (verified:make lint→ 0 issues), so local lint coverage is unchanged.-exclude-dir=test -exclude-dir=wstest. Both are separate Go modules never compiled into the server binary; this keeps the Security tab scoped to shipped code and closes the 14 test-harness alerts on the next upload..tmi-clients/**(CI checkout of the generated client repo — its Debug-gatedhttputil.DumpRequestOutis stock OpenAPI-generator template code) and refresh the known-FP documentation for GORM map-based queries with the validated safe sub-patterns (ColumnMap/OrderByCol literal keys, static-SQLclause.Expr), plus a new note on the slogging clear-text-logging sink FPs.scripts/check-sensitive-log-args.pyfails the build when a sensitive config/credential field (Password, LLMAPIKey, ClientSecret, …) is passed to a logger call — attribute-based redaction cannot catch format-string interpolation. Multi-line-call aware, string-literal stripped,len(x.Secret)allowed as a safe derived value. Wired intomake lintand the CI lint job.Triage results
Verification
make lint— 0 issues, including the new check (confirms golangci-lint gosec honors the #nosec form)logger.Info("key=%s", cfg.LLMAPIKey)+ a multi-line violation → both detected; removed → cleanmake build-server— passmake test-unit— 24 packages passed, 0 failedFixes #540
🤖 Generated with Claude Code
https://claude.ai/code/session_014VXEwai1f6hYn1Kd7BHepL