Skip to content

Commit d7745c3

Browse files
alnrclaude
andcommitted
fix: do not truncate proxy/tunnel responses after 10 seconds
The proxy and tunnel build their server with graceful.WithDefaults, which installs production-grade deadlines (10s write, 5s read). Any exchange slower than the write deadline was cut off mid-flight, so the client saw an empty reply rather than an error — indistinguishable from a crash. Read and write timeouts now default to "no limit" and are configurable via --read-timeout and --write-timeout. The override happens after WithDefaults because it only fills in zero values, so passing zero into it cannot express "no timeout". ReadHeaderTimeout is clamped down to ReadTimeout when smaller, since net/http prefers a non-zero ReadHeaderTimeout over ReadTimeout. Negative durations are rejected rather than silently meaning "no limit". Also adds responseStatusCatcher.Unwrap so http.ResponseController can reach the wrapped ResponseWriter. Without it the wrapper hid the underlying Hijacker and Flusher from every proxied response, breaking WebSocket upgrades and streaming — which removing the write deadline now makes possible in the first place. Closes #302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JYzGVwAKQ4ormxRHZg1eDu
1 parent 107a5e9 commit d7745c3

2 files changed

Lines changed: 233 additions & 4 deletions

File tree

cmd/cloudx/proxy/helpers.go

Lines changed: 65 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ const (
5454
AdditionalCORSHeadersFlag = "additional-cors-headers"
5555
RewriteHostFlag = "rewrite-host"
5656
APIKeyExpiryFlag = "api-key-expiry"
57+
ReadTimeoutFlag = "read-timeout"
58+
WriteTimeoutFlag = "write-timeout"
5759
)
5860

