Skip to content

Commit eae42d2

Browse files
authored
Fix off-by-one in WithHTTPGetMaxURLSize URL length check (#955)
The `WithHTTPGetMaxURLSize` option's docs describe an inclusive limit: it "sets the maximum allowable URL length", and the POST fallback applies when the URL "would be longer than the configured maximum value". The implementation uses a strict less-than, so a URL of exactly the configured size is downgraded to POST, or fails with the self-contradictory message when the fallback is disabled: "url size 4096 exceeds getURLMaxBytes 4096" This changes both comparisons in `marshalWithGet` to use less-than-or-equal and adds a regression test that asserts GET at the exact limit and POST one byte over.
1 parent 2cfd789 commit eae42d2

2 files changed

Lines changed: 49 additions & 2 deletions

File tree

client_ext_test.go

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,53 @@ func TestGetNoContentHeaders(t *testing.T) {
228228
assert.Equal(t, http.MethodGet, unaryReq.HTTPMethod())
229229
}
230230

231+
type urlSizeRecordingTransport struct {
232+
base http.RoundTripper
233+
urlLen int
234+
}
235+
236+
func (t *urlSizeRecordingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
237+
t.urlLen = len(req.URL.String())
238+
return t.base.RoundTrip(req)
239+
}
240+
241+
func TestGetURLSizeBoundary(t *testing.T) {
242+
t.Parallel()
243+
244+
mux := http.NewServeMux()
245+
mux.Handle(pingv1connect.NewPingServiceHandler(&pingServer{}))
246+
server := memhttptest.NewServer(t, mux)
247+
ctx := t.Context()
248+
call := func(httpClient connect.HTTPClient, options ...connect.ClientOption) (*connect.Request[pingv1.PingRequest], error) {
249+
client := pingv1connect.NewPingServiceClient(
250+
httpClient,
251+
server.URL(),
252+
append([]connect.ClientOption{connect.WithHTTPGet()}, options...)...,
253+
)
254+
request := connect.NewRequest(&pingv1.PingRequest{Text: "boundary"})
255+
_, err := client.Ping(ctx, request)
256+
return request, err
257+
}
258+
259+
transport := &urlSizeRecordingTransport{base: server.Client().Transport}
260+
unlimited, err := call(&http.Client{Transport: transport})
261+
assert.Nil(t, err)
262+
assert.Equal(t, unlimited.HTTPMethod(), http.MethodGet)
263+
urlSize := transport.urlLen
264+
265+
atLimit, err := call(server.Client(), connect.WithHTTPGetMaxURLSize(urlSize, false))
266+
assert.Nil(t, err)
267+
assert.Equal(t, atLimit.HTTPMethod(), http.MethodGet)
268+
269+
atLimitWithFallback, err := call(server.Client(), connect.WithHTTPGetMaxURLSize(urlSize, true))
270+
assert.Nil(t, err)
271+
assert.Equal(t, atLimitWithFallback.HTTPMethod(), http.MethodGet)
272+
273+
overLimit, err := call(server.Client(), connect.WithHTTPGetMaxURLSize(urlSize-1, true))
274+
assert.Nil(t, err)
275+
assert.Equal(t, overLimit.HTTPMethod(), http.MethodPost)
276+
}
277+
231278
func TestConnectionDropped(t *testing.T) {
232279
t.Parallel()
233280
ctx := t.Context()

protocol_connect.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,7 +1015,7 @@ func (m *connectUnaryRequestMarshaler) marshalWithGet(message any) *Error {
10151015
}
10161016
if !isTooBig {
10171017
url := m.buildGetURL(data, false /* compressed */)
1018-
if m.getURLMaxBytes <= 0 || len(url.String()) < m.getURLMaxBytes {
1018+
if m.getURLMaxBytes <= 0 || len(url.String()) <= m.getURLMaxBytes {
10191019
m.writeWithGet(url)
10201020
return nil
10211021
}
@@ -1042,7 +1042,7 @@ func (m *connectUnaryRequestMarshaler) marshalWithGet(message any) *Error {
10421042
return NewError(CodeResourceExhausted, fmt.Errorf("compressed message size %d exceeds sendMaxBytes %d", compressed.Len(), m.sendMaxBytes))
10431043
}
10441044
url := m.buildGetURL(compressed.Bytes(), true /* compressed */)
1045-
if m.getURLMaxBytes <= 0 || len(url.String()) < m.getURLMaxBytes {
1045+
if m.getURLMaxBytes <= 0 || len(url.String()) <= m.getURLMaxBytes {
10461046
m.writeWithGet(url)
10471047
return nil
10481048
}

0 commit comments

Comments
 (0)