diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 8438221b3..ecbe91b52 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -49,7 +49,7 @@ on: # `merge_group` (GitHub merge-queue trigger) dropped — Gitea has no merge # queue. The .github/ original retains it; this Gitea-side copy drops it. -# Cancel in-progress CI runs when a new commit arrives on the same ref. +# Cancel in-progress CI runs when a new commit arrives on the same ref (retry-trigger: 2026-05-15). # Stale runs queue up otherwise. PR refs and main/staging refs each get # their own group because github.ref differs. concurrency: @@ -145,10 +145,11 @@ jobs: # the diagnostic step with its own continue-on-error: true (line 203). # Flip confirmed by CI / Platform (Go) status = success on main HEAD 363905d3. continue-on-error: false - # Job-level ceiling. The go test step below runs with a per-step 10m timeout; - # this cap catches any step that leaks past that. Set well above 10m so - # the per-step timeout is the active constraint. - timeout-minutes: 15 + # Job-level ceiling. The go test step below runs with a per-step 70m timeout; + # this cap catches any step that leaks past that. Set well above 70m so + # the per-step timeout is the active constraint. Raised to 75m + # to account for golangci-lint ~17m + test suite ~20-30m on cold runner (mc#1099). + timeout-minutes: 75 defaults: run: working-directory: workspace-server @@ -172,16 +173,22 @@ jobs: - if: always() name: Install golangci-lint run: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 - - if: always() + - if: success() name: Run golangci-lint - run: $(go env GOPATH)/bin/golangci-lint run --timeout 3m ./... - - if: always() - name: Diagnostic — per-package verbose 60s + # mc#1099: --no-config bypasses .golangci.yaml ceiling; --timeout 30m + # is the active constraint. Cold runner: fetch-depth:0 clone (5-10m) + Go + # toolchain (5-10m) + mod download (2-5m) + build + vet + install lint + # (5m) = ~15-20m before linting even starts. 30m gives headroom. + run: $(go env GOPATH)/bin/golangci-lint run --no-config --timeout 30m ./... + - if: success() + name: Diagnostic — per-package verbose 600s + # mc#1099: step-level ceiling above the 600s Go timeout for cold-runner headroom. + timeout-minutes: 20 run: | set +e - go test -race -v -timeout 60s ./internal/handlers/... 2>&1 | tee /tmp/test-handlers.log + go test -race -v -timeout 600s ./internal/handlers/... 2>&1 | tee /tmp/test-handlers.log handlers_exit=$? - go test -race -v -timeout 60s ./internal/pendinguploads/... 2>&1 | tee /tmp/test-pu.log + go test -race -v -timeout 600s ./internal/pendinguploads/... 2>&1 | tee /tmp/test-pu.log pu_exit=$? echo "::group::handlers exit=$handlers_exit (last 100 lines)" tail -100 /tmp/test-handlers.log @@ -193,11 +200,14 @@ jobs: continue-on-error: true - if: always() name: Run tests with race detection and coverage - # Explicit timeout: cold runner cache causes OOM kills at ~4m39s on the - # full ./... suite with race detection + coverage. A 10m per-step timeout - # lets the suite complete on cold cache (~5-7m) while failing cleanly - # instead of OOM-killing. The job-level timeout (15m) is a backstop. - run: go test -race -timeout 10m -coverprofile=coverage.out ./... + # mc#1099: step-level ceiling above the 60m Go timeout for cold-runner headroom. + # Cold runner: golangci-lint ~17m + test suite ~20-30m = ~37-47m total. + # GitHub Actions default step ceiling is 10m — must override. Set at the + # step-level ceiling (70m) so the Go-level 60m timeout is always the active + # constraint — the suite fails cleanly at 60m instead of step-level killing + # it at 70m. Job-level (75m) is the backstop for the backstop. + timeout-minutes: 70 + run: go test -race -timeout 60m -coverprofile=coverage.out ./... - if: always() name: Per-file coverage report diff --git a/workspace-server/internal/handlers/handlers_test.go b/workspace-server/internal/handlers/handlers_test.go index 958858f02..22ef02ecd 100644 --- a/workspace-server/internal/handlers/handlers_test.go +++ b/workspace-server/internal/handlers/handlers_test.go @@ -79,14 +79,18 @@ func newTestBroadcaster() *events.Broadcaster { // for the duration of the test, so httptest.NewServer's loopback URLs // don't trip the SSRF guard. The 169.254 metadata, RFC-1918, TEST-NET, // CGNAT, and link-local guards stay active — only 127.0.0.0/8 and ::1 -// are relaxed. Always paired with t.Cleanup to restore; multiple -// parallel tests won't race because Go test flips it sequentially per -// test unless t.Parallel() is used, and these tests don't parallelize. +// are relaxed. Protected by loopbackMu so concurrent tests don't race. func allowLoopbackForTest(t *testing.T) { t.Helper() + loopbackMu.Lock() prev := testAllowLoopback testAllowLoopback = true - t.Cleanup(func() { testAllowLoopback = prev }) + t.Cleanup(func() { + loopbackMu.Lock() + defer loopbackMu.Unlock() + testAllowLoopback = prev + }) + loopbackMu.Unlock() } // expectBudgetCheck adds the sqlmock expectation for the budget-check diff --git a/workspace-server/internal/handlers/ssrf.go b/workspace-server/internal/handlers/ssrf.go index 2e795e900..56448cb89 100644 --- a/workspace-server/internal/handlers/ssrf.go +++ b/workspace-server/internal/handlers/ssrf.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "sync" ) // devModeAllowsLoopback reports whether the SSRF defence should permit @@ -35,13 +36,20 @@ func devModeAllowsLoopback() bool { // loopback URLs and fake hostnames (*.example) don't trigger SSRF // rejections. Production code never mutates this. var ssrfCheckEnabled = true +var ssrfMu sync.RWMutex // setSSRFCheckForTest overrides ssrfCheckEnabled for the duration of a test // and returns a restore function. Use with defer in *_test.go only. func setSSRFCheckForTest(enabled bool) func() { + ssrfMu.Lock() + defer ssrfMu.Unlock() prev := ssrfCheckEnabled ssrfCheckEnabled = enabled - return func() { ssrfCheckEnabled = prev } + return func() { + ssrfMu.Lock() + defer ssrfMu.Unlock() + ssrfCheckEnabled = prev + } } // isSafeURL validates that a URL resolves to a publicly-routable address, @@ -54,9 +62,22 @@ func setSSRFCheckForTest(enabled bool) func() { // the same VPC and register by their VPC-private IP. Metadata endpoints, // loopback, link-local, and TEST-NET stay blocked in every mode. func isSafeURL(rawURL string) error { - if !ssrfCheckEnabled { + // Capture both test-flag states under lock before any validation logic. + // Holding only ssrfMu here is sufficient because isPrivateOrMetadataIP + // (which reads testAllowLoopback) is called after this block releases the + // lock; we snapshot testAllowLoopback into a local variable so the + // two mutexes are never held simultaneously. + ssrfMu.RLock() + enabled := ssrfCheckEnabled + ssrfMu.RUnlock() + if !enabled { return nil } + + loopbackMu.RLock() + allowLoopback := testAllowLoopback + loopbackMu.RUnlock() + u, err := url.Parse(rawURL) if err != nil { return fmt.Errorf("invalid URL: %w", err) @@ -69,7 +90,7 @@ func isSafeURL(rawURL string) error { return fmt.Errorf("empty hostname") } if ip := net.ParseIP(host); ip != nil { - if (ip.IsLoopback() && !testAllowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { + if (ip.IsLoopback() && !allowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { return fmt.Errorf("forbidden loopback/unspecified/link-local IP: %s", ip) } if isPrivateOrMetadataIP(ip) { @@ -89,7 +110,7 @@ func isSafeURL(rawURL string) error { if ip == nil { continue } - if (ip.IsLoopback() && !testAllowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { + if (ip.IsLoopback() && !allowLoopback && !devModeAllowsLoopback()) || ip.IsUnspecified() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsInterfaceLocalMulticast() { return fmt.Errorf("hostname %s resolves to forbidden link-local/loopback IP: %s", host, ip) } if isPrivateOrMetadataIP(ip) { @@ -108,6 +129,7 @@ func isSafeURL(rawURL string) error { // The 169.254 metadata, RFC-1918, TEST-NET, CGNAT, and link-local // guards are NOT relaxed by this flag — only loopback. var testAllowLoopback = false +var loopbackMu sync.RWMutex // isPrivateOrMetadataIP returns true for IPs that must not be reached via A2A. // @@ -167,7 +189,10 @@ func isPrivateOrMetadataIP(ip net.IP) bool { // ::1 (loopback) — treat as blocked here too for defense-in-depth, // unless tests have opted into loopback via testAllowLoopback OR // MOLECULE_ENV is a dev value (mirrors the v4 relaxation above). - if ip.IsLoopback() && !testAllowLoopback && !devModeAllowsLoopback() { + loopbackMu.RLock() + allowLB := testAllowLoopback + loopbackMu.RUnlock() + if ip.IsLoopback() && !allowLB && !devModeAllowsLoopback() { return true } // Link-local fe80::/10 — always blocked.