fix(pier): refuse path values the mux never delivers and harden dispatch

- a path field must be required in the reflected schema (boot error), and a missing value, an empty, . or .. segment, or a / inside one segment is a tool error before dispatch (A-WR-01, A-WR-02) - a failed guard dispatch logs its cause and answers a fixed tool text; only an invalid token and an unknown or deactivated account keep their own fixed texts (A-WR-08) - path and query scalars decode with UseNumber, so integers above 2^53 keep their digits (A-IN-04) - the dispatched request carries the outer Host, X-Forwarded-Proto and X-Forwarded-Host (A-IN-05) - an embedded struct on a GET or DELETE tool fails boot (A-IN-07) - a dispatch whose guard list or principal kind contradicts the tool's guard is refused (A-IN-08)
2026-10-23 16:05:32 +02:00
parent 240457291f
commit 104db9959f
5 changed files with 134 additions and 23 deletions
--- a/modules/pier/README.md
+++ b/modules/pier/README.md
@@ -8,7 +8,7 @@ Opt-in MCP endpoint that lists the tools and prompts plugins declare per caller

 ## Overview

-pier mounts a streamable HTTP MCP endpoint (POST, GET and DELETE at `mcp.path`, default `/mcp`) inside the application binary, on the official Go SDK. A plugin takes part by implementing `pact.HasMCP`: each `pact.MCPTool` names one of the plugin's own routes by method and path, an input struct whose tags say which fields fill the path, the query string and the body, and the guard the tool runs as. At boot pier compiles every declaration against the route table (an unknown route, a duplicate or invalid name, a route that is not token-guarded, a non-object input all fail boot naming the plugin and the tool), reflects the input schema from the struct and prefixes the exposed name with the plugin's short code (`acme.blog` declaring `create_post` exposes `blog_create_post`).
+pier mounts a streamable HTTP MCP endpoint (POST, GET and DELETE at `mcp.path`, default `/mcp`) inside the application binary, on the official Go SDK. A plugin takes part by implementing `pact.HasMCP`: each `pact.MCPTool` names one of the plugin's own routes by method and path, an input struct whose tags say which fields fill the path, the query string and the body, and the guard the tool runs as. At boot pier compiles every declaration against the route table (an unknown route, a duplicate or invalid name, a route that is not token-guarded, a non-object input, a `path:` field the schema does not require, an embedded struct on a `GET` or `DELETE` tool all fail boot naming the plugin and the tool), reflects the input schema from the struct and prefixes the exposed name with the plugin's short code (`acme.blog` declaring `create_post` exposes `blog_create_post`).

 Every request to the endpoint is authorized afresh from its bearer: an OAuth access token through the `pact.MCPTokenVerifier` the OAuth server plugin publishes, or, behind `mcp.allow_personal_tokens`, a personal API token (an admin `g15_` token through cabana, a frontend token through the published `pact.MCPPersonalTokenResolver`). The resulting `pier.Identity` names a frontend subject and, when the grant carries the `backend` scope and the published `pact.MCPLinkedBackendResolver` confirms a link, a backend subject. `tools/list` is computed for that identity: a tool needs a subject under its guard, a grant without `write` sees only read-only tools, and the guard provider's own check (the same logic its route middleware enforces) decides the rest. A `tools/call` runs the bound route's compiled middleware chain in-process with the `bouncer.Dispatch` marker carrying the principal the guard provider resolved; the inner request never carries an `Authorization` or `Cookie` header. A 2xx answer becomes text plus structured content, anything else a tool error carrying the route's own body.

@@ -28,9 +28,9 @@ Use cases:

 - `pact.HasMCP` declarations compiled at boot: prefix from the plugin id (last dot-segment, lower-cased, runs of other characters as `_`), exposed names matching `[a-z0-9_]{1,128}`, duplicate names refused within and across plugins, descriptions capped at `pier.MaxDescriptionRunes`.
 - Route binding by exact method and pattern (D-10); a tool targets a route that carries its guard's token-accepting middleware or names its guard explicitly (D-15); the requirements a route's middleware implies come from the guard provider's `MCPRequirements` and are checked together with the tool's `Permissions` (D-16).
