Skip to content

fix: synchronize remotestorage SSRF test-seam swap with atomic pointers - #1274

Merged
Wikid82 merged 4 commits into
developmentfrom
fix/remotestorage-ssrf-seam-race
Aug 20, 2026
Merged

fix: synchronize remotestorage SSRF test-seam swap with atomic pointers#1274
Wikid82 merged 4 commits into
developmentfrom
fix/remotestorage-ssrf-seam-race

Conversation

@Wikid82

@Wikid82 Wikid82 commented Aug 20, 2026

Copy link
Copy Markdown
Owner

Summary

  • go test -race in CI (surfaced on an unrelated PR, chore(deps): update dependency golangci/golangci-lint to v2.13.1 #1271) caught a genuine data race in internal/services/remotestorage's SSRF test seam. Confirmed pre-existing on development, unrelated to that PR.
  • Test helpers swap ssrfValidateHost/ssrfValidateDialAddress to permissive no-ops for the duration of a test and restore the originals in t.Cleanup. Production dial logic reads the active check from inside net.Dialer.Control, which net/http.Transport can invoke on a background goroutine that outlives the synchronous test call that spawned it — so the previous unsynchronized swap/restore raced against that goroutine's read.
  • Backs the swap with atomic.Pointer[func] instead of plain vars, and consolidates the two previously-duplicated swap/restore implementations onto one shared swapSSRFValidators helper. No production behavior, defaults, or exported signatures change.

Test plan

  • Pre-fix baseline reproduced the race (go test -race -run TestWebDAVUploader_InsecureSkipVerify_AllowsSelfSignedCert ./internal/services/remotestorage/... -count=5)
  • Post-fix: same command, zero DATA RACE, all PASS — reproduced independently twice
  • New required test TestSwapSSRFValidators_ConcurrentAccess_NoRace (concurrent stress, not just probabilistic -race -count=N luck)
  • go test -race ./internal/services/remotestorage/... ./internal/services/... -count=2remotestorage clean, zero DATA RACE
  • go build ./..., go vet ./internal/services/remotestorage/... clean
  • make lint-staticcheck-only — 0 issues; lefthook run pre-commit clean (ran on the real commit)
  • Coverage: 91.7% statement / 88.3% line (gate 87%); patch coverage 100% (18/18 changed lines)

Note

The broader -race sweep across sibling package internal/services surfaced a separate, pre-existing test-isolation bug (TestAuthService_Register UNIQUE-constraint collision under repeated runs, a nil-pointer panic in TestProxyHostService_DomainNamesRequired). Confirmed via git stash that it reproduces identically on unmodified development HEAD — not a race, not caused by this change, out of scope for this single-concern fix. Flagging for a follow-up issue rather than absorbing into this PR.

Test helpers substitute permissive SSRF checks for the duration of a
test, then restore the originals in t.Cleanup. Production dial logic
reads the active check from inside net.Dialer.Control, which
net/http.Transport can invoke on a background goroutine that outlives
the synchronous test call that spawned it, so the previous
unsynchronized swap/restore raced against that goroutine's read under
go test -race.

Back the swap with atomic.Pointer instead of plain vars, and
consolidate the two previously-duplicated swap/restore
implementations onto one shared helper. No production behavior,
defaults, or exported signatures change.
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@github-advanced-security

Copy link
Copy Markdown
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:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

✅ Supply Chain Verification Results

PASSED

📦 SBOM Summary

  • Components: 1753

🔍 Vulnerability Scan

Severity Count
🔴 Critical 0
🟠 High 0
🟡 Medium 5
🟢 Low 2
Total 11

📎 Artifacts

  • SBOM (CycloneDX JSON) and Grype results available in workflow artifacts

Generated by Supply Chain Verification workflow • View Details

@Wikid82
Wikid82 merged commit db654f6 into development Aug 20, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants