diff --git a/AGENTS.md b/AGENTS.md index e92313f6de..6e5c39cbc2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -837,8 +837,10 @@ Resolution is narrow and fails loudly rather than degrading quietly: - **Same origin only** — released only to the exact `scheme://host:port` the caller addressed (read from the inbound request, not config), so a config naming a third-party host cannot exfiltrate the token. -- **Headers only** — `${caller:token}` in a query parameter is rejected. - `${caller:userId}` is allowed in headers and query parameters. +- **Headers only** — `${caller:token}` in a query parameter, request body or + path is rejected. `${caller:userId}` is allowed in headers and query + parameters. An MCP server's `apiKey` may also carry it, which sends the tool + call as the chatting user (see [`docs/mcp-server.md`](docs/mcp-server.md)). - **Authenticated turns only** — scheduled jobs and triggers cannot satisfy it. - **Fails closed** — an unsatisfiable reference errors instead of sending `"Bearer "` with an empty token. diff --git a/docs/changelog.md b/docs/changelog.md index fc4904d5bb..9306f99e96 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,45 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## 🔑 feat(operator): the foundation for an agent that can safely write (2026-07-29) + +**Repo:** EDDI (`feat/operator-write-foundation`) + +Groundwork for a workspace operator agent that manages a deployment — creating and updating agents and groups — while acting as the person chatting to it. Five commits, no new capability granted: the operator's endpoint allow-list is untouched and still read-only. What changes is that widening it is now safe, where before it was neither safe nor functional. + +**Generated writes did not work at all.** `McpApiToolBuilder.buildBodyTemplate` emitted Qute variables for a request body but registered none of them, and `AgentOrchestrator` builds the tool schema from `ApiCall.getParameters()` alone. So the model had no documented way to fill a body; with strict rendering off the variables rendered empty and every generated `POST`/`PUT`/`PATCH` went out structurally valid and semantically empty. Adding a write endpoint before this would have produced garbage requests that fail at the far end rather than at the config. + +**The body is now one model-written variable.** A per-property template looked more helpful and was worse three ways: every variable became a *required* tool parameter (a `Map` has nowhere to record optionality, so a `PATCH` of one field forced the model to restate all the others); values were substituted into the JSON unescaped, since the templating engine runs in `TEXT` mode, so a value containing a quote could break the body or add fields the schema never declared; and the HITL card shows tool *arguments*, so "the arguments are the request" only holds if the body is one of them. The shape a decomposed template implied now lives in the parameter description, which names each property with its type and marks which are required. + +**Approval patterns can address the endpoint a tool calls.** Names come from `operationId` or a slug and drift when a spec changes, and `ToolApprovalGate` allows an unmatched call — so a renamed or newly generated write arrived ungated and silently. Method and path were available and discarded one line into registration; they now travel alongside, so a pattern may match a bare name, `source:name`, or `source.method:path`: + +```json +{ "requireApproval": ["http.post:*", "http.put:*", "http.patch:*", "http.delete:*"], + "exempt": ["http.get:*"] } +``` + +gates every mutation without naming a tool, while `http.post:/agentstore/agents` addresses exactly one. Both speak the same `METHOD /path` vocabulary as the endpoint allow-list, so the two can be generated from one source instead of maintained in two. Documented in [`docs/hitl.md`](hitl.md). + +**Design decision — enumerate downward, never upward.** Whether something is gated stays in `requireApproval`/`exempt`; per-endpoint tuning may only *lower* friction. A missed exemption costs an approval prompt; a missed requirement is an ungated write. The same reasoning restricts the method qualifier to `http`: it is the only source whose tools record an endpoint, so `mcp.post:` is rejected at save time rather than saved as a pattern nothing could ever match. + +**MCP tool calls can now run as the chatting user.** They previously ran as whatever static credential the config named, and a `${caller:token}` there passed through the global-variable and secret resolvers untouched and was sent as the literal placeholder — failing silently rather than closed. The transport supported this all along: `customHeaders` has three overloads and EDDI used the constant one. The per-request `McpHeadersSupplier` overload makes the credential per-call while the client stays cached. + +**Discovery must be told apart from invocation explicitly.** The first version of this decided by asking whether a caller was bound to the thread — but discovery runs *inside* the turn, on a thread that is bound, so `initialize` and `tools/list` went out with the first caller's token. Since the client is cached, that session was then reused by everyone after them, and the tool list reflected one user's permissions while being offered to the next. langchain4j distinguishes the two — `DefaultMcpClient.listTools()` delegates with a null `InvocationContext` while `McpToolExecutor` always builds one — but it does not enforce anything; EDDI has to read `McpCallContext.invocationContext()` and act on it, which it now does. Only a tool call carries the caller; discovery goes unauthenticated on a caller-bound config and the server decides. + +**Fixed in passing — a privilege bug.** MCP clients were cached by URL alone, so two agents naming the same server with *different* credentials silently shared whichever client was constructed first. The key now includes a digest of the configured credential: a digest so a literal key never becomes a map key, taken unresolved so configs sharing a vault reference still share a client. This does not multiply clients per user — a caller-bound config yields one client whose supplier reads the caller per request. + +**Not solved, deliberately.** On an expired MCP session the transport retries `initialize()` on an HTTP callback thread where the caller binding does not exist. That path now sends the request unauthenticated rather than falling back to the static key under the caller's intent — a visible failure instead of the wrong authority. + +**Corrections to earlier analysis, recorded because they changed decisions.** `McpCallsConfiguration.toolsWhitelist` does *not* fail closed — `AgentOrchestrator` skips filtering when the list is empty, the same shape as the approval gate. `toolApprovals` is not agent-only: `LlmConfiguration` carries a per-task override that fully replaces the agent-level block. And a `PUT` on a config does not reach a running agent, because `HistorizedResourceStore.update` creates `version + 1` while agents pin a version — so self-modification takes a chain of write, re-point, redeploy, each independently gated. The thing to guard is whatever can re-point a version reference, not `PUT` in general. + +**Verification.** 566 tests pass on a clean build across the affected suites; checkstyle clean. Each fix mutation-checked by reverting it. Four tests written during this work were found vacuous by that check and rewritten — three asserted against a helper or state seeded through the very wrapper under test, one seeded a thread binding that the fix then restored, so it passed either way. + +**Next:** per-endpoint approval friction (`timeoutPolicy`, `approvalTimeout` and the pause message are still single scalars for every gated tool), agent-readable documentation (EDDI's MCP client never reads MCP *resources*, so `eddi://docs/*` is reachable from a desktop client but not from an agent), widening the allow-list, and the Manager scope picker and approval surface. + +--- + --- ## ⚙️ fix(runtime): code-review findings wave 3 — concurrency, lifecycle, cancellation, graceful shutdown, Dream wiring (2026-07-28) diff --git a/docs/hitl.md b/docs/hitl.md index aa3db025c2..f73635fd2b 100644 --- a/docs/hitl.md +++ b/docs/hitl.md @@ -261,10 +261,12 @@ All values are validated at **save time** and on ZIP import (`HitlConfigValidati Patterns are matched by `ToolApprovalPatterns` / `ToolApprovalGate`: - **`*` is the only wildcard** — it matches any run of characters (including empty). Every other character is a quoted literal, so compilation is **ReDoS-safe**. -- **Source-qualified or bare.** A pattern may carry a known source prefix (`mcp:read_*`, `http:*`) or match the bare tool name (`delete_account`). Each call is tested against `source:name` **first**, then the bare dispatch name — a tool with an unknown source still matches bare-name patterns (fail-safe). -- **Known sources** (the only accepted prefixes): `builtin`, `http`, `mcp`, `a2a`, `dynamic`, `memory`, `recall`. An unknown prefix is rejected at save time with a typo suggestion (Levenshtein ≤ 2). -- **Case-sensitive.** Patterns match tool names exactly; `Delete_*` does not match `delete_account`. -- Patterns may not start or end with `:`, and are capped at 256 characters. Allowed characters: `A-Za-z0-9_-.:*`. +- **Source-qualified or bare.** A pattern may carry a known source prefix (`mcp:read_*`, `http:*`) or match the bare tool name (`delete_account`). A call is tested against the endpoint-qualified form **first** (where one exists, see below), then `source:name`, then the bare dispatch name — a tool with an unknown source still matches bare-name patterns (fail-safe). +- **Endpoint-qualified** (`http` tools only). A pattern may also address *what a tool calls* rather than what it is named: `http.post:*` matches every POST, `http.post:/agentstore/agents` matches exactly one endpoint. This is the robust form for tools generated from an OpenAPI spec, whose names come from `operationId` or a slug and change when the spec does — `http.post:*` keeps gating every mutation even when a name changes or a new endpoint appears. The path is matched **as the httpcall config declares it**, normalised to a leading slash (an absolute URL contributes only its path). +- **Known sources** (the only accepted prefixes): `builtin`, `http`, `mcp`, `a2a`, `dynamic`, `memory`, `recall`. An unknown prefix is rejected at save time with a typo suggestion (Levenshtein ≤ 2). Only `http` may carry a method qualifier: it is the only source whose tools record an endpoint, so `mcp.post:` is rejected rather than saved as a pattern nothing could match. +- **Allowed characters:** `A-Za-z0-9_-.:*` plus `/` `{` `}` for endpoint path templates. Braces are matched literally, not as regex quantifiers. +- **Case-sensitive.** Patterns match tool names exactly; `Delete_*` does not match `delete_account`. An endpoint path is matched as the httpcall config declares it, so its case must match too; the method half is normalised to lower case. +- Patterns may not start or end with `:`, and are capped at 256 characters. ### Precedence diff --git a/docs/httpcalls.md b/docs/httpcalls.md index 1d0847b969..df6d57f718 100644 --- a/docs/httpcalls.md +++ b/docs/httpcalls.md @@ -71,9 +71,11 @@ than degrading quietly: inbound request, not from configuration, so a config naming a third-party host cannot exfiltrate a user's token — and no allow-list is needed for this to be safe by default. -- **Headers only.** `${caller:token}` in a query parameter is rejected; tokens - in URLs leak through access logs, proxies and browser history. - `${caller:userId}` may be used in headers and query parameters. +- **Headers only.** `${caller:token}` in a query parameter, request body or + request path is rejected. Tokens in URLs leak through access logs, proxies and + browser history, and nothing outside a header is substituted anyway — a + reference there would travel to the API as literal text. `${caller:userId}` + may be used in headers and query parameters. - **Authenticated turns only.** The identity comes from the request that drove the turn, so scheduled jobs and triggers cannot satisfy `${caller:token}`. - **Fails closed.** An unsatisfiable reference raises an error instead of @@ -85,6 +87,13 @@ headers are scrubbed before the request is recorded. Set `eddi.caller-identity.enabled=false` to forbid the feature outright. +The same reference works in an **MCP server's `apiKey`**, so a tool call reaches +that server as the chatting user rather than as a standing service principal. +Only the tool call carries the caller — the handshake and `tools/list` do not, +because the client is cached and a session opened with one user's token would be +reused by everyone after them. See +[`mcp-server.md`](mcp-server.md#calling-an-mcp-server-as-the-chatting-user). + ### Running behind a reverse proxy The origin is taken from the inbound request as EDDI sees it. Behind a diff --git a/docs/mcp-server.md b/docs/mcp-server.md index 5d99243430..c3ca7a01e1 100644 --- a/docs/mcp-server.md +++ b/docs/mcp-server.md @@ -68,7 +68,7 @@ EDDI uses **Streamable HTTP** transport, served by the Quarkus MCP Server extens | Tool | Description | | ------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `setup_agent` | Create a fully working agent in one call: creates behavior rules, LangChain config, optional output/greeting, package, agent, and deploys. Supports built-in tools, quick replies, and sentiment analysis. Default: `anthropic`/`claude-sonnet-4-6` | -| `create_api_agent` | Create an agent from an OpenAPI 3.0/3.1 spec. Parses the spec, generates HttpCalls configs (grouped by API tag), creates the full pipeline, and deploys. Supports endpoint filtering, base URL override, and auth header propagation | +| `create_api_agent` | Create an agent from an OpenAPI 3.0/3.1 spec. Parses the spec, generates HttpCalls configs (grouped by API tag), creates the full pipeline, and deploys. Supports endpoint filtering, base URL override, and auth header propagation. A generated write tool takes the whole request body as one `requestBody` parameter — see below | ### Schedule Management Tools (6) @@ -650,13 +650,60 @@ External MCP servers are configured as **`mcpcalls` workflow extensions** — a | ---------------- | ---------- | -------- | --------- | ------------------------------------------------------------------------------------------------------------------------ | | `mcpServerUrl` | string | **Yes** | — | MCP server URL | | `name` | string | No | — | Human-readable name for logging | -| `transport` | string | No | `"http"` | Informational only today — `McpToolProviderManager` always connects via Streamable HTTP | -| `apiKey` | string | No | — | API key, sent as `Authorization: Bearer `. Resolved through global variables and `${vault:key}` references | +| `transport` | string | No | `"http"` | Only Streamable HTTP is implemented; an unimplemented value is rejected as invalid configuration rather than silently substituted | +| `apiKey` | string | No | — | API key, sent as `Authorization: Bearer `. Resolved through global variables and `${vault:key}` references, or `${caller:token}` to call as the chatting user (see below) | | `timeoutMs` | long | No | `30000` | Connection and request timeout in milliseconds | | `toolsWhitelist` | string[] | No | — | If non-empty, only these tool names are exposed (names as returned by the server's `tools/list`) | | `toolsBlacklist` | string[] | No | — | Tool names to exclude. Applied *after* the whitelist | | `mcpCalls` | object[] | No | — | Deterministic, action-triggered tool bindings (see *Pipeline mode* below). Omit for agent-mode-only servers | +#### How `create_api_agent` builds a write tool's body + +A generated `POST`/`PUT`/`PATCH` tool takes the **entire request body as a single +`requestBody` parameter**, whose description names the schema's properties, their +types, and which are required. The model writes the JSON itself. + +It is worth knowing why, because the obvious alternative is worse. Decomposing the +schema into one parameter per property means every one becomes *required* (an +`ApiCall`'s parameter map has nowhere to record optionality), so a `PATCH` of one +field forces the model to restate all the others and a partial update silently +becomes a full overwrite. It also substitutes model-written values into JSON +unescaped, so a value containing a quote can break the body or add fields the +schema never declared. + +The whole-body form matters most under [HITL approval](hitl.md): the approval card +shows tool **arguments**, so "what the approver sees is what gets sent" only holds +while the body is one of them. + +#### Calling an MCP server as the chatting user + +Set `apiKey` to `${caller:token}` and the tool call carries the identity of the +person chatting, instead of a standing service credential: + +```json +{ "mcpServerUrl": "https://eddi.example/mcp", "apiKey": "${caller:token}" } +``` + +The same guarantees apply as for API call headers — same origin only, fails +closed rather than sending a placeholder, never persisted. See +[`httpcalls.md`](httpcalls.md#calling-as-the-signed-in-user). + +Two behaviours worth knowing, because they are deliberate: + +- **Only tool calls carry the caller.** The `initialize` handshake and + `tools/list` are sent unauthenticated, because the client is cached: a session + opened with one user's token would be reused by everyone after them, and a + tool list reflecting one user's permissions would be offered to the next. If + your server requires authentication to *list* tools, use a static key. +- **Clients are cached per credential, not per URL.** Two agents pointing at the + same server with different keys get separate clients. A caller-bound config + still yields one shared client — the credential is applied per request, so + there is no client per user. + +A `${caller:token}` key with `eddi.caller-identity.enabled=false` is rejected as +invalid configuration when the server is validated, rather than failing on every +tool call. + **Step 2 — add an `mcpcalls` step to the agent's workflow**, before the LLM step: ```json diff --git a/docs/security.md b/docs/security.md index 8d1548f733..d66ad9f5e3 100644 --- a/docs/security.md +++ b/docs/security.md @@ -127,7 +127,7 @@ When an LLM is given access to tools, every argument it supplies must be treated ## Caller Identity Forwarding — `CallerIdentityResolver` -**Applies to:** apicall headers that reference `${caller:token}` / `${caller:userId}`. +**Applies to:** apicall headers, and an MCP server's `apiKey`, that reference `${caller:token}` / `${caller:userId}`. An agent that calls an API needs a credential. Baking a static one into the config is the wrong shape when the API is EDDI's own: an OIDC token expires @@ -145,7 +145,7 @@ fails the call loudly rather than degrading quietly: | Control | Behaviour | | ------- | --------- | | **Same-origin only** | The token is released only when the outbound call targets the exact `scheme://host:port` the caller addressed. That origin comes from the inbound request, never from configuration — so an agent config naming a third-party host cannot exfiltrate the token, and no allow-list is required for this to hold by default. | -| **Headers only** | `${caller:token}` in a query parameter is rejected. Tokens in URLs leak through access logs, proxies and browser history. `${caller:userId}` is not a secret and is also resolved in query parameters. | +| **Headers only** | A token reference in a query parameter, request body or request path is rejected — only a header is ever substituted. `${caller:userId}` is permitted in headers and query parameters. | | **Authenticated turns only** | The identity is captured from the request driving the turn. Scheduled jobs and triggers have no caller and cannot satisfy the reference. | | **Fails closed** | An unsatisfiable reference throws rather than resolving to an empty string, which would send `Bearer ` and surface downstream as a confusing `401`. | | **Never persisted** | Resolution happens while building the request; `scrubSensitiveHeaders` strips authorization headers before the request is written to conversation memory. | diff --git a/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java b/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java index c67e393530..6263804863 100644 --- a/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java +++ b/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGate.java @@ -32,6 +32,26 @@ public record GateResult(List gated, public GateResult classify(List batch, Map toolSources, ToolApprovalsConfig cfg, Set clearedCallIds) { + return classify(batch, toolSources, Map.of(), cfg, clearedCallIds); + } + + /** + * As {@link #classify(List, Map, ToolApprovalsConfig, Set)}, additionally + * matching patterns against what an http tool actually calls. + *

