From 4a0aadeef8e710d9524ac583dbb65146d1aa8e7e Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 10:54:13 -0500 Subject: [PATCH 1/2] fix(http): bind every occurrence of a repeated query parameter bindFormParam learned to fill a []string from a repeated parameter, but bindQueryParam kept reading one value through Query. A []string query field therefore took the first occurrence and dropped the rest. That is quieter than it used to be, and worse. Before setFieldValue grew a slice case the same field failed the request outright with "unsupported field type". Now the lone value it does read gets split on commas instead, so the request succeeds carrying less than the caller sent. For an RFC 8707 resource indicator that means an access token scoped to fewer audiences than were asked for, with nothing logged. The query path now mirrors the form path exactly: a repeated parameter is taken verbatim, a lone value still expands on commas the way scope=openid,profile always has, an absent parameter falls back to the default tag, and a required one still reports itself missing. Header parameters have the same shape and are left alone here. Repeated headers are rarer and nothing is waiting on them. --- http/binder.go | 36 ++++++++++++- http/binder_query_multi_test.go | 89 +++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) create mode 100644 http/binder_query_multi_test.go diff --git a/http/binder.go b/http/binder.go index 2b88984..3aa1ca3 100644 --- a/http/binder.go +++ b/http/binder.go @@ -188,8 +188,6 @@ func (c *Ctx) bindQueryParam(field reflect.StructField, fieldValue reflect.Value paramName = field.Name } - value := c.Query(paramName) - // Determine if field is required using consistent precedence: // 1. optional:"true" - explicitly optional (highest priority) // 2. required:"true" - explicitly required @@ -198,6 +196,40 @@ func (c *Ctx) bindQueryParam(field reflect.StructField, fieldValue reflect.Value // 5. default: non-pointer types are required required := isBindFieldRequired(field, tag) + // Repeated parameters (resource=a&resource=b) fill a slice field. Reading + // a single value through Query would keep the first occurrence and drop + // the rest, which for something like an RFC 8707 resource indicator + // silently narrows what the caller asked for. + if isMultiValueTarget(fieldValue) { + present := c.queryValues()[paramName] + if len(present) == 0 { + if required { + errors.AddWithCode(paramName, "query parameter is required", val.ErrCodeRequired, nil) + + return nil + } + + if defaultVal := field.Tag.Get("default"); defaultVal != "" { + present = strings.Split(defaultVal, ",") + } + } + + switch len(present) { + case 0: + return nil + case 1: + // A single occurrence goes through setFieldValue so that a + // comma-separated value (scope=openid,profile) expands the same way + // it always has. Splitting only ever applies to a lone value; + // repeated parameters are taken verbatim. + return setFieldValue(fieldValue, present[0], paramName, errors) + default: + return setSliceFieldValue(fieldValue, present, paramName, errors) + } + } + + value := c.Query(paramName) + if required && value == "" { errors.AddWithCode(paramName, "query parameter is required", val.ErrCodeRequired, nil) diff --git a/http/binder_query_multi_test.go b/http/binder_query_multi_test.go new file mode 100644 index 0000000..a2e83b9 --- /dev/null +++ b/http/binder_query_multi_test.go @@ -0,0 +1,89 @@ +package http + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// QueryMultiRequest mirrors an OAuth2 authorization request: RFC 8707 defines +// `resource` as repeatable, so the field has to collect every occurrence +// rather than the first one. +type QueryMultiRequest struct { + ClientID string `query:"client_id"` + Resources []string `query:"resource,omitempty"` + Scopes []string `default:"openid,profile" query:"scope,omitempty"` +} + +func getQuery(t *testing.T, target string) *Ctx { + t.Helper() + + req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, target, nil) + + return NewContext(httptest.NewRecorder(), req, nil).(*Ctx) +} + +func TestBindRequest_RepeatedQueryParamFillsSlice(t *testing.T) { + var req QueryMultiRequest + require.NoError(t, getQuery(t, + "/authorize?client_id=abc&resource=https://a.example.com&resource=https://b.example.com"). + BindRequest(&req)) + + assert.Equal(t, "abc", req.ClientID) + assert.Equal(t, []string{"https://a.example.com", "https://b.example.com"}, req.Resources, + "every occurrence of a repeated query parameter must reach the slice") +} + +func TestBindRequest_SingleQueryParamStillFillsSlice(t *testing.T) { + var req QueryMultiRequest + require.NoError(t, getQuery(t, "/authorize?client_id=abc&resource=https://a.example.com"). + BindRequest(&req)) + + assert.Equal(t, []string{"https://a.example.com"}, req.Resources) +} + +// A lone value keeps expanding on commas, the way scope=openid,profile always +// has. Only repeated parameters are taken verbatim. +func TestBindRequest_LoneCommaValueStillExpands(t *testing.T) { + var req QueryMultiRequest + require.NoError(t, getQuery(t, "/authorize?client_id=abc&scope=openid,email").BindRequest(&req)) + + assert.Equal(t, []string{"openid", "email"}, req.Scopes) +} + +func TestBindRequest_RepeatedQueryParamIsTakenVerbatim(t *testing.T) { + var req QueryMultiRequest + require.NoError(t, getQuery(t, "/authorize?client_id=abc&scope=openid&scope=a,b").BindRequest(&req)) + + assert.Equal(t, []string{"openid", "a,b"}, req.Scopes, + "a repeated parameter must not be split further, the same as the form path") +} + +func TestBindRequest_AbsentRepeatedQueryParamLeavesSliceEmpty(t *testing.T) { + var req QueryMultiRequest + require.NoError(t, getQuery(t, "/authorize?client_id=abc").BindRequest(&req)) + + assert.Empty(t, req.Resources) +} + +func TestBindRequest_AbsentQuerySliceTakesItsDefault(t *testing.T) { + var req QueryMultiRequest + require.NoError(t, getQuery(t, "/authorize?client_id=abc").BindRequest(&req)) + + assert.Equal(t, []string{"openid", "profile"}, req.Scopes) +} + +func TestBindRequest_RequiredQuerySliceReportsWhenAbsent(t *testing.T) { + type required struct { + Resources []string `query:"resource" required:"true"` + } + + var req required + err := getQuery(t, "/authorize").BindRequest(&req) + require.Error(t, err) + assert.Contains(t, err.Error(), "resource") +} From 2aa1082eca629f20f98191fd1ff65eb0e03219a6 Mon Sep 17 00:00:00 2001 From: Rex Raphael Date: Tue, 25 Aug 2026 15:24:50 -0500 Subject: [PATCH 2/2] ci: fixed lint issues --- http/binder_query_multi_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/http/binder_query_multi_test.go b/http/binder_query_multi_test.go index a2e83b9..996dad3 100644 --- a/http/binder_query_multi_test.go +++ b/http/binder_query_multi_test.go @@ -16,7 +16,7 @@ import ( type QueryMultiRequest struct { ClientID string `query:"client_id"` Resources []string `query:"resource,omitempty"` - Scopes []string `default:"openid,profile" query:"scope,omitempty"` + Scopes []string `default:"openid,profile" query:"scope,omitempty"` } func getQuery(t *testing.T, target string) *Ctx { @@ -83,6 +83,7 @@ func TestBindRequest_RequiredQuerySliceReportsWhenAbsent(t *testing.T) { } var req required + err := getQuery(t, "/authorize").BindRequest(&req) require.Error(t, err) assert.Contains(t, err.Error(), "resource")