Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.

Expand Down
24 changes: 24 additions & 0 deletions docs/ARCHITECTURE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -86,6 +108,8 @@ Common runtime controls:
- `AUDIT_LOG_FILE`
- `TRUSTED_PROXIES`
- `MANDATORY_PKCE`
- `ANONYMOUS_ENABLED`
- `ANONYMOUS_PUBLIC_TOOLS`

## Security Model

Expand Down
126 changes: 126 additions & 0 deletions docs/CODEQL_REDIRECT_ALERTS_DIAGNOSTIC.md
Original file line number Diff line number Diff line change
@@ -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.
25 changes: 25 additions & 0 deletions docs/OPERATIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
5 changes: 5 additions & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
37 changes: 37 additions & 0 deletions internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
Loading
Loading