- Input schema reflected from the struct with `jsonschema.ForType` (`jsonschema:"..."` tags describe fields, fields without `omitempty` are required, no additional properties); every call's arguments are validated against it, then split by the `path:"x"` and `query:"q"` tags into path values, query string and the raw JSON body minus those keys (D-11); a `GET` or `DELETE` tool may carry only path and query fields. An optional `Output` struct produces the output schema.
+- Input schema reflected from the struct with `jsonschema.ForType` (`jsonschema:"..."` tags describe fields, fields without `omitempty` are required, no additional properties); every call's arguments are validated against it, then split by the `path:"x"` and `query:"q"` tags into path values, query string and the raw JSON body minus those keys (D-11); a `GET` or `DELETE` tool may carry only path and query fields, and an embedded struct (whose promoted fields would be body fields) fails boot there. A `path:` field must be required (no `omitempty` or `omitzero`), and at call time pier delivers only path values the HTTP mux could: a missing value, an empty, `.` or `..` segment, or a `/` inside a single-segment value is a tool error before dispatch. Numbers in path and query values keep their literal digits, so integer ids above 2^53 stay exact. An optional `Output` struct produces the output schema.
 - Annotations from the method (D-14): `GET` read-only and idempotent, `PUT` idempotent, `DELETE` destructive; `ReadOnly`, `Destructive`, `Idempotent` and `OpenWorld` override; the destructive and open-world hints are always explicit.
- In-process dispatch through `pier.Host.Handler`, the route's compiled chain, with `bouncer.WithDispatch` carrying the guard provider's `MCPDispatch` principal and credential, path values set as the mux would, `RemoteAddr`, `X-Forwarded-For`, `X-Real-IP` and `Accept-Language` copied from the MCP request, `mcp.call_timeout` on the context and the answer captured up to `mcp.max_result_bytes` (D-17).
+- In-process dispatch through `pier.Host.Handler`, the route's compiled chain, with `bouncer.WithDispatch` carrying the guard provider's `MCPDispatch` principal and credential, path values set as the mux would, `RemoteAddr`, the `Host`, `X-Forwarded-For`, `X-Forwarded-Proto`, `X-Forwarded-Host`, `X-Real-IP` and `Accept-Language` copied from the MCP request (never `Authorization` or `Cookie`), `mcp.call_timeout` on the context and the answer captured up to `mcp.max_result_bytes` (D-17). A failed guard dispatch answers the fixed tool error `pier: the tool could not be run for this caller` and logs the cause server-side, so no provider or database error reaches the agent (an invalid token or a deactivated account keep their own fixed texts); a dispatch whose guard list names `backend` for a frontend tool, or whose principal kind contradicts the tool's guard, is refused the same way.
 - Results (D-13): 2xx is text plus structured content (an object as is, any other JSON wrapped as `{"data": …}`); a 4xx or 5xx is a tool error whose text is the route's body; a too-large answer is a tool error with a fixed text.
 - Built-in `backend` guard provider (`pier.BackendGuard`): routes behind cabana's `backend` middleware, listing through `cabana.Allows` on the administrator's principal from `cabana.BackendPrincipal`, an admin personal token's scope limit computed per dispatched route method with `cabana.ScopeLimitFor`; a plugin that registers its own `backend` provider wins.
 - Identity per request (`pier.Identity`): `oas_at_` tokens through the published verifier, personal tokens only with `mcp.allow_personal_tokens`, the backend subject through the published link resolver with an in-process cache of at most `pier.LinkCacheTTL`; the binding key `oauth:<client_id>:<user_id>`, `pat:user:<token_id>` or `pat:backend:<token_id>` survives access-token rotation. One `pact.MCPPersonalTokenResolver` serves the application; a subject it answers without a `TokenID` is refused, so personal-token users never share a binding.
--- a/modules/pier/backend_guard.go
+++ b/modules/pier/backend_guard.go
@@ -2,6 +2,7 @@ package pier

 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"slices"
