Skip to content

Commit 25e2d35

Browse files
mromaszewiczclaude
andauthored
Fix panic binding numeric values into non-byte slice destinations (#156)
Closes: #155 The reflect.Slice case handled the base64 `format: byte` path and then fell through, with a comment claiming it landed in the default error case. It did not: the next case is the integer one, so a non-byte slice destination reached v.OverflowInt on a slice reflect.Value and panicked. Only sources that parse as an integer got that far — ParseInt failed first for anything else, masking the bug behind a plausible-looking error. This was reachable from generated server code. A nullable slice query parameter using the default form/explode serialization binds through the primitive path, and the nullable wrapper then binds the raw value into a fresh slice, so `?p=123` panicked while `?p=abc` returned a binding error. Replace the fallthrough with the explicit unhandled-type error, which restores the pre-v1.2.0 behavior (the fallthrough came in with 224825a, first released in v1.2.0) and makes the comment true. A []byte destination without Format "byte" took the same panicking path and now reports the same error; that path never bound successfully, so nothing could have depended on it. Regression tests pin both the numeric and non-numeric sources against []string, []int and []byte, plus the nullable.Nullable[[]string] exploded-query route through both BindQueryParameter and BindRawQueryParameter. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f2e468c commit 25e2d35

2 files changed

Lines changed: 81 additions & 2 deletions

File tree

bindstring.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,12 @@ func BindStringToObjectWithOptions(src string, dst interface{}, opts BindStringT
111111
v.SetBytes(decoded)
112112
return nil
113113
}
114-
// Non-binary slices fall through to the default error case.
115-
fallthrough
114+
// Non-binary slices have no string representation to parse, so they
115+
// get the same unhandled-type error as the default case below. This
116+
// can not be a fallthrough: the next case is the integer one, and a
117+
// source string that parses as an integer would reach v.OverflowInt
118+
// on a slice value, which panics.
119+
err = fmt.Errorf("can not bind to destination of type: %s", t.Kind())
116120
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
117121
var val int64
118122
val, err = strconv.ParseInt(src, 10, 64)

bindstring_test.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,11 @@ import (
1717
"encoding/base64"
1818
"fmt"
1919
"math"
20+
"net/url"
2021
"testing"
2122
"time"
2223

24+
"github.com/oapi-codegen/nullable"
2325
"github.com/stretchr/testify/assert"
2426
"github.com/stretchr/testify/require"
2527

@@ -274,3 +276,76 @@ func TestBindStringToObject_ByteSlice(t *testing.T) {
274276
assert.Error(t, err)
275277
})
276278
}
279+
280+
// A slice destination that isn't a base64-decoded []byte has no string
281+
// representation to parse, so it must report an unhandled-type error. It used
282+
// to fall through into the integer case instead, which meant any source that
283+
// parsed as an integer reached v.OverflowInt on a slice value and panicked,
284+
// while a non-numeric source happened to error out in ParseInt first.
285+
// See: https://github.com/oapi-codegen/runtime/issues/155
286+
func TestBindStringToObject_NonByteSliceDestination(t *testing.T) {
287+
const wantErr = "can not bind to destination of type: slice"
288+
289+
t.Run("numeric source", func(t *testing.T) {
290+
var dest []string
291+
require.NotPanics(t, func() {
292+
err := BindStringToObject("123", &dest)
293+
require.Error(t, err)
294+
assert.Contains(t, err.Error(), wantErr)
295+
})
296+
assert.Nil(t, dest)
297+
})
298+
299+
t.Run("non-numeric source", func(t *testing.T) {
300+
var dest []string
301+
err := BindStringToObject("abc", &dest)
302+
require.Error(t, err)
303+
assert.Contains(t, err.Error(), wantErr)
304+
})
305+
306+
t.Run("integer element type", func(t *testing.T) {
307+
var dest []int
308+
require.NotPanics(t, func() {
309+
err := BindStringToObject("123", &dest)
310+
require.Error(t, err)
311+
assert.Contains(t, err.Error(), wantErr)
312+
})
313+
})
314+
315+
t.Run("byte slice without byte format", func(t *testing.T) {
316+
// The base64 path is only taken for Format "byte"; without it a
317+
// []byte is just another slice.
318+
var dest []byte
319+
require.NotPanics(t, func() {
320+
err := BindStringToObject("123", &dest)
321+
require.Error(t, err)
322+
assert.Contains(t, err.Error(), wantErr)
323+
})
324+
})
325+
}
326+
327+
// The panic above was reachable from generated server code: a nullable slice
328+
// query parameter with the default form/explode serialization binds through
329+
// the primitive path, and the nullable wrapper then binds the raw value into a
330+
// fresh slice. `?p=123` panicked while `?p=abc` returned a binding error; both
331+
// must now be errors.
332+
// See: https://github.com/oapi-codegen/runtime/issues/155
333+
func TestBindQueryParameter_NullableSliceDestination(t *testing.T) {
334+
for _, src := range []string{"123", "abc"} {
335+
t.Run(src, func(t *testing.T) {
336+
require.NotPanics(t, func() {
337+
var dest nullable.Nullable[[]string]
338+
err := BindQueryParameter("form", true, true, "p", url.Values{"p": {src}}, &dest)
339+
require.Error(t, err)
340+
assert.Contains(t, err.Error(), "can not bind to destination of type: slice")
341+
})
342+
343+
require.NotPanics(t, func() {
344+
var dest nullable.Nullable[[]string]
345+
err := BindRawQueryParameter("form", true, true, "p", "p="+src, &dest)
346+
require.Error(t, err)
347+
assert.Contains(t, err.Error(), "can not bind to destination of type: slice")
348+
})
349+
})
350+
}
351+
}

0 commit comments

Comments
 (0)