Reconsider and document redirect policy #518

Activity

1. thc1006 commented on Jul 13, 2026

   

   thc1006

   on Jul 13, 2026

   More actions

   Confirming the "Edit" in the issue body: yes — specifying a non-nil CheckRedirect removes Go's default 10-redirect cap, so Dial currently follows redirects with no limit.

   net/http applies the 10-cap only through defaultCheckRedirect, which it uses solely when Client.CheckRedirect == nil:

   func defaultCheckRedirect(req *Request, via []*Request) error {
       if len(via) >= 10 {
           return errors.New("stopped after 10 redirects")
       }
       return nil
   }

   Dial always installs a non-nil wrapper (v1.8.15, dial.go), and when the caller's CheckRedirect is nil the wrapper returns nil (follow) with no count check:

   newClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
       switch req.URL.Scheme {
       case "ws":  req.URL.Scheme = "http"
       case "wss": req.URL.Scheme = "https"
       }
       if oldCheckRedirect != nil {
           return oldCheckRedirect(req, via)
       }
       return nil
   }

   So a plain &http.Client{} (the common case) follows redirects unbounded — limited only by a context deadline, if the caller set one. And because net/http accumulates every prior request in via, a redirect loop also grows memory for its duration.

   Repro (v1.8.15, go1.26.5, linux/amd64):

   • A server that 302s 30 times then 404s → the client makes 31 requests and fails with expected handshake response status code 101 but got 404 (the final 404), not stopped after 10 redirects.
   • A server that always 302s to itself, with the dial bounded by a 400 ms context → several thousand requests (~5k) in 400 ms, stopped only by context deadline exceeded.

   const chainLen = 30 // > Go's default cap of 10
   var hits int32
   var srv *httptest.Server
   srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
       if int(atomic.AddInt32(&hits, 1)) <= chainLen {
           http.Redirect(w, r, srv.URL+"/r", http.StatusFound)
           return
       }
       http.Error(w, "end", http.StatusNotFound)
   }))
   defer srv.Close()
   
   ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
   defer cancel()
   _, _, _ = websocket.Dial(ctx, strings.Replace(srv.URL, "http://", "ws://", 1),
       &websocket.DialOptions{HTTPClient: &http.Client{}}) // nil CheckRedirect
   // atomic.LoadInt32(&hits) == 31, not ~11

   Alongside the "don't follow redirects by default" direction already proposed here, it may be worth having the wrapper preserve a bound when it wraps a nil caller callback — e.g. mirror defaultCheckRedirect's len(via) >= 10 — so the count limit isn't silently lost even if following-by-default is kept for compatibility.

   (Workaround for callers today: pass HTTPClient: &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} to refuse redirects at the handshake.)