@@ -69,6 +70,11 @@ func (g backendGuard) MCPAllowsMethod(ctx context.Context, subject pact.MCPSubje
 	return cabana.Allows(principal, permissions), nil
 }

+// errSubjectUnavailable is what a dispatch answers when the subject's
+// account no longer exists or is deactivated; its text is safe to show the
+// MCP client.
+var errSubjectUnavailable = errors.New("pier: the caller's account is unknown or deactivated")
+
 // MCPDispatch builds the admin principal for one in-process call.
 func (g backendGuard) MCPDispatch(ctx context.Context, subject pact.MCPSubject, method string) (pact.MCPDispatch, error) {
 	principal, err := g.principal(ctx, subject, method)
@@ -76,7 +82,7 @@ func (g backendGuard) MCPDispatch(ctx context.Context, subject pact.MCPSubject,
 		return pact.MCPDispatch{}, err
 	}
 	if principal == nil {
-		return pact.MCPDispatch{}, fmt.Errorf("pier: backend user %d is unknown or deactivated", subject.UserID)
+		return pact.MCPDispatch{}, fmt.Errorf("%w (backend user %d)", errSubjectUnavailable, subject.UserID)
 	}
 	return pact.MCPDispatch{Principal: principal, Guards: []string{BackendGuard}}, nil
 }
--- a/modules/pier/catalog.go
+++ b/modules/pier/catalog.go
@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"reflect"
 	"regexp"
+	"slices"
 	"sort"
 	"strings"
 	"sync"
