Skip to content

fix(notifications): restrict push endpoints to known push services and bound dispatcher requests - #153

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1788455983-push-endpoint-hardening
Open

fix(notifications): restrict push endpoints to known push services and bound dispatcher requests#153
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1788455983-push-endpoint-hardening

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Closes the blind-SSRF / dispatcher-stall hole in web push: any authenticated hacker could POST /v1/notifications/subscribe with an arbitrary endpoint (only validate:"url" applied), and the dispatcher would later POST to it with webpush-go's default &http.Client{} — no timeout, follows redirects, sequential loop — so one endpoint that accepts a TCP connection and never replies froze push delivery for the rest of the event.

Endpoint allowlist (cmd/api/push_endpoint.go)

validatePushEndpoint(raw string, allowedHosts []string) error
// https only, no userinfo, host == allowed || host ends with "."+allowed
  • Default hosts: fcm.googleapis.com, android.googleapis.com, push.services.mozilla.com, push.apple.com, notify.windows.com, push.samsungosp.com (Chrome/Edge/Brave/Opera, Firefox, Safari, WNS, Samsung Internet).
  • Overridable via new env PUSH_ENDPOINT_ALLOWED_HOSTS (comma-separated) → config.vapid.allowedEndpointHosts. Documented in .env.example.
  • subscribePushHandler returns 400 for anything off the list.
  • deliverNotification re-checks each stored sub before sending and prunes disallowed rows (covers rows written before this change) without ever contacting them. Pruned rows don't count toward the all-auth-failed mass-delete guard.

Bounded HTTP client (newPushHTTPClient, injected as webpush.Options.HTTPClient via app.pushClient)

  • Timeout: 10s (covers connect → body read), CheckRedirect returns http.ErrUseLastResponse, response body drain capped at 64 KiB.

Tests: dispatcher tests now use httptest.NewTLSServer with 127.0.0.1 allowlisted and the server's client injected. New cases: disallowed endpoints pruned with zero requests made, redirects not followed, hanging endpoint returns within the client timeout; handler tests cover http://, private/link-local IPs, look-alike hosts, userinfo, and legitimate subdomains (updates.push.services.mozilla.com, web.push.apple.com, *.notify.windows.com).

Verified: gofmt -l . clean, go vet, go test ./cmd/api/ ./internal/... pass.

Link to Devin session: https://app.devin.ai/sessions/7730c9bccc574233a0c2650ba204bec2
Open in Devin Desktop: https://app.devin.ai/desktop/session/7730c9bccc574233a0c2650ba204bec2?variant=devin
Requested by: @balebbae

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

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.

1 participant