5961
// defaultAPIKeyExpiry is the default lifetime of the temporary API key the
@@ -80,6 +82,11 @@ type config struct {
8082
// automatically should that cleanup fail. A value of zero disables expiry.
8183
apiKeyExpiry time.Duration
8284

85+
// readTimeout and writeTimeout bound how long a request may take to be read
86+
// and answered. Zero means no limit, which is the default: the upstream may
87+
// legitimately take a long time to respond. See newServer.
88+
readTimeout, writeTimeout time.Duration
89+
8390
// rewriteHost means the host header will be rewritten to the upstream host.
8491
// This is useful in cases where upstream resolves requests based on Host.
8592
rewriteHost bool
@@ -100,6 +107,52 @@ func registerConfigFlags(conf *config, flags *pflag.FlagSet) {
100107
flags.BoolVar(&conf.isDebug, DebugFlag, false, "Use this flag to debug, for example, CORS requests.")
101108
flags.BoolVar(&conf.rewriteHost, RewriteHostFlag, false, "Use this flag to rewrite the host header to the upstream host.")
102109
flags.DurationVar(&conf.apiKeyExpiry, APIKeyExpiryFlag, defaultAPIKeyExpiry, "Sets the expiry of the temporary API key the Ory CLI creates to configure your project. The key is deleted on shutdown; this expiry ensures it is removed automatically if that cleanup fails. Set to 0 to disable expiry.")
110+
flags.DurationVar(&conf.readTimeout, ReadTimeoutFlag, 0, "Maximum duration for reading an entire request, including the body. Set to 0 for no limit.")
111+
flags.DurationVar(&conf.writeTimeout, WriteTimeoutFlag, 0, "Maximum duration before timing out writes of the response. Set to 0 for no limit.")
112+
}
113+
114+
// newServer builds the HTTP server the proxy and tunnel commands listen on. It
115+
// pairs graceful.WithDefaults with the timeout overrides below, so that no
116+
// call site can pick up graceful's deadlines by accident.
117+
//
118+
// graceful.WithDefaults installs production-grade deadlines (10s write, 5s
119+
// read). Those are wrong here: the proxy fronts the developer's own
120+
// application and the tunnel fronts Ory Network, and any exchange slower than
121+
// the write deadline is cut off mid-flight, so the client sees an empty reply
122+
// rather than an error — indistinguishable from a crash.
123+
//
124+
// The override has to happen after WithDefaults because WithDefaults only fills
125+
// in zero values, so it is impossible to express "no timeout" by passing zero
126+
// into it.
127+
//
128+
// With the default of "no timeout" only the header phase (ReadHeaderTimeout)
129+
// and idle keep-alive connections (IdleTimeout) stay bounded: a client that
130+
// dribbles a request body, or that never reads the response, can hold a
131+
// connection open indefinitely. That is an acceptable trade-off for a local
132+
// development tool, and --read-timeout/--write-timeout bring the bounds back
133+
// for anyone who needs them.
134+
func newServer(addr string, handler http.Handler, conf *config) (*http.Server, error) {
135+
// net/http treats a negative timeout as "no timeout", which silently does
136+
// the opposite of what someone passing one asked for.
137+
if conf.readTimeout < 0 {
138+
return nil, errors.Errorf("--%s must not be negative, use 0 to disable the timeout", ReadTimeoutFlag)
139+
}
140+
if conf.writeTimeout < 0 {
141+
return nil, errors.Errorf("--%s must not be negative, use 0 to disable the timeout", WriteTimeoutFlag)
142+
}
143+
144+
srv := graceful.WithDefaults(&http.Server{Addr: addr, Handler: handler})
145+
srv.ReadTimeout = conf.readTimeout
146+
srv.WriteTimeout = conf.writeTimeout
147+
148+
// A non-zero ReadHeaderTimeout wins over ReadTimeout in net/http, so a
149+
// --read-timeout below graceful's header deadline would not be enforced
150+
// while the headers are still arriving.
151+
if srv.ReadTimeout > 0 && srv.ReadTimeout < srv.ReadHeaderTimeout {
152+
srv.ReadHeaderTimeout = srv.ReadTimeout
153+
}
154+
155+
return srv, nil
103156
}
104157

105158
func portFromEnv() int {
@@ -120,6 +173,14 @@ func (r *responseStatusCatcher) WriteHeader(status int) {
120173
r.ResponseWriter.WriteHeader(status)
121174
}
122175

176+
// Unwrap lets http.ResponseController reach the wrapped ResponseWriter.
177+
// Without it this wrapper hides the underlying Hijacker and Flusher, and
178+
// httputil.ReverseProxy needs both: Hijack for protocol upgrades (WebSockets)
179+
// and Flush for streaming responses.
180+
func (r *responseStatusCatcher) Unwrap() http.ResponseWriter {
181+
return r.ResponseWriter
182+
}
183+
123184
func runReverseProxy(ctx context.Context, h *client.CommandHelper, stdErr io.Writer, conf *config, name string) error {
124185
signer, key, err := newJWTSigner()
125186
if err != nil {
@@ -236,10 +297,10 @@ func runReverseProxy(ctx context.Context, h *client.CommandHelper, stdErr io.Wri
236297
Debug: conf.isDebug,
237298
})
238299

239-
server := graceful.WithDefaults(&http.Server{
240-
Addr: addr,
241-
Handler: ch.Handler(mw),
242-
})
300+
server, err := newServer(addr, ch.Handler(mw), conf)
301+
if err != nil {
302+
return err
303+
}
243304

244305
if conf.isTunnel {
245306
_, _ = fmt.Fprintf(stdErr, `To access Ory's APIs, use URL

cmd/cloudx/proxy/helpers_test.go

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,185 @@
44
package proxy
55

66
import (
7+
"bufio"
8+
"io"
9+
"net"
710
"net/http"
811
"net/http/httputil"
912
"net/url"
1013
"testing"
14+
"time"
1115

1216
"github.com/stretchr/testify/assert"
1317
"github.com/stretchr/testify/require"
1418

19+
"github.com/ory/graceful"
1520
"github.com/ory/x/proxy"
1621
)
1722

23+
func TestNewServer(t *testing.T) {
24+
build := func(t *testing.T, conf *config) *http.Server {
25+
t.Helper()
26+
srv, err := newServer("", http.NewServeMux(), conf)
27+
require.NoError(t, err)
28+
return srv
29+
}
30+
31+
t.Run("case=defaults to no read or write timeout", func(t *testing.T) {
32+
srv := build(t, &config{})
33+
34+
// graceful.WithDefaults would otherwise impose 10s write / 5s read,
35+
// truncating any slower exchange with an empty reply.
36+
assert.Zero(t, srv.WriteTimeout)
37+
assert.Zero(t, srv.ReadTimeout)
38+
39+
// Headers must still arrive promptly and idle connections must still be
40+
// recycled.
41+
assert.Equal(t, graceful.DefaultReadHeaderTimeout, srv.ReadHeaderTimeout)
42+
assert.Equal(t, graceful.DefaultIdleTimeout, srv.IdleTimeout)
43+
})
44+
45+
t.Run("case=honors explicitly configured timeouts", func(t *testing.T) {
46+
srv := build(t, &config{readTimeout: 7 * time.Second, writeTimeout: 3 * time.Second})
47+
48+
assert.Equal(t, 7*time.Second, srv.ReadTimeout)
49+
assert.Equal(t, 3*time.Second, srv.WriteTimeout)
50+
assert.Equal(t, graceful.DefaultReadHeaderTimeout, srv.ReadHeaderTimeout)
51+
})
52+
53+
t.Run("case=a read timeout below the header deadline lowers it", func(t *testing.T) {
54+
// net/http prefers a non-zero ReadHeaderTimeout over ReadTimeout, so the
55+
// header deadline has to come down too or the configured limit is
56+
// silently exceeded while the headers arrive.
57+
srv := build(t, &config{readTimeout: time.Second})
58+
59+
assert.Equal(t, time.Second, srv.ReadTimeout)
60+
assert.Equal(t, time.Second, srv.ReadHeaderTimeout)
61+
})
62+
63+
t.Run("case=rejects negative timeouts", func(t *testing.T) {
64+
// net/http reads a negative timeout as "no timeout", the opposite of
65+
// what the caller asked for.
66+
_, err := newServer("", http.NewServeMux(), &config{readTimeout: -time.Second})
67+
assert.ErrorContains(t, err, ReadTimeoutFlag)
68+
69+
_, err = newServer("", http.NewServeMux(), &config{writeTimeout: -time.Second})
70+
assert.ErrorContains(t, err, WriteTimeoutFlag)
71+
})
72+
}
73+
74+
// TestServerWriteTimeout is the regression test for
75+
// https://github.com/ory/cli/issues/302: a response slower than the write
76+
// deadline used to be cut off mid-flight, leaving the client with an empty
77+
// reply and no error to go on.
78+
func TestServerWriteTimeout(t *testing.T) {
79+
const body = "this took a while"
80+
81+
// Slower than the configured deadline in the bounded case below, but still
82+
// fast enough to keep the test quick.
83+
const handlerDelay = 300 * time.Millisecond
84+
85+
for _, tc := range []struct {
86+
name string
87+
writeTimeout time.Duration
88+
expectBody bool
89+
}{
90+
{name: "case=slow response survives the default config", writeTimeout: 0, expectBody: true},
91+
{name: "case=slow response is cut off when a write timeout is set", writeTimeout: 50 * time.Millisecond, expectBody: false},
92+
} {
93+
t.Run(tc.name, func(t *testing.T) {
94+
mux := http.NewServeMux()
95+
mux.HandleFunc("/fast", func(w http.ResponseWriter, _ *http.Request) {
96+
_, _ = io.WriteString(w, body)
97+
})
98+
mux.HandleFunc("/slow", func(w http.ResponseWriter, _ *http.Request) {
99+
time.Sleep(handlerDelay)
100+
_, _ = io.WriteString(w, body)
101+
})
102+
103+
// Built exactly as runReverseProxy builds its server.
104+
srv, err := newServer("127.0.0.1:0", mux, &config{writeTimeout: tc.writeTimeout})
105+
require.NoError(t, err)
106+
107+
ln, err := net.Listen("tcp", srv.Addr)
108+
require.NoError(t, err)
109+
110+
served := make(chan struct{})
111+
go func() { defer close(served); _ = srv.Serve(ln) }()
112+
t.Cleanup(func() {
113+
_ = srv.Close()
114+
_ = ln.Close()
115+
<-served
116+
})
117+
118+
// Give every request its own connection: a cut-off response on a
119+
// pooled one would be silently retried by the transport.
120+
cl := &http.Client{Timeout: 10 * time.Second, Transport: &http.Transport{DisableKeepAlives: true}}
121+
t.Cleanup(cl.CloseIdleConnections)
122+
baseURL := "http://" + ln.Addr().String()
123+
124+
// Positive control: the server is up and answering, so a failure
125+
// below can only come from the write deadline.
126+
requireBody(t, cl, baseURL+"/fast", body)
127+
128+
if !tc.expectBody {
129+
_, err := cl.Get(baseURL + "/slow")
130+
require.Error(t, err, "server should have cut the response off")
131+
return
132+
}
133+
requireBody(t, cl, baseURL+"/slow", body)
134+
})
135+
}
136+
}
137+
138+
type recordingResponseWriter struct {
139+
header http.Header
140+
flushed, hijacked bool
141+
}
142+
143+
func (f *recordingResponseWriter) Header() http.Header { return f.header }
144+
func (f *recordingResponseWriter) Write(b []byte) (int, error) { return len(b), nil }
145+
func (f *recordingResponseWriter) WriteHeader(int) {}
146+
func (f *recordingResponseWriter) Flush() { f.flushed = true }
147+
func (f *recordingResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
148+
f.hijacked = true
149+
return nil, nil, nil
150+
}
151+
152+
// TestResponseStatusCatcher guards the ResponseWriter wrapper the proxy puts in
153+
// front of every response. httputil.ReverseProxy reaches the underlying writer
154+
// through http.ResponseController: Flush for streaming responses, Hijack for
155+
// protocol upgrades such as WebSockets. A wrapper that does not expose Unwrap
156+
// hides both, and the upgrade fails with "can't switch protocols using
157+
// non-Hijacker ResponseWriter type".
158+
func TestResponseStatusCatcher(t *testing.T) {
159+
inner := &recordingResponseWriter{header: http.Header{}}
160+
catcher := &responseStatusCatcher{ResponseWriter: inner}
161+
rc := http.NewResponseController(catcher)
162+
163+
require.NoError(t, rc.Flush())
164+
assert.True(t, inner.flushed, "flush must reach the wrapped ResponseWriter")
165+
166+
_, _, err := rc.Hijack()
167+
require.NoError(t, err)
168+
assert.True(t, inner.hijacked, "hijack must reach the wrapped ResponseWriter")
169+
170+
catcher.WriteHeader(http.StatusTeapot)
171+
assert.Equal(t, http.StatusTeapot, catcher.status)
172+
}
173+
174+
func requireBody(t *testing.T, cl *http.Client, url, expected string) {
175+
t.Helper()
176+
177+
res, err := cl.Get(url)
178+
require.NoError(t, err)
179+
defer res.Body.Close()
180+
181+
got, err := io.ReadAll(res.Body)
182+
require.NoError(t, err)
183+
assert.Equal(t, expected, string(got))
184+
}
185+
18186
func TestReqMiddleware(t *testing.T) {
19187
oryURL := &url.URL{Scheme: "https", Host: "example.projects.oryapis.com"}
20188
publicURL := &url.URL{Scheme: "http", Host: "localhost:4000"}

0 commit comments

Comments
 (0)