Skip to content

fix: stop following redirects on webhook deliveries - #1175

Open
alexluong wants to merge 3 commits into
mainfrom
fix/webhook-no-redirects
Open

alexluong wants to merge 3 commits into
mainfrom
fix/webhook-no-redirects

Conversation

@alexluong

Copy link
Copy Markdown
Collaborator

Webhook deliveries no longer follow redirects. A 3xx response is a failed attempt with its status code, retried like any other failure.

Until now the webhook client used Go's default redirect policy and followed up to 10 hops. On a 307 or 308 it re-sent the event to the new URL. On a 301, 302 or 303 it switched to a GET without the body, so the new URL never received the event, yet its 2xx was recorded as a successful delivery. A 3xx the client doesn't follow (300, 304, or one without a Location header) was also recorded as a success. The docs say an attempt fails on any non-2xx status; redirects now match that.

  1. The webhook HTTP client returns the first response instead of following Location.
  2. Any status >= 300 is a failed attempt, with the same code, response capture and retries as 4xx/5xx.
  3. The portal's destination metrics chart shows 3xx codes as errors.
  4. The webhook destination docs say redirects aren't followed.

Other: the redirect_error network error code is removed; it can no longer occur.

Behavior change (release notes): endpoints that answer with a redirect now fail. Common cases are an http:// URL redirected to https:// and a missing or extra trailing slash. Update the destination URL to the final address. These attempts are retried and, when enabled, count toward auto-disable.

🤖 Generated with Claude Code

if err != nil {
return nil, err
}
httpClient.CheckRedirect = func(*http.Request, []*http.Request) error {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexluong I think not following redirects is the right default, and it matches the Standard Webhooks spec (a 3xx is a failure). It's effectively a breaking change though: anyone whose endpoint redirects today (http to https, a trailing slash) starts failing after the upgrade, and those failures count toward auto-disable. Can we flag it as breaking in the release notes and upgrade guide?

Product question too: are we OK shipping with no option to follow redirects? I don't think a product like Outpost should be following them, but it's your call.

This branch has not been deployed

No deployments
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