fix: close open SSRF + TOTP vulnerabilities and unblock CI across the PR backlog - #374
fix: close open SSRF + TOTP vulnerabilities and unblock CI across the PR backlog#374arumes31 wants to merge 3 commits into
Conversation
…cklog Every open PR was failing the `build` job for the same reason: govulncheck flags GO-2026-5856 (crypto/tls ECH privacy leak) against the pinned Go 1.26.4 toolchain. Bump go.mod, the three workflows and the Dockerfile to 1.26.5, which is where that advisory is fixed. Verified locally: govulncheck now reports no vulnerabilities. SSRF (supersedes the intent of #313, #357, #372): those PRs validated the URL in AddExternalSource but left ExternalSourceService fetching it with a default http.Client, so DNS rebinding and inward redirects still reached internal endpoints on every scheduled refresh. Install the same guarded dialer the webhook path already uses (security.SafeSocketControl), bound redirect chasing, re-validate the stored URL on each refresh, and keep the input-time check as defense in depth. TOTP (supersedes the intent of #327, #354, #360, #363, #367): those PRs guarded GUIAdmin only. The broader flaw was that GetQR served *any* account's TOTP secret to any manage_admins holder, which lets the caller generate that account's codes indefinitely. Enrolment is self-service via the login flow, so scope GetQR to the caller's own account, refuse to render a QR for an unenrolled (empty) secret, block resetting GUIAdmin's TOTP, and require SudoMiddleware for resets as account deletion already does. Also bumps golang.org/x/text to v0.39.0 per the open Dependabot PRs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d bump Folds in the direct upgrades carried by the newest nightly PR (#371) and the outstanding Dependabot PRs: prometheus/client_golang 1.23.2 -> 1.24.1 (#365) and golang.org/x/crypto 0.53.0 -> 0.54.0 (#325), letting go mod tidy settle the transitive set. Build, vet, the full test suite and govulncheck all pass on the result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IsInternalIP missed address space that is not "private" by Go's classification but is routable inside many hosting environments, most notably RFC 6598 shared address space (100.64.0.0/10). The doc comment also claimed multicast coverage while only link-local multicast was actually checked. Cover CGNAT, "this network", IETF protocol assignments, the benchmarking range, limited broadcast, and all multicast, and fail closed on an address that cannot be classified. Unmap first so IPv4-mapped IPv6 forms cannot slip past the IPv4 blocks. This is the classifier behind both the webhook and external-source fetch paths, so it hardens the pre-existing webhook protection as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Work stopped as the fix was superseded by PR #374, which introduced a more robust architectural fix (scoping GetQR to the caller, enforcing SudoMiddleware). Co-authored-by: arumes31 <114224498+arumes31@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/api/handlers.go (1)
369-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a route-level sudo regression test.
The supplied tests call
ChangeAdminTOTPdirectly, so they do not verify this middleware chain. Cover a stale/no-sudo request being rejected and a fresh-sudo request reaching the handler.🤖 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 `@internal/api/handlers.go` around lines 369 - 371, Add a route-level regression test for the admin POST /change_totp route, exercising the SudoMiddleware chain rather than calling ChangeAdminTOTP directly. Verify stale or missing sudo credentials are rejected before reaching the handler, and fresh sudo credentials are allowed through to ChangeAdminTOTP.internal/api/excluded_handlers_test.go (1)
59-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest performs a live DNS lookup against
example.com.
AddExternalSourcecallssecurity.IsSafeURL, which resolves the hostname vianet.LookupIP. Since the URL here isn't an IP literal, this test depends on outbound DNS resolution succeeding (or gracefully timing out) rather than being fully hermetic — it won't flake on lookup failure today, but it adds real network I/O and potential latency to a unit test. Consider using a known-public IP literal (e.g.https://93.184.216.34/list.txt) to keep the test fully offline.🤖 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 `@internal/api/excluded_handlers_test.go` around lines 59 - 69, Update TestAPIHandler_AddExternalSource_AcceptsPublicURL to use a known-public IP-literal URL instead of the example.com hostname in the request and matching LogAction expectation, keeping the test’s accepted-URL behavior unchanged while eliminating DNS/network access.
🤖 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 `@internal/api/admin_handlers_test.go`:
- Around line 234-248: Verify the configured GetAdmin expectations in both tests
by adding pgRepo.AssertExpectations(t) after the status assertion in
internal/api/admin_handlers_test.go lines 234-248 and after the response
assertions in lines 250-265.
In `@internal/api/excluded_handlers.go`:
- Around line 301-318: Extend security.IsSafeURL to accept a context.Context and
use net.Resolver{}.LookupIPAddr(ctx, host) for DNS resolution. In
internal/api/excluded_handlers.go, derive a deadline-bounded context from
c.Request.Context() before calling IsSafeURL; in
internal/service/external_source_service.go, pass the existing ctx from
fetchAndParse. Update both call sites and preserve their current rejection
behavior.
---
Nitpick comments:
In `@internal/api/excluded_handlers_test.go`:
- Around line 59-69: Update TestAPIHandler_AddExternalSource_AcceptsPublicURL to
use a known-public IP-literal URL instead of the example.com hostname in the
request and matching LogAction expectation, keeping the test’s accepted-URL
behavior unchanged while eliminating DNS/network access.
In `@internal/api/handlers.go`:
- Around line 369-371: Add a route-level regression test for the admin POST
/change_totp route, exercising the SudoMiddleware chain rather than calling
ChangeAdminTOTP directly. Verify stale or missing sudo credentials are rejected
before reaching the handler, and fresh sudo credentials are allowed through to
ChangeAdminTOTP.
🪄 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 Plus
Run ID: 8f0cd2a8-084e-4225-a066-71fdfacab095
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.github/workflows/codeql.yml.github/workflows/go-ci.yml.github/workflows/release.yml.jules/sentinel.mdDockerfilego.modinternal/api/admin_handlers.gointernal/api/admin_handlers_test.gointernal/api/excluded_handlers.gointernal/api/excluded_handlers_test.gointernal/api/handlers.gointernal/security/ssrf.gointernal/security/ssrf_test.gointernal/service/external_source_service.gointernal/service/external_source_service_test.go
| func TestAPIHandler_GetQR_RejectsUnenrolledAccount(t *testing.T) { | ||
| h, _, pgRepo, _, _ := setupTest() | ||
|
|
||
| pgRepo.On("GetAdmin", "admin").Return(&models.AdminAccount{Username: "admin", Token: ""}, nil) | ||
|
|
||
| w := httptest.NewRecorder() | ||
| c, _ := gin.CreateTestContext(w) | ||
| c.Request, _ = http.NewRequest("GET", "/get_qr/admin", nil) | ||
| c.Params = gin.Params{{Key: "username", Value: "admin"}} | ||
| c.Set("username", "admin") | ||
|
|
||
| h.GetQR(c) | ||
|
|
||
| assert.Equal(t, http.StatusNotFound, w.Code) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the configured repository expectations.
These tests configure GetAdmin but never verify it was called, weakening the intended enrollment and credential-retrieval regression coverage.
internal/api/admin_handlers_test.go#L234-L248: addpgRepo.AssertExpectations(t)after the status assertion.internal/api/admin_handlers_test.go#L250-L265: addpgRepo.AssertExpectations(t)after the response assertions.
📍 Affects 1 file
internal/api/admin_handlers_test.go#L234-L248(this comment)internal/api/admin_handlers_test.go#L250-L265
🤖 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 `@internal/api/admin_handlers_test.go` around lines 234 - 248, Verify the
configured GetAdmin expectations in both tests by adding
pgRepo.AssertExpectations(t) after the status assertion in
internal/api/admin_handlers_test.go lines 234-248 and after the response
assertions in lines 250-265.
| if len(req.Name) > maxSourceNameLen || len(req.SourceType) > maxSourceNameLen { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Name and source_type must be at most 255 characters"}) | ||
| return | ||
| } | ||
| if len(req.URL) > maxSourceURLLen { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "URL must be at most 2048 characters"}) | ||
| return | ||
| } | ||
|
|
||
| // Reject URLs that point into internal address space before we ever store | ||
| // them. This is a pre-flight check only: it cannot survive DNS rebinding, so | ||
| // the refresh path re-validates and dials through a guarded socket as well. | ||
| if err := security.IsSafeURL(req.URL); err != nil { | ||
| zlog.Warn().Err(err).Str("actor", username.(string)).Msg("Rejected unsafe external source URL") | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid or unsafe URL"}) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(excluded_handlers\.go|external_source_service\.go|ssrf\.go)$' || true
echo "== relevant code =="
for f in internal/api/excluded_handlers.go internal/service/external_source_service.go internal/security/ssrf.go; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,220p' "$f" | nl -ba
fi
done
echo "== IsSafeURL usages =="
rg -n 'IsSafeURL|LookupIP|LookupIPAddr|ResolveIPAddr|go env ' internal || true
echo "== git diff stat/name-status for context =="
git diff --stat HEAD..origin/main 2>/dev/null || git diff --stat 2>/dev/null || true
git diff -- internal/api/excluded_handlers.go internal/service/external_source_service.go internal/security/ssrf.go 2>/dev/null | sed -n '1,220p' || trueRepository: arumes31/blocklist
Length of output: 1922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo status =="
pwd
git rev-parse --show-toplevel 2>/dev/null || true
git status --short 2>/dev/null | sed -n '1,40p' || true
echo "== candidate files =="
git ls-files | rg '(_excluded_handlers\.go|excluded_handlers\.go|external_source_service\.go|ssrf\.go|security.*\.go)$' || true
echo "== relevant code =="
for f in internal/api/excluded_handlers.go internal/service/external_source_service.go internal/security/ssrf.go; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f" 2>/dev/null || echo 0) lines) ---"
sed -n '1,220p' "$f" | nl -ba
fi
done
echo "== IsSafeURL and DNS calls =="
rg -n 'IsSafeURL|LookupIP|LookupIPAddr|ResolveIPAddr|context.Background|go env CGO_ENABLED' . || trueRepository: arumes31/blocklist
Length of output: 1922
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo status =="
pwd
git rev-parse --show-toplevel 2>/dev/null || true
git status --short 2>/dev/null | sed -n '1,60p' || true
echo "== candidate files =="
git ls-files 2>/dev/null | rg '(\.go$)' | sed -n '1,200p'
echo "== find target files =="
find . -type f -name 'excluded_handlers.go' -o -name 'external_source_service.go' -o -name 'ssrf.go' 2>/dev/null | sed -n '1,50p'
echo "== relevant code when files present =="
for f in internal/api/excluded_handlers.go internal/service/external_source_service.go internal/security/ssrf.go; do
if [ -f "$f" ]; then
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,220p' "$f" | nl -ba
fi
done
echo "== DNS/caller usages =="
rg -n 'IsSafeURL|func LookupIP|LookupIP|LookupIPAddr|ResolveIPAddr|context\.Background|net\.Resolver|cgo_enabled|CGO_ENABLED' . || trueRepository: arumes31/blocklist
Length of output: 1922
🌐 Web query:
Go net.LookupIP source context.Background LookupIPAddr source
💡 Result:
In Go's net package, the net.LookupIP(host string) function is a convenience wrapper that provides a simplified interface for DNS lookups [1][2]. Source Code Implementation: The function is implemented in src/net/lookup.go [3][4]. Its implementation uses context.Background() to perform the lookup because the function signature does not accept a user-provided context.Context [1][4]. Internally, net.LookupIP(host) calls DefaultResolver.LookupIPAddr(context.Background(), host) [3][1][5]. Key Differences: 1. Context Handling: net.LookupIP(host) uses context.Background() internally, meaning the caller cannot control the deadline or cancellation of the DNS request [1][4][2]. Conversely, (*net.Resolver).LookupIPAddr(ctx, host) and (*net.Resolver).LookupIP(ctx, network, host) require the caller to provide a context.Context, allowing for proper control over timeouts and cancellations [1][2]. 2. Return Types: net.LookupIP returns a slice of []net.IP [1][2]. Methods like (*net.Resolver).LookupIPAddr return a slice of []net.IPAddr, which includes both the IP address and, if applicable, the Zone identifier [3][2]. 3. Flexibility: (*net.Resolver).LookupIP(ctx, network, host) allows the caller to specify the network type ("ip", "ip4", or "ip6"), whereas the package-level net.LookupIP(host) defaults to looking up both IPv4 and IPv6 addresses [1][2]. Summary: The package-level net.LookupIP is intended for simple use cases where background-context lookups are acceptable [2]. For production applications requiring network control, timeout management, or cancellation, it is recommended to use an explicit *net.Resolver and its associated context-aware methods [2].
Citations:
- 1: https://go.googlesource.com/go/+/go1.16/src/net/lookup.go
- 2: https://pkg.go.dev/net@go1.26.3
- 3: https://go.dev/src/net/lookup.go?s=5598:5689
- 4: https://github.com/golang/go/blob/master/src/net/lookup.go
- 5: https://go.dev/src/net/lookup.go
🌐 Web query:
Go net package cgo getaddrinfo timeout cgo_resolver
💡 Result:
When using the Go net package with the cgo resolver (which calls C library routines like getaddrinfo), Go does not have direct control over the internal timeout behavior of those C functions [1][2]. Because getaddrinfo is a blocking C call, the Go runtime cannot easily intercept or enforce custom timeouts on the underlying system resolution process [3][4][1]. Key points regarding the cgo resolver and timeouts include: 1. Resolver Behavior: When cgo is enabled, the resolver uses the host's native system libraries [3]. These libraries often lack granular timeout or retry configuration interfaces exposed to the Go runtime [1]. Consequently, if the system DNS resolver hangs or experiences latency, the cgo call will block the underlying operating system thread, potentially impacting application performance [3][4]. 2. Configuration via Environment: Because the cgo resolver relies on the system's libc resolver, you can sometimes influence its behavior using standard system environment variables, such as RES_OPTIONS (e.g., setting RES_OPTIONS="timeout:2 attempts:3") [2]. These settings are specific to the libc implementation and are ignored by the pure Go resolver [2]. 3. Forcing the Pure Go Resolver: If you require fine-grained control over DNS timeouts, retries, or custom DNS server configuration (via net.Resolver), you should use the pure Go resolver [2][5]. You can force the use of the pure Go resolver in several ways: - At runtime: Set the environment variable GODEBUG=netdns=go [3][2]. - At build time: Use the -tags netgo build tag [3][6]. 4. Practical Considerations: The pure Go resolver supports context-aware DNS lookups (e.g., using net.Resolver.LookupHost with context.Context) and respects custom dialers, allowing for effective timeout management that is impossible with the cgo resolver [2][5]. If your application suffers from unexpected "no such host" errors or latency spikes caused by system DNS, switching to the pure Go resolver is the recommended approach to gain visibility and control over DNS behavior [1][2].
Citations:
- 1: net: LookupHost returns "no such host" when dns request timeout golang/go#10417
- 2: https://gist.github.com/p-sherratt/e326953e12cf2e6c74cb88484985e480
- 3: https://pkg.go.dev/net
- 4: https://go.dev/src/net/net.go
- 5: net: better docs for PreferGo in Resolver golang/go#35561
- 6: https://go.dev/src/net/conf.go
Add a timeout around security.IsSafeURL for request-bound DNS.
Both internal/api/excluded_handlers.go and internal/service/external_source_service.go call security.IsSafeURL synchronously, but net.LookupIP uses context.Background() and the cgo resolver has no bounded timeout. Extend IsSafeURL to accept a context.Context and resolve via net.Resolver{}.LookupIPAddr(ctx, host); pass a deadline derived from c.Request.Context() in the handler and the existing ctx in fetchAndParse. A stalled DNS lookup can otherwise pin a goroutine/OS thread with no cancellation path.
📍 Affects 2 files
internal/api/excluded_handlers.go#L301-L318(this comment)internal/service/external_source_service.go#L144-L150
🤖 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 `@internal/api/excluded_handlers.go` around lines 301 - 318, Extend
security.IsSafeURL to accept a context.Context and use
net.Resolver{}.LookupIPAddr(ctx, host) for DNS resolution. In
internal/api/excluded_handlers.go, derive a deadline-bounded context from
c.Request.Context() before calling IsSafeURL; in
internal/service/external_source_service.go, pass the existing ctx from
fetchAndParse. Update both call sites and preserve their current rejection
behavior.
1 participant
Supersedes the security and dependency backlog with one verified change. Closes the two vulnerabilities that were still open in
main, and unblocks CI for every other PR.Why every open PR was failing CI
The
buildjob fails for one reason that has nothing to do with the PRs' own code:govulncheckflags GO-2026-5856 (Encrypted Client Hello privacy leak incrypto/tls) against the pinned Go 1.26.4 toolchain. It is fixed in 1.26.5.This explains the pass/fail split exactly. PRs that happened to bundle a
1.26.4 -> 1.26.5bump showbuild=SUCCESS; those that did not — including all four Dependabot PRs and the two TOTP PRs that added tests — showbuild=FAILURE. #327 and #360 were never broken; they just lacked the bump.Bumped in
go.mod, the three workflows, and theDockerfile. Verified locally:govulncheck ./...now reports no vulnerabilities.SSRF in the external-source refresh (supersedes #313, #357, #372)
internal/security/ssrf.goalready existed and the webhook path used it correctly, butExternalSourceServicefetched operator-supplied URLs with a defaulthttp.Client.All three SSRF PRs validate the URL in
AddExternalSourceand leave the fetch path untouched. That does not close the hole:169.254.169.254.Because refresh runs on a schedule, this fires repeatedly rather than once.
Fix: dial through
security.SafeSocketControl(the same guard the webhook path already uses), which re-checks the address actually being dialed on every connection including redirects; bound redirect chasing; re-validate the stored URL on each refresh, since a row can predate validation or be repointed later. Input-time validation is kept as defense in depth, not as the only layer. Client injection follows the existingNewWebhookTaskHandlerWithClientidiom so tests can reach a loopback server.TOTP secret disclosure and 2FA reset (supersedes #327, #354, #360, #363, #367)
Those PRs all guard
GUIAdmin. The deeper flaw is thatGetQRserved any account's TOTP secret to any holder ofmanage_admins— and whoever reads that secret can generate that account's codes indefinitely.Enrollment is entirely self-service through the login flow (which already refuses to overwrite an enrolled token), so handing one account's QR to another user had no legitimate workflow behind it. The template already hid these buttons for
GUIAdmin, so the protection was client-side only while the API enforced nothing.Fix: scope
GetQRto the caller's own account; refuse to render a QR for an unenrolled (empty) secret; block clearingGUIAdmin's TOTP; and requireSudoMiddlewarefor resets, as account deletion already does.Shared SSRF classifier hardening
IsInternalIPmissed space that is not "private" by Go's classification but is routable in many hosting environments — most notably RFC 6598 shared address space (100.64.0.0/10). Its doc comment also claimed multicast coverage while only link-local multicast was checked.Now covers CGNAT, "this network", IETF protocol assignments, benchmarking, limited broadcast, and all multicast; unmaps first so IPv4-mapped IPv6 forms cannot slip past; and fails closed on an unclassifiable address. This is the classifier behind both fetch paths, so it hardens the pre-existing webhook protection too.
Dependencies
Consolidates the nightly backlog and the open Dependabot PRs into one verified bump:
prometheus/client_golang1.23.2 -> 1.24.1 (#365) andgolang.org/x/crypto0.53.0 -> 0.54.0 (#325), withgo mod tidysettling the transitive set.Rejected
#347 (
X-XSS-Protection: 1; mode=block) is not included. The XSS Auditor was removed from Chrome in 2019 and never existed in Firefox; OWASP recommends0because the auditor was itself exploitable. This app already sends a nonce-based CSP, so the header adds risk without benefit.#343 (
actions/setup-gov6 -> v7) is independent and left open; its CI should pass once this lands.Verification
go build,go vet, the full test suite (including the Docker/testcontainersrepositoryandapppackages) andgovulncheckall pass locally. New regression tests cover refresh-time SSRF rejection, unsafe/oversized source input, cross-account and unenrolledGetQR, andGUIAdminTOTP reset.Behavior change to be aware of
Restricting
GetQRto the caller's own account breaks no live UI —showQRis dead JavaScript, never wired to a button. But if you have an out-of-band process where an admin shows a new user their QR, that path is intentionally gone; those users enroll through the login flow instead.🤖 Generated with Claude Code
Summary by CodeRabbit
Security Enhancements
Maintenance