ci: add blocking SAST gate (gosec, govulncheck, semgrep)#197
Merged
Conversation
Enforce SAST compliance in CI rather than via a bypassable pre-commit hook. Adds a `sast` job to ci.yml (parallel with lint/test, fail on medium+) plus `make tools`/`make sast` so local and CI runs are byte-identical. Establishes a clean baseline by annotating intentional, reviewed findings with justified gosec-native `#nosec` comments (InsecureSkipVerify opt-ins; lossless serialization conversions) and scoping the gosec scan to shipped product code (excludes dev/ops utilities under tools/). https://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
📝 WalkthroughWalkthroughThe PR introduces SAST infrastructure: pinned SAST tool installation and orchestration in the Makefile, a blocking ChangesSAST Security Scanning
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@Makefile`:
- Around line 128-130: The Makefile currently lets `make tools` succeed when
semgrep cannot be installed because the `|| echo ...` branch returns success;
update the failing branch so it returns a non-zero exit code (for example
replace the `|| echo ...` fallback with a shell block that prints the message
and exits non‑zero) so the `@command -v pipx ... && pipx install ... || ...`
command will fail the target when pipx/installation is unavailable; modify the
line containing the pipx check/installation (the shell command using `command -v
pipx` and `pipx install --force semgrep==$(SEMGREP_VERSION)`) to use a failing
fallback (e.g., `|| { echo "..."; exit 1; }` or an equivalent that writes to
stderr and returns non‑zero).
🪄 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: badc66ae-1e4d-4559-8ea6-be52446edcd5
📒 Files selected for processing (6)
.github/workflows/ci.ymlCLAUDE.mdMakefilehistory-service/internal/cassrepo/walker.gopkg/oidc/oidc.gopkg/searchengine/factory.go
The sast job failed at `make tools`: gosec v2.21.4 pins
golang.org/x/tools@v0.25.0, which fails to compile under any Go 1.25.x
("invalid array length -delta * delta"). Bump GOSEC_VERSION to v2.26.1
(Go 1.25-compatible) and pin GOTOOLCHAIN for reproducible tool builds.
v2.26.1's new G117 rule flags the intentional RoomKeyEvent.PrivateKey
serialization in roomkeysender (room-key distribution to the authorized
account over its auth-gated per-user subject) — suppressed with a
justified gosec-native annotation, consistent with the clean baseline.
Also fail `make tools` (instead of silently passing) when semgrep
cannot be installed and is not already on PATH (CodeRabbit review).
https://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
govulncheck (now running in the SAST gate) flagged reachable known stdlib advisories in net / net/mail / html/template, fixed in the May 2026 Go security release. Bump every build/CI pin to 1.25.10: go.mod go directive, ci.yml GO_VERSION, all service Dockerfiles and azure-pipelines.yml, and the SAST tool-build toolchain pin. Patches the actually-shipped binaries, not just the scanner's view. Validated: gosec builds + make lint + make sast-gosec all green under go1.25.10. https://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
auth-service/deploy/Dockerfile (1)
9-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the runtime container as a non-root user.
The final stage defaults to root, which weakens container isolation if the process is compromised. Add a dedicated unprivileged user/group before
ENTRYPOINT.Suggested hardening diff
FROM alpine:3.21 RUN apk add --no-cache ca-certificates +RUN addgroup -S app && adduser -S -G app app COPY --from=builder /auth-service /auth-service +USER app ENTRYPOINT ["/auth-service"]🤖 Prompt for 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. In `@auth-service/deploy/Dockerfile` around lines 9 - 12, The Dockerfile's final stage runs as root (default) which weakens isolation; update the final stage to create an unprivileged user/group (e.g., addgroup/adduser or adduser -D), chown the binary /auth-service to that user/group, switch to that user with the USER instruction before ENTRYPOINT, and ensure the binary has executable permissions; reference the Dockerfile's final stage, the /auth-service file and ENTRYPOINT to locate where to add the group/user creation, chown and USER directives.mock-user-service/deploy/Dockerfile (1)
13-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the runtime container as non-root.
Final stage has no
USER, so the service runs as root by default. Please drop privileges in the runtime image.Suggested patch
FROM alpine:3.21 RUN apk add --no-cache ca-certificates +RUN addgroup -S app && adduser -S -G app app COPY --from=builder /mock-user-service /mock-user-service +USER app ENTRYPOINT ["/mock-user-service"]🤖 Prompt for 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. In `@mock-user-service/deploy/Dockerfile` around lines 13 - 19, The final Dockerfile stage runs the binary as root because no USER is set; create a non-root user and group (e.g., app user), chown the copied binary (/mock-user-service) to that user, and add a USER instruction before ENTRYPOINT so the container drops privileges; update the Dockerfile final stage to create the user, ensure the binary is owned by that user, and use USER <username> prior to ENTRYPOINT ["/mock-user-service"] to run the service as non-root.
🤖 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.
Outside diff comments:
In `@auth-service/deploy/Dockerfile`:
- Around line 9-12: The Dockerfile's final stage runs as root (default) which
weakens isolation; update the final stage to create an unprivileged user/group
(e.g., addgroup/adduser or adduser -D), chown the binary /auth-service to that
user/group, switch to that user with the USER instruction before ENTRYPOINT, and
ensure the binary has executable permissions; reference the Dockerfile's final
stage, the /auth-service file and ENTRYPOINT to locate where to add the
group/user creation, chown and USER directives.
In `@mock-user-service/deploy/Dockerfile`:
- Around line 13-19: The final Dockerfile stage runs the binary as root because
no USER is set; create a non-root user and group (e.g., app user), chown the
copied binary (/mock-user-service) to that user, and add a USER instruction
before ENTRYPOINT so the container drops privileges; update the Dockerfile final
stage to create the user, ensure the binary is owned by that user, and use USER
<username> prior to ENTRYPOINT ["/mock-user-service"] to run the service as
non-root.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 741cf2b3-f9b2-4128-abfa-4411911014e4
📒 Files selected for processing (30)
.github/workflows/ci.ymlCLAUDE.mdMakefileauth-service/deploy/Dockerfileauth-service/deploy/azure-pipelines.ymlbroadcast-worker/deploy/Dockerfilebroadcast-worker/deploy/azure-pipelines.ymlgo.modhistory-service/deploy/Dockerfilehistory-service/deploy/azure-pipelines.ymlinbox-worker/deploy/Dockerfileinbox-worker/deploy/azure-pipelines.ymlmessage-gatekeeper/deploy/Dockerfilemessage-gatekeeper/deploy/azure-pipelines.ymlmessage-worker/deploy/Dockerfilemessage-worker/deploy/azure-pipelines.ymlmock-user-service/deploy/Dockerfilemock-user-service/deploy/azure-pipelines.ymlnotification-worker/deploy/Dockerfilenotification-worker/deploy/azure-pipelines.ymlpkg/roomkeysender/roomkeysender.goroom-service/deploy/Dockerfileroom-service/deploy/azure-pipelines.ymlroom-worker/deploy/Dockerfileroom-worker/deploy/azure-pipelines.ymlsearch-service/deploy/Dockerfilesearch-sync-worker/deploy/Dockerfilesearch-sync-worker/deploy/azure-pipelines.ymltools/loadgen/deploy/Dockerfiletools/nats-debug/deploy/Dockerfile
✅ Files skipped from review due to trivial changes (13)
- message-worker/deploy/azure-pipelines.yml
- search-service/deploy/Dockerfile
- tools/nats-debug/deploy/Dockerfile
- pkg/roomkeysender/roomkeysender.go
- history-service/deploy/azure-pipelines.yml
- notification-worker/deploy/azure-pipelines.yml
- message-gatekeeper/deploy/azure-pipelines.yml
- inbox-worker/deploy/azure-pipelines.yml
- auth-service/deploy/azure-pipelines.yml
- search-sync-worker/deploy/azure-pipelines.yml
- mock-user-service/deploy/azure-pipelines.yml
- room-service/deploy/azure-pipelines.yml
- room-worker/deploy/azure-pipelines.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
The sast job failed with only "exit code 2" visible — the failing scanner and its findings were not inspectable without signing in. Tee `make sast` to a log and, reusing the repo's test-integration pattern, write it to the step summary and surface the tail as an ::error:: annotation on failure. Add a per-scanner PASS/FAIL summary line to the `sast` make target so the failing scanner is unambiguous in the captured output. https://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
CodeRabbit flagged the alpine runtime stages running as root (CWE-250). Mirror the existing search-service Dockerfile convention (adduser -D -u 10001 app + USER app before ENTRYPOINT) across the other 13 service/tool images. search-service already complied and is left unchanged. Binaries are world-executable so no chown is needed; only nats-debug exposes a port (8090, unprivileged). https://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
Diagnosis from the now self-reporting sast annotation: gosec=PASS, govulncheck=PASS (Go 1.25.10 cleared the stdlib advisories), semgrep=FAIL — but a Python crash, not a finding: "ModuleNotFoundError: No module named 'pkg_resources'". semgrep 1.86.0 imports pkg_resources, which setuptools-less Python 3.12+ on ubuntu-latest no longer provides. Bump semgrep to 1.163.0 and `pipx inject semgrep setuptools` so pkg_resources is importable regardless of runner Python. https://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
semgrep (now running: gosec=PASS, govulncheck=PASS) flagged one real blocking finding — missing-ssl-minversion at pkg/oidc/oidc.go: the tls.Config set InsecureSkipVerify but no MinVersion. Add MinVersion: tls.VersionTLS12, matching the existing pkg/searchengine/factory.go convention. A genuine (minor) hardening gap fixed properly rather than suppressed. https://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
Merged
3 tasks
Joey0538
pushed a commit
that referenced
this pull request
May 19, 2026
Service containers run as non-root (uid 10001, since the runtime hardening in #197) and bind-mount docker-local/backend.creds read-only at /etc/nats/backend.creds. setup.sh wrote it 0600, so the in-container user got "permission denied" on `make up`. Generate it 0644 instead. Safe only because this is a throwaway local-dev credential created by this script; the .env file stays 0600 (read by the compose CLI on the host, never mounted).
Joey0538
added a commit
that referenced
this pull request
May 19, 2026
… frontend search contract (#201) * fix(search): serve search.rooms from the ES spotlight index search.rooms previously matched in ES then re-hydrated each room from the Mongo `subscriptions` collection. The spotlight index already carries roomId/roomName/roomType/siteId per (account, room), so the Mongo hop was redundant — it added a cross-store dependency, a round trip, and a consistency window for fields ES already has. Serve the response directly from the spotlight hit; drop HydrateRooms and the subscriptions collection (added solely for this — apps does its own $lookup). Add SiteID to model.SearchRoom. Frontend: correct the search.rooms / search.messages wire contract. The payload field is `query` (was incorrectly sending `searchText`), the rooms filter is `roomType` (was `scope`, and dropped the server-rejected `app` value), responses are `{messages,total}` / `{rooms}` (were `results`), and the room hit field is `name` (was `roomName`). Drop phantom fields; keep siteId now that the backend returns it. Update consumers, tests, and docs/client-api.md. * chore: trim redundant //nolint:gosec justification The `// #nosec G402 -- ...` line directly above already states the justification; the trailing `//nolint:gosec // ...` restated it in slightly different words. Both directives remain (standalone gosec and golangci-lint are independent mechanisms) — only the duplicated prose is removed. Addresses the pending simplify nit from #197. * refactor(search): drop unreachable nil guard; rename stale test simplify review: parseRooms always returns a non-nil slice (nil only with a non-nil error, handled above), so the `if rooms == nil` guard in searchRooms was dead defensive code — removed. Renamed TestSearchRoomsResponseJSON_EmptySubscriptions → _EmptyRooms now that the path no longer touches subscriptions. * test/docs: address CodeRabbit review on #201 - docs/client-api.md (search.rooms): fix stale #5 → #6 error-envelope anchor (every other ref uses #6); the `internal` reason no longer says "ES or MongoDB" — this endpoint is ES-only now. - SearchResultsPane.test.jsx: assert the full search.rooms wire payload {query,roomType,size}, not just query. - integration_test.go: seed a room owned by another account and assert it does not leak. With Mongo hydration removed, the spotlight userAccount term filter is the sole access boundary — this guards that regression directly. * ci: deepen base fetch in prewarm affected-detection The "Detect affected integration targets" step did `git fetch --depth=1 origin <base>` then a three-dot `origin/<base>...HEAD` diff. Three-dot needs a merge base, but a depth-1 base fetch has no history, so the step intermittently failed with "no merge base" (exit 128), blocking every downstream job. Drop --depth=1; checkout already uses fetch-depth: 0, so a full base fetch resolves the merge base reliably. * fix(local-dev): make backend.creds readable by non-root containers Service containers run as non-root (uid 10001, since the runtime hardening in #197) and bind-mount docker-local/backend.creds read-only at /etc/nats/backend.creds. setup.sh wrote it 0600, so the in-container user got "permission denied" on `make up`. Generate it 0644 instead. Safe only because this is a throwaway local-dev credential created by this script; the .env file stays 0600 (read by the compose CLI on the host, never mounted). --------- Co-authored-by: Claude <noreply@anthropic.com>
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
3 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
Sets up SAST compliance enforced in CI (not a bypassable pre-commit hook), gating on medium+ severity. Adds a
sastjob toci.ymlplusmake tools/make sastso local and CI runs are byte-identical.What this does
.github/workflows/ci.yml— newsastjob, parallel withlint/test(needs: prewarm). Runsmake toolsthenmake sast; fails the run on any medium+ gosec finding, any reachable govulncheck vuln, or any semgrep WARNING/ERROR.Makefile—make tools(pinnedgolangci-lint/gosec/govulncheckviago install,semgrepvia pipx),make sast(runs all three, aggregates exit code), andmake sast-gosec/sast-vuln/sast-semgrep. gosec scoped to shipped product code (-exclude-dir=tools,-tests=false,-exclude-generated).#noseccomments so the gate catches only new issues:pkg/oidc/oidc.go,pkg/searchengine/factory.go— the two opt-inInsecureSkipVerifytransports (already//nolint:gosecfor golangci; standalone gosec needs its own#nosec).history-service/internal/cassrepo/walker.go— three G115 false positives (losslessint64↔uint64framing round-trip + auint16(len)already bounded by the guard above it).make tools/make sastin the command table + a SAST guardrail note (documents that//nolint:gosecdoes not suppress standalone gosec).Verification
make sast-gosec— clean (0 issues;Nosec: 2confirms G402 suppressions work)make lint— 0 issues (golangci still honors the existing//nolint:gosec)make -n tools/make -n sastdry-run cleangovulncheck/semgrepnot live-validated in the authoring sandbox (egress tovuln.go.devblocked; container Python broken). Both run on GitHub-hosted runners — this PR's own CI run is the end-to-end verification.All source edits are comment-only — zero behavior change.
Follow-up (needs a human)
To make this actually block merges, add
sastas a required status check in branch-protection formain.Test plan
sastjob goes green on this PR (validates gosec + govulncheck + semgrep end-to-end on a real runner)lint/test/test-integrationunaffectedmake tools && make sastpasses on a machine withvuln.go.devreachablehttps://claude.ai/code/session_01ERurgA9KHk8wMUJi2dtYxf
Generated by Claude Code
Summary by CodeRabbit
Chores
Documentation