From 6b4c44eaa7164ea5ed7fe4f109e9aabe6d84d9ca Mon Sep 17 00:00:00 2001 From: Jm Rohmer <276982731+jmrGrav@users.noreply.github.com> Date: Sun, 7 Jun 2026 01:40:51 +0200 Subject: [PATCH 1/2] test: document allowlisted OAuth redirects --- docs/CODEQL_REDIRECT_ALERTS_DIAGNOSTIC.md | 126 ++++++++++++++++++++++ internal/oauthproxy/handlers_test.go | 97 +++++++++++++++++ 2 files changed, 223 insertions(+) create mode 100644 docs/CODEQL_REDIRECT_ALERTS_DIAGNOSTIC.md diff --git a/docs/CODEQL_REDIRECT_ALERTS_DIAGNOSTIC.md b/docs/CODEQL_REDIRECT_ALERTS_DIAGNOSTIC.md new file mode 100644 index 0000000..4417fac --- /dev/null +++ b/docs/CODEQL_REDIRECT_ALERTS_DIAGNOSTIC.md @@ -0,0 +1,126 @@ +# CodeQL redirect alerts diagnostic + +Repo: `jmrGrav/mcp-runtime-go` + +Scope: +- Alert #2: `go/unvalidated-url-redirection` +- Alert #3: `go/unvalidated-url-redirection` +- File: `internal/oauthproxy/handlers.go` + +## Executive summary + +Both open CodeQL alerts point to explicit redirects in `HandleAuthorize`, but neither one is an exploitable open redirect in the current code path. + +The redirect target comes from `redirect_uri` in the authorize request, but it is validated by `security.IsAllowedRedirect()` before any redirect occurs. Invalid `redirect_uri` values fail closed with `400 invalid_redirect_uri`, and the service also re-validates the redirect URI in the auth-code issuance path. + +Verdict: +- Alert #2: `ACCEPTABLE RISK` +- Alert #3: `ACCEPTABLE RISK` + +Rationale: +- The sink is real (`http.Redirect`), but the source is constrained by a strict allowlist and invalid inputs are rejected before redirect. +- This is not a classic open redirect exploit. +- CodeQL is flagging the pattern because it sees user-controlled data reaching `http.Redirect`, but it does not fully model the allowlist semantics here. + +## Alert #2 + +### Location +- Sink line: `internal/oauthproxy/handlers.go:158` +- Function: `(*Service).HandleAuthorize` + +### Source +- `redirectURI := q.Get("redirect_uri")` at `internal/oauthproxy/handlers.go:120` + +### Validation present +- `security.IsAllowedRedirect(redirectURI)` at `internal/oauthproxy/handlers.go:125` +- If validation fails, the handler returns `400 invalid_redirect_uri` at `internal/oauthproxy/handlers.go:126-128` +- `client_id` is also checked with constant-time comparison at `internal/oauthproxy/handlers.go:130` + +### Data flow +- User input `redirect_uri` is read from the query string. +- It is rejected unless it passes the allowlist check. +- Only then does the error branch build the RFC 6749 error parameters and call `http.Redirect(w, r, redirectURI+"?"+params.Encode(), http.StatusFound)`. + +### Verdict +- `ACCEPTABLE RISK` + +### Why this is not a true open redirect +- The redirect destination is not arbitrary. +- `internal/security/redirect_uri.go` only allows: + - exact hosts `claude.ai` and `anthropic.com` + - suffixes `.claude.ai` and `.anthropic.com` + - `https` only +- Invalid redirect URIs fail closed before redirect. + +### Recommendation +- No functional code change is required for security. +- Keep the allowlist and the fail-closed `invalid_redirect_uri` path. +- If the goal is to reduce future security noise, add or strengthen tests that assert the redirect location host is allowlisted and that invalid URIs do not produce a `Location` header. + +### Suppression option +- If the team treats this as a benign, intentional redirect pattern, dismiss the alert in GitHub as `false positive` or `used in tests` only if the policy permits. +- Justification: the redirect target is constrained by a server-side allowlist and the invalid case returns `400` instead of redirecting. + +## Alert #3 + +### Location +- Sink line: `internal/oauthproxy/handlers.go:172` +- Function: `(*Service).HandleAuthorize` + +### Source +- `redirectURI := q.Get("redirect_uri")` at `internal/oauthproxy/handlers.go:120` +- The redirect used here is also stored in `req.RedirectURI` at `internal/oauthproxy/handlers.go:139` + +### Validation present +- `security.IsAllowedRedirect(redirectURI)` at `internal/oauthproxy/handlers.go:125` +- Invalid URIs fail closed with `400 invalid_redirect_uri` at `internal/oauthproxy/handlers.go:126-128` +- `IssueAuthCode(req)` repeats the redirect check in `internal/oauthproxy/service.go:242-244` +- The token exchange path also enforces redirect match at `internal/oauthproxy/service.go:294-296` + +### Data flow +- The handler builds `AuthorizeRequest` from the already validated query values. +- `IssueAuthCode()` validates the same redirect URI again. +- On success, the redirect goes to `req.RedirectURI` with an auth code and optional state. + +### Verdict +- `ACCEPTABLE RISK` + +### Why this is not a true open redirect +- The sink is a redirect to a registered OAuth redirect URI, not to arbitrary user input. +- The code validates that URI against the allowlist before issue and again during auth-code handling. +- The token exchange later requires the exact same redirect URI, which reduces the chance of redirect abuse across the OAuth flow. + +### Recommendation +- No functional code change is required for security. +- Add a stronger test that inspects the `Location` header for a valid authorize request and confirms it targets an allowlisted host only. + +### Suppression option +- Same as alert #2: dismissal as `false positive` is defensible if the team accepts the allowlist as authoritative. +- Include a note that the redirect is intentional and constrained by `security.IsAllowedRedirect()`. + +## Existing tests already covering the control paths + +Relevant tests in `internal/oauthproxy/handlers_test.go`: +- `TestHandleAuthorize` around `:150-225` + - includes the `Invalid redirect_uri` case and expects `400` +- `TestHandleAuthorize_RFC6749_ErrorRedirect` around `:573-637` + - verifies that non-redirect_uri errors still produce a `302` and an `error` parameter in `Location` +- `TestHandleAuthorize_CIDR` around `:355-388` + - verifies the IP allowlist gate + +Relevant allowlist tests: +- `internal/security/redirect_uri_test.go:5-29` + - covers exact hosts, suffixes, `http` rejection, malformed URLs, and hostile host patterns + +## Tests to add or strengthen + +Recommended additions: +- Assert that valid `/authorize` requests redirect to an allowlisted host only. +- Assert that invalid `redirect_uri` requests do not set a `Location` header. +- Add a case that exercises the `IssueAuthCode` failure branch and verifies the redirected URL host is still the validated allowlist target. + +## Bottom line + +The two open CodeQL alerts are the same pattern at two redirect sinks in `HandleAuthorize`. +They are not exploitable open redirects under the current code because `redirect_uri` is validated against a strict allowlist and invalid values fail closed. +The practical classification is `ACCEPTABLE RISK`, not a code fix. diff --git a/internal/oauthproxy/handlers_test.go b/internal/oauthproxy/handlers_test.go index 25c7469..8ca6313 100644 --- a/internal/oauthproxy/handlers_test.go +++ b/internal/oauthproxy/handlers_test.go @@ -6,6 +6,7 @@ import ( "mcp-runtime-go/internal/config" mcpctx "mcp-runtime-go/internal/context" "mcp-runtime-go/internal/observability" + "mcp-runtime-go/internal/security" "mcp-runtime-go/internal/storage" "net/http" "net/http/httptest" @@ -43,6 +44,32 @@ func setupTestService(t *testing.T) (*Service, *config.Config) { return s, cfg } +func mustParseLocation(t *testing.T, loc string) *url.URL { + t.Helper() + if loc == "" { + t.Fatal("expected Location header, got empty string") + } + u, err := url.Parse(loc) + if err != nil { + t.Fatalf("invalid Location header %q: %v", loc, err) + } + return u +} + +func assertAllowlistedRedirectLocation(t *testing.T, loc string) { + t.Helper() + u := mustParseLocation(t, loc) + if !security.IsAllowedRedirect(loc) { + t.Fatalf("expected allowlisted redirect, got %q", loc) + } + if u.Scheme != "https" { + t.Fatalf("expected https redirect, got scheme %q in %q", u.Scheme, loc) + } + if u.User != nil { + t.Fatalf("expected no userinfo in redirect URL, got %q in %q", u.User.String(), loc) + } +} + func TestHandleMetadata(t *testing.T) { s, _ := setupTestService(t) @@ -225,6 +252,75 @@ func TestHandleAuthorize(t *testing.T) { } } +func TestHandleAuthorize_ValidRedirectTargetsAreAllowlisted(t *testing.T) { + s, cfg := setupTestService(t) + + query := url.Values{ + "response_type": {"code"}, + "client_id": {cfg.OAuthProxy.ClientID}, + "redirect_uri": {"https://claude.ai/callback"}, + "state": {"test-state"}, + "code_challenge": {"JBbiqONGWPaAmwXk_8bT6UnlPfrn65D32eZlJS-zGG0"}, + "code_challenge_method": {"S256"}, + } + + req := httptest.NewRequest("GET", "/authorize?"+query.Encode(), nil) + req.RemoteAddr = "127.0.0.1:1234" + rr := httptest.NewRecorder() + + s.HandleAuthorize(rr, req) + + if rr.Code != http.StatusFound { + t.Fatalf("expected 302, got %d: %s", rr.Code, rr.Body.String()) + } + assertAllowlistedRedirectLocation(t, rr.Header().Get("Location")) +} + +func TestHandleAuthorize_InvalidRedirectURIsFailClosed(t *testing.T) { + s, cfg := setupTestService(t) + + baseQuery := url.Values{ + "response_type": {"code"}, + "client_id": {cfg.OAuthProxy.ClientID}, + "state": {"test-state"}, + "code_challenge": {"JBbiqONGWPaAmwXk_8bT6UnlPfrn65D32eZlJS-zGG0"}, + "code_challenge_method": {"S256"}, + } + + tests := []struct { + name string + redirectURI string + }{ + {"direct evil host", "https://evil.com/callback"}, + {"suffix trick", "https://claude.ai.evil.com/callback"}, + {"userinfo trick", "https://claude.ai@evil.com/callback"}, + {"http scheme", "http://claude.ai/callback"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + query := url.Values{} + for k, v := range baseQuery { + query[k] = append([]string(nil), v...) + } + query.Set("redirect_uri", tt.redirectURI) + + req := httptest.NewRequest("GET", "/authorize?"+query.Encode(), nil) + req.RemoteAddr = "127.0.0.1:1234" + rr := httptest.NewRecorder() + + s.HandleAuthorize(rr, req) + + if rr.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", rr.Code, rr.Body.String()) + } + if loc := rr.Header().Get("Location"); loc != "" { + t.Fatalf("expected no Location header, got %q", loc) + } + }) + } +} + func TestHandleToken(t *testing.T) { s, cfg := setupTestService(t) @@ -626,6 +722,7 @@ func TestHandleAuthorize_RFC6749_ErrorRedirect(t *testing.T) { return } loc := rr.Header().Get("Location") + assertAllowlistedRedirectLocation(t, loc) u, err := url.Parse(loc) if err != nil { t.Fatalf("invalid Location header %q: %v", loc, err) From 49a441764687ae09226971917493adc17ad69bd1 Mon Sep 17 00:00:00 2001 From: Jm Rohmer <276982731+jmrGrav@users.noreply.github.com> Date: Thu, 2 Jul 2026 07:29:18 +0200 Subject: [PATCH 2/2] feat: support optional anonymous read-only MCP --- README.md | 3 + docs/ARCHITECTURE.md | 24 ++++ docs/OPERATIONS.md | 25 ++++ internal/config/config.go | 5 + internal/config/config_test.go | 37 ++++++ internal/oauthproxy/proxy.go | 210 +++++++++++++++++++++++++++++- internal/oauthproxy/proxy_test.go | 155 ++++++++++++++++++++++ 7 files changed, 453 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 3d24961..7563af6 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,7 @@ systemctl status mcp-runtime --no-pager - OAuth 2.0 Authorization Code + PKCE - Dynamic Client Registration - Authenticated MCP reverse proxy +- Optional anonymous read-only MCP mode with an explicit public tool allowlist - SQLite WAL token storage - Structured audit logging - `/healthz`, `/readyz`, and loopback metrics @@ -57,6 +58,8 @@ Common settings: - `USE_SQLITE=true` - `TOKENS_DB=/var/lib/mcp-runtime-go/tokens.db` - `AUDIT_LOG_FILE=/var/log/mcp-runtime-go/audit.jsonl` +- `ANONYMOUS_ENABLED=false` +- `ANONYMOUS_PUBLIC_TOOLS=search_pages,get_page` Legacy `GRAV_*` variables remain compatibility fallbacks only. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 9b5e301..72a4e00 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -47,6 +47,28 @@ Claude.ai 4. `/token` exchanges the authorization code for an access token. 5. `/mcp` proxies authenticated requests to the Hugo backend. +## Optional Anonymous MCP Mode + +By default, `/mcp` remains authenticated and requires a valid bearer token. + +When `ANONYMOUS_ENABLED=true`, the proxy accepts unauthenticated MCP JSON-RPC +requests only for a narrow public surface: + +- protocol setup methods: `initialize`, `notifications/initialized`, `ping` +- `tools/list` +- `tools/call` only when `params.name` appears in `ANONYMOUS_PUBLIC_TOOLS` + +Invalid bearer tokens are still rejected with `401`; anonymous fallback is only +available when no `Authorization` header is present. + +Anonymous `tools/list` responses are filtered so only tools named in +`ANONYMOUS_PUBLIC_TOOLS` are advertised to anonymous clients. The filter supports +plain JSON-RPC responses and server-sent event `data:` JSON payloads. + +This mode is intended for public read-only MCP servers. It must not be used in +front of a backend that exposes write or administrative tools unless every +publicly callable tool is intentionally allowlisted and tested. + Important guarantees: - redirect URIs must match the registered allowlist @@ -86,6 +108,8 @@ Common runtime controls: - `AUDIT_LOG_FILE` - `TRUSTED_PROXIES` - `MANDATORY_PKCE` +- `ANONYMOUS_ENABLED` +- `ANONYMOUS_PUBLIC_TOOLS` ## Security Model diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 808be0f..cad32b6 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -38,9 +38,34 @@ Common production settings: - `TRUSTED_PROXIES=127.0.0.1,::1` - `MANDATORY_PKCE=true` - `ALLOW_TOKEN_STORE_RECOVERY=false` +- `ANONYMOUS_ENABLED=false` +- `ANONYMOUS_PUBLIC_TOOLS=` Legacy `GRAV_*` variables are supported only as compatibility fallback. +## Optional Anonymous Read-Only Mode + +Anonymous MCP access is disabled by default. + +Enable it only for public read-only MCP backends: + +```bash +ANONYMOUS_ENABLED=true +ANONYMOUS_PUBLIC_TOOLS=search_pages,get_page,list_pages +``` + +Behavior: + +- no `Authorization` header: allowed only for protocol setup, `tools/list`, and + `tools/call` names present in `ANONYMOUS_PUBLIC_TOOLS`; +- anonymous `tools/list`: response is filtered to advertise only + `ANONYMOUS_PUBLIC_TOOLS`; +- invalid `Authorization: Bearer ...`: always rejected with `401`; +- valid bearer token: authenticated proxy behavior is unchanged. + +Do not enable this mode in front of an administrative MCP backend unless every +write-capable tool is excluded from the public allowlist and separately tested. + ## Systemd The service is expected to run as a hardened unit with: diff --git a/internal/config/config.go b/internal/config/config.go index 5195fad..e703f82 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -32,6 +32,8 @@ type OAuthProxyConfig struct { TrustedAuthorizeCIDRs []string `env:"TRUSTED_AUTHORIZE_CIDRS" envDefault:"127.0.0.1/32,::1/128"` MandatoryPKCE bool `env:"MANDATORY_PKCE" envDefault:"true"` AllowTokenStoreRecovery bool `env:"ALLOW_TOKEN_STORE_RECOVERY" envDefault:"false"` + AnonymousEnabled bool `env:"ANONYMOUS_ENABLED" envDefault:"false"` + AnonymousPublicTools []string `env:"ANONYMOUS_PUBLIC_TOOLS" envDefault:""` } type RuntimeConfig struct { @@ -104,6 +106,9 @@ func (c *Config) Validate() error { if c.OAuthProxy.AccessTokenTTL <= 0 { return fmt.Errorf("ACCESS_TOKEN_TTL must be > 0, got %d", c.OAuthProxy.AccessTokenTTL) } + if c.OAuthProxy.AnonymousEnabled && len(c.OAuthProxy.AnonymousPublicTools) == 0 { + return fmt.Errorf("ANONYMOUS_PUBLIC_TOOLS must not be empty when ANONYMOUS_ENABLED=true") + } return nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 97a0da0..3b4d343 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -36,6 +36,27 @@ func TestLoad(t *testing.T) { } } +func TestLoad_AnonymousMCPConfig(t *testing.T) { + t.Setenv("CLIENT_ID", "test-client") + t.Setenv("CLIENT_SECRET", "test-secret") + t.Setenv("HUGO_TOKEN", "test-token") + t.Setenv("ANONYMOUS_ENABLED", "true") + t.Setenv("ANONYMOUS_PUBLIC_TOOLS", "search_posts, read_page") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() failed: %v", err) + } + + if !cfg.OAuthProxy.AnonymousEnabled { + t.Fatal("expected anonymous mode to be enabled") + } + want := []string{"search_posts", "read_page"} + if !reflect.DeepEqual(cfg.OAuthProxy.AnonymousPublicTools, want) { + t.Fatalf("anonymous public tools = %#v, want %#v", cfg.OAuthProxy.AnonymousPublicTools, want) + } +} + func TestValidate(t *testing.T) { tests := []struct { name string @@ -177,6 +198,22 @@ func TestValidate(t *testing.T) { }, true, }, + { + "Anonymous enabled without public tools", + Config{ + OAuthProxy: OAuthProxyConfig{ + ClientID: "id", + ClientSecret: "secret", + HugoToken: "token", + HugoMCPURL: "http://127.0.0.1/api/mcp", + ProxyBaseURL: "https://example.com", + AuthCodeTTL: 300, + AccessTokenTTL: 86400, + AnonymousEnabled: true, + }, + }, + true, + }, } for _, tt := range tests { diff --git a/internal/oauthproxy/proxy.go b/internal/oauthproxy/proxy.go index e1af589..23c04f5 100644 --- a/internal/oauthproxy/proxy.go +++ b/internal/oauthproxy/proxy.go @@ -1,8 +1,11 @@ package oauthproxy import ( + "bytes" "context" + "encoding/json" "fmt" + "io" mcpctx "mcp-runtime-go/internal/context" "mcp-runtime-go/internal/observability" "net/http" @@ -12,6 +15,10 @@ import ( "time" ) +const anonymousInspectionLimitBytes = 1 << 20 + +type anonymousToolsListContextKey struct{} + func appendSubPath(ctx context.Context, subPath string) context.Context { return context.WithValue(ctx, subPathContextKey{}, subPath) } @@ -73,6 +80,13 @@ func (s *Service) buildReverseProxy() *httputil.ReverseProxy { } }, Transport: s.httpClient.Transport, + ModifyResponse: func(resp *http.Response) error { + anonymousToolsList, _ := resp.Request.Context().Value(anonymousToolsListContextKey{}).(bool) + if !anonymousToolsList { + return nil + } + return s.filterAnonymousToolsListResponse(resp) + }, ErrorHandler: func(w http.ResponseWriter, r *http.Request, err error) { s.auditLog(r, "proxy_error", map[string]interface{}{"error": err.Error()}) observability.ProxyErrorsTotal.Inc() @@ -84,14 +98,27 @@ func (s *Service) buildReverseProxy() *httputil.ReverseProxy { func (s *Service) HandleProxy(w http.ResponseWriter, r *http.Request) { // 1. Auth validation auth := r.Header.Get("Authorization") - if !strings.HasPrefix(auth, "Bearer ") { + if auth != "" { + if !strings.HasPrefix(auth, "Bearer ") { + s.unauthorized(w, "Bearer token required") + return + } + token := strings.TrimSpace(strings.TrimPrefix(auth, "Bearer ")) + if !s.ValidateAccessToken(token) { + s.unauthorized(w, "Invalid or expired token") + return + } + } else if !s.cfg.OAuthProxy.AnonymousEnabled { s.unauthorized(w, "Bearer token required") return - } - token := strings.TrimSpace(strings.TrimPrefix(auth, "Bearer ")) - if !s.ValidateAccessToken(token) { - s.unauthorized(w, "Invalid or expired token") - return + } else { + allowed, toolsList := s.allowAnonymousMCPRequest(w, r) + if !allowed { + return + } + if toolsList { + r = r.WithContext(context.WithValue(r.Context(), anonymousToolsListContextKey{}, true)) + } } // 2. Path validation & boundary check @@ -138,3 +165,174 @@ func (s *Service) HandleProxy(w http.ResponseWriter, r *http.Request) { "client_id": s.cfg.OAuthProxy.ClientID, }) } + +type jsonRPCRequest struct { + Method string `json:"method"` + Params json.RawMessage `json:"params"` +} + +type toolCallParams struct { + Name string `json:"name"` +} + +func (s *Service) allowAnonymousMCPRequest(w http.ResponseWriter, r *http.Request) (allowed bool, toolsList bool) { + if r.Method != http.MethodPost { + s.auditLog(r, "anonymous_proxy_rejected", map[string]interface{}{"reason": "method_not_allowed", "method": r.Method}) + http.Error(w, "anonymous MCP requires POST", http.StatusMethodNotAllowed) + return false, false + } + + body, err := io.ReadAll(http.MaxBytesReader(w, r.Body, anonymousInspectionLimitBytes)) + if err != nil { + s.auditLog(r, "anonymous_proxy_rejected", map[string]interface{}{"reason": "body_too_large_or_unreadable"}) + http.Error(w, "invalid anonymous MCP request", http.StatusBadRequest) + return false, false + } + r.Body = io.NopCloser(bytes.NewReader(body)) + + allowed, toolsList, reason := s.anonymousPayloadAllowed(body) + if !allowed { + s.auditLog(r, "anonymous_proxy_rejected", map[string]interface{}{"reason": reason}) + http.Error(w, "anonymous MCP request is not allowed", http.StatusForbidden) + return false, false + } + + s.auditLog(r, "anonymous_proxy_allowed", nil) + return true, toolsList +} + +func (s *Service) anonymousPayloadAllowed(body []byte) (allowed bool, toolsList bool, reason string) { + var batch []json.RawMessage + if err := json.Unmarshal(body, &batch); err == nil { + if len(batch) == 0 { + return false, false, "empty_batch" + } + containsToolsList := false + for _, raw := range batch { + ok, isToolsList, reason := s.anonymousMessageAllowed(raw) + if !ok { + return false, false, reason + } + containsToolsList = containsToolsList || isToolsList + } + return true, containsToolsList, "" + } + + return s.anonymousMessageAllowed(body) +} + +func (s *Service) anonymousMessageAllowed(raw []byte) (allowed bool, toolsList bool, reason string) { + var msg jsonRPCRequest + if err := json.Unmarshal(raw, &msg); err != nil { + return false, false, "invalid_json" + } + + switch msg.Method { + case "initialize", "notifications/initialized", "ping": + return true, false, "" + case "tools/list": + return true, true, "" + case "tools/call": + var params toolCallParams + if err := json.Unmarshal(msg.Params, ¶ms); err != nil { + return false, false, "invalid_tool_call_params" + } + if params.Name == "" { + return false, false, "missing_tool_name" + } + if s.isAnonymousPublicTool(params.Name) { + return true, false, "" + } + return false, false, "tool_not_public" + default: + return false, false, "method_not_public" + } +} + +func (s *Service) isAnonymousPublicTool(name string) bool { + for _, tool := range s.cfg.OAuthProxy.AnonymousPublicTools { + if tool == name { + return true + } + } + return false +} + +func (s *Service) filterAnonymousToolsListResponse(resp *http.Response) error { + body, err := io.ReadAll(io.LimitReader(resp.Body, anonymousInspectionLimitBytes+1)) + if err != nil { + return err + } + if err := resp.Body.Close(); err != nil { + return err + } + if len(body) > anonymousInspectionLimitBytes { + return fmt.Errorf("anonymous tools/list response exceeds inspection limit") + } + + filtered := s.filterAnonymousToolsListBody(body) + resp.Body = io.NopCloser(bytes.NewReader(filtered)) + resp.ContentLength = int64(len(filtered)) + resp.Header.Set("Content-Length", fmt.Sprintf("%d", len(filtered))) + return nil +} + +func (s *Service) filterAnonymousToolsListBody(body []byte) []byte { + var value interface{} + if err := json.Unmarshal(body, &value); err == nil { + s.filterAnonymousToolsListJSON(value) + if filtered, err := json.Marshal(value); err == nil { + return filtered + } + return body + } + + lines := bytes.Split(body, []byte("\n")) + for i, line := range lines { + data, ok := bytes.CutPrefix(line, []byte("data: ")) + if !ok { + continue + } + var event interface{} + if err := json.Unmarshal(data, &event); err != nil { + continue + } + s.filterAnonymousToolsListJSON(event) + filtered, err := json.Marshal(event) + if err != nil { + continue + } + lines[i] = append([]byte("data: "), filtered...) + } + return bytes.Join(lines, []byte("\n")) +} + +func (s *Service) filterAnonymousToolsListJSON(value interface{}) { + switch typed := value.(type) { + case []interface{}: + for _, item := range typed { + s.filterAnonymousToolsListJSON(item) + } + case map[string]interface{}: + result, ok := typed["result"].(map[string]interface{}) + if !ok { + return + } + tools, ok := result["tools"].([]interface{}) + if !ok { + return + } + filtered := make([]interface{}, 0, len(tools)) + for _, tool := range tools { + toolObj, ok := tool.(map[string]interface{}) + if !ok { + continue + } + name, ok := toolObj["name"].(string) + if ok && s.isAnonymousPublicTool(name) { + filtered = append(filtered, tool) + } + } + result["tools"] = filtered + } +} diff --git a/internal/oauthproxy/proxy_test.go b/internal/oauthproxy/proxy_test.go index fb12c0c..01b02fd 100644 --- a/internal/oauthproxy/proxy_test.go +++ b/internal/oauthproxy/proxy_test.go @@ -1,6 +1,7 @@ package oauthproxy import ( + "bytes" "mcp-runtime-go/internal/config" mcpctx "mcp-runtime-go/internal/context" "mcp-runtime-go/internal/observability" @@ -216,6 +217,160 @@ func TestHandleProxy_AuthFailures(t *testing.T) { }) } +func TestHandleProxy_AnonymousReadOnlyTools(t *testing.T) { + var backendHits int + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + backendHits++ + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{"ok":true}}`)) + })) + defer backend.Close() + + tmpDir := t.TempDir() + cfg := &config.Config{ + OAuthProxy: config.OAuthProxyConfig{ + HugoMCPURL: backend.URL, + HugoToken: "test-token", + ClientID: "hugo-mcp", + AnonymousEnabled: true, + AnonymousPublicTools: []string{"search_posts", "read_page"}, + }, + } + audit := observability.NewAuditLogger(filepath.Join(tmpDir, "audit.log")) + store := storage.NewTokenStore(filepath.Join(tmpDir, "tokens.json"), false) + s, _ := NewService(cfg, store, audit, nil) + + t.Run("public tool is proxied without bearer token", func(t *testing.T) { + body := []byte(`{"jsonrpc":"2.0","id":1,"method":"tools/call","params":{"name":"search_posts","arguments":{"q":"hugo"}}}`) + req := httptest.NewRequest("POST", "/mcp", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + s.HandleProxy(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if backendHits != 1 { + t.Fatalf("expected backend to be hit once, got %d", backendHits) + } + }) + + t.Run("non public tool is rejected without reaching backend", func(t *testing.T) { + before := backendHits + body := []byte(`{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"publish_post","arguments":{}}}`) + req := httptest.NewRequest("POST", "/mcp", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + s.HandleProxy(rr, req) + + if rr.Code != http.StatusForbidden { + t.Fatalf("expected 403, got %d: %s", rr.Code, rr.Body.String()) + } + if backendHits != before { + t.Fatalf("backend was hit for forbidden anonymous tool") + } + }) + + t.Run("invalid bearer is rejected even when anonymous is enabled", func(t *testing.T) { + before := backendHits + body := []byte(`{"jsonrpc":"2.0","id":3,"method":"tools/call","params":{"name":"search_posts","arguments":{}}}`) + req := httptest.NewRequest("POST", "/mcp", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer invalid-token") + rr := httptest.NewRecorder() + + s.HandleProxy(rr, req) + + if rr.Code != http.StatusUnauthorized { + t.Fatalf("expected 401, got %d: %s", rr.Code, rr.Body.String()) + } + if backendHits != before { + t.Fatalf("backend was hit for invalid bearer") + } + }) +} + +func TestHandleProxy_AnonymousToolsListFiltersProtectedTools(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"jsonrpc":"2.0","id":1,"result":{"tools":[{"name":"search_posts"},{"name":"publish_post"}]}}`)) + })) + defer backend.Close() + + tmpDir := t.TempDir() + cfg := &config.Config{ + OAuthProxy: config.OAuthProxyConfig{ + HugoMCPURL: backend.URL, + HugoToken: "test-token", + ClientID: "hugo-mcp", + AnonymousEnabled: true, + AnonymousPublicTools: []string{"search_posts"}, + }, + } + audit := observability.NewAuditLogger(filepath.Join(tmpDir, "audit.log")) + store := storage.NewTokenStore(filepath.Join(tmpDir, "tokens.json"), false) + s, _ := NewService(cfg, store, audit, nil) + + req := httptest.NewRequest("POST", "/mcp", bytes.NewReader([]byte(`{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{}}`))) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + s.HandleProxy(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), `"search_posts"`) { + t.Fatalf("public tool missing from response: %s", rr.Body.String()) + } + if strings.Contains(rr.Body.String(), "publish_post") { + t.Fatalf("protected tool leaked in anonymous tools/list: %s", rr.Body.String()) + } +} + +func TestHandleProxy_AnonymousToolsListFiltersServerSentEvents(t *testing.T) { + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + w.Write([]byte("event: message\n")) + w.Write([]byte(`data: {"jsonrpc":"2.0","id":1,"result":{"tools":[{"name":"search_posts"},{"name":"publish_post"}]}}` + "\n\n")) + })) + defer backend.Close() + + tmpDir := t.TempDir() + cfg := &config.Config{ + OAuthProxy: config.OAuthProxyConfig{ + HugoMCPURL: backend.URL, + HugoToken: "test-token", + ClientID: "hugo-mcp", + AnonymousEnabled: true, + AnonymousPublicTools: []string{"search_posts"}, + }, + } + audit := observability.NewAuditLogger(filepath.Join(tmpDir, "audit.log")) + store := storage.NewTokenStore(filepath.Join(tmpDir, "tokens.json"), false) + s, _ := NewService(cfg, store, audit, nil) + + req := httptest.NewRequest("POST", "/mcp", bytes.NewReader([]byte(`{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{}}`))) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + s.HandleProxy(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if !strings.Contains(rr.Body.String(), `"search_posts"`) { + t.Fatalf("public tool missing from SSE response: %s", rr.Body.String()) + } + if strings.Contains(rr.Body.String(), "publish_post") { + t.Fatalf("protected tool leaked in anonymous SSE tools/list: %s", rr.Body.String()) + } +} + func TestHandleProxy_BackendMissing(t *testing.T) { tmpDir := t.TempDir() cfg := &config.Config{