+ * A pattern may address a tool three ways: its bare name, {@code source:name}, + * or — for httpcall tools — {@code source.method:path}, e.g. + * {@code http.post:/agentstore/agents}. The third form is the useful one for a + * generated API client: names come from {@code operationId} or a slug and drift + * when the spec changes, whereas {@code http.post:*} gates every mutation + * whether or not anyone maintained a list, and + * {@code http.post:/agentstore/agents} says exactly which one needs more care. + * + * @param toolEndpoints + * tool name to {@code method:path}; empty for non-http tools + */ + public GateResult classify(List batch, Map toolSources, + Map toolEndpoints, ToolApprovalsConfig cfg, Set clearedCallIds) { if (cfg == null || cfg.getRequireApproval() == null || cfg.getRequireApproval().isEmpty()) { return new GateResult(List.of(), List.copyOf(batch), Map.of()); } @@ -57,11 +77,14 @@ public GateResult classify(List batch, Map } String source = toolSources.get(request.name()); String qualified = source != null ? source + ":" + request.name() : null; - if (firstMatch(exempt, qualified, request.name()) != null) { + // e.g. "http" + "." + "post:/agentstore/agents" + String endpoint = toolEndpoints.get(request.name()); + String endpointQualified = source != null && endpoint != null ? source + "." + endpoint : null; + if (firstMatch(exempt, qualified, endpointQualified, request.name()) != null) { allowed.add(request); continue; } - CompiledPattern match = firstMatch(require, qualified, request.name()); + CompiledPattern match = firstMatch(require, qualified, endpointQualified, request.name()); if (match != null) { gated.add(request); if (request.id() != null) { @@ -84,8 +107,11 @@ private static List compileAll(List globs) { return globs.stream().map(g -> new CompiledPattern(g, ToolApprovalPatterns.compile(g))).toList(); } - private static CompiledPattern firstMatch(List patterns, String qualified, String bare) { + private static CompiledPattern firstMatch(List patterns, String qualified, String endpointQualified, String bare) { for (CompiledPattern cp : patterns) { + if (endpointQualified != null && cp.pattern().matcher(endpointQualified).matches()) { + return cp; + } // bare (= request.name()) can be null: langchain4j's ToolExecutionRequest // does not guarantee a non-null name (some providers emit malformed tool // calls). Guard both matchers so a null name matches nothing and the call diff --git a/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java b/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java index 44df2540f3..4f03a90551 100644 --- a/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java +++ b/src/main/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalPatterns.java @@ -18,7 +18,20 @@ */ public final class ToolApprovalPatterns { public static final List KNOWN_SOURCES = List.of("builtin", "http", "mcp", "a2a", "dynamic", "memory", "recall"); - private static final Pattern LEGAL_CHARS = Pattern.compile("[A-Za-z0-9_\\-.:*]+"); + /** + * Also permits {@code / { }} so a pattern can name an endpoint path template + * such as {@code http.post:/agentstore/agents/{id}}. Braces are safe in the + * compiled regex because {@link #compile} quotes every non-wildcard segment. + */ + private static final Pattern LEGAL_CHARS = Pattern.compile("[A-Za-z0-9_\\-.:*/{}]+"); + + /** + * The only source whose tools carry an endpoint, so the only one qualifiable. + */ + private static final String HTTP_SOURCE = "http"; + + /** HTTP methods that may qualify the http source, as {@code http.post:…}. */ + private static final List KNOWN_METHODS = List.of("get", "post", "put", "patch", "delete", "head", "options"); private static final int MAX_LENGTH = 256; private ToolApprovalPatterns() { @@ -50,7 +63,8 @@ public static Optional validate(String pattern) { return Optional.of("pattern exceeds " + MAX_LENGTH + " characters"); } if (!LEGAL_CHARS.matcher(pattern).matches()) { - return Optional.of("pattern '" + pattern + "' contains illegal characters — allowed: A-Za-z0-9_-.:* (tool names never contain spaces)"); + return Optional.of("pattern '" + pattern + "' contains illegal characters — allowed: A-Za-z0-9_-.:* plus / { } for endpoint paths " + + "(tool names never contain spaces)"); } if (pattern.startsWith(":") || pattern.endsWith(":")) { return Optional.of("pattern '" + pattern @@ -59,14 +73,73 @@ public static Optional validate(String pattern) { int colon = pattern.indexOf(':'); if (colon > 0) { String prefix = pattern.substring(0, colon); - if (!prefix.contains("*") && !KNOWN_SOURCES.contains(prefix)) { + String target = pattern.substring(colon + 1); + boolean wildcardPrefix = prefix.contains("*"); + boolean methodQualified = isMethodQualifiedSource(prefix); + + if (!wildcardPrefix && !KNOWN_SOURCES.contains(prefix) && !methodQualified) { return Optional.of("unknown tool source prefix '" + prefix + ":' in pattern '" + pattern + "'" - + suggestionFor(prefix) + " — known sources: " + String.join(", ", KNOWN_SOURCES)); + + suggestionFor(prefix) + " — known sources: " + String.join(", ", KNOWN_SOURCES) + + "; only the http source may be qualified by method, e.g. 'http.post:'"); + } + // The remaining shapes save cleanly and match nothing at runtime, which for + // a require-pattern is an ungated call. Only a method-qualified http prefix + // is paired with an endpoint path, and an endpoint path always begins with + // a slash (AgentOrchestrator#normalizeEndpointPath), so anything else here + // is a pattern its author believes is protecting them. + if (target.startsWith("/") && !methodQualified && !prefixCanMatchAnHttpEndpoint(prefix)) { + return Optional.of("pattern '" + pattern + "' targets an endpoint path but its prefix can never match one, so the pattern" + + " matches nothing — endpoint patterns look like 'http.post:" + target + "'"); + } + if (methodQualified && !target.startsWith("/") && !target.startsWith("*")) { + return Optional.of("pattern '" + pattern + "' is method-qualified, so its target is an endpoint path and must begin with '/'" + + " (or '*') — did you mean '" + prefix + ":/" + target + "'?"); } } return Optional.empty(); } + /** + * Whether a prefix could match a real endpoint key, i.e. some + * {@code http.}. + *

+ * A wildcard prefix is not automatically fine: {@code mcp*} contains a + * {@code *} yet can never match {@code http.post}, so {@code mcp*:/x} would + * save and gate nothing. Rather than reason about glob shapes, compile the + * prefix and try it against every method EDDI actually emits — the same + * question the gate asks at runtime. + */ + private static boolean prefixCanMatchAnHttpEndpoint(String prefix) { + Pattern compiled = compile(prefix); + for (String method : KNOWN_METHODS) { + if (compiled.matcher(HTTP_SOURCE + "." + method).matches()) { + return true; + } + } + return false; + } + + /** + * Whether a prefix is {@code http.}, e.g. {@code http.post}. + *

+ * Deliberately restricted to {@code http}: it is the only source whose tools + * carry an endpoint (see {@code AgentOrchestrator} — the mcp and a2a branches + * register a source and no endpoint). Accepting {@code mcp.post:} would let a + * pattern save cleanly and then match nothing at runtime, and since the gate + * allows an unmatched call, that is an ungated write — the very failure this + * validation exists to prevent. Widen this only together with whatever + * populates endpoints for the other source. + *

+ * A typo like {@code http.pots:} is reported for the same reason. + */ + private static boolean isMethodQualifiedSource(String prefix) { + int dot = prefix.indexOf('.'); + if (dot <= 0) { + return false; + } + return HTTP_SOURCE.equals(prefix.substring(0, dot)) && KNOWN_METHODS.contains(prefix.substring(dot + 1)); + } + private static String suggestionFor(String prefix) { for (String known : KNOWN_SOURCES) { if (levenshtein(prefix, known) <= 2) { diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java b/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java index 199dfb4662..042a925826 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java @@ -279,7 +279,32 @@ private static ApiCall buildApiCall(String method, String path, Operation operat MediaType jsonMedia = content.get("application/json"); if (jsonMedia != null) { request.setContentType("application/json"); - request.setBody(buildBodyTemplate(jsonMedia.getSchema())); + var body = buildBodyTemplate(jsonMedia.getSchema()); + // The body template's variables must be declared as tool parameters, + // or the model has no documented way to fill them: the tool schema is + // built from getParameters() alone (AgentOrchestrator), and with + // strict-rendering off an undeclared variable renders as empty. The + // request would go out structurally valid and semantically empty. + // + // A path or query parameter of the same name wins — those are + // structural — so the body variable is RENAMED rather than dropped. + // Dropping it is the empty-body bug all over again, for a spec that + // happens to name a parameter "requestBody". + String bodyTemplate = body.template(); + for (var variable : body.variables().entrySet()) { + String variableName = variable.getKey(); + if (paramDescriptions.containsKey(variableName)) { + String renamed = variableName; + while (paramDescriptions.containsKey(renamed) || body.variables().containsKey(renamed)) { + renamed = renamed + "Body"; + } + bodyTemplate = bodyTemplate.replace("{" + variableName + "}", "{" + renamed + "}"); + paramDescriptions.put(renamed, variable.getValue()); + } else { + paramDescriptions.put(variableName, variable.getValue()); + } + } + request.setBody(bodyTemplate); } } @@ -323,42 +348,101 @@ static String generateSlug(String method, String path) { * Note: Only handles flat schemas (direct properties). Nested objects and * arrays fall back to a single {@code {requestBody}} template variable. */ - private static String buildBodyTemplate(Schema schema) { + /** + * A request-body template together with the tool parameters the model must + * supply to fill it. + *

+ * The two travel together on purpose. A template variable that is not also + * declared as a parameter is invisible to the model, and renders empty rather + * than failing, so the call succeeds and the body is wrong. + * + * @param template + * the Qute body template + * @param variables + * variable name to description, for {@code ApiCall.parameters} + */ + private record BodyTemplate(String template, Map variables) { + } + + /** Name of the whole-body variable used when the schema has no properties. */ + static final String WHOLE_BODY_VARIABLE = "requestBody"; + + private static BodyTemplate buildBodyTemplate(Schema schema) { if (schema == null) { - return "{}"; + // A declared body with no schema still needs a variable, or the model has + // no way to fill it and every write goes out empty — the exact failure the + // whole-body form exists to prevent. + return new BodyTemplate("{" + WHOLE_BODY_VARIABLE + "}", + Map.of(WHOLE_BODY_VARIABLE, "The complete JSON request body. The spec declares no schema for it.")); } + // One variable carrying the whole body, always — never a per-property + // template. Decomposing looks more helpful and is worse in three ways: + // + // 1. Every variable becomes a REQUIRED tool parameter (AgentOrchestrator + // builds the schema from ApiCall.parameters and marks all of them + // required, and a Map has nowhere to record optionality), + // so a PATCH of one field forces the model to restate every other — + // turning a partial update into a full overwrite. + // 2. Values are substituted into the JSON unescaped: the templating engine + // runs in TEXT mode and escapes nothing, so a model-supplied value + // containing a quote can break the body or add fields the schema never + // declared. With the whole body in one variable there is no substitution + // boundary to cross. + // 3. It is what the approver sees. The HITL card shows tool arguments, so + // "the arguments are the request" only holds if the model wrote the body + // itself. + // + // The shape the model would have inferred from a decomposed template is + // preserved in the parameter description instead. + return new BodyTemplate("{" + WHOLE_BODY_VARIABLE + "}", Map.of(WHOLE_BODY_VARIABLE, describeBodySchema(schema))); + } + + /** + * A one-line description of the body shape, for the tool parameter. + *

+ * This is the model's only clue about what to write, so it names the properties + * and marks which are required. Types are included because the model must + * produce real JSON — an integer field unquoted, a string field quoted. + */ + private static String describeBodySchema(Schema schema) { + // Name the container the schema actually declares. Saying "a single JSON + // object" for a top-level array makes the model wrap the payload in braces, + // and the request is malformed in a way the API reports and the config does + // not explain. + var description = new StringBuilder("The complete JSON request body, as ").append(switch (schema.getType() == null ? "" : schema.getType()) { + case "array" -> "a JSON array."; + case "string" -> "a JSON string."; + case "integer", "number" -> "a JSON number."; + case "boolean" -> "a JSON boolean."; + default -> "a single JSON object."; + }); @SuppressWarnings("rawtypes") Map properties = schema.getProperties(); if (properties == null || properties.isEmpty()) { - // No properties — use the raw template variable based on schema - return "{requestBody}"; + return description.toString(); } - var sb = new StringBuilder("{\n"); - var entries = new ArrayList<>(properties.entrySet()); - for (int i = 0; i < entries.size(); i++) { - var entry = entries.get(i); - String propName = entry.getKey(); - Schema propSchema = entry.getValue(); - - sb.append(" \"").append(propName).append("\": "); - - String type = propSchema.getType(); - if ("string".equals(type)) { - sb.append("\"{").append(propName).append("}\""); - } else { - // number, integer, boolean, object, array — unquoted template - sb.append("{").append(propName).append("}"); + List required = schema.getRequired() != null ? schema.getRequired() : List.of(); + var parts = new ArrayList(); + for (var entry : properties.entrySet()) { + var propSchema = entry.getValue(); + var part = new StringBuilder(entry.getKey()); + if (propSchema.getType() != null) { + part.append(" (").append(propSchema.getType()); + if (required.contains(entry.getKey())) { + part.append(", required"); + } + part.append(")"); + } else if (required.contains(entry.getKey())) { + part.append(" (required)"); } - - if (i < entries.size() - 1) { - sb.append(","); + if (propSchema.getDescription() != null && !propSchema.getDescription().isBlank()) { + part.append(": ").append(propSchema.getDescription()); } - sb.append("\n"); + parts.add(part.toString()); } - sb.append("}"); - return sb.toString(); + return description.append(" Properties — ").append(String.join("; ", parts)).append(".").toString(); } /** diff --git a/src/main/java/ai/labs/eddi/engine/security/CallerIdentityResolver.java b/src/main/java/ai/labs/eddi/engine/security/CallerIdentityResolver.java index 4429fd64ac..3bd414cf90 100644 --- a/src/main/java/ai/labs/eddi/engine/security/CallerIdentityResolver.java +++ b/src/main/java/ai/labs/eddi/engine/security/CallerIdentityResolver.java @@ -100,6 +100,17 @@ public CallerIdentityResolver(CallerIdentityContext callerIdentityContext, this.enabled = enabled; } + /** + * Whether caller-identity forwarding is switched on for this deployment. + *

+ * Exposed so a config that depends on it — an MCP server whose credential is a + * caller reference, say — can be rejected while it is being validated, rather + * than throwing once per request for the lifetime of the agent. + */ + public boolean isEnabled() { + return enabled; + } + /** Whether a value contains any {@code ${caller:...}} reference. */ public static boolean containsReference(String value) { return value != null && CALLER_PATTERN.matcher(value).find(); diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java b/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java index 4f211d6c7a..4f208a0b5b 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java @@ -84,6 +84,9 @@ import java.io.IOException; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; +import ai.labs.eddi.utils.LogSanitizer; + +import java.net.URI; import java.util.*; import java.util.concurrent.ConcurrentHashMap; @@ -855,7 +858,7 @@ void auditOutcomeUnknown(IConversationMemory memory, PendingToolCallBatch.Pendin */ record ToolSetup(List toolSpecs, Map toolExecutors, Map toolSources, List builtInSpecs, - Map toolCanonicalNames) { + Map toolCanonicalNames, Map toolEndpoints) { } /** @@ -891,6 +894,7 @@ ToolSetup buildToolSetup(LlmConfiguration.Task task, IConversationMemory memory) List toolSpecs = new ArrayList<>(); Map toolExecutors = new HashMap<>(); Map toolSources = new HashMap<>(); + Map toolEndpoints = new HashMap<>(); Map toolCanonicalNames = new HashMap<>(); for (Object tool : tools) { @@ -926,6 +930,9 @@ ToolSetup buildToolSetup(LlmConfiguration.Task task, IConversationMemory memory) // Merge httpcall tools discovered from workflow (if any) if (httpCallTools != null) { mergeExternalTools(httpCallTools.toolSpecs(), httpCallTools.executors(), "http", toolSpecs, toolExecutors, toolSources); + // Endpoint provenance travels beside the source so an approval pattern can + // address what a tool calls, not just what it is named. + toolEndpoints.putAll(httpCallTools.endpoints()); } // Merge mcpcalls tools discovered from workflow (if any) @@ -938,7 +945,7 @@ ToolSetup buildToolSetup(LlmConfiguration.Task task, IConversationMemory memory) mergeExternalTools(a2aTools.toolSpecs(), a2aTools.executors(), "a2a", toolSpecs, toolExecutors, toolSources); } - return new ToolSetup(toolSpecs, toolExecutors, toolSources, builtInSpecs, Map.copyOf(toolCanonicalNames)); + return new ToolSetup(toolSpecs, toolExecutors, toolSources, builtInSpecs, Map.copyOf(toolCanonicalNames), Map.copyOf(toolEndpoints)); } /** @@ -1216,7 +1223,7 @@ private String runToolCallLoop(ChatModel chatModel, List initialMes // calls. clearedCallIds carries the human-approved ids on resume so // they are never re-gated. Inert when effectiveToolApprovals is // null/empty — byte-identical to the pre-HITL path. - var gateResult = toolApprovalGate.classify(aiMessage.toolExecutionRequests(), toolSources, + var gateResult = toolApprovalGate.classify(aiMessage.toolExecutionRequests(), toolSources, setup.toolEndpoints(), effectiveToolApprovals, clearedCallIds); if (!gateResult.gated().isEmpty()) { @@ -2473,8 +2480,63 @@ private DynamicAgentConfig createDefaultDynamicConfig() { /** * Result of httpcall tool discovery. + * + * @param endpoints + * tool name to {@code method:path} (e.g. + * {@code post:/agentstore/agents}), so an approval pattern can + * address the endpoint a tool calls rather than its generated name. + * Names come from {@code operationId} or a slug, which drift; the + * method and path are what the agent designer actually wrote in the + * endpoint allow-list. + */ + record HttpCallToolsResult(List toolSpecs, Map executors, Map endpoints) { + } + + /** + * Normalise a configured path to the shape an approval pattern is written in. + *

+ * The config accepts three shapes for the same endpoint — {@code /a/b}, + * {@code a/b}, and an absolute {@code https://host/a/b} (see + * {@code ApiCallExecutor#buildRequest}, which applies the same leading-slash + * rule). Storing the raw value would make {@code http.post:/a/b} miss two of + * them, and a require-pattern that misses is an ungated write. + *

+ * An absolute URL keeps only its path, so the same pattern matches however the + * target server was configured. */ - record HttpCallToolsResult(List toolSpecs, Map executors) { + static String normalizeEndpointPath(String rawPath) { + String path = rawPath.trim(); + // Request.path defaults to "", and an empty path means the target server's + // root — ApiCallExecutor sends the request to targetServerUrl unchanged. + // Returning "" here would make the key "post:", which no pattern can match, + // so a root endpoint would be silently ungateable. + if (path.isEmpty()) { + return "/"; + } + // Only a real absolute URL, not merely a path that happens to begin "http". + // ApiCallExecutor tests startsWith("http"), which is loose enough that a + // relative path like "httpcalls/agents" would be parsed as a URI here — and an + // opaque one collapses to an empty path, losing the endpoint entirely. + String lower = path.toLowerCase(Locale.ROOT); + if (lower.startsWith("http://") || lower.startsWith("https://")) { + try { + String extracted = URI.create(path).getPath(); + // A URL with no path ("https://host", "https://host?x=1") yields an + // empty path. Left empty the endpoint key would be "post:", which no + // pattern expecting a leading slash can match — and an unmatched + // require-pattern is an ungated call. A root endpoint is "/". + path = extracted == null || extracted.isEmpty() ? "/" : extracted; + } catch (IllegalArgumentException e) { + // Not parseable as a URI — a templated host, most likely. Leave it be: + // matching something odd is better than throwing during discovery. + LOGGER.debugf("Could not normalise endpoint path '%s' for approval matching", LogSanitizer.sanitize(rawPath)); + return path; + } + } + if (!path.isEmpty() && !path.startsWith("/")) { + path = "/" + path; + } + return path; } /** @@ -2488,6 +2550,7 @@ record HttpCallToolsResult(List toolSpecs, Map toolSpecs = new ArrayList<>(); Map executors = new HashMap<>(); + Map endpoints = new HashMap<>(); try { LOGGER.infof("Discovering httpcall tools for agent: %s v%s", memory.getAgentId(), memory.getAgentVersion()); @@ -2518,6 +2581,14 @@ HttpCallToolsResult discoverHttpCallTools(IConversationMemory memory) { toolSpecs.add(specBuilder.build()); + // Record what this tool actually calls, so approval patterns can be + // written against the endpoint rather than the generated name. + var apiRequest = apiCall.getRequest(); + if (apiRequest != null && apiRequest.getMethod() != null && apiRequest.getPath() != null) { + endpoints.put(apiCall.getName(), + apiRequest.getMethod().toLowerCase(Locale.ROOT) + ":" + normalizeEndpointPath(apiRequest.getPath())); + } + executors.put(apiCall.getName(), (toolRequest, memoryId) -> { try { Map templateData = memoryItemConverter.convert(memory); @@ -2552,7 +2623,7 @@ HttpCallToolsResult discoverHttpCallTools(IConversationMemory memory) { LOGGER.warn("Failed to discover httpcall tools from workflow", e); } - return new HttpCallToolsResult(toolSpecs, executors); + return new HttpCallToolsResult(toolSpecs, executors, endpoints); } // --- McpCalls auto-discovery from workflow --- diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java b/src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java index 7fe5262b56..cc1d799e95 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java @@ -5,17 +5,21 @@ package ai.labs.eddi.modules.llm.impl; import ai.labs.eddi.configs.variables.GlobalVariableResolver; +import ai.labs.eddi.engine.security.CallerIdentityContext; +import ai.labs.eddi.engine.security.CallerIdentityResolver; import ai.labs.eddi.modules.llm.model.LlmConfiguration.McpServerConfig; import ai.labs.eddi.modules.llm.tools.UrlValidationUtils; import ai.labs.eddi.secrets.SecretResolver; import dev.langchain4j.agent.tool.ToolSpecification; import dev.langchain4j.mcp.McpToolProvider; import dev.langchain4j.mcp.client.DefaultMcpClient; +import dev.langchain4j.mcp.client.McpCallContext; import dev.langchain4j.mcp.client.McpClient; import dev.langchain4j.mcp.client.transport.McpTransport; import dev.langchain4j.mcp.client.transport.http.StreamableHttpMcpTransport; import dev.langchain4j.service.tool.ToolExecutor; import dev.langchain4j.service.tool.ToolProviderResult; +import io.micrometer.core.instrument.MeterRegistry; import jakarta.annotation.PreDestroy; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; @@ -24,6 +28,10 @@ import static ai.labs.eddi.utils.LogSanitizer.sanitize; +import java.net.URI; +import java.security.NoSuchAlgorithmException; +import java.security.MessageDigest; +import java.nio.charset.StandardCharsets; import java.time.Duration; import java.time.Instant; import java.util.*; @@ -54,6 +62,8 @@ public class McpToolProviderManager { private final GlobalVariableResolver globalVariableResolver; private final SecretResolver secretResolver; + private final CallerIdentityResolver callerIdentityResolver; + private final CallerIdentityContext callerIdentityContext; private final boolean ssrfProtectionEnabled; private final int maxDescriptionChars; private final long toolCacheTtlMillis; @@ -105,24 +115,71 @@ record CachedTools(List toolSpecs, Map /** Recent failure timestamps per server URL. */ private final Map> failureTimestamps = new ConcurrentHashMap<>(); + /** + * Field-injected so a directly constructed instance (tests, non-CDI callers) + * keeps working without one — {@link #count} is null-safe. + */ + @Inject + MeterRegistry meterRegistry; + + /** + * Count an MCP discovery outcome. + *

+ * Tags are a fixed vocabulary and never carry a URL or a credential: a server + * URL is unbounded cardinality, and the credential is the thing the cache key + * goes to lengths to hash. A rising {@code circuit_open} or {@code failure} + * count is the signal worth alerting on. + */ + private void count(String outcome) { + if (meterRegistry == null) { + return; + } + meterRegistry.counter("eddi.mcp.discovery", "outcome", outcome).increment(); + } + @Inject public McpToolProviderManager(GlobalVariableResolver globalVariableResolver, SecretResolver secretResolver, + CallerIdentityResolver callerIdentityResolver, CallerIdentityContext callerIdentityContext, @ConfigProperty(name = "eddi.security.ssrf-protection.enabled", defaultValue = "false") boolean ssrfProtectionEnabled, @ConfigProperty(name = "eddi.mcp.tool-description.max-chars", defaultValue = "1024") int maxDescriptionChars, @ConfigProperty(name = "eddi.mcp.tool-cache.ttl-ms", defaultValue = "300000") long toolCacheTtlMillis) { this.globalVariableResolver = globalVariableResolver; this.secretResolver = secretResolver; + this.callerIdentityResolver = callerIdentityResolver; + this.callerIdentityContext = callerIdentityContext; this.ssrfProtectionEnabled = ssrfProtectionEnabled; this.maxDescriptionChars = maxDescriptionChars > 0 ? maxDescriptionChars : DEFAULT_MAX_DESCRIPTION_CHARS; this.toolCacheTtlMillis = toolCacheTtlMillis >= 0 ? toolCacheTtlMillis : DEFAULT_TOOL_CACHE_TTL_MILLIS; } + /** + * A caller context for the back-compat constructor below. + *

+ * Safe to share: the binding {@code CallerIdentityContext} reads lives in a + * static {@link ThreadLocal}, so every instance observes the same per-thread + * caller. One instance keeps the two constructors consistent. + */ + private static final CallerIdentityContext UNBOUND_CALLER_CONTEXT = new CallerIdentityContext(null, null); + + /** + * Test/back-compat constructor for callers that set the operational knobs but + * have no caller context — the caller falls back to whatever is bound to the + * calling thread, which is nothing in a unit test. + */ + McpToolProviderManager(GlobalVariableResolver globalVariableResolver, SecretResolver secretResolver, boolean ssrfProtectionEnabled, + int maxDescriptionChars, long toolCacheTtlMillis) { + this(globalVariableResolver, secretResolver, new CallerIdentityResolver(UNBOUND_CALLER_CONTEXT, true), UNBOUND_CALLER_CONTEXT, + ssrfProtectionEnabled, maxDescriptionChars, toolCacheTtlMillis); + } + /** * Test/back-compat constructor using the shipped defaults (SSRF validation off, - * 1024-char description cap, 5-minute tool cache). + * 1024-char description cap, 5-minute tool cache) and a caller context that + * resolves whatever is bound to the calling thread. */ McpToolProviderManager(GlobalVariableResolver globalVariableResolver, SecretResolver secretResolver) { - this(globalVariableResolver, secretResolver, false, DEFAULT_MAX_DESCRIPTION_CHARS, DEFAULT_TOOL_CACHE_TTL_MILLIS); + this(globalVariableResolver, secretResolver, new CallerIdentityResolver(UNBOUND_CALLER_CONTEXT, true), UNBOUND_CALLER_CONTEXT, false, + DEFAULT_MAX_DESCRIPTION_CHARS, DEFAULT_TOOL_CACHE_TTL_MILLIS); } /** @@ -217,7 +274,9 @@ public McpToolsResult discoverTools(List mcpServers) { } // Circuit breaker: skip servers that failed too often recently - if (isCircuitOpen(url)) { + String circuitKey = cacheKey(serverConfig); + if (isCircuitOpen(circuitKey)) { + count("circuit_open"); LOGGER.warnf("Circuit breaker OPEN for MCP server '%s' — skipping (>=%d failures in last %ds)", sanitize(serverName), CIRCUIT_FAILURE_THRESHOLD, CIRCUIT_WINDOW_SECONDS); failures.add(new McpServerFailure(serverName, url, McpFailureKind.CIRCUIT_OPEN, @@ -229,9 +288,10 @@ public McpToolsResult discoverTools(List mcpServers) { // a live tools/list RPC per server per turn was pure overhead. Nothing // reaches the cache without having passed validation first, so this is // checked before the (potentially DNS-resolving) validation below. - CachedTools cached = toolCache.get(url); + CachedTools cached = toolCache.get(cacheKey(serverConfig)); if (cached != null && (System.currentTimeMillis() - cached.timestamp()) < toolCacheTtlMillis) { LOGGER.debugf("Serving %d cached tools for MCP server '%s'", cached.toolSpecs().size(), sanitize(serverName)); + count("cache_hit"); mergeServerTools(allSpecs, allExecutors, cached.toolSpecs(), cached.executors(), serverName); continue; } @@ -243,6 +303,7 @@ public McpToolsResult discoverTools(List mcpServers) { try { validateServerUrl(url); validateTransport(serverConfig.getTransport()); + validateCallerBoundKey(serverConfig); } catch (IllegalArgumentException e) { LOGGER.errorf("MCP server '%s' was NOT contacted because its configuration is invalid: %s — " + "the agent runs without this server's tools until the config is fixed", @@ -285,17 +346,19 @@ public McpToolsResult discoverTools(List mcpServers) { LOGGER.infof("Discovered %d tools from MCP server '%s'", serverSpecs.size(), sanitize(serverName)); } - toolCache.put(url, new CachedTools(List.copyOf(serverSpecs), Map.copyOf(serverExecutors), + toolCache.put(cacheKey(serverConfig), new CachedTools(List.copyOf(serverSpecs), Map.copyOf(serverExecutors), System.currentTimeMillis())); mergeServerTools(allSpecs, allExecutors, serverSpecs, serverExecutors, serverName); // Success — clear failure history for this server - recordSuccess(url); + recordSuccess(circuitKey); + count("success"); } catch (Exception e) { LOGGER.warnf(e, "Failed to connect to MCP server '%s': %s", sanitize(serverName), e.getMessage()); failures.add(new McpServerFailure(serverName, url, McpFailureKind.CONNECTION_FAILURE, e.getMessage())); - recordFailure(url); + recordFailure(circuitKey); + count("failure"); } } @@ -348,12 +411,46 @@ ToolProviderResult fetchToolsFromServer(McpServerConfig serverConfig) { return toolProvider.provideTools(null); } + /** + * Cache key for a client: the URL plus a digest of the configured credential. + *

+ * Keying on the URL alone let two agents configured against the same server + * with different credentials share whichever client was constructed + * first — the second silently borrowed the first's authorization. That is a + * privilege boundary, not a caching detail. + *

+ * The credential is digested rather than used directly so a literal key never + * becomes a map key that could reach a heap dump or a log line. The value is + * taken unresolved, so two configs sharing one vault reference still + * share a client — the distinction that matters is which credential a config + * names, not what it resolves to. + *

+ * This does not multiply clients per user: a {@code ${caller:...}} config + * yields one client whose header supplier reads the caller per request, so + * every user of that config shares it. + */ + private static String cacheKey(McpServerConfig config) { + String apiKey = config.getApiKey(); + if (isNullOrEmpty(apiKey)) { + return config.getUrl() + "|anonymous"; + } + try { + var digest = MessageDigest.getInstance("SHA-256").digest(apiKey.getBytes(StandardCharsets.UTF_8)); + return config.getUrl() + "|" + HexFormat.of().formatHex(digest); + } catch (NoSuchAlgorithmException e) { + // SHA-256 is mandated by the platform; if it is truly absent, fall back to + // isolating by identity so distinct credentials still cannot share. + return config.getUrl() + "|" + System.identityHashCode(apiKey); + } + } + /** * Get or create an MCP client for the given server configuration. Clients are - * cached by URL for connection reuse. + * cached per URL and credential for connection reuse. */ private McpClient getOrCreateClient(McpServerConfig config) { - return clientCache.computeIfAbsent(config.getUrl(), url -> { + return clientCache.computeIfAbsent(cacheKey(config), key -> { + String url = config.getUrl(); LOGGER.infof("Creating MCP client for '%s' (%s transport)", sanitize(config.getName() != null ? config.getName() : url), sanitize(config.getTransport())); @@ -385,6 +482,30 @@ private McpClient getOrCreateClient(McpServerConfig config) { * @throws IllegalArgumentException * if the URL is unusable or targets an internal address */ + /** + * Refuse a credential the deployment cannot resolve. + *

+ * A {@code ${caller:...}} key needs caller-identity forwarding switched on. + * With it off, every tool call would throw from {@code authorizationHeader} — + * once per request, forever, for a mistake made once in configuration. This + * reports it alongside the URL and transport checks, so it surfaces as + * {@code INVALID_CONFIGURATION} where an operator will actually see it. + */ + private void validateCallerBoundKey(McpServerConfig config) { + try { + // A bare {caller:token} or a typo like ${caller:tokn} is a config mistake. + // Left to createTransport it surfaces as CONNECTION_FAILURE and trips the + // circuit breaker, blaming the server for something the config did. + callerIdentityResolver.rejectUnsupportedReference(config.getApiKey()); + } catch (CallerIdentityResolver.CallerIdentityException e) { + throw new IllegalArgumentException("apiKey: " + e.getMessage(), e); + } + if (CallerIdentityResolver.containsReference(config.getApiKey()) && !callerIdentityResolver.isEnabled()) { + throw new IllegalArgumentException("apiKey uses ${caller:...} but caller-identity forwarding is disabled " + + "(eddi.caller-identity.enabled=false) — set a static key or enable forwarding"); + } + } + void validateServerUrl(String url) { if (isNullOrEmpty(url)) { throw new IllegalArgumentException("MCP server URL must not be empty"); @@ -455,9 +576,17 @@ private McpTransport createTransport(McpServerConfig config, Duration timeout) { // logging it and quietly using StreamableHTTP. validateTransport(config.getTransport()); - // Resolve API key if it's a global variable or vault reference + // Resolve the static half once: a global variable or vault reference does not + // change between requests, and the handshake needs a credential before any + // conversation exists. A caller reference is left alone — it is resolved per + // request below. String apiKey = config.getApiKey(); - if (!isNullOrEmpty(apiKey)) { + // A bare {caller:token} — the natural Qute namespace form — is a reference the + // resolver never substitutes. Left to containsReference() it would look like a + // static key and be sent as literal text; this turns it into a clear error. + callerIdentityResolver.rejectUnsupportedReference(apiKey); + boolean callerBound = CallerIdentityResolver.containsReference(apiKey); + if (!isNullOrEmpty(apiKey) && !callerBound) { apiKey = globalVariableResolver.resolveValue(apiKey); apiKey = secretResolver.resolveValue(apiKey); } @@ -466,23 +595,112 @@ private McpTransport createTransport(McpServerConfig config, Duration timeout) { // HttpMcpTransport) var transportBuilder = StreamableHttpMcpTransport.builder().url(config.getUrl()).timeout(timeout); - // Add API key as Authorization header if configured if (!isNullOrEmpty(apiKey)) { - final String resolvedKey = apiKey; - transportBuilder.customHeaders(Map.of("Authorization", "Bearer " + resolvedKey)); + final String configuredKey = apiKey; + // Per request, not per client. The Map overload would freeze this value + // into the cached client, which is why a ${caller:...} reference has to be + // resolved here: the client is shared across conversations, the caller is + // not. Applies to every POST the transport makes. + transportBuilder.customHeaders(callContext -> authorizationHeader(configuredKey, config, callerBound, callContext)); } return transportBuilder.build(); } /** - * Close a specific MCP client connection and remove it from the cache. + * Give a bare token the {@code Bearer} scheme, leaving an already-qualified + * value alone. + *

+ * {@code McpServerConfig.apiKey} is documented as a key or token, and the + * static path has always prefixed {@code Bearer }. A caller-bound key must + * behave the same or {@code apiKey: "${caller:token}"} — the form the + * documentation implies — would send a raw token with no scheme and the server + * would reject it. An author who spells the scheme out (as apicall headers do) + * is not double-prefixed. + */ + private static String withBearerPrefix(String value) { + if (value == null || value.isBlank()) { + return value; + } + // Trim before use, not just for the check: stray whitespace — a newline most + // of all — has no business reaching an Authorization header, and returning + // the untrimmed value would emit " Bearer …" for a padded config. + String trimmed = value.trim(); + // A scheme is one token followed by a space; anything else is a bare secret. + return trimmed.matches("(?i)(bearer|basic|token|apikey)\\s.+") ? trimmed : "Bearer " + trimmed; + } + + /** + * Build the {@code Authorization} header for a single MCP request. + *

+ * A static key behaves exactly as before. A {@code ${caller:...}} reference + * resolves against the caller bound to this thread, so the tool call reaches + * the MCP server as the person chatting rather than as a standing service + * principal — the same guarantee {@code ApiCallExecutor} gives apicall headers, + * including the same-origin restriction and failing closed rather than sending + * a placeholder. + *

+ * Only a tool call may carry the caller. Discovery — the {@code initialize} + * handshake and {@code tools/list} — must not, even though it runs on a thread + * that is bound to a caller: the client is cached, so a session + * established with one user's token would be reused by everyone after them, and + * a tool list reflecting one user's permissions would be offered to the next. + *

+ * langchain4j draws that line for us. {@code DefaultMcpClient.listTools()} + * delegates with a null invocation context, while {@code McpToolExecutor} + * always builds one — so {@link McpCallContext#invocationContext()} is the + * discriminator. The context itself is null on some transport-internal + * requests, which count as not-a-tool-call. + *

+ * A caller-bound config therefore has nothing to offer discovery and sends it + * unauthenticated, letting the server decide. + */ + private Map authorizationHeader(String configuredKey, McpServerConfig config, boolean callerBound, + McpCallContext callContext) { + if (!callerBound) { + return Map.of("Authorization", withBearerPrefix(configuredKey)); + } + if (callContext == null || callContext.invocationContext() == null) { + LOGGER.debugf("MCP discovery request to '%s' carries no caller by design — sending it unauthenticated", + sanitize(config.getUrl())); + return Map.of(); + } + if (callerIdentityContext.current() == null) { + // A scheduled turn, or a retry that landed on an HTTP callback thread where + // the binding does not exist. Sending the placeholder text would be + // nonsense and sending nothing is honest: the server refuses if it requires + // authentication, which is a visible failure rather than the wrong + // authority. + LOGGER.debugf("No caller bound for MCP tool call to '%s' — sending it unauthenticated", sanitize(config.getUrl())); + return Map.of(); + } + String resolved = callerIdentityResolver.resolveValue(configuredKey, URI.create(config.getUrl())); + return Map.of("Authorization", withBearerPrefix(resolved)); + } + + /** + * Close every cached client for a server URL and remove them from the cache. + *

+ * A URL can now hold more than one client — one per configured credential — so + * this closes all of them. Callers identify a server, not a credential. */ void closeClient(String url) { - // The cached executors are bound to this client — drop them with it. - toolCache.remove(url); - McpClient client = clientCache.remove(url); - if (client != null) { + if (url == null) { + return; + } + String prefix = url + "|"; + // The cached executors hold a reference to the client, so they are keyed the + // same way and must be dropped with it — a bare toolCache.remove(url) would + // leave executors bound to a closed client behind. + toolCache.keySet().removeIf(key -> key.startsWith(prefix)); + for (var key : List.copyOf(clientCache.keySet())) { + if (!key.startsWith(prefix)) { + continue; + } + McpClient client = clientCache.remove(key); + if (client == null) { + continue; + } try { client.close(); LOGGER.infof("Closed MCP client for '%s'", sanitize(url)); @@ -526,12 +744,29 @@ int getCachedToolServerCount() { // ========================== Circuit Breaker ========================== /** - * Check whether the circuit breaker is open for a given server URL. The circuit - * opens when the server has failed {@value #CIRCUIT_FAILURE_THRESHOLD} or more - * times within the last {@value #CIRCUIT_WINDOW_SECONDS} seconds. + * Whether the circuit is open for a configured server. It opens when the server + * has failed {@value #CIRCUIT_FAILURE_THRESHOLD} or more times within the last + * {@value #CIRCUIT_WINDOW_SECONDS} seconds. + *

+ * This is the contract callers want: the key is an implementation detail, and a + * credential-aware one, so asking by URL gives the wrong answer for a server + * configured twice with different credentials. + * + * @param config + * the server whose circuit is in question + */ + boolean isCircuitOpen(McpServerConfig config) { + return isCircuitOpen(cacheKey(config)); + } + + /** + * @param circuitKey + * the credential-aware cache key, not the bare URL: two configs may + * point at one server with different credentials, and one of them + * failing to authenticate must not suppress discovery for the other */ - boolean isCircuitOpen(String url) { - List failures = failureTimestamps.get(url); + boolean isCircuitOpen(String circuitKey) { + List failures = failureTimestamps.get(circuitKey); if (failures == null) { return false; } diff --git a/src/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGateTest.java b/src/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGateTest.java index b3dcb3edd0..cbc6cf1bb6 100644 --- a/src/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGateTest.java +++ b/src/test/java/ai/labs/eddi/engine/hitl/tools/ToolApprovalGateTest.java @@ -6,6 +6,7 @@ import ai.labs.eddi.configs.hitl.model.ToolApprovalsConfig; import dev.langchain4j.agent.tool.ToolExecutionRequest; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import java.util.List; @@ -27,6 +28,108 @@ private static ToolApprovalsConfig cfg(List require, List exempt return c; } + // ==================== endpoint-qualified patterns ==================== + + /** Two generated tools whose names say nothing about what they do. */ + private static final Map SOURCES = Map.of("listAgents", "http", "createAgent", "http", "updateLlm", "http"); + private static final Map ENDPOINTS = Map.of( + "listAgents", "get:/agentstore/agents/descriptors", + "createAgent", "post:/agentstore/agents", + "updateLlm", "put:/llmstore/llms/{id}"); + + private static List allThree() { + return List.of(req("1", "listAgents"), req("2", "createAgent"), req("3", "updateLlm")); + } + + @Test + void methodQualifiedPattern_gatesEveryMutationWithoutNamingAnyTool() { + // The point of the form: nobody maintains a list of write tool names, so a + // newly generated endpoint cannot arrive ungated. + var gate = new ToolApprovalGate(); + var result = gate.classify(allThree(), SOURCES, ENDPOINTS, + cfg(List.of("http.post:*", "http.put:*", "http.patch:*", "http.delete:*"), null), Set.of()); + + assertEquals(List.of("createAgent", "updateLlm"), result.gated().stream().map(ToolExecutionRequest::name).toList()); + assertEquals(List.of("listAgents"), result.allowed().stream().map(ToolExecutionRequest::name).toList()); + } + + @Test + void endpointQualifiedPattern_addressesOneEndpoint() { + // Different POSTs carry different weight, so a pattern must be able to name + // the endpoint rather than only the method. + var gate = new ToolApprovalGate(); + var result = gate.classify(allThree(), SOURCES, ENDPOINTS, cfg(List.of("http.post:/agentstore/agents"), null), Set.of()); + + assertEquals(List.of("createAgent"), result.gated().stream().map(ToolExecutionRequest::name).toList()); + } + + @Test + void pathTemplateBracesAreLiterals_notRegexQuantifiers() { + var gate = new ToolApprovalGate(); + var result = gate.classify(allThree(), SOURCES, ENDPOINTS, cfg(List.of("http.put:/llmstore/llms/{id}"), null), Set.of()); + + assertEquals(List.of("updateLlm"), result.gated().stream().map(ToolExecutionRequest::name).toList()); + } + + @Test + void exemptMayAlsoBeEndpointQualified() { + // Gate everything, then exempt reads — the only safe direction, since a + // missed exemption costs an approval prompt rather than an ungated write. + var gate = new ToolApprovalGate(); + var result = gate.classify(allThree(), SOURCES, ENDPOINTS, cfg(List.of("http:*"), List.of("http.get:*")), Set.of()); + + assertEquals(List.of("createAgent", "updateLlm"), result.gated().stream().map(ToolExecutionRequest::name).toList()); + } + + @Test + @DisplayName("an endpoint-shaped pattern that could never match is rejected") + void endpointShapedPatternsThatCannotMatchAreRejected() { + // Each of these saved cleanly and matched nothing: runtime emits + // "http.:/path", so a require-rule written any other way leaves the + // call allowed while the config looks like protection. + for (String pattern : List.of("http:/agentstore/agents", "mcp:/x", "http.post:agents", "mcp*:/x", "a2a*:/x")) { + assertTrue(ToolApprovalPatterns.validate(pattern).isPresent(), pattern + " can never match, so it must not save"); + } + // The forms that do match stay valid, including a wildcard method. + for (String pattern : List.of("http.post:/agentstore/agents", "http.post:*", "http:*", "http.*:/x", "*:/x")) { + assertTrue(ToolApprovalPatterns.validate(pattern).isEmpty(), + pattern + " is a working pattern: " + ToolApprovalPatterns.validate(pattern)); + } + } + + @Test + void methodQualifierIsRejectedForSourcesThatCarryNoEndpoint() { + // mcp and a2a tools register a source and no endpoint, so "mcp.post:*" + // would save cleanly and then match nothing — and an unmatched call is + // allowed. Saving must fail rather than produce a gate that does nothing. + for (String pattern : List.of("mcp.post:*", "a2a.delete:*", "memory.put:x", "builtin.get:*")) { + assertTrue(ToolApprovalPatterns.validate(pattern).isPresent(), pattern + " must be rejected: nothing can ever match it"); + } + assertTrue(ToolApprovalPatterns.validate("http.post:*").isEmpty(), "http is the one source that carries an endpoint"); + } + + @Test + void aMethodQualifiedMcpPatternCannotSilentlyGateNothing() { + // The runtime half of the guarantee above: even if such a pattern reached + // the gate, it must not look like protection. + var gate = new ToolApprovalGate(); + var batch = List.of(req("1", "mcp_write_thing")); + var sources = Map.of("mcp_write_thing", "mcp"); + + var result = gate.classify(batch, sources, Map.of(), cfg(List.of("mcp.post:*"), null), Set.of()); + assertTrue(result.gated().isEmpty(), "documents the fail-open: this is why save-time validation must reject it"); + } + + @Test + void existingSourcePatternsKeepWorkingWithoutEndpointData() { + // Backward compatibility: agents configured before endpoint provenance + // existed pass an empty map and must gate exactly as they did. + var gate = new ToolApprovalGate(); + var result = gate.classify(allThree(), SOURCES, Map.of(), cfg(List.of("http:*"), null), Set.of()); + + assertEquals(3, result.gated().size(), "http:* must still gate every http tool"); + } + @Test void nullOrEmptyConfig_gatesNothing() { var gate = new ToolApprovalGate(); diff --git a/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java b/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java index efff108618..5d986f8ac4 100644 --- a/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java +++ b/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java @@ -5,6 +5,7 @@ package ai.labs.eddi.engine.mcp; import ai.labs.eddi.configs.apicalls.model.ApiCall; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import java.util.List; @@ -223,10 +224,167 @@ void parseAndBuild_requestBody() { assertEquals("application/json", createPet.getRequest().getContentType()); String body = createPet.getRequest().getBody(); assertNotNull(body); - assertTrue(body.contains("\"name\""), "Body should have 'name' field"); - assertTrue(body.contains("\"age\""), "Body should have 'age' field"); - // String field should be quoted - assertTrue(body.contains("\"{name}\""), "String param should be quoted in template"); + // One variable for the whole body: the model writes the JSON itself, so + // there is no unescaped substitution into a per-property template. + assertEquals("{" + McpApiToolBuilder.WHOLE_BODY_VARIABLE + "}", body); + // The shape a per-property template would have implied is carried in the + // parameter description instead, so the model still knows what to write. + String bodyDescription = createPet.getParameters().get(McpApiToolBuilder.WHOLE_BODY_VARIABLE); + assertTrue(bodyDescription.contains("name"), bodyDescription); + assertTrue(bodyDescription.contains("age"), bodyDescription); + assertTrue(bodyDescription.contains("integer"), "the model must know age is not a quoted string: " + bodyDescription); + } + + @Test + @DisplayName("a parameter named requestBody does not silently drop the body variable") + void parseAndBuild_bodyVariableIsRenamedOnCollision() { + // putIfAbsent would skip the body variable here, leaving the template + // referencing something undeclared — the empty-body bug again, for a spec + // that happens to name a parameter "requestBody". + String spec = """ + {"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{"/things/{requestBody}":{"post":{ + "operationId":"createThing","tags":["things"], + "parameters":[{"name":"requestBody","in":"path","required":true,"description":"A path id","schema":{"type":"string"}}], + "requestBody":{"content":{"application/json":{"schema":{"$ref":"#/components/schemas/T"}}}}, + "responses":{"200":{"description":"ok"}}}}},"components":{"schemas":{"T":{}}}} + """; + var result = McpApiToolBuilder.parseAndBuild(spec, null, null, null); + ApiCall call = result.configsByGroup().get("things").getHttpCalls().get(0); + + assertEquals("A path id", call.getParameters().get("requestBody"), "the path parameter keeps the name"); + // Every variable the body template references must still be declared. + var matcher = java.util.regex.Pattern.compile("\\{([A-Za-z0-9_]+)}").matcher(call.getRequest().getBody()); + assertTrue(matcher.find()); + assertTrue(call.getParameters().containsKey(matcher.group(1)), + "the body variable was renamed to " + matcher.group(1) + " and must be declared"); + assertNotEquals("requestBody", matcher.group(1), "it cannot keep the colliding name"); + } + + @Test + @DisplayName("a declared body with no schema still gets a variable") + void parseAndBuild_bodyWithNoSchemaStillDeclaresAVariable() { + // Returning "{}" and declaring nothing recreates the original bug for this + // spec shape: the model cannot fill a body and every write goes out empty. + String spec = """ + {"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{"/things":{"post":{ + "operationId":"createThing","tags":["things"], + "requestBody":{"content":{"application/json":{}}}, + "responses":{"200":{"description":"ok"}}}}}} + """; + var result = McpApiToolBuilder.parseAndBuild(spec, null, null, null); + ApiCall createThing = result.configsByGroup().get("things").getHttpCalls().get(0); + + assertEquals("{" + McpApiToolBuilder.WHOLE_BODY_VARIABLE + "}", createThing.getRequest().getBody()); + assertTrue(createThing.getParameters().containsKey(McpApiToolBuilder.WHOLE_BODY_VARIABLE)); + } + + @Test + @DisplayName("the body description names the container the schema declares") + void parseAndBuild_bodyDescriptionNamesTheRealContainer() { + // Telling the model "a single JSON object" for an array body makes it wrap the + // payload in braces, and the API rejects a request the config cannot explain. + String spec = """ + {"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{"/bulk":{"post":{ + "operationId":"createMany","tags":["bulk"], + "requestBody":{"content":{"application/json":{"schema":{"type":"array","items":{"type":"string"}}}}}, + "responses":{"200":{"description":"ok"}}}}}} + """; + var result = McpApiToolBuilder.parseAndBuild(spec, null, null, null); + ApiCall createMany = result.configsByGroup().get("bulk").getHttpCalls().get(0); + + String description = createMany.getParameters().get(McpApiToolBuilder.WHOLE_BODY_VARIABLE); + assertTrue(description.contains("JSON array"), description); + assertFalse(description.contains("single JSON object"), description); + } + + @Test + @DisplayName("required properties are marked so, and optional ones are not forced") + void parseAndBuild_bodyDescriptionMarksRequiredProperties() { + // Every declared parameter becomes a REQUIRED tool parameter, so optionality + // has to live in the description or a PATCH of one field would force the + // model to restate all the others. + String spec = """ + {"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{"/things":{"post":{ + "operationId":"createThing","tags":["things"], + "requestBody":{"content":{"application/json":{"schema":{"type":"object", + "required":["name"], + "properties":{"name":{"type":"string"},"nickname":{"type":"string"}}}}}}, + "responses":{"200":{"description":"ok"}}}}}} + """; + var result = McpApiToolBuilder.parseAndBuild(spec, null, null, null); + ApiCall createThing = result.configsByGroup().get("things").getHttpCalls().get(0); + + assertEquals(1, createThing.getParameters().size(), "a body contributes exactly one parameter"); + String description = createThing.getParameters().get(McpApiToolBuilder.WHOLE_BODY_VARIABLE); + assertTrue(description.contains("name (string, required)"), description); + assertTrue(description.contains("nickname (string)") && !description.contains("nickname (string, required)"), description); + } + + @Test + @DisplayName("body template variables are declared as parameters, or the model cannot fill them") + void parseAndBuild_requestBodyVariablesAreDeclaredAsParameters() { + // The tool schema handed to the LLM is built from getParameters() alone. An + // undeclared body variable is invisible to the model and — with strict + // rendering off — renders empty, so the call succeeds with an empty body. + var result = McpApiToolBuilder.parseAndBuild(PETSTORE_SPEC, null, null, null); + var petsConfig = result.configsByGroup().get("pets"); + + ApiCall createPet = petsConfig.getHttpCalls().stream().filter(c -> "createPet".equals(c.getName())).findFirst().orElseThrow(); + + assertNotNull(createPet.getParameters(), "a call with a body must declare parameters"); + // Every variable the template references must be declared — that is the + // invariant, whatever shape the template takes. + var matcher = java.util.regex.Pattern.compile("\\{([A-Za-z0-9_]+)}").matcher(createPet.getRequest().getBody()); + int found = 0; + while (matcher.find()) { + found++; + assertTrue(createPet.getParameters().containsKey(matcher.group(1)), + matcher.group(1) + " is in the body template but not declared as a parameter"); + } + assertTrue(found > 0, "a call with a request body must reference at least one variable"); + } + + @Test + @DisplayName("a body with no decomposable properties is declared as one whole-body parameter") + void parseAndBuild_wholeBodyVariableIsDeclared() { + // The common case for this API: an unresolved $ref collapses to a single + // variable carrying the entire JSON body. + String spec = """ + {"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{"/things":{"post":{ + "operationId":"createThing","tags":["things"], + "requestBody":{"content":{"application/json":{"schema":{"$ref":"#/components/schemas/Thing"}}}}, + "responses":{"200":{"description":"ok"}}}}}, + "components":{"schemas":{"Thing":{}}}} + """; + var result = McpApiToolBuilder.parseAndBuild(spec, null, null, null); + ApiCall createThing = result.configsByGroup().get("things").getHttpCalls().get(0); + + assertEquals("{" + McpApiToolBuilder.WHOLE_BODY_VARIABLE + "}", createThing.getRequest().getBody()); + assertNotNull(createThing.getParameters()); + assertTrue(createThing.getParameters().containsKey(McpApiToolBuilder.WHOLE_BODY_VARIABLE), + "the whole-body variable must be declared, otherwise every POST sends an empty body"); + } + + @Test + @DisplayName("a path parameter and the body no longer share a variable") + void parseAndBuild_pathParameterAndBodyDoNotCollide() { + String spec = """ + {"openapi":"3.0.0","info":{"title":"t","version":"1"},"paths":{"/things/{id}":{"put":{ + "operationId":"updateThing","tags":["things"], + "parameters":[{"name":"id","in":"path","required":true,"description":"The path id","schema":{"type":"string"}}], + "requestBody":{"content":{"application/json":{"schema":{"type":"object","properties":{ + "id":{"type":"string","description":"A body id"},"label":{"type":"string"}}}}}}, + "responses":{"200":{"description":"ok"}}}}}} + """; + var result = McpApiToolBuilder.parseAndBuild(spec, null, null, null); + ApiCall updateThing = result.configsByGroup().get("things").getHttpCalls().get(0); + + // With the whole body in one variable there is no collision left to resolve: + // the path keeps {id}, and the body's own id is the model's to write. + assertEquals("The path id", updateThing.getParameters().get("id")); + assertTrue(updateThing.getParameters().containsKey(McpApiToolBuilder.WHOLE_BODY_VARIABLE)); + assertFalse(updateThing.getParameters().containsKey("label"), "body properties are no longer separate parameters"); + assertTrue(updateThing.getParameters().get(McpApiToolBuilder.WHOLE_BODY_VARIABLE).contains("label")); } @Test diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java index 81c8a87b70..5a37d7e025 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorCoverageTest.java @@ -889,7 +889,7 @@ void tenantQuotaServiceNull_skipsQuotaCheck() throws Exception { // ═══════════════════════════════════════════════════════════════════ private AgentOrchestrator.ToolSetup setupWith(List all, List builtIn) { - return new AgentOrchestrator.ToolSetup(all, Map.of(), Map.of(), builtIn, Map.of()); + return new AgentOrchestrator.ToolSetup(all, Map.of(), Map.of(), builtIn, Map.of(), Map.of()); } private ToolSpecification spec(String name) { diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java index 588af21755..c6e7bce493 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorExtendedTest.java @@ -639,7 +639,7 @@ class HttpCallToolsResultTests { void testRecordFields() { var spec = ToolSpecification.builder().name("test").description("test").build(); var result = new AgentOrchestrator.HttpCallToolsResult( - List.of(spec), Map.of()); + List.of(spec), Map.of(), Map.of()); assertEquals(1, result.toolSpecs().size()); assertEquals("test", result.toolSpecs().get(0).name()); diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java index 5c7dc624fc..1154b7dc10 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/AgentOrchestratorTest.java @@ -25,6 +25,7 @@ import ai.labs.eddi.modules.llm.tools.impl.*; import dev.langchain4j.agent.tool.ToolSpecification; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -740,7 +741,7 @@ void httpCallToolsResult_recordCreation() { List specs = List.of(spec); Map executors = Map.of(); - var result = new AgentOrchestrator.HttpCallToolsResult(specs, executors); + var result = new AgentOrchestrator.HttpCallToolsResult(specs, executors, Map.of()); assertNotNull(result); assertEquals(1, result.toolSpecs().size()); @@ -902,7 +903,7 @@ void safeTemplateMerge_emptyArgs_noChange() throws Exception { @Test void httpCallToolsResult_emptySpecs() { - var result = new AgentOrchestrator.HttpCallToolsResult(List.of(), Map.of()); + var result = new AgentOrchestrator.HttpCallToolsResult(List.of(), Map.of(), Map.of()); assertTrue(result.toolSpecs().isEmpty()); assertTrue(result.executors().isEmpty()); @@ -934,4 +935,56 @@ void collectEnabledTools_userMemoryPropsNull_addsToolWithEmptyGroupIds() { assertEquals(1, result.size()); assertTrue(result.get(0) instanceof UserMemoryTool); } + + // ========================================================================= + // Endpoint provenance for approval matching + // ========================================================================= + + @Test + void normalizeEndpointPath_acceptsEveryConfiguredShape() { + // The httpcall config accepts all of these for the same endpoint. Storing + // them verbatim would make "http.post:/agentstore/agents" miss most — and a + // require-pattern that misses is an ungated write. + assertEquals("/agentstore/agents", AgentOrchestrator.normalizeEndpointPath("/agentstore/agents")); + assertEquals("/agentstore/agents", AgentOrchestrator.normalizeEndpointPath("agentstore/agents")); + assertEquals("/agentstore/agents", AgentOrchestrator.normalizeEndpointPath(" /agentstore/agents ")); + assertEquals("/agentstore/agents", AgentOrchestrator.normalizeEndpointPath("https://eddi.example:7070/agentstore/agents")); + } + + @Test + @DisplayName("a value that merely begins \"http\" is not parsed as a URL") + void normalizeEndpointPath_doesNotMistakeANonUrlForAUrl() { + // The case that actually distinguishes the two implementations: a value + // beginning "http" that parses as an OPAQUE URI (scheme, then no slash). + // URI.getPath() is null for those, so the loose startsWith("http") test + // collapsed it to an empty string and the endpoint provenance vanished — + // taking the approval pattern's ability to match with it. + assertEquals("/httpfoo:bar", AgentOrchestrator.normalizeEndpointPath("httpfoo:bar")); + + // These agree under either implementation — a relative URI keeps its path — + // but they are the shapes a config is actually likely to hold. + assertEquals("/httpcalls/agents", AgentOrchestrator.normalizeEndpointPath("httpcalls/agents")); + assertEquals("/httpstuff", AgentOrchestrator.normalizeEndpointPath("httpstuff")); + + // A real absolute URL still contributes only its path, whatever its case. + assertEquals("/agentstore/agents", AgentOrchestrator.normalizeEndpointPath("HTTPS://eddi.example/agentstore/agents")); + + // A URL with no path is the root, not nothing: an empty key ("post:") can be + // matched by no pattern expecting a slash, and an unmatched require-pattern + // is an ungated call. + assertEquals("/", AgentOrchestrator.normalizeEndpointPath("https://eddi.example")); + assertEquals("/", AgentOrchestrator.normalizeEndpointPath("https://eddi.example?x=1")); + + // Request.path defaults to "" and means the target server's root, so it has + // to be gateable too — "" would make the key "post:" and match nothing. + assertEquals("/", AgentOrchestrator.normalizeEndpointPath("")); + assertEquals("/", AgentOrchestrator.normalizeEndpointPath(" ")); + } + + @Test + void normalizeEndpointPath_leavesAnUnparseableValueAlone() { + // A templated host is not a URI; matching something odd beats throwing + // during tool discovery. + assertEquals("http://{host}/x", AgentOrchestrator.normalizeEndpointPath("http://{host}/x")); + } } diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java index 1ff67f52ba..1b868886be 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerAdditionalTest.java @@ -5,6 +5,11 @@ package ai.labs.eddi.modules.llm.impl; import ai.labs.eddi.configs.variables.GlobalVariableResolver; +import dev.langchain4j.mcp.client.McpCallContext; +import dev.langchain4j.invocation.InvocationContext; +import ai.labs.eddi.engine.security.CallerIdentityResolver; +import ai.labs.eddi.engine.security.CallerIdentityContext; +import ai.labs.eddi.engine.security.CallerIdentity; import ai.labs.eddi.modules.llm.model.LlmConfiguration.McpServerConfig; import ai.labs.eddi.secrets.SecretResolver; import dev.langchain4j.mcp.client.McpClient; @@ -17,6 +22,7 @@ import java.util.concurrent.ConcurrentHashMap; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; import static org.mockito.MockitoAnnotations.openMocks; @@ -69,7 +75,7 @@ class CloseClientTests { @DisplayName("closing cached client — removes and closes it") void closeCachedClient() throws Exception { McpClient mockClient = mock(McpClient.class); - getClientCache().put("http://test-server:8080", mockClient); + getClientCache().put("http://test-server:8080|anonymous", mockClient); assertEquals(1, manager.getActiveConnectionCount()); manager.closeClient("http://test-server:8080"); @@ -83,13 +89,244 @@ void closeCachedClient() throws Exception { void closeClientWithException() throws Exception { McpClient mockClient = mock(McpClient.class); doThrow(new RuntimeException("close error")).when(mockClient).close(); - getClientCache().put("http://error-server:8080", mockClient); + getClientCache().put("http://error-server:8080|anonymous", mockClient); assertDoesNotThrow(() -> manager.closeClient("http://error-server:8080")); assertEquals(0, manager.getActiveConnectionCount()); } } + // ==================== credential isolation ==================== + + @Nested + @DisplayName("client cache keys on the credential, not just the URL") + class CredentialIsolationTests { + + private McpServerConfig serverWith(String apiKey) { + var config = new McpServerConfig(); + config.setUrl("http://shared-server:8080"); + config.setApiKey(apiKey); + return config; + } + + private String keyFor(McpServerConfig config) throws Exception { + var method = McpToolProviderManager.class.getDeclaredMethod("cacheKey", McpServerConfig.class); + method.setAccessible(true); + return (String) method.invoke(null, config); + } + + @Test + @DisplayName("two credentials against one URL do not share a client") + void differentCredentialsGetDifferentKeys() throws Exception { + // Keying on the URL alone meant the second agent silently borrowed the + // first's authorization — a privilege boundary, not a caching detail. + assertNotEquals(keyFor(serverWith("key-alpha")), keyFor(serverWith("key-beta"))); + } + + @Test + @DisplayName("the same credential shares one client, so users do not multiply connections") + void sameCredentialSharesAKey() throws Exception { + assertEquals(keyFor(serverWith("${caller:token}")), keyFor(serverWith("${caller:token}"))); + } + + @Test + @DisplayName("the configured credential never appears in the key") + void keyDoesNotLeakTheCredential() throws Exception { + String key = keyFor(serverWith("super-secret-literal-key")); + assertFalse(key.contains("super-secret-literal-key"), "a heap dump or log line must not reveal it: " + key); + assertTrue(key.startsWith("http://shared-server:8080|")); + } + + @Test + void aCredentiallessServerStillGetsAStableKey() throws Exception { + assertEquals(keyFor(serverWith(null)), keyFor(serverWith(null))); + assertTrue(keyFor(serverWith(null)).endsWith("|anonymous")); + } + + @Test + @DisplayName("the lookup actually uses that key, not the bare URL") + void getOrCreateClientLooksUpByCredentialAwareKey() throws Exception { + // Asserting cacheKey() alone proves nothing about the call site: with the + // lookup reverted to config.getUrl() the key tests still pass. Seeding the + // credential-aware key and requiring a cache HIT is what pins it — a + // URL-keyed lookup misses and tries to build a real client instead. + var config = serverWith("key-alpha"); + McpClient seeded = mock(McpClient.class); + getClientCache().put(keyFor(config), seeded); + + var method = McpToolProviderManager.class.getDeclaredMethod("getOrCreateClient", McpServerConfig.class); + method.setAccessible(true); + assertSame(seeded, method.invoke(manager, config), "must resolve the client cached under the credential-aware key"); + assertEquals(1, manager.getActiveConnectionCount(), "no second client should have been constructed"); + } + } + + // ==================== caller identity on the wire ==================== + + @Nested + @DisplayName("only a tool call carries the caller") + class CallerIdentityHeaderTests { + + private final CallerIdentityContext context = new CallerIdentityContext(null, null); + + private McpServerConfig callerBoundServer() { + var config = new McpServerConfig(); + config.setUrl("https://eddi.example:443/mcp"); + config.setApiKey("Bearer ${caller:token}"); + return config; + } + + /** A context shaped like a tools/call, i.e. carrying an invocation context. */ + private McpCallContext toolCall() { + return new McpCallContext(InvocationContext.builder().chatMemoryId(null).build(), null); + } + + /** A context shaped like tools/list, i.e. no invocation context. */ + private McpCallContext discovery() { + return new McpCallContext(null, null); + } + + private Map headersFor(McpServerConfig config, McpCallContext callContext) throws Exception { + var manager = new McpToolProviderManager(globalVariableResolver, secretResolver, + new CallerIdentityResolver(context, true), context, false, 1024, 300000L); + var method = McpToolProviderManager.class.getDeclaredMethod("authorizationHeader", String.class, McpServerConfig.class, + boolean.class, McpCallContext.class); + method.setAccessible(true); + @SuppressWarnings("unchecked") + var headers = (Map) method.invoke(manager, config.getApiKey(), config, true, callContext); + return headers; + } + + @AfterEach + void unbind() { + context.clear(); + } + + @Test + @DisplayName("a tool call sends the caller's token") + void toolCallCarriesTheCaller() throws Exception { + context.bind(new CallerIdentity("alice-token", "alice", "https://eddi.example:443")); + assertEquals("Bearer alice-token", headersFor(callerBoundServer(), toolCall()).get("Authorization")); + } + + @Test + @DisplayName("discovery does not, even on a thread that is bound") + void discoveryNeverCarriesTheCaller() throws Exception { + // The client is cached: a session established with alice's token would be + // reused by everyone after her, and a tool list reflecting her permissions + // would be offered to the next user. + context.bind(new CallerIdentity("alice-token", "alice", "https://eddi.example:443")); + + assertTrue(headersFor(callerBoundServer(), discovery()).isEmpty(), "tools/list must not be authenticated as alice"); + assertTrue(headersFor(callerBoundServer(), null).isEmpty(), "a transport-internal request has no caller either"); + } + + @Test + @DisplayName("a bare token gets the Bearer scheme, like the static path") + void callerTokenIsGivenTheBearerScheme() throws Exception { + // apiKey is documented as a key/token and the static path prefixes + // "Bearer ". A caller-bound key must match, or apiKey: "${caller:token}" + // would send a raw token with no scheme and the server would reject it. + context.bind(new CallerIdentity("alice-token", "alice", "https://eddi.example:443")); + var config = new McpServerConfig(); + config.setUrl("https://eddi.example:443/mcp"); + config.setApiKey("${caller:token}"); + + assertEquals("Bearer alice-token", headersFor(config, toolCall()).get("Authorization")); + } + + @Test + @DisplayName("an author who spells the scheme out is not double-prefixed") + void anExplicitSchemeIsLeftAlone() throws Exception { + context.bind(new CallerIdentity("alice-token", "alice", "https://eddi.example:443")); + assertEquals("Bearer alice-token", headersFor(callerBoundServer(), toolCall()).get("Authorization")); + } + + @Test + @DisplayName("an unbound tool call is refused rather than sent as a placeholder") + void unboundToolCallSendsNothing() throws Exception { + assertTrue(headersFor(callerBoundServer(), toolCall()).isEmpty(), "must not ship the literal ${caller:token}"); + } + + @Test + @DisplayName("a static key is unaffected and reaches every request") + void staticKeyStillWorks() throws Exception { + var config = new McpServerConfig(); + config.setUrl("https://eddi.example:443/mcp"); + config.setApiKey("static-key"); + + var manager = new McpToolProviderManager(globalVariableResolver, secretResolver, + new CallerIdentityResolver(context, true), context, false, 1024, 300000L); + var method = McpToolProviderManager.class.getDeclaredMethod("authorizationHeader", String.class, McpServerConfig.class, + boolean.class, McpCallContext.class); + method.setAccessible(true); + @SuppressWarnings("unchecked") + var onDiscovery = (Map) method.invoke(manager, "static-key", config, false, discovery()); + assertEquals("Bearer static-key", onDiscovery.get("Authorization"), "discovery keeps the service credential"); + } + } + + @Nested + @DisplayName("the circuit breaker isolates by credential") + class CircuitIsolationTests { + + private McpServerConfig serverWith(String apiKey) { + var config = new McpServerConfig(); + config.setUrl("http://shared-server:8080"); + config.setApiKey(apiKey); + return config; + } + + @Test + @DisplayName("one credential failing does not suppress discovery for another") + void oneCredentialsFailuresDoNotOpenTheCircuitForAnother() throws Exception { + // Keyed by bare URL, an agent whose key was revoked would take the other + // agent's working config down with it. + var failing = serverWith("revoked-key"); + var working = serverWith("valid-key"); + + var record = McpToolProviderManager.class.getDeclaredMethod("recordFailure", String.class); + record.setAccessible(true); + var key = McpToolProviderManager.class.getDeclaredMethod("cacheKey", McpServerConfig.class); + key.setAccessible(true); + for (int i = 0; i < 3; i++) { + record.invoke(manager, key.invoke(null, failing)); + } + + assertTrue(manager.isCircuitOpen(failing), "the failing credential trips its own circuit"); + assertFalse(manager.isCircuitOpen(working), "the other credential must still be discoverable"); + } + } + + @Nested + @DisplayName("discovery metrics") + class MetricsTests { + + @Test + @DisplayName("outcomes are counted without leaking a URL or a credential") + void discoveryOutcomesAreCountedSafely() throws Exception { + var registry = new io.micrometer.core.instrument.simple.SimpleMeterRegistry(); + // A spy with the network seam stubbed: pointing the real client at an + // unreachable host would make this test depend on DNS and wait out the + // 30-second default timeout. + var spied = spy(new McpToolProviderManager(globalVariableResolver, secretResolver)); + doThrow(new RuntimeException("connection refused")).when(spied).fetchToolsFromServer(any()); + spied.meterRegistry = registry; + + var config = new McpServerConfig(); + config.setUrl("http://unreachable-metrics-test:9999/mcp"); + config.setApiKey("super-secret-literal-key"); + spied.discoverTools(List.of(config)); + + var tagValues = registry.getMeters().stream().flatMap(m -> m.getId().getTags().stream()) + .map(io.micrometer.core.instrument.Tag::getValue).toList(); + assertFalse(tagValues.contains("super-secret-literal-key"), "a credential must never become a metric tag"); + assertFalse(tagValues.stream().anyMatch(v -> v.contains("unreachable-metrics-test")), + "a server URL is unbounded cardinality and must not be a tag: " + tagValues); + assertFalse(registry.getMeters().isEmpty(), "the outcome should have been counted"); + } + } + // ==================== shutdown ==================== @Nested diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java index 0a8aa0943c..dc3d95ee06 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerCircuitBreakerTest.java @@ -152,8 +152,7 @@ void connectionFailure_recordedForCircuitBreaker() throws Exception { } // After 3 failures, the circuit should be open - assertTrue(manager.isCircuitOpen("http://unreachable-xyz-circuit-test:9999/mcp"), - "Circuit should open after 3 consecutive failures"); + assertTrue(manager.isCircuitOpen(config), "Circuit should open after 3 consecutive failures"); } } diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerDiscoveryTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerDiscoveryTest.java index 6f16e68c37..2ed69f1812 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerDiscoveryTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerDiscoveryTest.java @@ -309,7 +309,7 @@ void connectionFailureIsClassifiedAndTripsTheCircuit() { assertFalse(result.hasConfigurationErrors(), "a flaky server is not a config error"); } - assertTrue(manager.isCircuitOpen(URL_A)); + assertTrue(manager.isCircuitOpen(cfg), "the circuit is keyed per credential, so ask by config"); var afterTrip = manager.discoverTools(List.of(cfg)); assertEquals(McpFailureKind.CIRCUIT_OPEN, afterTrip.failures().get(0).kind()); verify(manager, times(3)).fetchToolsFromServer(any());