@@ -216,6 +217,11 @@ func compileTool(pluginID, prefix string, t pact.MCPTool, routes map[string]Rout
 		if f.kind != argPath {
 			continue
 		}
+		// Over HTTP a {segment} never matches an empty value, so a path
+		// field the schema lets the agent omit is refused (A-WR-02).
+		if !slices.Contains(schema.Required, f.name) {
+			return nil, fmt.Errorf("%s: path field %q must be required (drop omitempty/omitzero)", errPrefix, f.name)
+		}
 		found := false
 		for _, s := range segments {
 			if s == f.key {
@@ -239,6 +245,11 @@ func compileTool(pluginID, prefix string, t pact.MCPTool, routes map[string]Rout
 				return nil, fmt.Errorf("%s: field %q would be a request body, but a %s route takes only path and query fields", errPrefix, f.name, method)
 			}
 		}
+		// An embedded struct's promoted fields are in the schema but would
+		// never be sent, since the request has no body (A-IN-07).
+		if name, ok := embeddedBodyField(rt); ok {
+			return nil, fmt.Errorf("%s: embedded field %q would promote request body fields, but a %s route takes only path and query fields", errPrefix, name, method)
+		}
 	}
 	var output *jsonschema.Schema
 	if t.Output != nil {
@@ -389,6 +400,37 @@ func inputFields(rt reflect.Type) ([]argField, error) {
 	return out, nil
 }

+// embeddedBodyField names the first embedded struct field of the input
+// that carries no path or query tag and is not hidden with json:"-": its
+// promoted fields would be body fields.
+func embeddedBodyField(rt reflect.Type) (string, bool) {
+	for i := 0; i < rt.NumField(); i++ {
+		f := rt.Field(i)
+		if !f.Anonymous {
+			continue
+		}
+		if tag, ok := f.Tag.Lookup("json"); ok {
+			if n, _, _ := strings.Cut(tag, ","); n == "-" {
+				continue
+			}
+		}
+		if _, ok := f.Tag.Lookup("path"); ok {
+			continue
+		}
+		if _, ok := f.Tag.Lookup("query"); ok {
+			continue
+		}
+		ft := f.Type
+		for ft.Kind() == reflect.Pointer {
+			ft = ft.Elem()
+		}
+		if ft.Kind() == reflect.Struct {
+			return f.Name, true
+		}
+	}
+	return "", false
+}
+
 // patternSegments lists the {name} and {name...} segments of a route
 // pattern in order; {$} is not a parameter.
 func patternSegments(pattern string) []string {
--- a/modules/pier/dispatch.go
+++ b/modules/pier/dispatch.go
@@ -4,16 +4,19 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"slices"
 	"strconv"
 	"strings"
 	"time"

 	"git.golem15.com/golem15/summercms/modules/backpack"
 	"git.golem15.com/golem15/summercms/modules/bouncer"
+	"git.golem15.com/golem15/summercms/modules/pact"
 	"github.com/modelcontextprotocol/go-sdk/mcp"
 )

@@ -21,12 +24,24 @@ import (
 // mcp.max_result_bytes.
 const resultTooLarge = "pier: the route's answer exceeds mcp.max_result_bytes"

+// dispatchFailed is the fixed text of the tool error for a guard dispatch
+// that failed or contradicted the tool's guard; the cause (which may name a
+// database host or user) is logged server-side only (A-WR-08).
+const dispatchFailed = "pier: the tool could not be run for this caller"
+
+// tokenInvalid is the tool error text for a provider answering
+// pact.ErrMCPTokenInvalid.
+const tokenInvalid = "pier: the caller's token is no longer valid"
+
 // callInfo is what a dispatched call copies from the outer MCP request: the
-// client address and the forwarding, real-ip and language headers, plus
-// the session the call belongs to, for the audit line.
+// client address, the Host, the forwarding, real-ip and language headers,
+// plus the session the call belongs to, for the audit line.
 type callInfo struct {
 	remoteAddr     string
+	host           string
 	forwardedFor   string
+	forwardedProto string
+	forwardedHost  string
 	realIP         string
 	acceptLanguage string
 	session        string
@@ -39,7 +54,10 @@ func callInfoFrom(r *http.Request) callInfo {
 	}
 	return callInfo{
 		remoteAddr:     r.RemoteAddr,
+		host:           r.Host,
 		forwardedFor:   r.Header.Get("X-Forwarded-For"),
+		forwardedProto: r.Header.Get("X-Forwarded-Proto"),
+		forwardedHost:  r.Header.Get("X-Forwarded-Host"),
 		realIP:         r.Header.Get("X-Real-IP"),
 		acceptLanguage: r.Header.Get("Accept-Language"),
 	}
@@ -101,6 +119,13 @@ func (d *dispatcher) run(ctx context.Context, id Identity, info callInfo, t *too
 			if s == "" || (strings.Contains(s, "/") && !strings.Contains(t.pattern, "{"+f.key+"...}")) {
 				return toolError(fmt.Sprintf("pier: invalid arguments: %q must be one non-empty path segment", f.name)), 0
 			}
+			// The mux cleans a request path before any route sees it, so a
+			// route never receives an empty, . or .. segment (A-WR-01).
+			for _, seg := range strings.Split(s, "/") {
+				if seg == "" || seg == "." || seg == ".." {
+					return toolError(fmt.Sprintf("pier: invalid arguments: %q must not contain empty, . or .. segments", f.name)), 0
+				}
+			}
 			pathValues[f.key] = s
 			delete(parts, f.name)
 		case argQuery:
@@ -114,6 +139,13 @@ func (d *dispatcher) run(ctx context.Context, id Identity, info callInfo, t *too
 			delete(parts, f.name)
 		}
 	}
+	// Every {segment} gets a value, as over HTTP (A-WR-02; boot already
+	// requires path fields, this guards the dispatch on its own).
+	for _, f := range t.fields {
+		if _, ok := pathValues[f.key]; f.kind == argPath && !ok {
+			return toolError(fmt.Sprintf("pier: invalid arguments: %q is required", f.name)), 0
+		}
+	}
 	path := t.pattern
 	for key, value := range pathValues {
 		if strings.Contains(path, "{"+key+"...}") {
@@ -146,12 +178,27 @@ func (d *dispatcher) run(ctx context.Context, id Identity, info callInfo, t *too
 	defer cancel()
 	disp, err := t.guard.MCPDispatch(callCtx, *subject, t.method)
 	if err != nil {
-		return toolError("pier: " + err.Error()), 0
+		switch {
+		case errors.Is(err, pact.ErrMCPTokenInvalid):
+			return toolError(tokenInvalid), 0
+		case errors.Is(err, errSubjectUnavailable):
+			return toolError(errSubjectUnavailable.Error()), 0
+		}
+		logger().Warn("pier: guard dispatch failed", "tool", t.name, "guard", t.guardName, "error", err)
+		return toolError(dispatchFailed), 0
 	}
 	principal, ok := disp.Principal.(*bouncer.Principal)
 	if !ok || principal == nil {
 		return toolError(fmt.Sprintf("pier: guard %s did not resolve a principal", t.guardName)), 0
 	}
+	// The marker's guard list and the principal's kind must agree with the
+	// tool's guard: a frontend provider naming the backend guard, or a
+	// backend principal on a frontend tool, is a provider bug (A-IN-08).
+	backendTool := t.guardName == BackendGuard
+	if principal.Backend != backendTool || (!backendTool && slices.Contains(disp.Guards, BackendGuard)) {
+		logger().Warn("pier: guard dispatch contradicts the tool's guard", "tool", t.name, "guard", t.guardName, "guards", disp.Guards, "backend_principal", principal.Backend)
+		return toolError(dispatchFailed), 0
+	}
 	credential := disp.Credential
 	if credential == nil && t.guardName == BackendGuard {
 		credential = id.credential
@@ -169,9 +216,18 @@ func (d *dispatcher) run(ctx context.Context, id Identity, info callInfo, t *too
 	if info.remoteAddr != "" {
 		req.RemoteAddr = info.remoteAddr
 	}
+	if info.host != "" {
+		req.Host = info.host
+	}
 	if info.forwardedFor != "" {
 		req.Header.Set("X-Forwarded-For", info.forwardedFor)
 	}
+	if info.forwardedProto != "" {
+		req.Header.Set("X-Forwarded-Proto", info.forwardedProto)
+	}
+	if info.forwardedHost != "" {
+		req.Header.Set("X-Forwarded-Host", info.forwardedHost)
+	}
 	if info.realIP != "" {
 		req.Header.Set("X-Real-IP", info.realIP)
 	}
@@ -253,27 +309,32 @@ func anyJSON(body []byte) (any, bool) {
 	return v, true
 }

-// scalarString renders a JSON string or number as a path value.
-func scalarString(raw json.RawMessage) (string, error) {
+// decodeNumbers decodes one JSON value keeping numbers as json.Number, so
+// an integer above 2^53 keeps its digits (A-IN-04).
+func decodeNumbers(raw json.RawMessage) (any, error) {
+	dec := json.NewDecoder(bytes.NewReader(raw))
+	dec.UseNumber()
 	var v any
-	if err := json.Unmarshal(raw, &v); err != nil {
+	if err := dec.Decode(&v); err != nil {
+		return nil, err
+	}
+	return v, nil
+}
+
+// scalarString renders a JSON string, number or boolean as a path value; a
+// number is rendered as its literal.
+func scalarString(raw json.RawMessage) (string, error) {
+	v, err := decodeNumbers(raw)
+	if err != nil {
 		return "", err
 	}
-	switch x := v.(type) {
-	case string:
-		return x, nil
-	case float64:
-		return strconv.FormatFloat(x, 'f', -1, 64), nil
-	case bool:
-		return strconv.FormatBool(x), nil
-	}
-	return "", fmt.Errorf("not a scalar")
+	return scalarAny(v)
 }

 // queryStrings renders a JSON scalar or list of scalars as query values.
 func queryStrings(raw json.RawMessage) ([]string, error) {
-	var v any
-	if err := json.Unmarshal(raw, &v); err != nil {
+	v, err := decodeNumbers(raw)
+	if err != nil {
 		return nil, err
 	}
 	switch x := v.(type) {
@@ -302,6 +363,8 @@ func scalarAny(v any) (string, error) {
 	switch x := v.(type) {
 	case string:
 		return x, nil
+	case json.Number:
+		return x.String(), nil
 	case float64:
 		return strconv.FormatFloat(x, 'f', -1, 64), nil
 	case bool: