From 9ca6946dad3f2c098c934cb391fdec03c64661e1 Mon Sep 17 00:00:00 2001 From: mohammed naji Date: Sun, 12 Jul 2026 23:43:56 +0400 Subject: [PATCH 1/3] feat(policy): classify risk from trusted tool annotations --- CHANGELOG.md | 2 + README.md | 2 +- docs/architecture.md | 2 +- docs/config.md | 13 +- docs/library-api.md | 4 +- docs/security.md | 3 + src/audit/audit-trail.ts | 4 + src/audit/audit-types.ts | 9 +- src/config/schema.ts | 9 +- src/config/types.ts | 5 + src/index.ts | 1 + src/mcp/server/miftah-server.ts | 40 ++++++- src/mcp/server/operation-pipeline.ts | 8 +- src/mcp/server/tool-registry.ts | 53 +++++++-- src/policy/policy-engine.ts | 24 ++-- src/policy/policy-types.ts | 24 +++- src/policy/risk-classifier.ts | 71 ++++++++++- tests/config-runtime-parity.test.ts | 23 ++++ tests/config-schema-contract.test.ts | 23 +++- tests/fixtures/fake-upstream.mjs | 9 +- tests/mcp-wrapper.test.ts | 108 ++++++++++++++++- tests/package-contract.test.ts | 7 +- tests/public-api.test.ts | 3 + .../risk-classification-docs-contract.test.ts | 34 ++++++ tests/routing-policy.test.ts | 112 +++++++++++++++++- tests/tool-registry.test.ts | 59 +++++++++ 26 files changed, 601 insertions(+), 51 deletions(-) create mode 100644 tests/risk-classification-docs-contract.test.ts create mode 100644 tests/tool-registry.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 725bd433..fdca105a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,12 @@ All notable changes to this project will be documented in this file. The format - [#21](https://github.com/mohanagy/miftah/issues/21) Delivered opt-in upstream identity fingerprint verification: strict expected/probe configuration, safe in-memory status and bounded caching, explicit MCP verification, required write/destructive gating, redacted audit evidence, and doctor readiness reporting. - [#22](https://github.com/mohanagy/miftah/issues/22) Delivered typed internal secret providers for environment, dotenv, opt-in plaintext, OS keychains, and 1Password; strict external-reference parsing, bounded no-shell execution and process-tree cleanup, automatic redaction registration, provider timeout configuration, and target-scoped doctor readiness diagnostics. - [#23](https://github.com/mohanagy/miftah/issues/23) Delivered opt-in active-profile persistence with explicit process, session, workspace, and config-identity-namespaced global scope; atomic restrictive state writes, safe restore diagnostics, lock precedence, and selection metadata in MCP current-profile output. +- [#26](https://github.com/mohanagy/miftah/issues/26) Policy risk classification now records source and confidence, accepts MCP annotations only from explicitly trusted configured upstreams, preserves local override precedence, fails closed on contradictory hints, and defaults unknown tools to destructive risk unless an operator selects the compatible write default. ### Changed - [#16](https://github.com/mohanagy/miftah/issues/16) The library root export is now an intentional, documented public API. Internal server, process, profile, routing, policy, audit, and secret-management classes are no longer available from `@lubab/miftah`; use the configuration utilities and `createMiftahRuntime()` instead. This pre-1.0 breaking change requires a minor release. +- [#26](https://github.com/mohanagy/miftah/issues/26) Unmatched tool names now default to destructive risk instead of write risk. Set `tooling.unknownToolRisk: "write"` only when the compatible, less restrictive default is intentional. ## [0.1.1] - 2026-07-11 diff --git a/README.md b/README.md index cd93901e..bdb63c3a 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ Routing can use the active profile or rules matching tool arguments: When several profiles match, Miftah refuses to guess. Use explicit profile switching for write and destructive actions. The same routing, policy, redaction, and audit pipeline applies to upstream tool calls, resource reads, and prompt retrieval. Policy patterns use each upstream tool's original name for tools, `resources/read` for reads, and `prompts/get` for prompt retrieval. A deny, confirmation-required, blocked, or ambiguous decision is returned before Miftah forwards the read or prompt request. Provider token scopes still matter: local policy cannot make a write-capable provider token read-only. Profiles that set a policy name must reference an existing entry in `policies`, while profiles with no `policy` field keep the default allow behavior. -Miftah can also match bounded workspace metadata without treating a project file as configuration. It resolves a valid environment hint, then the nearest valid project-marker hint, then matching rules over tool arguments and collected context, then the configured fallback. Rules that select different profiles return `ROUTING_AMBIGUOUS`, and a context hint never authorizes a destructive operation that requires an explicit rule. `miftah_route_preview` and eligible audit records expose only sanitized routing evidence, not raw project environment values or project file contents. See [routing context](docs/config.md#routing-context) for the marker schema, root behavior, and evidence boundary. +Miftah can also match bounded workspace metadata without treating a project file as configuration. It resolves a valid environment hint, then the nearest valid project-marker hint, then matching rules over tool arguments and collected context, then the configured fallback. Rules that select different profiles return `ROUTING_AMBIGUOUS`, and a context hint never authorizes a destructive operation that requires an explicit rule. `miftah_route_preview` and eligible audit records expose only sanitized routing evidence plus risk-classification source/confidence, not raw project environment values, upstream metadata, or project file contents. See [routing context](docs/config.md#routing-context) for the marker schema, root behavior, and evidence boundary. ## Identity verification diff --git a/docs/architecture.md b/docs/architecture.md index f173c9cc..86d6fdca 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -22,7 +22,7 @@ The server advertises management tools plus tools discovered from the active pro The metadata-only routing context collector is the sole source of workspace context, profile hints, preview evidence, and proxied-operation audit evidence. It reads only allowlisted bounded metadata: file roots, cwd, two environment selectors, strict project markers, package/workspace fields, and a local Git origin. The runtime configuration file remains separate from that input. Once initialized, `MiftahServer` capability-gates `roots/list`, caches only root URIs per client connection, and refreshes only on an advertised roots-list-changed notification. Every proxied operation and route preview receives one immutable snapshot; it uses that snapshot for rule matching, hint selection, and redacted evidence so routing cannot change between those steps. -Every supported MCP request enters one outer audit scope. The scope records one terminal operation event on success or safe failure and also covers discovery/list failures, unknown names, and management tools that do not enter the proxy pipeline. `OperationPipeline` enriches proxied tool calls, resource reads, and prompt retrieval with captured source/target profile, upstream, routing, policy, risk, and sanitized `routingEvidence`; it does not emit its own record. It captures the source profile state before awaiting work, resolves routing against that fixed active-profile fallback, evaluates the selected profile policy, resolves the exact target upstream route, executes, and redacts the result or error. Route preview uses the same captured fallback and collector contract. Tools retain their original upstream names for routing and policy compatibility; resource reads and prompt retrieval use the stable policy names `resources/read` and `prompts/get`. Denied, confirmation-required, blocked, and ambiguous operations never resolve or execute an upstream read/get route. Upstream managers publish typed lifecycle transitions, which the server records as separate audit events without letting audit I/O interrupt cleanup or recovery. +Every supported MCP request enters one outer audit scope. The scope records one terminal operation event on success or safe failure and also covers discovery/list failures, unknown names, and management tools that do not enter the proxy pipeline. `OperationPipeline` enriches proxied tool calls, resource reads, and prompt retrieval with captured source/target profile, upstream, routing, policy, risk, risk source/confidence, and sanitized `routingEvidence`; it does not emit its own record. It captures the source tool snapshot before awaiting policy work, normalizes only the four MCP behavioral booleans, and accepts them for classification only when the matching configured base upstream explicitly trusts tool annotations. The snapshot fingerprint includes client-visible annotations, so a routed profile with different metadata fails the same schema-compatibility guard as any other differing tool contract. It then captures the source profile state, resolves routing against that fixed active-profile fallback, evaluates the selected profile policy, resolves the exact target upstream route, executes, and redacts the result or error. Route preview remains side-effect-free: it examines only cached compatible snapshots and falls back conservatively when none exists. Tools retain their original upstream names for routing and policy compatibility; resource reads and prompt retrieval use the stable policy names `resources/read` and `prompts/get`. Denied, confirmation-required, blocked, and ambiguous operations never resolve or execute an upstream read/get route. Upstream managers publish typed lifecycle transitions, which the server records as separate audit events without letting audit I/O interrupt cleanup or recovery. `IdentityManager` is an opt-in, process-only fingerprint verifier keyed by the exact profile/upstream target and live upstream session generation. A named upstream identity replaces the profile identity for that target. It discovers the configured read-risk, no-required-input probe and calls it with `{}`; it never treats a probe as credential, authentication, authorization, or scope validation. Verified results are cached only in memory for the bounded `maxAgeMs`; concurrent verification for the same live target coalesces. A stale verified result is reported as `expired`, and a required protected request refreshes it once. Restart, crash, idle replacement, or wrapper replacement invalidates the session generation and requires re-verification. diff --git a/docs/config.md b/docs/config.md index a90fa44c..fd77b6ee 100644 --- a/docs/config.md +++ b/docs/config.md @@ -111,7 +111,7 @@ Selection order is environment hint, project-marker hint, configured rule, then MCP roots are optional client metadata. After initialization, Miftah calls `roots/list` only when the client advertises `roots` capability, stores a URI-only snapshot for that connection, and refreshes it only on an advertised `notifications/roots/list_changed`. An unsupported or failed roots request yields an empty-root snapshot; Miftah does not poll roots or request them for every operation. -`miftah_route_preview` resolves against one collector snapshot using the same context inputs as a proxied operation. Its response contains the selected profile, reason, policy decision, and sanitized `evidence`. Proxied operation audit records carry the same snapshot as additive `routingEvidence`, including an ambiguity that prevents forwarding. Evidence contains only allowlisted metadata and redacted URI components; it never contains arbitrary project file content or the raw `MIFTAH_PROJECT` value. +`miftah_route_preview` resolves against one collector snapshot using the same context inputs as a proxied operation. Its response contains the selected profile, reason, policy decision (including `riskSource` and `riskConfidence`), and sanitized `evidence`. It never starts an upstream to inspect a tool: it uses only an already-cached compatible tool snapshot and otherwise reports the conservative heuristic or unknown classification. Proxied operation audit records carry the same snapshot as additive `routingEvidence`, including an ambiguity that prevents forwarding. Evidence contains only allowlisted metadata and redacted URI components; it never contains arbitrary project file content or the raw `MIFTAH_PROJECT` value. ## Identity verification @@ -171,7 +171,16 @@ Miftah applies one safety pipeline to every proxied upstream tool call, resource Routing rules receive a tool's original arguments unchanged. Resource reads expose the requested URI as `args.uri`; prompt retrieval exposes the prompt arguments and always sets `args.name` to the requested prompt name. Policies evaluate upstream tools by their original tool name, resource reads as `resources/read`, and prompt retrieval as `prompts/get`. For example, `deny: ["resources/read"]` blocks all resource reads for the selected profile, while `requireConfirmation: ["prompts/get"]` returns `POLICY_CONFIRMATION_REQUIRED` without forwarding the request. -Policies classify these operation names as `read`, `write`, or `destructive` using configurable overrides and conservative name heuristics. `denyRisk` takes precedence over `allowRisk`; `requireConfirmation` returns a structured error instead of forwarding the operation. +Policies classify operations as `read`, `write`, or `destructive` in this order: + +1. an exact local `tooling.toolRiskOverrides` entry; +2. MCP tool annotations only when that exact base `upstream` or named `upstreams.` declaration sets `trustToolAnnotations: true`; +3. conservative name heuristics; then +4. `tooling.unknownToolRisk`, which defaults to `"destructive"` and may be set only to `"write"` or `"destructive"`. + +MCP annotations are hints, not authority. They cannot lower risk unless the operator explicitly trusts that configured upstream; profile-level upstream overrides cannot alter this trust boundary. A trusted `readOnlyHint: true` classifies as read, a trusted non-read-only `destructiveHint: false` classifies as write, and any contradictory `readOnlyHint`/`destructiveHint` combination classifies as destructive. `idempotentHint` and `openWorldHint` are retained as metadata but never lower mutation risk. Exact local overrides always win, including when an upstream annotation is incorrect. + +Every policy decision carries stable `riskSource` and `riskConfidence` values. Route previews and audit events expose only those enum values, never raw upstream annotations or tool output. `denyRisk` takes precedence over `allowRisk`; `requireConfirmation` returns a structured error instead of forwarding the operation. Audit logging writes local JSONL when a path is configured. Every supported MCP request emits one terminal operation event with a request ID, per-process session ID, source/selected profiles, stable outcome/error code, duration, and any available upstream, routing, policy, and risk metadata; route previews and proxied operations add sanitized `routingEvidence` when a collector snapshot is available. Wrapper and upstream lifecycle transitions emit separate event records. Arguments are excluded unless `includeArguments` is true, and all configured secret values are redacted before writing. Audit directories and files are created with owner-only permissions where the platform supports them. diff --git a/docs/library-api.md b/docs/library-api.md index d6e8f3ea..d786cbcf 100644 --- a/docs/library-api.md +++ b/docs/library-api.md @@ -26,10 +26,12 @@ await runtime.connect(new StdioServerTransport()); ## Type exports -The configuration contract exposes `ActiveProfileStateScope`, `AuditConfig`, `IdentityConfig`, `IdentityFingerprint`, `IdentityProbeConfig`, `MiftahConfig`, `PolicyConfig`, `ProcessConfig`, `ProfileConfig`, `ProfileUpstreamOverride`, `RiskLevel`, `RoutingConfig`, `RoutingRule`, `SecurityConfig`, `SecretsConfig`, `StateConfig`, `ToolDiscoveryMode`, `ToolingConfig`, `TransportType`, `UpstreamConfig`, and `ValidatedRoutingConfig`. +The configuration contract exposes `ActiveProfileStateScope`, `AuditConfig`, `IdentityConfig`, `IdentityFingerprint`, `IdentityProbeConfig`, `MiftahConfig`, `PolicyConfig`, `ProcessConfig`, `ProfileConfig`, `ProfileUpstreamOverride`, `RiskLevel`, `RoutingConfig`, `RoutingRule`, `SecurityConfig`, `SecretsConfig`, `StateConfig`, `ToolDiscoveryMode`, `ToolingConfig`, `TransportType`, `UnknownToolRisk`, `UpstreamConfig`, and `ValidatedRoutingConfig`. `StateConfig` makes active-profile persistence explicit. Its durable `workspace` and `global` scopes require `persistActiveProfile: true`; custom state-file paths are intentionally not part of the public API. +`UpstreamConfig.trustToolAnnotations` is opt-in and defaults to false. `ToolingConfig.unknownToolRisk` uses the exported `UnknownToolRisk` union (`"write" | "destructive"`) and defaults to `"destructive"`; callers can use exact `toolRiskOverrides` for known read tools. + For identity configurations, format-dependent structural constraints and unique `requiredForRisk` tuples are static. For text probes, `validateConfig` runtime-validates equality between `expected.provider` and a static `probe.provider`; JSON probes do not permit a static provider. Programmatic diagnostics expose `ConfigDiagnostic`, `MiftahErrorCode`, and `MiftahErrorDetails`. The wrapper factory exposes `MiftahRuntime`. diff --git a/docs/security.md b/docs/security.md index 58c46d64..64e10166 100644 --- a/docs/security.md +++ b/docs/security.md @@ -11,6 +11,7 @@ Miftah is a credential broker, so safe defaults are part of the product contract - audit records contain metadata, not sensitive payloads or arguments, by default; - audit files and directories are owner-only where platform support permits it, and audit-write failures are explicit; - durable active-profile state is opt-in, uses derived owner-restricted paths, and stores no credentials; +- MCP tool annotations are ignored for risk downgrades unless the operator explicitly trusts the configured upstream that supplied them; - provider tokens should be separate, least-privilege tokens per account and risk level. External secret providers execute only fixed programs with argument arrays and bounded stdout/stderr capture. Miftah does not expose provider output in an error, audit record, health entry, or doctor report. On Windows it resolves provider executables without current-directory lookup and uses a static System32 PowerShell launcher that joins a `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` Job Object before creating the provider. Timeout, cancellation, output-limit, or launcher termination closes that job and terminates ordinary provider descendants as well. This process-tree guarantee does not cover providers that intentionally escape through services, scheduled tasks, elevation brokers, or WMI process creation. @@ -21,6 +22,8 @@ Active-profile state is configuration-owned: MCP callers can select a profile bu The explicit runtime configuration is trusted operator input; workspace routing metadata is not. Project markers cannot inject configuration because their only accepted shape maps the configured wrapper name to an already-known profile. They cannot add credentials, environment variables, headers, upstreams, policies, audit controls, or secret references. Miftah reads only named, bounded metadata files within the applicable working-directory/root boundary and does not scan arbitrary project content. +MCP `tools/list` annotations are behavioral hints supplied by the upstream, not proof of safety. Miftah ignores them by default. An operator may set `trustToolAnnotations: true` only on the exact base upstream declaration they trust; a profile override cannot change that decision. Even for a trusted upstream, missing or contradictory hints never reduce risk, and `idempotentHint`/`openWorldHint` never lower it. Miftah records only the resulting classification source and confidence in route preview and audit data, never raw annotation objects. + Routing evidence is deliberately narrower than routing context. It is passed through audit redaction before it reaches a client or JSONL record, strips URI userinfo/fragments and redacts URI query values, and never contains the raw `MIFTAH_PROJECT` value or arbitrary project file content. An unrecognized environment profile or ambiguous matching rules fails closed instead of selecting an account; standard project-marker discovery deterministically uses the nearest valid marker. Profile hints also cannot satisfy an explicit-rule requirement for destructive operations. Miftah cannot reduce privileges granted by a provider token. A read-only Miftah policy is a local blocklist, not a replacement for provider-side scopes. Avoid putting real credentials in examples, commits, or support logs. diff --git a/src/audit/audit-trail.ts b/src/audit/audit-trail.ts index 59c24223..c0d1db76 100644 --- a/src/audit/audit-trail.ts +++ b/src/audit/audit-trail.ts @@ -20,6 +20,8 @@ export interface AuditScopeUpdate { policyName?: string; policyDecision?: AuditEvent["policyDecision"]; risk?: AuditEvent["risk"]; + riskSource?: AuditEvent["riskSource"]; + riskConfidence?: AuditEvent["riskConfidence"]; identity?: AuditEvent["identity"]; routingEvidence?: RoutingContextEvidence; } @@ -145,6 +147,8 @@ export class AuditScope { ...(this.event.policyName === undefined ? {} : { policyName: this.event.policyName }), ...(this.event.policyDecision === undefined ? {} : { policyDecision: this.event.policyDecision }), ...(this.event.risk === undefined ? {} : { risk: this.event.risk }), + ...(this.event.riskSource === undefined ? {} : { riskSource: this.event.riskSource }), + ...(this.event.riskConfidence === undefined ? {} : { riskConfidence: this.event.riskConfidence }), ...(this.event.identity === undefined ? {} : { identity: this.event.identity }), ...(this.event.routingEvidence === undefined ? {} : { routingEvidence: this.event.routingEvidence }), ...(this.event.arguments === undefined ? {} : { arguments: this.event.arguments }), diff --git a/src/audit/audit-types.ts b/src/audit/audit-types.ts index 3bc5892f..66c6504a 100644 --- a/src/audit/audit-types.ts +++ b/src/audit/audit-types.ts @@ -1,4 +1,9 @@ -import type { PolicyAction, RiskLevel } from "../policy/policy-types.js"; +import type { + PolicyAction, + RiskClassificationConfidence, + RiskClassificationSource, + RiskLevel +} from "../policy/policy-types.js"; import type { RoutingContextEvidence } from "../routing/routing-types.js"; import type { IdentityStatus } from "../identity/identity-types.js"; @@ -38,6 +43,8 @@ export interface AuditEvent { policyName?: string; policyDecision?: PolicyAction; risk?: RiskLevel; + riskSource?: RiskClassificationSource; + riskConfidence?: RiskClassificationConfidence; identity?: IdentityStatus | readonly IdentityStatus[]; routingEvidence?: RoutingContextEvidence; arguments?: unknown; diff --git a/src/config/schema.ts b/src/config/schema.ts index 4b519498..07030f1a 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -23,7 +23,8 @@ const upstreamBaseShape = { env: z.record(z.string(), z.string()).optional(), cwd: z.string().optional(), url: z.string().url().optional(), - headers: z.record(z.string(), z.string()).optional() + headers: z.record(z.string(), z.string()).optional(), + trustToolAnnotations: z.boolean().optional() }; const upstreamSchema = z.object(upstreamBaseShape).strict().superRefine((value, context) => { @@ -283,7 +284,8 @@ const publicToolingSchema = z .object({ collisionStrategy: z.enum(["prefix-upstream", "fail"]).optional(), toolDiscoveryMode: z.enum(["permissive", "strict"]).optional(), - toolRiskOverrides: z.record(z.string(), z.enum(["read", "write", "destructive"])).optional() + toolRiskOverrides: z.record(z.string(), z.enum(["read", "write", "destructive"])).optional(), + unknownToolRisk: z.enum(["write", "destructive"]).optional() }) .strict(); @@ -293,7 +295,8 @@ const toolingSchema = z upstreamToolNamespace: unsupportedOptionSchema, collisionStrategy: z.enum(["prefix-upstream", "fail"]).optional(), toolDiscoveryMode: z.enum(["permissive", "strict"]).optional(), - toolRiskOverrides: z.record(z.string(), z.enum(["read", "write", "destructive"])).optional() + toolRiskOverrides: z.record(z.string(), z.enum(["read", "write", "destructive"])).optional(), + unknownToolRisk: z.enum(["write", "destructive"]).optional() }) .strict(); diff --git a/src/config/types.ts b/src/config/types.ts index c769897b..a7a07d05 100644 --- a/src/config/types.ts +++ b/src/config/types.ts @@ -8,6 +8,8 @@ export interface UpstreamConfig { cwd?: string; url?: string; headers?: Record; + /** Explicitly permits this configured upstream's MCP tool annotations to influence risk classification. */ + trustToolAnnotations?: boolean; } /** Non-secret account attributes used to validate an upstream identity. */ @@ -112,6 +114,8 @@ export interface ValidatedRoutingConfig extends Omit { } export type RiskLevel = "read" | "write" | "destructive"; +/** Conservative defaults allowed for tools whose risk cannot be classified. */ +export type UnknownToolRisk = "write" | "destructive"; export type ToolDiscoveryMode = "permissive" | "strict"; export interface PolicyConfig { @@ -153,6 +157,7 @@ export interface ToolingConfig { collisionStrategy?: "prefix-upstream" | "fail"; toolDiscoveryMode?: ToolDiscoveryMode; toolRiskOverrides?: Record; + unknownToolRisk?: UnknownToolRisk; } /** Configures provider-backed secret resolution. */ diff --git a/src/index.ts b/src/index.ts index f7428d39..81624468 100644 --- a/src/index.ts +++ b/src/index.ts @@ -27,6 +27,7 @@ export type { ToolDiscoveryMode, ToolingConfig, TransportType, + UnknownToolRisk, UpstreamConfig, ValidatedRoutingConfig } from "./config/types.js"; diff --git a/src/mcp/server/miftah-server.ts b/src/mcp/server/miftah-server.ts index f9af5ecf..fa719d9c 100644 --- a/src/mcp/server/miftah-server.ts +++ b/src/mcp/server/miftah-server.ts @@ -30,6 +30,7 @@ import type { RoutingContextSnapshot } from "../../routing/routing-types.js"; import { PolicyEngine } from "../../policy/policy-engine.js"; +import type { ToolRiskMetadata } from "../../policy/risk-classifier.js"; import { IdentityManager } from "../../identity/identity-manager.js"; import type { IdentityStatus } from "../../identity/identity-types.js"; import { AuditLogger } from "../../audit/audit-logger.js"; @@ -54,6 +55,7 @@ import { canonicalJson, ToolRegistry, type DiscoveredTools, + type RegisteredTool, type ToolDiscoveryResult, type ToolSnapshot } from "./tool-registry.js"; @@ -210,7 +212,9 @@ export class MiftahServer { } ); this.routing = new RoutingEngine(config.routing, profiles.current().activeProfile, config.defaultProfile); - this.policy = new PolicyEngine(config.policies, config.tooling?.toolRiskOverrides ?? {}); + this.policy = new PolicyEngine(config.policies, config.tooling?.toolRiskOverrides ?? {}, { + unknownRisk: config.tooling?.unknownToolRisk + }); this.identities = new IdentityManager(config); this.toolRegistry = new ToolRegistry( (profile) => this.discoverTools(profile), @@ -528,6 +532,7 @@ export class MiftahServer { policyName: mapped.originalName, name: mapped.originalName, args, + riskMetadata: this.riskMetadata(mapped), requireExplicitRuleForDestructive: this.config.security?.requireExplicitProfileForDestructive, resolveTarget: async (profile) => { const target = (await this.toolRegistry.get(profile)).resolve(name); @@ -689,13 +694,27 @@ export class MiftahServer { profileHints: snapshot.profileHints }, source.activeProfile); const profile = this.profiles.get(route.profile); - const policy = this.policy.evaluate(profile.policy, toolName); + const sourceTool = this.toolRegistry.peek(source.activeProfile)?.resolve(toolName); + const targetTool = + sourceTool === undefined + ? undefined + : this.toolRegistry.peek(route.profile)?.resolve(toolName); + const hasCompatibleCachedTarget = + sourceTool !== undefined && targetTool !== undefined && sourceTool.fingerprint === targetTool.fingerprint; + const policyName = sourceTool?.originalName ?? toolName; + const policy = this.policy.evaluate( + profile.policy, + policyName, + hasCompatibleCachedTarget && sourceTool !== undefined ? this.riskMetadata(sourceTool) : undefined + ); audit.update({ profile: route.profile, routingReason: route.reason, policyName: profile.policy ?? "default", policyDecision: policy.action, risk: policy.risk, + riskSource: policy.riskSource, + riskConfidence: policy.riskConfidence, routingEvidence: evidence }); return textResult(JSON.stringify({ ...route, policy, evidence, identity: this.identityStatuses(route.profile) })); @@ -707,6 +726,23 @@ export class MiftahServer { return resolveClientVisibleToolName(name, upstreamName, this.config.tooling?.collisionStrategy); } + private riskMetadata(tool: RegisteredTool): ToolRiskMetadata { + return { + trusted: this.trustsToolAnnotations(tool.upstreamName), + ...(tool.annotations === undefined ? {} : { annotations: tool.annotations }) + }; + } + + private trustsToolAnnotations(upstreamName: string | undefined): boolean { + if (this.config.upstream !== undefined) return this.config.upstream.trustToolAnnotations === true; + return ( + upstreamName !== undefined && + this.config.upstreams !== undefined && + Object.hasOwn(this.config.upstreams, upstreamName) && + this.config.upstreams[upstreamName]!.trustToolAnnotations === true + ); + } + private upstreamNames(): (string | undefined)[] { if (this.upstreams instanceof MultiUpstreamProcessManager) return this.upstreams.listUpstreams(); return [undefined]; diff --git a/src/mcp/server/operation-pipeline.ts b/src/mcp/server/operation-pipeline.ts index 18227c33..df9c362d 100644 --- a/src/mcp/server/operation-pipeline.ts +++ b/src/mcp/server/operation-pipeline.ts @@ -2,6 +2,7 @@ import type { AuditScope } from "../../audit/audit-trail.js"; import { IdentityManager } from "../../identity/identity-manager.js"; import { PolicyEngine } from "../../policy/policy-engine.js"; import type { PolicyDecision } from "../../policy/policy-types.js"; +import type { ToolRiskMetadata } from "../../policy/risk-classifier.js"; import { ProfileManager } from "../../profiles/profile-manager.js"; import { RoutingEngine } from "../../routing/routing-engine.js"; import type { RoutingContextSnapshot, RoutingDecision } from "../../routing/routing-types.js"; @@ -33,6 +34,7 @@ export interface ProxiedOperation { readonly policyName: string; readonly name: string; readonly args: Record; + readonly riskMetadata?: ToolRiskMetadata; readonly requireExplicitRuleForDestructive?: boolean; resolveTarget(profile: string): Promise>; } @@ -70,14 +72,16 @@ export class OperationPipeline { ); const profile = route.profile; const profileConfig = this.options.profiles.get(profile); - const decision = this.options.policy.evaluate(profileConfig.policy, operation.policyName); + const decision = this.options.policy.evaluate(profileConfig.policy, operation.policyName, operation.riskMetadata); audit.update({ profile, routingReason: route.reason, routingSource: routingSource(route), policyName: profileConfig.policy ?? "default", policyDecision: decision.action, - risk: decision.risk + risk: decision.risk, + riskSource: decision.riskSource, + riskConfidence: decision.riskConfidence }); this.assertPolicyAllows(operation, route, decision, profile); diff --git a/src/mcp/server/tool-registry.ts b/src/mcp/server/tool-registry.ts index 2f10d8f6..240d40a2 100644 --- a/src/mcp/server/tool-registry.ts +++ b/src/mcp/server/tool-registry.ts @@ -1,4 +1,5 @@ import type { Tool } from "@modelcontextprotocol/sdk/types.js"; +import type { ToolRiskAnnotations } from "../../policy/policy-types.js"; import { MiftahError } from "../../utils/errors.js"; export interface DiscoveredTools { @@ -12,11 +13,12 @@ export interface ToolDiscoveryResult { } export interface RegisteredTool { - exposedName: string; - originalName: string; - upstreamName?: string; - profile: string; - fingerprint: string; + readonly exposedName: string; + readonly originalName: string; + readonly upstreamName?: string; + readonly profile: string; + readonly fingerprint: string; + readonly annotations?: ToolRiskAnnotations; } export interface ToolSnapshot { @@ -120,7 +122,8 @@ export class ToolRegistry { originalName: tool.name, upstreamName, profile, - fingerprint: canonicalJson(exposedTool) + fingerprint: canonicalJson(exposedTool), + annotations: normalizeRiskAnnotations(tool) }); tools.push(exposedTool); } @@ -132,7 +135,7 @@ export class ToolRegistry { profile, fingerprint: canonicalJson(snapshotTools), getTools: () => snapshotTools.map(cloneTool), - resolve: (exposedName) => routes.get(exposedName), + resolve: (exposedName) => cloneRegisteredTool(routes.get(exposedName)), isComplete: () => !discovery.incomplete }; } @@ -150,6 +153,42 @@ function cloneTool(tool: Tool): Tool { return structuredClone(tool); } +function cloneRegisteredTool(tool: RegisteredTool | undefined): RegisteredTool | undefined { + return tool === undefined + ? undefined + : { + ...tool, + ...(tool.annotations === undefined ? {} : { annotations: { ...tool.annotations } }) + }; +} + +function normalizeRiskAnnotations(tool: Tool): ToolRiskAnnotations | undefined { + const annotations = tool.annotations; + if (annotations === undefined) return undefined; + const readOnlyHint = booleanHint(annotations.readOnlyHint); + const destructiveHint = booleanHint(annotations.destructiveHint); + const idempotentHint = booleanHint(annotations.idempotentHint); + const openWorldHint = booleanHint(annotations.openWorldHint); + if ( + readOnlyHint === undefined && + destructiveHint === undefined && + idempotentHint === undefined && + openWorldHint === undefined + ) { + return undefined; + } + return { + ...(readOnlyHint === undefined ? {} : { readOnlyHint }), + ...(destructiveHint === undefined ? {} : { destructiveHint }), + ...(idempotentHint === undefined ? {} : { idempotentHint }), + ...(openWorldHint === undefined ? {} : { openWorldHint }) + }; +} + +function booleanHint(value: unknown): boolean | undefined { + return typeof value === "boolean" ? value : undefined; +} + export function canonicalJson(value: unknown): string { if (value === null || typeof value !== "object") return JSON.stringify(value); if (Array.isArray(value)) return `[${value.map(canonicalJson).join(",")}]`; diff --git a/src/policy/policy-engine.ts b/src/policy/policy-engine.ts index 5a8c4874..19db49b5 100644 --- a/src/policy/policy-engine.ts +++ b/src/policy/policy-engine.ts @@ -1,6 +1,6 @@ import type { PolicyConfig, RiskLevel } from "../config/types.js"; import type { PolicyDecision } from "./policy-types.js"; -import { classifyRisk } from "./risk-classifier.js"; +import { classifyToolRisk, type RiskClassifierOptions, type ToolRiskMetadata } from "./risk-classifier.js"; /** Matches a tool name against an anchored glob pattern where `*` spans any characters. */ function matchesPattern(value: string, pattern: string): boolean { @@ -18,25 +18,29 @@ export class PolicyEngine { /** Creates an engine from policy definitions and optional per-tool risk overrides. */ constructor( private readonly policies: Record = {}, - private readonly riskOverrides: Record = {} + private readonly riskOverrides: Record = {}, + private readonly riskOptions: Omit = {} ) {} /** Returns whether a tool call is allowed, denied, or requires confirmation. */ - evaluate(policyName: string | undefined, toolName: string): PolicyDecision { - const risk = classifyRisk(toolName, this.riskOverrides); - if (policyName !== undefined && !Object.hasOwn(this.policies, policyName)) return { action: "deny", risk }; + evaluate(policyName: string | undefined, toolName: string, metadata?: ToolRiskMetadata): PolicyDecision { + const classification = classifyToolRisk(toolName, { ...this.riskOptions, overrides: this.riskOverrides }, metadata); + const { risk } = classification; + if (policyName !== undefined && !Object.hasOwn(this.policies, policyName)) { + return { action: "deny", ...classification }; + } const policy = policyName !== undefined ? this.policies[policyName] : undefined; - if (!policy) return { action: "allow", risk }; + if (!policy) return { action: "allow", ...classification }; if (policy.deny?.some((pattern) => matchesPattern(toolName, pattern))) { - return { action: "deny", risk }; + return { action: "deny", ...classification }; } const allowed = policy.allowRisk ?? policy.allow; if (policy.denyRisk?.includes(risk) || (allowed && !allowed.includes(risk))) { - return { action: "deny", risk }; + return { action: "deny", ...classification }; } if (policy.requireConfirmation?.some((pattern) => matchesPattern(toolName, pattern) || pattern === risk)) { - return { action: "confirm", risk }; + return { action: "confirm", ...classification }; } - return { action: "allow", risk }; + return { action: "allow", ...classification }; } } diff --git a/src/policy/policy-types.ts b/src/policy/policy-types.ts index abdc5d58..b1dbec21 100644 --- a/src/policy/policy-types.ts +++ b/src/policy/policy-types.ts @@ -1,10 +1,30 @@ import type { PolicyConfig, RiskLevel } from "../config/types.js"; export type PolicyAction = "allow" | "deny" | "confirm"; +export type RiskClassificationSource = + | "local-override" + | "trusted-upstream-annotation" + | "annotation-conflict" + | "name-heuristic" + | "unknown-default"; +export type RiskClassificationConfidence = "high" | "medium" | "low"; -export interface PolicyDecision { - action: PolicyAction; +/** The four MCP behavioral hints that can inform risk only after explicit upstream trust. */ +export interface ToolRiskAnnotations { + readonly readOnlyHint?: boolean; + readonly destructiveHint?: boolean; + readonly idempotentHint?: boolean; + readonly openWorldHint?: boolean; +} + +export interface RiskClassification { risk: RiskLevel; + riskSource: RiskClassificationSource; + riskConfidence: RiskClassificationConfidence; +} + +export interface PolicyDecision extends RiskClassification { + action: PolicyAction; } export type { PolicyConfig, RiskLevel }; diff --git a/src/policy/risk-classifier.ts b/src/policy/risk-classifier.ts index 688a2261..7438ca90 100644 --- a/src/policy/risk-classifier.ts +++ b/src/policy/risk-classifier.ts @@ -1,13 +1,72 @@ -import type { RiskLevel } from "../config/types.js"; +import type { RiskLevel, UnknownToolRisk } from "../config/types.js"; +import type { RiskClassification, ToolRiskAnnotations } from "./policy-types.js"; const destructivePattern = /(delete|remove|destroy|revoke|archive|close|merge)/i; const writePattern = /(create|update|edit|post|comment|send|resolve|assign|move|set)/i; const readPattern = /(get|list|search|read|fetch|query|find|whoami|status|health)/i; +export interface ToolRiskMetadata { + readonly trusted?: boolean; + readonly annotations?: ToolRiskAnnotations; +} + +export interface RiskClassifierOptions { + readonly overrides?: Record; + readonly unknownRisk?: UnknownToolRisk; +} + +/** Classifies one operation and preserves the evidence used for the safety decision. */ +export function classifyToolRisk( + toolName: string, + options: RiskClassifierOptions = {}, + metadata: ToolRiskMetadata = {} +): RiskClassification { + const override = options.overrides; + if (override !== undefined && Object.hasOwn(override, toolName)) { + return { risk: override[toolName]!, riskSource: "local-override", riskConfidence: "high" }; + } + + if (metadata.trusted && metadata.annotations !== undefined) { + const annotationRisk = classifyTrustedAnnotations(metadata.annotations); + if (annotationRisk !== undefined) return annotationRisk; + } + + if (destructivePattern.test(toolName)) return heuristic("destructive"); + if (writePattern.test(toolName)) return heuristic("write"); + if (readPattern.test(toolName)) return heuristic("read"); + return { + risk: options.unknownRisk ?? "destructive", + riskSource: "unknown-default", + riskConfidence: "low" + }; +} + +/** Backwards-compatible risk-only classifier for callers that do not need provenance. */ export function classifyRisk(toolName: string, overrides: Record = {}): RiskLevel { - if (overrides[toolName]) return overrides[toolName]; - if (destructivePattern.test(toolName)) return "destructive"; - if (writePattern.test(toolName)) return "write"; - if (readPattern.test(toolName)) return "read"; - return "write"; + return classifyToolRisk(toolName, { overrides }).risk; +} + +function classifyTrustedAnnotations(annotations: ToolRiskAnnotations): RiskClassification | undefined { + const readOnly = booleanHint(annotations.readOnlyHint); + const destructive = booleanHint(annotations.destructiveHint); + if (readOnly === true && destructive === true) { + return { risk: "destructive", riskSource: "annotation-conflict", riskConfidence: "low" }; + } + if (readOnly === true) return trusted("read"); + if (destructive === true) return trusted("destructive"); + if (readOnly === false && destructive === false) return trusted("write"); + if (readOnly === false) return trusted("destructive"); + return undefined; +} + +function booleanHint(value: unknown): boolean | undefined { + return typeof value === "boolean" ? value : undefined; +} + +function trusted(risk: RiskLevel): RiskClassification { + return { risk, riskSource: "trusted-upstream-annotation", riskConfidence: "medium" }; +} + +function heuristic(risk: RiskLevel): RiskClassification { + return { risk, riskSource: "name-heuristic", riskConfidence: "low" }; } diff --git a/tests/config-runtime-parity.test.ts b/tests/config-runtime-parity.test.ts index 7b381a99..5d01a876 100644 --- a/tests/config-runtime-parity.test.ts +++ b/tests/config-runtime-parity.test.ts @@ -87,6 +87,29 @@ describe("config runtime parity", () => { expect(malformedPersistence.message).toContain("state.persistActiveProfile"); }); + it("requires explicit annotation trust and validates the unknown-tool risk default", () => { + const config = validateConfig( + baseConfig({ + upstream: { transport: "stdio", command: "node", trustToolAnnotations: true }, + tooling: { unknownToolRisk: "destructive" } + }) + ); + + expect(config.upstream?.trustToolAnnotations).toBe(true); + expect(config.tooling?.unknownToolRisk).toBe("destructive"); + expect(() => validateConfig(baseConfig({ upstream: { transport: "stdio", command: "node", trustToolAnnotations: "true" } }))).toThrow( + /upstream\.trustToolAnnotations/u + ); + expect(() => validateConfig(baseConfig({ tooling: { unknownToolRisk: "read" } }))).toThrow( + /tooling\.unknownToolRisk/u + ); + expect(() => validateConfig(baseConfig({ + upstreams: { primary: { transport: "stdio", command: "node" } }, + upstream: undefined, + profiles: { default: { upstreams: { primary: { trustToolAnnotations: true } } } } + }))).toThrow(/profiles\.default\.upstreams\.primary\.trustToolAnnotations/u); + }); + it.each([ ["transport", "stdio"], ["transport", true], diff --git a/tests/config-schema-contract.test.ts b/tests/config-schema-contract.test.ts index 425239f3..1886f940 100644 --- a/tests/config-schema-contract.test.ts +++ b/tests/config-schema-contract.test.ts @@ -45,6 +45,22 @@ const validStateConfig: MiftahConfig = { }; void validStateConfig; +const validRiskClassificationConfig: MiftahConfig = { + ...publicConfig, + upstream: { transport: "stdio", command: "node", trustToolAnnotations: true }, + tooling: { unknownToolRisk: "destructive" } +}; +void validRiskClassificationConfig; + +const invalidRiskClassificationConfig: MiftahConfig = { + ...publicConfig, + tooling: { + // @ts-expect-error Unknown tool risk must be a supported risk level. + unknownToolRisk: "unsafe" + } +}; +void invalidRiskClassificationConfig; + const invalidInMemoryStateConfig: MiftahConfig = { ...publicConfig, // @ts-expect-error In-memory scopes cannot opt in to durable persistence. @@ -104,6 +120,7 @@ describe("published config schema", () => { it("advertises only the runtime-supported configuration surface", () => { const schema = generateConfigSchema() as unknown as ConfigSchema; const root = schema.properties; + const upstream = root?.upstream?.properties; const routing = root?.routing?.properties; const profile = mapValue(root?.profiles, "profiles"); const profileUpstreamOverride = mapValue(profile.properties?.upstreams, "profile upstreams"); @@ -120,6 +137,7 @@ describe("published config schema", () => { additionalProperties: false }); expect(root).not.toHaveProperty("ui"); + expect(upstream).toMatchObject({ trustToolAnnotations: { type: "boolean" } }); expect(routing).toMatchObject({ mode: { const: "hybrid" } }); expect(routing).not.toHaveProperty("plugins"); expect(profile.properties).toHaveProperty("headers"); @@ -150,7 +168,10 @@ describe("published config schema", () => { }); expect(tooling).not.toHaveProperty("managementToolPrefix"); expect(tooling).not.toHaveProperty("upstreamToolNamespace"); - expect(tooling).toMatchObject({ toolDiscoveryMode: { enum: ["permissive", "strict"] } }); + expect(tooling).toMatchObject({ + toolDiscoveryMode: { enum: ["permissive", "strict"] }, + unknownToolRisk: { enum: ["write", "destructive"] } + }); expect(secrets).toMatchObject({ providerTimeoutMs: { minimum: 100, maximum: 120_000 } }); diff --git a/tests/fixtures/fake-upstream.mjs b/tests/fixtures/fake-upstream.mjs index 3befac04..eee28982 100644 --- a/tests/fixtures/fake-upstream.mjs +++ b/tests/fixtures/fake-upstream.mjs @@ -139,7 +139,11 @@ const whoamiInputSchema = } : process.env.TEST_WHOAMI_SCHEMA === "malformed-required" ? { type: "object", properties: {}, required: "account" } - : { type: "object", properties: {} }; + : { type: "object", properties: {} }; +const createItemAnnotations = + process.env.TEST_CREATE_ITEM_ANNOTATIONS === undefined + ? undefined + : JSON.parse(process.env.TEST_CREATE_ITEM_ANNOTATIONS); const server = new Server( { name: "fake-upstream", version: "1.0.0" }, { capabilities: { tools: {}, resources: {}, prompts: {} } } @@ -233,7 +237,8 @@ server.setRequestHandler(ListToolsRequestSchema, async (request) => { type: "object", properties: { name: { type: "string" } }, required: ["name"] - } + }, + ...(createItemAnnotations === undefined ? {} : { annotations: createItemAnnotations }) } ]), ...(includeIdentityTool && !secondPage diff --git a/tests/mcp-wrapper.test.ts b/tests/mcp-wrapper.test.ts index ce93ffc4..1f54da76 100644 --- a/tests/mcp-wrapper.test.ts +++ b/tests/mcp-wrapper.test.ts @@ -173,9 +173,114 @@ interface ProfileManagementHost { } describe("Miftah MCP wrapper", () => { + it("uses explicitly trusted tool annotations and records risk provenance", async () => { + const directory = await mkdtemp(join(tmpdir(), "miftah-risk-annotations-")); + const auditPath = join(directory, "audit.jsonl"); + const config = validateConfig({ + version: "1", + name: "accounts", + defaultProfile: "work", + upstream: { transport: "stdio", command: process.execPath, args: [fixture], trustToolAnnotations: true }, + profiles: { + work: { + policy: "readonly", + env: { + TEST_CREATE_ITEM_ANNOTATIONS: JSON.stringify({ + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: true + }) + } + } + }, + policies: { readonly: { allowRisk: ["read"] } }, + audit: { path: auditPath } + }); + const upstreams = new UpstreamProcessManager(config.upstream!, config.profiles, { startupTimeoutMs: 5_000 }); + const wrapper = new MiftahServer(config, new ProfileManager(config), upstreams); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "risk-annotation-client", version: "1.0.0" }); + + try { + await Promise.all([wrapper.connect(serverTransport), client.connect(clientTransport)]); + await client.listTools(); + + expect(parseJsonToolResult(await client.callTool({ name: "miftah_route_preview", arguments: { toolName: "create_item" } }))).toMatchObject({ + policy: { action: "allow", risk: "read", riskSource: "trusted-upstream-annotation", riskConfidence: "medium" } + }); + expect(await client.callTool({ name: "create_item", arguments: { name: "x" } })).not.toMatchObject({ isError: true }); + + const events = (await readFile(auditPath, "utf8")) + .trim() + .split("\n") + .map((line) => JSON.parse(line) as Record); + expect(events.find((event) => event.operation === "tools/call" && event.name === "create_item")).toMatchObject({ + risk: "read", + riskSource: "trusted-upstream-annotation", + riskConfidence: "medium" + }); + } finally { + await client.close(); + await wrapper.close(); + await rm(directory, { recursive: true, force: true }); + } + }); + + it("scopes annotation trust to each named base upstream", async () => { + const config = validateConfig({ + version: "1", + name: "accounts", + defaultProfile: "work", + upstreams: { + trusted: { transport: "stdio", command: process.execPath, args: [fixture], trustToolAnnotations: true }, + untrusted: { transport: "stdio", command: process.execPath, args: [fixture] } + }, + profiles: { + work: { + policy: "readonly", + upstreams: { + trusted: { + env: { TEST_CREATE_ITEM_ANNOTATIONS: JSON.stringify({ readOnlyHint: true, destructiveHint: false }) } + }, + untrusted: { + env: { TEST_CREATE_ITEM_ANNOTATIONS: JSON.stringify({ readOnlyHint: true, destructiveHint: false }) } + } + } + } + }, + policies: { readonly: { allowRisk: ["read"] } } + }); + const upstreams = new MultiUpstreamProcessManager(config, { startupTimeoutMs: 5_000 }); + const wrapper = new MiftahServer(config, new ProfileManager(config), upstreams); + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "named-risk-annotation-client", version: "1.0.0" }); + + try { + await Promise.all([wrapper.connect(serverTransport), client.connect(clientTransport)]); + await client.listTools(); + + expect(parseJsonToolResult(await client.callTool({ name: "miftah_route_preview", arguments: { toolName: "trusted__create_item" } }))).toMatchObject({ + policy: { action: "allow", risk: "read", riskSource: "trusted-upstream-annotation", riskConfidence: "medium" } + }); + expect(await client.callTool({ name: "trusted__create_item", arguments: { name: "x" } })).not.toMatchObject({ isError: true }); + expect(parseJsonToolResult(await client.callTool({ name: "miftah_route_preview", arguments: { toolName: "untrusted__create_item" } }))).toMatchObject({ + policy: { action: "deny", risk: "write", riskSource: "name-heuristic", riskConfidence: "low" } + }); + expect(await client.callTool({ name: "untrusted__create_item", arguments: { name: "x" } })).toMatchObject({ + isError: true, + content: [{ type: "text", text: expect.stringContaining("POLICY_BLOCKED") }] + }); + } finally { + await client.close(); + await wrapper.close(); + } + }); + it("reports configured identity status without probing upstreams from management surfaces", async () => { const directory = await mkdtemp(join(tmpdir(), "miftah-identity-management-")); const callCountPath = join(directory, "tool-call-count"); + const listCountPath = join(directory, "tool-list-count"); const config = validateConfig({ version: "1", name: "accounts", @@ -183,7 +288,7 @@ describe("Miftah MCP wrapper", () => { upstream: { transport: "stdio", command: process.execPath, args: [fixture] }, profiles: { work: { - env: { TEST_CALL_TOOL_COUNT_PATH: callCountPath }, + env: { TEST_CALL_TOOL_COUNT_PATH: callCountPath, TEST_LIST_TOOLS_COUNT_PATH: listCountPath }, identity: { expected: { provider: "github", login: "work" }, probe: { tool: "whoami", resultFormat: "text", provider: "github" }, @@ -217,6 +322,7 @@ describe("Miftah MCP wrapper", () => { expect(health.identity).toEqual([expectedStatus]); expect(preview.identity).toEqual([expectedStatus]); await expect(access(callCountPath)).rejects.toThrow(); + await expect(access(listCountPath)).rejects.toThrow(); } finally { await client.close(); await wrapper.close(); diff --git a/tests/package-contract.test.ts b/tests/package-contract.test.ts index 0084c25c..f8bea230 100644 --- a/tests/package-contract.test.ts +++ b/tests/package-contract.test.ts @@ -394,13 +394,13 @@ describe("packed artifact contract", () => { await writeFile( typeConsumerPath, [ - 'import { createMiftahRuntime, MIFTAH_VERSION, type ActiveProfileStateScope, type AuditConfig, type ConfigDiagnostic, type IdentityConfig, type IdentityFingerprint, type IdentityProbeConfig, type MiftahConfig, type MiftahErrorCode, type MiftahErrorDetails, type MiftahRuntime, type PolicyConfig, type ProcessConfig, type ProfileConfig, type ProfileUpstreamOverride, type RiskLevel, type RoutingConfig, type RoutingRule, type SecurityConfig, type StateConfig, type ToolDiscoveryMode, type ToolingConfig, type TransportType, type UpstreamConfig, type ValidatedRoutingConfig } from "@lubab/miftah";', + 'import { createMiftahRuntime, MIFTAH_VERSION, type ActiveProfileStateScope, type AuditConfig, type ConfigDiagnostic, type IdentityConfig, type IdentityFingerprint, type IdentityProbeConfig, type MiftahConfig, type MiftahErrorCode, type MiftahErrorDetails, type MiftahRuntime, type PolicyConfig, type ProcessConfig, type ProfileConfig, type ProfileUpstreamOverride, type RiskLevel, type RoutingConfig, type RoutingRule, type SecurityConfig, type StateConfig, type ToolDiscoveryMode, type ToolingConfig, type TransportType, type UnknownToolRisk, type UpstreamConfig, type ValidatedRoutingConfig } from "@lubab/miftah";', "", "type SupportedTypes = [", " ActiveProfileStateScope, AuditConfig, ConfigDiagnostic, IdentityConfig, IdentityFingerprint, IdentityProbeConfig, MiftahConfig,", " MiftahErrorCode, MiftahErrorDetails, MiftahRuntime,", " PolicyConfig, ProcessConfig, ProfileConfig, ProfileUpstreamOverride, RiskLevel, RoutingConfig,", - " RoutingRule, SecurityConfig, StateConfig, ToolDiscoveryMode, ToolingConfig, TransportType, UpstreamConfig,", + " RoutingRule, SecurityConfig, StateConfig, ToolDiscoveryMode, ToolingConfig, TransportType, UnknownToolRisk, UpstreamConfig,", " ValidatedRoutingConfig", "];", "declare const types: SupportedTypes;", @@ -409,6 +409,7 @@ describe("packed artifact contract", () => { 'const globalScope: ActiveProfileStateScope = "global";', 'const validState: StateConfig = { persistActiveProfile: true, scope: "workspace" };', 'const validSessionState: StateConfig = { scope: "session" };', + 'const unknownRisk: UnknownToolRisk = "destructive";', "// @ts-expect-error Durable profile state requires explicit opt-in.", 'const invalidState: StateConfig = { scope: "global" };', 'const validTextIdentity: IdentityConfig = { expected: { provider: "github", login: "mona" }, probe: { tool: "whoami", resultFormat: "text", provider: "github" }, maxAgeMs: 60_000, requiredForRisk: ["write"] };', @@ -460,7 +461,7 @@ describe("packed artifact contract", () => { ' tool: "identity", resultFormat: "json",', ' provider: "github"', "};", - "void [types, version, runtime, globalScope, validState, validSessionState, invalidState, validTextIdentity, mismatchedTextProviderIdentity, validDestructiveIdentity, validWriteThenDestructiveIdentity, validDestructiveThenWriteIdentity, invalidDuplicateRiskIdentity, validJsonIdentity, invalidTextIdentity, invalidTextOrganization, invalidTextProviderWithoutProbeProvider, invalidJsonStaticProvider, invalidJsonEmptyExpected, invalidJsonProbe];" + "void [types, version, runtime, globalScope, validState, validSessionState, unknownRisk, invalidState, validTextIdentity, mismatchedTextProviderIdentity, validDestructiveIdentity, validWriteThenDestructiveIdentity, validDestructiveThenWriteIdentity, invalidDuplicateRiskIdentity, invalidTextIdentity, invalidTextOrganization, invalidTextProviderWithoutProbeProvider, invalidJsonStaticProvider, invalidJsonEmptyExpected, invalidJsonProbe];" ].join("\n") ); const typecheck = spawnSync( diff --git a/tests/public-api.test.ts b/tests/public-api.test.ts index 2a80f73f..090b9c42 100644 --- a/tests/public-api.test.ts +++ b/tests/public-api.test.ts @@ -30,6 +30,7 @@ import type { ToolDiscoveryMode, ToolingConfig, TransportType, + UnknownToolRisk, UpstreamConfig, ValidatedRoutingConfig } from "../src/index.js"; @@ -82,6 +83,7 @@ const supportedTypeExports = [ "ToolDiscoveryMode", "ToolingConfig", "TransportType", + "UnknownToolRisk", "UpstreamConfig", "ValidatedRoutingConfig" ] as const; @@ -109,6 +111,7 @@ type PublicTypeImportCoverage = [ ToolDiscoveryMode, ToolingConfig, TransportType, + UnknownToolRisk, UpstreamConfig, ValidatedRoutingConfig ]; diff --git a/tests/risk-classification-docs-contract.test.ts b/tests/risk-classification-docs-contract.test.ts new file mode 100644 index 00000000..f76cbae2 --- /dev/null +++ b/tests/risk-classification-docs-contract.test.ts @@ -0,0 +1,34 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; + +function readRepositoryFile(path: string): string { + return readFileSync(new URL(`../${path}`, import.meta.url), "utf8"); +} + +describe("risk classification documentation contract", () => { + it("documents the explicit trust boundary, conservative fallback, and safe provenance output", () => { + const config = readRepositoryFile("docs/config.md"); + const security = readRepositoryFile("docs/security.md"); + const architecture = readRepositoryFile("docs/architecture.md"); + const libraryApi = readRepositoryFile("docs/library-api.md"); + const changelog = readRepositoryFile("CHANGELOG.md"); + + for (const claim of [ + "trustToolAnnotations: true", + "tooling.unknownToolRisk", + "riskSource", + "riskConfidence", + "`idempotentHint`", + "`openWorldHint`", + "defaults to `\"destructive\"`", + "never starts an upstream" + ]) { + expect(config).toContain(claim); + } + expect(security).toContain("behavioral hints"); + expect(security).toContain("profile override cannot change"); + expect(architecture).toContain("normalizes only the four MCP behavioral booleans"); + expect(libraryApi).toContain("UnknownToolRisk"); + expect(changelog).toMatch(/\[#26\][\s\S]*risk classification/iu); + }); +}); diff --git a/tests/routing-policy.test.ts b/tests/routing-policy.test.ts index 98147686..3868d14f 100644 --- a/tests/routing-policy.test.ts +++ b/tests/routing-policy.test.ts @@ -196,12 +196,97 @@ describe("routing and policy", () => { { create_item: "write" } ); - expect(engine.evaluate("readonly", "create_item")).toEqual({ action: "deny", risk: "write" }); + expect(engine.evaluate("readonly", "create_item")).toEqual({ + action: "deny", + risk: "write", + riskSource: "local-override", + riskConfidence: "high" + }); expect(engine.evaluate("safe", "create_item")).toEqual({ action: "confirm", - risk: "write" + risk: "write", + riskSource: "local-override", + riskConfidence: "high" + }); + expect(engine.evaluate("safe", "get_item")).toEqual({ + action: "allow", + risk: "read", + riskSource: "name-heuristic", + riskConfidence: "low" + }); + }); + + it("records ordered risk classification from local overrides, trusted annotations, heuristics, and defaults", () => { + const readonly = new PolicyEngine( + { readonly: { allowRisk: ["read"], denyRisk: ["write", "destructive"] } }, + { delete_workspace: "read" } + ); + + expect( + readonly.evaluate("readonly", "delete_workspace", { + trusted: true, + annotations: { readOnlyHint: false, destructiveHint: true } + }) + ).toEqual({ action: "allow", risk: "read", riskSource: "local-override", riskConfidence: "high" }); + + expect( + new PolicyEngine({ readonly: { allowRisk: ["read"] } }).evaluate("readonly", "delete_workspace", { + trusted: true, + annotations: { readOnlyHint: true, destructiveHint: false } + }) + ).toEqual({ action: "allow", risk: "read", riskSource: "trusted-upstream-annotation", riskConfidence: "medium" }); + + expect( + new PolicyEngine({ readonly: { allowRisk: ["read"] } }).evaluate("readonly", "get_workspace", { + trusted: true, + annotations: { readOnlyHint: true, destructiveHint: true } + }) + ).toEqual({ action: "deny", risk: "destructive", riskSource: "annotation-conflict", riskConfidence: "low" }); + + expect( + new PolicyEngine({ readonly: { allowRisk: ["read"] } }).evaluate("readonly", "delete_workspace", { + trusted: false, + annotations: { readOnlyHint: true } + }) + ).toEqual({ action: "deny", risk: "destructive", riskSource: "name-heuristic", riskConfidence: "low" }); + + expect( + new PolicyEngine({ readonly: { allowRisk: ["read"] } }).evaluate("readonly", "delete_workspace", { + trusted: true, + annotations: { destructiveHint: false } + }) + ).toEqual({ action: "deny", risk: "destructive", riskSource: "name-heuristic", riskConfidence: "low" }); + + expect( + new PolicyEngine().evaluate(undefined, "upsert_workspace", { + trusted: true, + annotations: { + readOnlyHint: false, + destructiveHint: false, + idempotentHint: true, + openWorldHint: true + } + }) + ).toEqual({ action: "allow", risk: "write", riskSource: "trusted-upstream-annotation", riskConfidence: "medium" }); + + expect(new PolicyEngine().evaluate(undefined, "frobnicate")).toEqual({ + action: "allow", + risk: "destructive", + riskSource: "unknown-default", + riskConfidence: "low" + }); + expect(new PolicyEngine({}, {}, { unknownRisk: "write" }).evaluate(undefined, "frobnicate")).toEqual({ + action: "allow", + risk: "write", + riskSource: "unknown-default", + riskConfidence: "low" + }); + expect(new PolicyEngine().evaluate(undefined, "toString")).toEqual({ + action: "allow", + risk: "destructive", + riskSource: "unknown-default", + riskConfidence: "low" }); - expect(engine.evaluate("safe", "get_item")).toEqual({ action: "allow", risk: "read" }); }); it("fails closed when a profile references a missing named policy", () => { @@ -212,18 +297,33 @@ describe("routing and policy", () => { { create_item: "write" } ); - expect(engine.evaluate("missing-policy", "create_item")).toEqual({ action: "deny", risk: "write" }); + expect(engine.evaluate("missing-policy", "create_item")).toEqual({ + action: "deny", + risk: "write", + riskSource: "local-override", + riskConfidence: "high" + }); }); it("fails closed when a policy name resolves to an inherited object property", () => { const engine = new PolicyEngine(); - expect(engine.evaluate("toString", "delete_repository")).toEqual({ action: "deny", risk: "destructive" }); + expect(engine.evaluate("toString", "delete_repository")).toEqual({ + action: "deny", + risk: "destructive", + riskSource: "name-heuristic", + riskConfidence: "low" + }); }); it("fails closed when a policy name is explicitly empty", () => { const engine = new PolicyEngine(); - expect(engine.evaluate("", "delete_repository")).toEqual({ action: "deny", risk: "destructive" }); + expect(engine.evaluate("", "delete_repository")).toEqual({ + action: "deny", + risk: "destructive", + riskSource: "name-heuristic", + riskConfidence: "low" + }); }); }); diff --git a/tests/tool-registry.test.ts b/tests/tool-registry.test.ts new file mode 100644 index 00000000..63b986e1 --- /dev/null +++ b/tests/tool-registry.test.ts @@ -0,0 +1,59 @@ +import type { Tool } from "@modelcontextprotocol/sdk/types.js"; +import { describe, expect, it } from "vitest"; +import { ToolRegistry } from "../src/mcp/server/tool-registry.js"; + +const emptyInputSchema = { type: "object", properties: {} } as const; + +describe("tool registry risk metadata", () => { + it("keeps only behavioral annotation booleans in immutable registered-tool metadata", async () => { + const tools: Tool[] = [ + { + name: "annotated", + description: "Metadata should not be copied into policy input.", + inputSchema: emptyInputSchema, + annotations: { + title: "Sensitive title", + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: false + } + }, + { + name: "partial", + inputSchema: emptyInputSchema, + annotations: { readOnlyHint: false } + }, + { + name: "title_only", + inputSchema: emptyInputSchema, + annotations: { title: "No risk signal" } + }, + { name: "plain", inputSchema: emptyInputSchema } + ]; + const registry = new ToolRegistry( + async () => ({ discovered: [{ tools }], incomplete: false }), + (name) => name + ); + + const snapshot = await registry.get("work"); + const annotated = snapshot.resolve("annotated"); + const partial = snapshot.resolve("partial"); + const titleOnly = snapshot.resolve("title_only"); + const plain = snapshot.resolve("plain"); + + expect(annotated?.annotations).toEqual({ + readOnlyHint: true, + destructiveHint: false, + idempotentHint: true, + openWorldHint: false + }); + expect(partial?.annotations).toEqual({ readOnlyHint: false }); + expect(titleOnly?.annotations).toBeUndefined(); + expect(plain?.annotations).toBeUndefined(); + + if (!annotated?.annotations) throw new Error("Expected normalized annotations."); + (annotated.annotations as { readOnlyHint?: boolean }).readOnlyHint = false; + expect(snapshot.resolve("annotated")?.annotations?.readOnlyHint).toBe(true); + }); +}); From f86bca8757d1abe79158b8146cbec201455ad8a6 Mon Sep 17 00:00:00 2001 From: Mohammed Naji Date: Mon, 13 Jul 2026 07:46:44 +0400 Subject: [PATCH 2/3] fix(policy): bind annotation trust to routed targets --- src/mcp/server/miftah-server.ts | 26 +++++++- src/mcp/server/operation-pipeline.ts | 8 ++- tests/config-runtime-parity.test.ts | 11 ++-- tests/fake-upstream-fixture.test.ts | 19 ++++++ tests/fixtures/fake-upstream.mjs | 14 +++-- tests/operation-pipeline.test.ts | 59 +++++++++++++++++++ .../risk-classification-docs-contract.test.ts | 4 +- 7 files changed, 127 insertions(+), 14 deletions(-) create mode 100644 tests/fake-upstream-fixture.test.ts diff --git a/src/mcp/server/miftah-server.ts b/src/mcp/server/miftah-server.ts index fa719d9c..5798c538 100644 --- a/src/mcp/server/miftah-server.ts +++ b/src/mcp/server/miftah-server.ts @@ -532,7 +532,18 @@ export class MiftahServer { policyName: mapped.originalName, name: mapped.originalName, args, - riskMetadata: this.riskMetadata(mapped), + riskMetadataForProfile: (profile) => { + const target = this.toolRegistry.peek(profile)?.resolve(name); + if ( + target === undefined || + target.fingerprint !== mapped.fingerprint || + target.originalName !== mapped.originalName || + target.upstreamName !== mapped.upstreamName + ) { + return undefined; + } + return this.riskMetadata(target); + }, requireExplicitRuleForDestructive: this.config.security?.requireExplicitProfileForDestructive, resolveTarget: async (profile) => { const target = (await this.toolRegistry.get(profile)).resolve(name); @@ -548,6 +559,12 @@ export class MiftahServer { `TOOL_SCHEMA_MISMATCH: tool '${name}' has a different schema for routed profile '${profile}'` ); } + if (target.upstreamName !== mapped.upstreamName) { + throw new MiftahError( + "TOOL_SCHEMA_MISMATCH", + `TOOL_SCHEMA_MISMATCH: tool '${name}' resolves to a different upstream for routed profile '${profile}'` + ); + } return { upstreamName: this.auditUpstreamName(target.upstreamName), identityUpstreamName: target.upstreamName, @@ -700,12 +717,15 @@ export class MiftahServer { ? undefined : this.toolRegistry.peek(route.profile)?.resolve(toolName); const hasCompatibleCachedTarget = - sourceTool !== undefined && targetTool !== undefined && sourceTool.fingerprint === targetTool.fingerprint; + sourceTool !== undefined && + targetTool !== undefined && + sourceTool.fingerprint === targetTool.fingerprint && + sourceTool.upstreamName === targetTool.upstreamName; const policyName = sourceTool?.originalName ?? toolName; const policy = this.policy.evaluate( profile.policy, policyName, - hasCompatibleCachedTarget && sourceTool !== undefined ? this.riskMetadata(sourceTool) : undefined + hasCompatibleCachedTarget && targetTool !== undefined ? this.riskMetadata(targetTool) : undefined ); audit.update({ profile: route.profile, diff --git a/src/mcp/server/operation-pipeline.ts b/src/mcp/server/operation-pipeline.ts index df9c362d..abe5ecde 100644 --- a/src/mcp/server/operation-pipeline.ts +++ b/src/mcp/server/operation-pipeline.ts @@ -35,6 +35,8 @@ export interface ProxiedOperation { readonly name: string; readonly args: Record; readonly riskMetadata?: ToolRiskMetadata; + /** Reads only already-cached target risk evidence; it must not discover or start an upstream. */ + riskMetadataForProfile?(profile: string): ToolRiskMetadata | undefined; readonly requireExplicitRuleForDestructive?: boolean; resolveTarget(profile: string): Promise>; } @@ -72,7 +74,11 @@ export class OperationPipeline { ); const profile = route.profile; const profileConfig = this.options.profiles.get(profile); - const decision = this.options.policy.evaluate(profileConfig.policy, operation.policyName, operation.riskMetadata); + const decision = this.options.policy.evaluate( + profileConfig.policy, + operation.policyName, + operation.riskMetadataForProfile?.(profile) ?? operation.riskMetadata + ); audit.update({ profile, routingReason: route.reason, diff --git a/tests/config-runtime-parity.test.ts b/tests/config-runtime-parity.test.ts index 5d01a876..8b54127a 100644 --- a/tests/config-runtime-parity.test.ts +++ b/tests/config-runtime-parity.test.ts @@ -11,6 +11,9 @@ const checkedInExamples = [ "sentry.miftah.json" ]; const unsupportedConfigOption = "UNSUPPORTED_CONFIG_OPTION"; +const upstreamTrustPathPattern = /upstream\.trustToolAnnotations/u; +const unknownToolRiskPathPattern = /tooling\.unknownToolRisk/u; +const profileUpstreamTrustPathPattern = /profiles\.default\.upstreams\.primary\.trustToolAnnotations/u; function baseConfig(overrides: Record): Record { return { @@ -98,16 +101,14 @@ describe("config runtime parity", () => { expect(config.upstream?.trustToolAnnotations).toBe(true); expect(config.tooling?.unknownToolRisk).toBe("destructive"); expect(() => validateConfig(baseConfig({ upstream: { transport: "stdio", command: "node", trustToolAnnotations: "true" } }))).toThrow( - /upstream\.trustToolAnnotations/u - ); - expect(() => validateConfig(baseConfig({ tooling: { unknownToolRisk: "read" } }))).toThrow( - /tooling\.unknownToolRisk/u + upstreamTrustPathPattern ); + expect(() => validateConfig(baseConfig({ tooling: { unknownToolRisk: "read" } }))).toThrow(unknownToolRiskPathPattern); expect(() => validateConfig(baseConfig({ upstreams: { primary: { transport: "stdio", command: "node" } }, upstream: undefined, profiles: { default: { upstreams: { primary: { trustToolAnnotations: true } } } } - }))).toThrow(/profiles\.default\.upstreams\.primary\.trustToolAnnotations/u); + }))).toThrow(profileUpstreamTrustPathPattern); }); it.each([ diff --git a/tests/fake-upstream-fixture.test.ts b/tests/fake-upstream-fixture.test.ts new file mode 100644 index 00000000..95a58415 --- /dev/null +++ b/tests/fake-upstream-fixture.test.ts @@ -0,0 +1,19 @@ +import { spawnSync } from "node:child_process"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it } from "vitest"; + +const fixture = fileURLToPath(new URL("./fixtures/fake-upstream.mjs", import.meta.url)); + +describe("fake upstream fixture", () => { + it("identifies malformed tool annotations without echoing the invalid value", () => { + const result = spawnSync(process.execPath, [fixture], { + env: { ...process.env, TEST_CREATE_ITEM_ANNOTATIONS: "{not-json" }, + encoding: "utf8", + timeout: 5_000 + }); + + expect(result.status).not.toBe(0); + expect(result.stderr).toContain("TEST_CREATE_ITEM_ANNOTATIONS must contain valid JSON"); + expect(result.stderr).not.toContain("{not-json"); + }); +}); diff --git a/tests/fixtures/fake-upstream.mjs b/tests/fixtures/fake-upstream.mjs index eee28982..51fc3452 100644 --- a/tests/fixtures/fake-upstream.mjs +++ b/tests/fixtures/fake-upstream.mjs @@ -140,10 +140,7 @@ const whoamiInputSchema = : process.env.TEST_WHOAMI_SCHEMA === "malformed-required" ? { type: "object", properties: {}, required: "account" } : { type: "object", properties: {} }; -const createItemAnnotations = - process.env.TEST_CREATE_ITEM_ANNOTATIONS === undefined - ? undefined - : JSON.parse(process.env.TEST_CREATE_ITEM_ANNOTATIONS); +const createItemAnnotations = parseOptionalJson(process.env.TEST_CREATE_ITEM_ANNOTATIONS, "TEST_CREATE_ITEM_ANNOTATIONS"); const server = new Server( { name: "fake-upstream", version: "1.0.0" }, { capabilities: { tools: {}, resources: {}, prompts: {} } } @@ -154,6 +151,15 @@ server.oninitialized = () => { } }; +function parseOptionalJson(value, variableName) { + if (value === undefined) return undefined; + try { + return JSON.parse(value); + } catch { + throw new Error(`${variableName} must contain valid JSON`); + } +} + if (failInitialize || clientInfoPath) { server.setRequestHandler(InitializeRequestSchema, async (request) => { if (clientInfoPath) { diff --git a/tests/operation-pipeline.test.ts b/tests/operation-pipeline.test.ts index f2102644..c74288c3 100644 --- a/tests/operation-pipeline.test.ts +++ b/tests/operation-pipeline.test.ts @@ -5,10 +5,16 @@ import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { describe, expect, it } from "vitest"; +import { AuditTrail } from "../src/audit/audit-trail.js"; import { validateConfig } from "../src/config/validate-config.js"; +import { IdentityManager } from "../src/identity/identity-manager.js"; import { MiftahServer } from "../src/mcp/server/miftah-server.js"; +import { OperationPipeline } from "../src/mcp/server/operation-pipeline.js"; +import { PolicyEngine } from "../src/policy/policy-engine.js"; import { ProfileManager } from "../src/profiles/profile-manager.js"; +import { RoutingEngine } from "../src/routing/routing-engine.js"; import type { RoutingContextSnapshot } from "../src/routing/routing-types.js"; +import { SecretRedactor } from "../src/secrets/redact.js"; import { MultiUpstreamProcessManager } from "../src/upstream/multi-upstream-process-manager.js"; import { UpstreamProcessManager } from "../src/upstream/upstream-process-manager.js"; @@ -1222,3 +1228,56 @@ describe("operation pipeline", () => { } }); }); + +describe("operation pipeline routed risk", () => { + it("uses cached target metadata before policy without resolving the target", async () => { + const profiles = new ProfileManager({ + defaultProfile: "source", + profiles: { source: { policy: "readonly" }, target: { policy: "readonly" } } + }); + let targetResolved = false; + let upstreamRequested = false; + const pipeline = new OperationPipeline({ + profiles, + routing: new RoutingEngine({ rules: [{ when: { "args.account": "target" }, profile: "target" }] }, "source"), + policy: new PolicyEngine({ readonly: { allowRisk: ["read"] } }), + upstreams: { + get: async () => { + upstreamRequested = true; + throw new Error("The untrusted routed target must be blocked before it is contacted."); + } + } as unknown as UpstreamProcessManager, + redactor: new SecretRedactor(), + routingContext: async () => ({ context: {}, evidence: { cwd: "", fileRoots: [] }, profileHints: [] }), + identities: { requiresVerification: () => false } as unknown as IdentityManager + }); + const audit = new AuditTrail("test").beginOperation({ + operation: "tools/call", + name: "create_item", + sourceProfile: "source" + }); + + await expect( + pipeline.execute( + { + source: profiles.current(), + operation: "tools/call", + routingName: "create_item", + policyName: "create_item", + name: "create_item", + args: { account: "target" }, + riskMetadata: { trusted: true, annotations: { readOnlyHint: true } }, + riskMetadataForProfile: () => ({ trusted: false, annotations: { readOnlyHint: true } }), + resolveTarget: async () => { + targetResolved = true; + throw new Error("The target must not be resolved before policy blocks it."); + } + }, + audit + ) + ).rejects.toMatchObject({ code: "POLICY_BLOCKED" }); + + expect(targetResolved).toBe(false); + expect(upstreamRequested).toBe(false); + }); +}); diff --git a/tests/risk-classification-docs-contract.test.ts b/tests/risk-classification-docs-contract.test.ts index f76cbae2..2486ac35 100644 --- a/tests/risk-classification-docs-contract.test.ts +++ b/tests/risk-classification-docs-contract.test.ts @@ -1,6 +1,8 @@ import { readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; +const changelogRiskClassificationPattern = /\[#26\][\s\S]*risk classification/iu; + function readRepositoryFile(path: string): string { return readFileSync(new URL(`../${path}`, import.meta.url), "utf8"); } @@ -29,6 +31,6 @@ describe("risk classification documentation contract", () => { expect(security).toContain("profile override cannot change"); expect(architecture).toContain("normalizes only the four MCP behavioral booleans"); expect(libraryApi).toContain("UnknownToolRisk"); - expect(changelog).toMatch(/\[#26\][\s\S]*risk classification/iu); + expect(changelog).toMatch(changelogRiskClassificationPattern); }); }); From 6c1d35d0e336059e4b74d831c8af25f5426a99f9 Mon Sep 17 00:00:00 2001 From: Mohammed Naji Date: Mon, 13 Jul 2026 08:01:24 +0400 Subject: [PATCH 3/3] fix(policy): verify preview tool identity --- src/mcp/server/miftah-server.ts | 32 +++++++++++++++++--------------- tests/mcp-wrapper.test.ts | 22 +++++++++++++++++++++- 2 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/mcp/server/miftah-server.ts b/src/mcp/server/miftah-server.ts index 5798c538..f4b3ca46 100644 --- a/src/mcp/server/miftah-server.ts +++ b/src/mcp/server/miftah-server.ts @@ -100,6 +100,20 @@ export function resolveClientVisibleToolName( return name; } +/** Checks that a cached routed tool still denotes the same upstream operation before using its risk hints. */ +export function hasCompatibleCachedToolTarget( + source: RegisteredTool | undefined, + target: RegisteredTool | undefined +): target is RegisteredTool { + return ( + source !== undefined && + target !== undefined && + source.fingerprint === target.fingerprint && + source.originalName === target.originalName && + source.upstreamName === target.upstreamName + ); +} + function tool(name: string, description: string, required: string[] = [], optional: string[] = []): Tool { const fields = [...new Set([...required, ...optional])]; return { @@ -534,15 +548,7 @@ export class MiftahServer { args, riskMetadataForProfile: (profile) => { const target = this.toolRegistry.peek(profile)?.resolve(name); - if ( - target === undefined || - target.fingerprint !== mapped.fingerprint || - target.originalName !== mapped.originalName || - target.upstreamName !== mapped.upstreamName - ) { - return undefined; - } - return this.riskMetadata(target); + return hasCompatibleCachedToolTarget(mapped, target) ? this.riskMetadata(target) : undefined; }, requireExplicitRuleForDestructive: this.config.security?.requireExplicitProfileForDestructive, resolveTarget: async (profile) => { @@ -553,7 +559,7 @@ export class MiftahServer { `TOOL_NOT_FOUND: tool '${name}' is not exposed for routed profile '${profile}'` ); } - if (target.fingerprint !== mapped.fingerprint) { + if (target.fingerprint !== mapped.fingerprint || target.originalName !== mapped.originalName) { throw new MiftahError( "TOOL_SCHEMA_MISMATCH", `TOOL_SCHEMA_MISMATCH: tool '${name}' has a different schema for routed profile '${profile}'` @@ -716,11 +722,7 @@ export class MiftahServer { sourceTool === undefined ? undefined : this.toolRegistry.peek(route.profile)?.resolve(toolName); - const hasCompatibleCachedTarget = - sourceTool !== undefined && - targetTool !== undefined && - sourceTool.fingerprint === targetTool.fingerprint && - sourceTool.upstreamName === targetTool.upstreamName; + const hasCompatibleCachedTarget = hasCompatibleCachedToolTarget(sourceTool, targetTool); const policyName = sourceTool?.originalName ?? toolName; const policy = this.policy.evaluate( profile.policy, diff --git a/tests/mcp-wrapper.test.ts b/tests/mcp-wrapper.test.ts index 1f54da76..cfb7099a 100644 --- a/tests/mcp-wrapper.test.ts +++ b/tests/mcp-wrapper.test.ts @@ -19,7 +19,8 @@ import { validateConfig } from "../src/config/validate-config.js"; import type { MiftahConfig } from "../src/config/types.js"; import type { AuditScope } from "../src/audit/audit-trail.js"; import { ProfileManager } from "../src/profiles/profile-manager.js"; -import { MiftahServer } from "../src/mcp/server/miftah-server.js"; +import { hasCompatibleCachedToolTarget, MiftahServer } from "../src/mcp/server/miftah-server.js"; +import type { RegisteredTool } from "../src/mcp/server/tool-registry.js"; import { createMiftahRuntime } from "../src/runtime/create-miftah-runtime.js"; import type { RoutingContextSnapshot } from "../src/routing/routing-types.js"; import { MultiUpstreamProcessManager } from "../src/upstream/multi-upstream-process-manager.js"; @@ -28,6 +29,25 @@ import { UpstreamProcessManager } from "../src/upstream/upstream-process-manager const fixture = join(dirname(fileURLToPath(import.meta.url)), "fixtures", "fake-upstream.mjs"); const toolCollisionPattern = /TOOL_COLLISION/; +function registeredTool(originalName: string): RegisteredTool { + return { + exposedName: "trusted__shared_tool", + originalName, + upstreamName: "trusted", + profile: "work", + fingerprint: "same-client-contract" + }; +} + +describe("cached routed-tool compatibility", () => { + it("requires the original upstream tool name in addition to client shape and upstream identity", () => { + const source = registeredTool("source_tool"); + + expect(hasCompatibleCachedToolTarget(source, registeredTool("source_tool"))).toBe(true); + expect(hasCompatibleCachedToolTarget(source, registeredTool("different_tool"))).toBe(false); + }); +}); + interface RuntimeRoutingFixture { readonly directory: string; readonly configPath: string;