diff --git a/pkg/http/middleware/request_config.go b/pkg/http/middleware/request_config.go index 6102825192..5405c378c7 100644 --- a/pkg/http/middleware/request_config.go +++ b/pkg/http/middleware/request_config.go @@ -21,32 +21,32 @@ func WithRequestConfig(next http.Handler) http.Handler { ctx := r.Context() // Readonly mode - if relaxedParseBool(r.Header.Get(headers.MCPReadOnlyHeader)) { + if slices.ContainsFunc(r.Header.Values(headers.MCPReadOnlyHeader), relaxedParseBool) { ctx = ghcontext.WithReadonly(ctx, true) } // Toolsets - if toolsets := headers.ParseCommaSeparated(r.Header.Get(headers.MCPToolsetsHeader)); len(toolsets) > 0 { + if toolsets := headers.ParseCommaSeparated(strings.Join(r.Header.Values(headers.MCPToolsetsHeader), ",")); len(toolsets) > 0 { ctx = ghcontext.WithToolsets(ctx, toolsets) } // Tools - if tools := headers.ParseCommaSeparated(r.Header.Get(headers.MCPToolsHeader)); len(tools) > 0 { + if tools := headers.ParseCommaSeparated(strings.Join(r.Header.Values(headers.MCPToolsHeader), ",")); len(tools) > 0 { ctx = ghcontext.WithTools(ctx, tools) } // Lockdown mode - if relaxedParseBool(r.Header.Get(headers.MCPLockdownHeader)) { + if slices.ContainsFunc(r.Header.Values(headers.MCPLockdownHeader), relaxedParseBool) { ctx = ghcontext.WithLockdownMode(ctx, true) } // Excluded tools - if excludeTools := headers.ParseCommaSeparated(r.Header.Get(headers.MCPExcludeToolsHeader)); len(excludeTools) > 0 { + if excludeTools := headers.ParseCommaSeparated(strings.Join(r.Header.Values(headers.MCPExcludeToolsHeader), ",")); len(excludeTools) > 0 { ctx = ghcontext.WithExcludeTools(ctx, excludeTools) } // Insiders mode - if relaxedParseBool(r.Header.Get(headers.MCPInsidersHeader)) { + if slices.ContainsFunc(r.Header.Values(headers.MCPInsidersHeader), relaxedParseBool) { ctx = ghcontext.WithInsidersMode(ctx, true) } @@ -54,9 +54,9 @@ func WithRequestConfig(next http.Handler) http.Handler { _, hasHeaderFeatures := r.Header[http.CanonicalHeaderKey(headers.MCPFeaturesHeader)] _, hasQueryFeatures := query[queryParamFeatures] if hasHeaderFeatures { - ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(r.Header.Get(headers.MCPFeaturesHeader))) + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(strings.Join(r.Header.Values(headers.MCPFeaturesHeader), ","))) } else if hasQueryFeatures { - ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(query.Get(queryParamFeatures))) + ctx = ghcontext.WithHeaderFeatures(ctx, headers.ParseCommaSeparated(strings.Join(query[queryParamFeatures], ","))) } next.ServeHTTP(w, r.WithContext(ctx)) diff --git a/pkg/http/middleware/request_config_test.go b/pkg/http/middleware/request_config_test.go index 1937d0986b..b9b3ecf041 100644 --- a/pkg/http/middleware/request_config_test.go +++ b/pkg/http/middleware/request_config_test.go @@ -1,6 +1,7 @@ package middleware import ( + "context" "net/http" "net/http/httptest" "testing" @@ -10,6 +11,76 @@ import ( "github.com/stretchr/testify/assert" ) +func TestWithRequestConfigRepeatedListHeaders(t *testing.T) { + tests := []struct { + name string + header string + get func(context.Context) []string + }{ + {"toolsets", headers.MCPToolsetsHeader, ghcontext.GetToolsets}, + {"tools", headers.MCPToolsHeader, ghcontext.GetTools}, + {"excluded tools", headers.MCPExcludeToolsHeader, ghcontext.GetExcludeTools}, + {"features", headers.MCPFeaturesHeader, ghcontext.GetHeaderFeatures}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/?features=query_flag", nil) + req.Header.Add(tc.header, "first, second") + req.Header.Add(tc.header, " third, , fourth ") + handler := WithRequestConfig(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + assert.Equal(t, []string{"first", "second", "third", "fourth"}, tc.get(r.Context())) + })) + handler.ServeHTTP(httptest.NewRecorder(), req) + }) + } +} + +func TestWithRequestConfigRepeatedBooleanHeaders(t *testing.T) { + headersToTest := []struct { + name string + header string + get func(context.Context) bool + }{ + {"readonly", headers.MCPReadOnlyHeader, ghcontext.IsReadonly}, + {"lockdown", headers.MCPLockdownHeader, ghcontext.IsLockdownMode}, + {"insiders", headers.MCPInsidersHeader, ghcontext.IsInsidersMode}, + } + tests := []struct { + name string + values []string + want bool + }{ + {"absent", nil, false}, + {"all false", []string{"", " false ", "0", " NO ", "off", "N", "f"}, false}, + {"false then true", []string{"false", "true"}, true}, + {"true then false", []string{"true", "false"}, true}, + {"empty then one", []string{"", "1"}, true}, + {"zero then yes", []string{"0", "yes"}, true}, + {"no then on", []string{"no", "on"}, true}, + {"off then y", []string{"off", "y"}, true}, + {"n then t", []string{"n", "t"}, true}, + {"mixed case and whitespace", []string{" F ", " TrUe "}, true}, + {"arbitrary truthy value", []string{"false", "enabled"}, true}, + {"comma value retains scalar semantics", []string{"false,false"}, true}, + } + for _, header := range headersToTest { + t.Run(header.name, func(t *testing.T) { + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/", nil) + for _, value := range tc.values { + req.Header.Add(header.header, value) + } + handler := WithRequestConfig(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + assert.Equal(t, tc.want, header.get(r.Context())) + })) + handler.ServeHTTP(httptest.NewRecorder(), req) + }) + } + }) + } +} + func TestWithRequestConfigFeatureSelection(t *testing.T) { tests := []struct { name string @@ -25,6 +96,39 @@ func TestWithRequestConfigFeatureSelection(t *testing.T) { wantFeatures: []string{"mcp_holdback_consolidated_projects"}, wantPresent: true, }, + { + name: "repeated query parameters combine comma separated values", + url: "/?features=first,%20second&features=&features=%20third,%20,%20fourth%20", + wantFeatures: []string{"first", "second", "third", "fourth"}, + wantPresent: true, + }, + { + name: "empty first query value does not hide later features", + url: "/?features=&features=flag_from_query", + wantFeatures: []string{"flag_from_query"}, + wantPresent: true, + }, + { + name: "repeated empty query values store an explicit empty selection", + url: "/?features=&features=%20,%20", + wantFeatures: []string{}, + wantPresent: true, + }, + { + name: "header wins over repeated query parameters", + url: "/?features=first&features=second", + headerSet: true, + headerValue: "flag_from_header", + wantFeatures: []string{"flag_from_header"}, + wantPresent: true, + }, + { + name: "empty header suppresses repeated query parameters", + url: "/?features=first&features=second", + headerSet: true, + wantFeatures: []string{}, + wantPresent: true, + }, { name: "header only", url: "/",