From aa3832b227435fc4f9b7464248ca92b94403a0cb Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 14 Jul 2026 13:28:44 -0500 Subject: [PATCH 1/3] fix: Bind inherited MCP credentials to configured REST origin Signed-off-by: Kyle Felter --- helm/rest/nico-mcp/values.yaml | 11 +- rest-api/Makefile | 2 +- rest-api/cli/README.md | 10 +- rest-api/cli/pkg/mcp.go | 14 +- rest-api/mcp/cmd/nico-mcp/main.go | 6 +- rest-api/mcp/internal/server/config.go | 62 +++--- rest-api/mcp/internal/server/config_test.go | 76 ++++++- rest-api/mcp/internal/server/schema.go | 6 +- rest-api/mcp/internal/server/server.go | 51 ++++- rest-api/mcp/internal/server/server_test.go | 27 +++ .../mcp/internal/server/transport_test.go | 188 ++++++++++++++++++ 11 files changed, 389 insertions(+), 64 deletions(-) diff --git a/helm/rest/nico-mcp/values.yaml b/helm/rest/nico-mcp/values.yaml index 1fd44897e3..4224fc9b08 100644 --- a/helm/rest/nico-mcp/values.yaml +++ b/helm/rest/nico-mcp/values.yaml @@ -21,15 +21,14 @@ service: type: ClusterIP port: 8080 -# MCP server configuration. The server is stateless and parameter-driven: -# per-call tool arguments and the inbound Authorization header normally supply -# the connection details, so the defaults below are optional and are typically -# left empty. Tokens are intentionally NOT exposed here -- supply them per-call -# or via the inbound Authorization header. +# MCP server configuration. Set baseURL when forwarding inbound Authorization +# headers; a per-call base_url must match it. When baseURL is empty, only a +# token supplied in the same call may be sent to a per-call base_url. Tokens +# are intentionally NOT exposed here. config: path: /mcp shutdownTimeout: 10s - # -- Optional server-side default base URL (a per-call base_url overrides it) + # -- Trusted NICo REST base URL (a per-call base_url must match when set) baseURL: "" # -- Optional server-side default org (a per-call org overrides it) org: "" diff --git a/rest-api/Makefile b/rest-api/Makefile index abe16c1978..97172ca9a0 100644 --- a/rest-api/Makefile +++ b/rest-api/Makefile @@ -632,7 +632,7 @@ helm-deploy-site-agent: # Deploy the standalone MCP server chart. The server is parameter-driven; for # local dev we point its default base URL at the in-cluster API so a tool call -# only needs to supply org + token (a per-call base_url still overrides this). +# only needs to supply org + token (a per-call base_url must match this value). helm-deploy-mcp: helm upgrade --install nico-mcp $(MCP_CHART)/ \ --namespace nico-rest --create-namespace $(HELM_SET) \ diff --git a/rest-api/cli/README.md b/rest-api/cli/README.md index 91f5dd1bf8..19a72f2a61 100644 --- a/rest-api/cli/README.md +++ b/rest-api/cli/README.md @@ -374,7 +374,7 @@ Install the binaries with `make nico-cli` and `make nico-mcp`, run from the `res - **Read-only.** Only `GET` operations are exposed. Mutating routes (`POST`, `PATCH`, `PUT`, `DELETE`) are intentionally excluded. - **Tool naming.** Tools are named `nico_` (e.g. `nico_get_all_site`, `nico_validate_rack`). - **Stateless and request/response only.** The server sets `Stateless: true` and `JSONResponse: true` on the MCP streamable-HTTP handler -- responses are always `Content-Type: application/json`, never `text/event-stream`, and the server retains no per-session state. -- **JWT passthrough.** The `Authorization: Bearer ` header on the inbound MCP request is forwarded unchanged to NICo REST. NICo REST validates the JWT, resolves the caller org, and enforces role-based authorization. The MCP layer never makes the authz decision itself. +- **JWT passthrough.** The `Authorization: Bearer ` header on the inbound MCP request is forwarded unchanged only to the server's configured NICo REST base URL. NICo REST validates the JWT, resolves the caller org, and enforces role-based authorization. The MCP layer never makes the authz decision itself. ### Flags @@ -384,11 +384,11 @@ Install the binaries with `make nico-cli` and `make nico-mcp`, run from the `res | `--path` | `NICO_MCP_PATH` | HTTP path the MCP handler is mounted at (default `/mcp`) | | `--shutdown-timeout` | `NICO_MCP_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout (default `10s`) | -`--base-url`, `--org`, `--api-name`, and `--token` are accepted directly by `nico-mcp` and provide optional server-side defaults; each also reads its `NICO_*` environment variable. The MCP server does **not** read `~/.nico/config.yaml`: it is stateless and entirely parameter-driven, so it starts cleanly with no config file present and every connection detail is supplied per tool call (see below), falling back to these flags only when an argument is omitted. +`--base-url`, `--org`, `--api-name`, and `--token` are accepted directly by `nico-mcp`; each also reads its `NICO_*` environment variable. `--base-url` pins the trusted NICo REST destination, so a per-call `base_url` must match it when configured. Without `--base-url`, a tool call may select its destination, but the server rejects the call rather than forwarding an inbound Authorization header or default token to it. The MCP server does **not** read `~/.nico/config.yaml` and starts cleanly with no config file present. -### Per-call config overrides +### Per-call config -Every typical config value can also be passed as an argument on each MCP tool call, layered on top of the server defaults: +Connection values can be passed on each MCP tool call. `org`, `api_name`, and `token` override server defaults; `base_url` supplies the destination only when no server base URL is configured and otherwise must match it: | Tool arg | Equivalent flag | Config field | |----------|-----------------|--------------| @@ -397,7 +397,7 @@ Every typical config value can also be passed as an argument on each MCP tool ca | `api_name` | `--api-name` | `api.name` | | `token` | `--token` | `auth.token` | -Precedence per tool call (first non-empty wins): tool argument -> inbound `Authorization` header (token only) -> server startup flag/env. The MCP server does not read the on-disk config file. OIDC credentials and NGC api_key settings are NOT exposed as tool arguments -- they are login-flow inputs configured server-side via flags/env. +Token precedence per tool call is: tool argument -> inbound `Authorization` header -> server startup flag/env. Inbound and startup tokens are eligible only when a server base URL is configured; a caller-selected destination can use an explicit per-call token or no token. The MCP server does not read the on-disk config file or resolve OIDC and NGC credentials; clients or an upstream gateway must supply the resolved bearer through the per-call `token` argument or inbound `Authorization` header. ### Probing the server diff --git a/rest-api/cli/pkg/mcp.go b/rest-api/cli/pkg/mcp.go index fc1d90fcea..4e00674e8f 100644 --- a/rest-api/cli/pkg/mcp.go +++ b/rest-api/cli/pkg/mcp.go @@ -40,17 +40,19 @@ Common flags (each also reads its NICO_* environment variable): --listen address:port to listen on (default ":8080") --path HTTP path the MCP handler is mounted at (default "/mcp") - --base-url default NICo REST base URL + --base-url trusted NICo REST base URL; per-call base_url must match --org default org used in /v2/org//... paths --api-name API path segment in /v2/org///... (default "nico") - --token default bearer token + --token default bearer token for the configured base URL --shutdown-timeout graceful shutdown timeout (default 10s) --debug enable debug logging -The server is stateless: base_url, org, and token can be supplied per tool -call (token also via an inbound Authorization header), with the flags above as -defaults. Point your MCP client at http:// (default -http://localhost:8080/mcp). Run "nico-mcp --help" for the full flag list. +The server is stateless. A configured base URL pins the destination and is the +only destination that may receive an inbound or default bearer token. Without +one, a per-call base_url may use an explicit per-call token or no token; the +server rejects inherited credentials. Point your MCP client at +http:// (default http://localhost:8080/mcp). Run "nico-mcp +--help" for the full flag list. ` func printMCPInstructions(c *cli.Context) error { diff --git a/rest-api/mcp/cmd/nico-mcp/main.go b/rest-api/mcp/cmd/nico-mcp/main.go index 1f7c30cedb..5ce5575f75 100644 --- a/rest-api/mcp/cmd/nico-mcp/main.go +++ b/rest-api/mcp/cmd/nico-mcp/main.go @@ -19,9 +19,9 @@ func main() { Description: "Serves the NICo REST read surface as MCP tools at the configured\n" + "path and listen address. The server is stateless and never emits\n" + "text/event-stream responses; every tool/call returns a single JSON\n" + - "body. Authentication is per-call: a token argument or the inbound\n" + - "Authorization header is forwarded to NICo REST, which makes the\n" + - "authorization decision.", + "body. Inbound or default credentials are forwarded only to the\n" + + "configured NICo REST base URL. A caller-selected base_url can use\n" + + "only a token supplied in the same tool call.", Flags: server.ServeFlags(), Action: func(c *cli.Context) error { return server.Run(c, openapi.Spec) diff --git a/rest-api/mcp/internal/server/config.go b/rest-api/mcp/internal/server/config.go index e844d4839f..5731e216e1 100644 --- a/rest-api/mcp/internal/server/config.go +++ b/rest-api/mcp/internal/server/config.go @@ -12,19 +12,19 @@ import ( "github.com/sirupsen/logrus" ) -// Options carry the server-side defaults that every tool invocation -// starts from. Individual tool calls override these per request through -// resolveCallConfig. +// Options carry the server-side settings that tool invocations resolve against +// through FromCallConfig. BaseURL also bounds the allowed destination. type Options struct { - // BaseURL is the NICo REST base URL (e.g. https://nico.example.com). + // BaseURL is the trusted NICo REST base URL (e.g. https://nico.example.com). + // When set, a per-call base_url must resolve to the same value. BaseURL string // Org is the default organisation used in /v2/org//... paths. Org string // APIName is the API path segment between org and resource (default // "nico", overridable via api.name in config). APIName string - // Token is the static bearer used when no inbound bearer or tool - // arg token is provided. + // Token is the static bearer used with BaseURL when no inbound bearer or + // tool arg token is provided. It is not sent to a caller-selected base URL. Token string // Debug enables logrus debug-level HTTP request/response logging // through to the appcli.Client. @@ -47,22 +47,22 @@ func (o Options) withDefaults() Options { return o } -// commonConfigDescriptions documents the four per-call config overrides -// that are merged into every tool's input schema. Kept as a slice (not +// commonConfigDescriptions documents the four per-call config fields that are +// merged into every tool's input schema. Kept as a slice (not // a map) so the schema render order is stable. var commonConfigDescriptions = []struct { Name string Desc string }{ {"org", "Org used in /v2/org//... paths for this call. Overrides the server startup flag/env default when set."}, - {"base_url", "NICo REST base URL for this call. Overrides the server startup flag/env default when set; useful when one MCP server fronts multiple NICo REST deployments."}, + {"base_url", "NICo REST base URL for this call. When the server has a configured base URL, this value must match it. Otherwise, only a token supplied in the same tool call may be sent to this destination."}, {"api_name", "Override the API path segment used in /v2/org///... (api.name; default \"nico\")."}, - {"token", "Bearer token for this call. Overrides the inbound Authorization header. Omit it when an upstream proxy injects the Authorization header, which is passed through to NICo REST unchanged."}, + {"token", "Bearer token for this call. Overrides the inbound Authorization header. Inbound or default credentials are forwarded only to the server's configured base URL."}, } -// resolvedConfig is the result of merging Options with the per-call -// overrides for one tool invocation. It is consumed by registerGET to -// construct a fresh appcli.Client. +// resolvedConfig is the result of resolving Options with the per-call values +// for one tool invocation. It is consumed by registerGET to construct a fresh +// appcli.Client. type resolvedConfig struct { BaseURL string Org string @@ -74,28 +74,42 @@ type resolvedConfig struct { // documented in the design plan: // // 1. Tool-call argument (org, base_url, api_name, token) -// 2. Inbound Authorization header (token only) +// 2. Inbound Authorization header (token only, for a configured BaseURL) // 3. Server startup flag / Options (BaseURL, Org, APIName, Token) // -// It returns an error when a required field (org, base_url) ends up -// empty so the tool handler can surface a JSON-RPC error instead of -// letting the call go out with an invalid URL. +// A configured BaseURL binds all calls to that destination. Without one, +// inherited inbound or default credentials are not accepted for a per-call +// destination; if authentication is needed, callers must provide the token in +// the same tool call. It returns an error when this policy is violated or a +// required field (org, base_url) ends up empty, before the handler constructs +// an outbound request. func (cfg *resolvedConfig) FromCallConfig(in map[string]any, req *mcp.CallToolRequest, opts Options) error { - cfg.BaseURL = normalizeBaseURL(cmp.Or(stringArg(in, "base_url"), opts.BaseURL)) + callBaseURL := normalizeBaseURL(stringArg(in, "base_url")) + configuredBaseURL := normalizeBaseURL(opts.BaseURL) + if configuredBaseURL != "" && callBaseURL != "" && callBaseURL != configuredBaseURL { + return fmt.Errorf("per-call base_url does not match the configured server base URL") + } + + callToken := stringArg(in, "token") + inboundToken := bearerFromExtra(req) + if configuredBaseURL == "" && callBaseURL != "" && callToken == "" && + (inboundToken != "" || opts.Token != "") { + return fmt.Errorf("refusing to forward inherited credentials to a per-call base_url; pass token in the same tool call or configure the server base URL") + } + + cfg.BaseURL = cmp.Or(callBaseURL, configuredBaseURL) cfg.Org = cmp.Or(stringArg(in, "org"), opts.Org) cfg.APIName = cmp.Or(stringArg(in, "api_name"), opts.APIName) cfg.Token = normalizeToken(cmp.Or( - stringArg(in, "token"), - bearerFromExtra(req), + callToken, + inboundToken, opts.Token, )) return cfg.requireNonEmpty() } -// requireNonEmpty returns a descriptive error when org or BaseURL are -// blank. Token can be empty -- NICo REST will reject the request with -// 401 and the response surfaces to the caller as an MCP error result; -// that path is exercised by the bearer-passthrough integration test. +// requireNonEmpty returns a descriptive error when org or BaseURL are blank. +// Token may be empty; appcli.Client then sends no Authorization header. func (c resolvedConfig) requireNonEmpty() error { missing := []string{} if c.Org == "" { diff --git a/rest-api/mcp/internal/server/config_test.go b/rest-api/mcp/internal/server/config_test.go index d2fa8c9284..6f03bff165 100644 --- a/rest-api/mcp/internal/server/config_test.go +++ b/rest-api/mcp/internal/server/config_test.go @@ -14,7 +14,7 @@ import ( func TestFromCallConfig_PrecedenceChain(t *testing.T) { type expect struct { baseURL, org, apiName, token string - wantErr bool + errContains string } cases := []struct { name string @@ -24,7 +24,7 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { expected expect }{ { - name: "tool_args_win_every_field", + name: "tool_args_resolve_every_field_without_configured_base_url", in: map[string]any{ "base_url": "https://from-arg.example.com/", "org": "arg-org", @@ -32,7 +32,7 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { "token": "Bearer arg-token", }, req: requestWithBearer("inbound-bearer"), - opts: Options{BaseURL: "https://opts.example.com", Org: "opts-org", APIName: "opts-name", Token: "opts-token"}, + opts: Options{Org: "opts-org", APIName: "opts-name", Token: "opts-token"}, expected: expect{ baseURL: "https://from-arg.example.com", org: "arg-org", @@ -40,6 +40,66 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { token: "arg-token", }, }, + { + name: "configured_base_url_rejects_different_call_destination", + in: map[string]any{ + "base_url": "https://from-arg.example.com", + "org": "arg-org", + "token": "arg-token", + }, + req: requestWithBearer("inbound-bearer"), + opts: Options{BaseURL: "https://opts.example.com", Org: "opts-org", Token: "opts-token"}, + expected: expect{ + errContains: "does not match the configured server base URL", + }, + }, + { + name: "configured_base_url_accepts_matching_normalized_call_destination", + in: map[string]any{ + "base_url": "https://opts.example.com///", + }, + req: requestWithBearer("from-header"), + opts: Options{BaseURL: "https://opts.example.com/", Org: "opts-org", APIName: "nico", Token: "opts-token"}, + expected: expect{ + baseURL: "https://opts.example.com", + org: "opts-org", + apiName: "nico", + token: "from-header", + }, + }, + { + name: "dynamic_destination_rejects_inbound_bearer_without_call_token", + in: map[string]any{ + "base_url": "https://from-arg.example.com", + }, + req: requestWithBearer("from-header"), + opts: Options{Org: "opts-org", APIName: "nico"}, + expected: expect{ + errContains: "refusing to forward inherited credentials", + }, + }, + { + name: "dynamic_destination_rejects_default_token_without_call_token", + in: map[string]any{ + "base_url": "https://from-arg.example.com", + }, + opts: Options{Org: "opts-org", APIName: "nico", Token: "opts-token"}, + expected: expect{ + errContains: "refusing to forward inherited credentials", + }, + }, + { + name: "dynamic_destination_without_credentials_is_allowed", + in: map[string]any{ + "base_url": "https://from-arg.example.com/", + }, + opts: Options{Org: "opts-org", APIName: "nico"}, + expected: expect{ + baseURL: "https://from-arg.example.com", + org: "opts-org", + apiName: "nico", + }, + }, { name: "inbound_bearer_wins_when_no_token_arg", in: map[string]any{}, @@ -70,7 +130,7 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { req: nil, opts: Options{BaseURL: "https://opts.example.com"}, expected: expect{ - wantErr: true, + errContains: "missing required config value(s): org", }, }, { @@ -79,7 +139,7 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { req: nil, opts: Options{Org: "opts-org"}, expected: expect{ - wantErr: true, + errContains: "missing required config value(s): base_url", }, }, { @@ -88,7 +148,7 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { req: nil, opts: Options{}, expected: expect{ - wantErr: true, + errContains: "missing required config value(s): org, base_url", }, }, { @@ -129,8 +189,8 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { t.Run(c.name, func(t *testing.T) { var cfg resolvedConfig err := cfg.FromCallConfig(c.in, c.req, c.opts) - if c.expected.wantErr { - require.Error(t, err) + if c.expected.errContains != "" { + require.ErrorContains(t, err, c.expected.errContains) return } require.NoError(t, err) diff --git a/rest-api/mcp/internal/server/schema.go b/rest-api/mcp/internal/server/schema.go index 1b46214325..34db7e4c98 100644 --- a/rest-api/mcp/internal/server/schema.go +++ b/rest-api/mcp/internal/server/schema.go @@ -14,7 +14,7 @@ import ( // NicoOpenApiHandler builds the MCP tool input schema for a single OpenAPI // GET operation, combining the path-item and operation-level parameters with -// the common per-call config override fields. +// the common per-call config fields. type NicoOpenApiHandler struct { schema jsonschema.Schema } @@ -49,9 +49,9 @@ func mergeParameters(item *openapi3.PathItem, op *openapi3.Operation) []*openapi } // buildInput populates the handler's schema from the operation's path and -// query parameters merged with the four common config override fields (org, +// query parameters merged with the four common config fields (org, // base_url, api_name, token) and returns it. Path parameters are required; -// OpenAPI-required query parameters are required; the config overrides are +// OpenAPI-required query parameters are required; the config fields are // always optional. func (h *NicoOpenApiHandler) buildInput(item *openapi3.PathItem, op *openapi3.Operation) *jsonschema.Schema { props := map[string]*jsonschema.Schema{} diff --git a/rest-api/mcp/internal/server/server.go b/rest-api/mcp/internal/server/server.go index 5a6a4d86af..d440557137 100644 --- a/rest-api/mcp/internal/server/server.go +++ b/rest-api/mcp/internal/server/server.go @@ -14,6 +14,7 @@ import ( "fmt" "maps" "net/http" + "net/url" "os" "os/signal" "slices" @@ -32,9 +33,9 @@ import ( // BuildServer constructs an *mcp.Server with one tool registered for // every GET operation in the supplied OpenAPI spec. Tool names follow // the SDD: nico_. Each tool handler builds a -// fresh appcli.Client per call from resolvedConfig.FromCallConfig and -// forwards the bearer token from the inbound MCP request to NICo REST -// unchanged. +// fresh appcli.Client per call from resolvedConfig.FromCallConfig. Inbound +// bearer tokens are forwarded unchanged only to the configured NICo REST +// base URL. // // BuildServer does not start a listener; callers wrap the result with // NewHandler to get an *http.Handler ready for ListenAndServe. @@ -123,7 +124,7 @@ func ServeFlags() []urfave.Flag { }, &urfave.StringFlag{ Name: "base-url", - Usage: "default NICo REST base URL (a per-call base_url argument overrides this)", + Usage: "trusted NICo REST base URL (a per-call base_url must match when this is set)", EnvVars: []string{"NICO_BASE_URL"}, }, &urfave.StringFlag{ @@ -139,7 +140,7 @@ func ServeFlags() []urfave.Flag { }, &urfave.StringFlag{ Name: "token", - Usage: "default bearer token (a per-call token argument or inbound Authorization header overrides this)", + Usage: "default bearer token for the configured base URL (a per-call token or inbound Authorization header overrides this)", EnvVars: []string{"NICO_TOKEN"}, }, &urfave.BoolFlag{ @@ -237,6 +238,7 @@ func registerGET(server *mcp.Server, path string, item *openapi3.PathItem, opts return errorResult(err), nil, nil } client := appcli.NewClient(cfg.BaseURL, cfg.Org, cfg.Token, opts.Log, opts.Debug) + client.HTTPClient.CheckRedirect = sameOriginRedirectPolicy client.APIName = cfg.APIName pathParams, queryParams, err := splitArgs(in, allParams) @@ -251,6 +253,38 @@ func registerGET(server *mcp.Server, path string, item *openapi3.PathItem, opts }) } +// sameOriginRedirectPolicy preserves net/http's redirect limit while refusing +// to send an MCP request to a different origin. +func sameOriginRedirectPolicy(req *http.Request, via []*http.Request) error { + if len(via) >= 10 { + return errors.New("stopped after 10 redirects") + } + if len(via) == 0 || sameOrigin(via[0].URL, req.URL) { + return nil + } + return errors.New("refusing cross-origin redirect") +} + +func sameOrigin(a, b *url.URL) bool { + return strings.EqualFold(a.Scheme, b.Scheme) && + strings.EqualFold(a.Hostname(), b.Hostname()) && + effectivePort(a) == effectivePort(b) +} + +func effectivePort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + switch strings.ToLower(u.Scheme) { + case "http": + return "80" + case "https": + return "443" + default: + return "" + } +} + // toolName converts an operationId to the SDD's canonical MCP tool name // nico_. It handles kebab-case (get-all-site -> // nico_get_all_site) and camelCase (getFooStatus -> nico_get_foo_status) @@ -366,9 +400,10 @@ func registerHandler(mux *http.ServeMux, path string, handler http.Handler) (err // process flags (each of which also reads its NICO_* environment // variable). Unlike the dynamically-generated CLI commands, nico-mcp does // NOT read ~/.nico/config.yaml: the server is stateless and entirely -// parameter-driven, so every connection detail is supplied per tool call -// via resolvedConfig.FromCallConfig, with these flag values as the only -// fallback. This lets nico-mcp start cleanly with no config file present. +// parameter-driven, so connection details are resolved per tool call via +// resolvedConfig.FromCallConfig, with these flag values as fallbacks. A +// configured base URL binds requests and inherited credentials to that +// destination. This lets nico-mcp start cleanly with no config file present. func buildServeOptions(c *urfave.Context) Options { log := logrus.NewEntry(logrus.StandardLogger()) if c.Bool("debug") { diff --git a/rest-api/mcp/internal/server/server_test.go b/rest-api/mcp/internal/server/server_test.go index 3ba0e74227..903dfb0ec5 100644 --- a/rest-api/mcp/internal/server/server_test.go +++ b/rest-api/mcp/internal/server/server_test.go @@ -6,6 +6,7 @@ package server import ( "encoding/json" "net/http" + "net/url" "testing" "github.com/getkin/kin-openapi/openapi3" @@ -53,6 +54,32 @@ func TestToolDescription(t *testing.T) { }) } +func TestSameOrigin(t *testing.T) { + tests := []struct { + name string + a string + b string + want bool + }{ + {name: "same_origin", a: "https://example.com/a", b: "https://example.com/b", want: true}, + {name: "host_is_case_insensitive", a: "https://EXAMPLE.com/a", b: "https://example.com/b", want: true}, + {name: "implicit_and_explicit_default_port", a: "https://example.com/a", b: "https://example.com:443/b", want: true}, + {name: "different_scheme", a: "http://example.com/a", b: "https://example.com/b", want: false}, + {name: "different_host", a: "https://one.example.com/a", b: "https://two.example.com/b", want: false}, + {name: "different_effective_port", a: "https://example.com/a", b: "https://example.com:8443/b", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a, err := url.Parse(tt.a) + require.NoError(t, err) + b, err := url.Parse(tt.b) + require.NoError(t, err) + require.Equal(t, tt.want, sameOrigin(a, b)) + }) + } +} + func TestSplitArgs(t *testing.T) { params := []*openapi3.Parameter{ {Name: "org", In: "path"}, diff --git a/rest-api/mcp/internal/server/transport_test.go b/rest-api/mcp/internal/server/transport_test.go index 53a6c6633f..0faf869c74 100644 --- a/rest-api/mcp/internal/server/transport_test.go +++ b/rest-api/mcp/internal/server/transport_test.go @@ -228,6 +228,194 @@ func TestHandler_ToolsCall_TokenArgWins(t *testing.T) { require.Equal(t, "Bearer explicit-tool-arg-token", recordedAuth.Load()) } +func TestHandler_ToolsCall_RejectsUnapprovedCredentialDestination(t *testing.T) { + tests := []struct { + name string + opts Options + authorization string + wantError string + }{ + { + name: "configured_base_url_with_inbound_bearer", + opts: Options{BaseURL: "https://configured.example.com", Org: "tester"}, + authorization: "Bearer inbound-bearer", + wantError: "does not match the configured server base URL", + }, + { + name: "configured_base_url_with_default_token", + opts: Options{BaseURL: "https://configured.example.com", Org: "tester", Token: "default-token"}, + wantError: "does not match the configured server base URL", + }, + { + name: "dynamic_base_url_with_inbound_bearer", + opts: Options{Org: "tester"}, + authorization: "Bearer inbound-bearer", + wantError: "refusing to forward inherited credentials", + }, + { + name: "dynamic_base_url_with_default_token", + opts: Options{Org: "tester", Token: "default-token"}, + wantError: "refusing to forward inherited credentials", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var requests atomic.Int64 + capture := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + })) + defer capture.Close() + + server, err := BuildServer([]byte(synthSpec), tt.opts) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, tt.authorization, jsonrpcRequest(4, "tools/call", map[string]any{ + "name": "nico_get_all_foo", + "arguments": map[string]any{ + "base_url": capture.URL, + }, + })) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + result := decodeToolCallResult(t, body) + require.True(t, result.IsError, "tool call should be rejected: %s", body) + require.Contains(t, firstText(result), tt.wantError) + require.Zero(t, requests.Load(), "unapproved destination received an outbound request") + }) + } +} + +func TestHandler_ToolsCall_DynamicDestinationUsesExplicitToken(t *testing.T) { + var recordedAuth atomic.Value + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + recordedAuth.Store(r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + })) + defer upstream.Close() + + server, err := BuildServer([]byte(synthSpec), Options{Org: "tester", Token: "default-token"}) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, "Bearer inbound-bearer", jsonrpcRequest(5, "tools/call", map[string]any{ + "name": "nico_get_all_foo", + "arguments": map[string]any{ + "base_url": upstream.URL, + "token": "explicit-tool-arg-token", + }, + })) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + result := decodeToolCallResult(t, body) + require.False(t, result.IsError, "tool call should succeed: %s", body) + require.Equal(t, "Bearer explicit-tool-arg-token", recordedAuth.Load()) +} + +func TestHandler_ToolsCall_CrossOriginRedirectRejected(t *testing.T) { + for _, tt := range []struct { + name string + authorization string + }{ + {name: "authenticated", authorization: "Bearer caller-jwt"}, + {name: "unauthenticated"}, + } { + t.Run(tt.name, func(t *testing.T) { + var ( + captureRequests atomic.Int64 + captureAuth atomic.Value + sourceAuth atomic.Value + ) + captureAuth.Store("") + sourceAuth.Store("") + capture := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captureRequests.Add(1) + captureAuth.Store(r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + })) + defer capture.Close() + + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sourceAuth.Store(r.Header.Get("Authorization")) + http.Redirect(w, r, capture.URL, http.StatusTemporaryRedirect) + })) + defer source.Close() + + server, err := BuildServer([]byte(synthSpec), Options{BaseURL: source.URL, Org: "tester"}) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, tt.authorization, jsonrpcRequest(6, "tools/call", map[string]any{ + "name": "nico_get_all_foo", + "arguments": map[string]any{}, + })) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + result := decodeToolCallResult(t, body) + require.True(t, result.IsError, "tool call should be rejected: %s", body) + require.Contains(t, firstText(result), "refusing cross-origin redirect") + require.Equal(t, tt.authorization, sourceAuth.Load()) + require.Zero(t, captureRequests.Load(), "cross-origin redirect destination received a request") + require.Empty(t, captureAuth.Load(), "cross-origin redirect destination received Authorization") + }) + } +} + +func TestHandler_ToolsCall_SameOriginRedirectAllowed(t *testing.T) { + var ( + requests atomic.Int64 + redirectedAuth atomic.Value + ) + redirectedAuth.Store("") + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + if r.URL.Path != "/redirected" { + http.Redirect(w, r, "/redirected", http.StatusTemporaryRedirect) + return + } + redirectedAuth.Store(r.Header.Get("Authorization")) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + })) + defer upstream.Close() + + server, err := BuildServer([]byte(synthSpec), Options{BaseURL: upstream.URL, Org: "tester", Token: "default-token"}) + require.NoError(t, err) + ts := httptest.NewServer(NewHandler(server)) + defer ts.Close() + + resp := mcpPost(t, ts.URL, "", jsonrpcRequest(7, "tools/call", map[string]any{ + "name": "nico_get_all_foo", + "arguments": map[string]any{}, + })) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + result := decodeToolCallResult(t, body) + require.False(t, result.IsError, "tool call should succeed: %s", body) + require.Equal(t, int64(2), requests.Load()) + require.Equal(t, "Bearer default-token", redirectedAuth.Load()) +} + // TestHandler_ConcurrentCallersDoNotBleedTokens proves the // statelessness invariant: two callers hitting the same handler with // different bearers each get their own bearer forwarded to NICo REST. From 6bc415cd7f16bce93feed54e89bb30da0ded9b8f Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 14 Jul 2026 17:12:51 -0500 Subject: [PATCH 2/3] fix: Compare configured MCP base URLs canonically --- rest-api/cli/README.md | 2 +- rest-api/mcp/internal/server/config.go | 2 +- rest-api/mcp/internal/server/config_test.go | 24 +++++++++++++++++++++ rest-api/mcp/internal/server/util.go | 17 +++++++++++++++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/rest-api/cli/README.md b/rest-api/cli/README.md index 19a72f2a61..15aba0a81f 100644 --- a/rest-api/cli/README.md +++ b/rest-api/cli/README.md @@ -384,7 +384,7 @@ Install the binaries with `make nico-cli` and `make nico-mcp`, run from the `res | `--path` | `NICO_MCP_PATH` | HTTP path the MCP handler is mounted at (default `/mcp`) | | `--shutdown-timeout` | `NICO_MCP_SHUTDOWN_TIMEOUT` | Graceful shutdown timeout (default `10s`) | -`--base-url`, `--org`, `--api-name`, and `--token` are accepted directly by `nico-mcp`; each also reads its `NICO_*` environment variable. `--base-url` pins the trusted NICo REST destination, so a per-call `base_url` must match it when configured. Without `--base-url`, a tool call may select its destination, but the server rejects the call rather than forwarding an inbound Authorization header or default token to it. The MCP server does **not** read `~/.nico/config.yaml` and starts cleanly with no config file present. +`--base-url`, `--org`, `--api-name`, and `--token` are accepted directly by `nico-mcp`; each also reads its `NICO_*` environment variable. `--base-url` pins the trusted NICo REST destination, so a per-call `base_url` must match it when configured. Without `--base-url`, a tool call may select its destination and proceed without credentials or with an explicit per-call token; only inherited credentials from the inbound Authorization header or server default token are rejected rather than forwarded. The MCP server does **not** read `~/.nico/config.yaml` and starts cleanly with no config file present. ### Per-call config diff --git a/rest-api/mcp/internal/server/config.go b/rest-api/mcp/internal/server/config.go index 5731e216e1..810e814d32 100644 --- a/rest-api/mcp/internal/server/config.go +++ b/rest-api/mcp/internal/server/config.go @@ -86,7 +86,7 @@ type resolvedConfig struct { func (cfg *resolvedConfig) FromCallConfig(in map[string]any, req *mcp.CallToolRequest, opts Options) error { callBaseURL := normalizeBaseURL(stringArg(in, "base_url")) configuredBaseURL := normalizeBaseURL(opts.BaseURL) - if configuredBaseURL != "" && callBaseURL != "" && callBaseURL != configuredBaseURL { + if configuredBaseURL != "" && callBaseURL != "" && !sameBaseURL(callBaseURL, configuredBaseURL) { return fmt.Errorf("per-call base_url does not match the configured server base URL") } diff --git a/rest-api/mcp/internal/server/config_test.go b/rest-api/mcp/internal/server/config_test.go index 6f03bff165..40536da8f6 100644 --- a/rest-api/mcp/internal/server/config_test.go +++ b/rest-api/mcp/internal/server/config_test.go @@ -67,6 +67,30 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { token: "from-header", }, }, + { + name: "configured_base_url_accepts_matching_scheme_and_host_case", + in: map[string]any{ + "base_url": "https://OPTS.example.com/v2/", + }, + req: requestWithBearer("from-header"), + opts: Options{BaseURL: "HTTPS://opts.example.com/v2", Org: "opts-org", APIName: "nico"}, + expected: expect{ + baseURL: "https://OPTS.example.com/v2", + org: "opts-org", + apiName: "nico", + token: "from-header", + }, + }, + { + name: "configured_base_url_rejects_different_path_case", + in: map[string]any{ + "base_url": "https://opts.example.com/V2", + }, + opts: Options{BaseURL: "https://opts.example.com/v2", Org: "opts-org", APIName: "nico"}, + expected: expect{ + errContains: "does not match the configured server base URL", + }, + }, { name: "dynamic_destination_rejects_inbound_bearer_without_call_token", in: map[string]any{ diff --git a/rest-api/mcp/internal/server/util.go b/rest-api/mcp/internal/server/util.go index 5e6f9f4f21..b099142cb5 100644 --- a/rest-api/mcp/internal/server/util.go +++ b/rest-api/mcp/internal/server/util.go @@ -5,6 +5,7 @@ package server import ( "encoding/json" + "net/url" "strconv" "strings" "unicode" @@ -27,6 +28,22 @@ func normalizeBaseURL(v string) string { return strings.TrimRight(v, "/") } +func sameBaseURL(a, b string) bool { + left, err := url.Parse(a) + if err != nil { + return false + } + right, err := url.Parse(b) + if err != nil { + return false + } + left.Scheme = strings.ToLower(left.Scheme) + left.Host = strings.ToLower(left.Host) + right.Scheme = strings.ToLower(right.Scheme) + right.Host = strings.ToLower(right.Host) + return left.String() == right.String() +} + // normalizeToken strips a leading "Bearer " scheme (case-insensitive) so // callers can pass either a raw token or a full Authorization value. func normalizeToken(v string) string { From 43983c5a22dd681f467d6368475b6cb898a5eca3 Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 14 Jul 2026 17:21:18 -0500 Subject: [PATCH 3/3] fix: Treat explicit default MCP ports as equivalent --- rest-api/mcp/internal/server/config_test.go | 14 ++++++++++++++ rest-api/mcp/internal/server/util.go | 14 +++++++++----- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/rest-api/mcp/internal/server/config_test.go b/rest-api/mcp/internal/server/config_test.go index 40536da8f6..92cd42ffcf 100644 --- a/rest-api/mcp/internal/server/config_test.go +++ b/rest-api/mcp/internal/server/config_test.go @@ -81,6 +81,20 @@ func TestFromCallConfig_PrecedenceChain(t *testing.T) { token: "from-header", }, }, + { + name: "configured_base_url_accepts_matching_explicit_default_port", + in: map[string]any{ + "base_url": "https://opts.example.com/v2", + }, + req: requestWithBearer("from-header"), + opts: Options{BaseURL: "https://opts.example.com:443/v2", Org: "opts-org", APIName: "nico"}, + expected: expect{ + baseURL: "https://opts.example.com/v2", + org: "opts-org", + apiName: "nico", + token: "from-header", + }, + }, { name: "configured_base_url_rejects_different_path_case", in: map[string]any{ diff --git a/rest-api/mcp/internal/server/util.go b/rest-api/mcp/internal/server/util.go index b099142cb5..4fa5f714c5 100644 --- a/rest-api/mcp/internal/server/util.go +++ b/rest-api/mcp/internal/server/util.go @@ -37,11 +37,15 @@ func sameBaseURL(a, b string) bool { if err != nil { return false } - left.Scheme = strings.ToLower(left.Scheme) - left.Host = strings.ToLower(left.Host) - right.Scheme = strings.ToLower(right.Scheme) - right.Host = strings.ToLower(right.Host) - return left.String() == right.String() + if !sameOrigin(left, right) || (left.User == nil) != (right.User == nil) { + return false + } + if left.User != nil && left.User.String() != right.User.String() { + return false + } + left.Scheme, left.Host, left.User = "", "", nil + right.Scheme, right.Host, right.User = "", "", nil + return *left == *right } // normalizeToken strips a leading "Bearer " scheme (case-insensitive) so