Split out from {@link #principal(McpSyncServerExchange)} so the identity rules are + * reachable without an {@code McpSyncServerExchange} — that is an SDK type this project has no + * mocking library to fabricate, which is why this logic had no test at all until CB-513. + * + * @param role the stashed {@link Role} name, or {@code null} on the legacy path + * @param terminal the worker terminal, or {@code null} for a non-worker + * @param pid the calling pid, or {@code -1} + */ + static Principal principalFrom(Object role, String terminal, long pid) { + if (role == null) { // No role stashed (legacy path): fall back to the historical interpretation. return terminal != null ? Principal.worker(terminal, pid) : Principal.primary(pid); } - return new Principal(Role.valueOf(r.toString()), terminal, pid); + return new Principal(Role.valueOf(role.toString()), terminal, pid); } /** @@ -232,13 +245,30 @@ public final class BridgeMcp { */ private McpSchema.CallToolResult deny(McpSyncServerExchange exchange, Authz.Action action, String target) { + return denyFor(principal(exchange), action, target); + } + + /** + * The policy half of {@link #deny}: everything except pulling the caller out of the MCP + * exchange. Kept separate so the authorization decision — the actual control — is unit-testable + * without fabricating an SDK {@code McpSyncServerExchange}. + * + *
This surface exists because the enforcement was previously unreachable from a test: no + * test constructs a {@code BridgeMcp}, so the whole MCP-side gate ran zero times in the suite + * while the REST-side equivalent had ten tests. A security control nothing exercises is a + * claim, not a control. + * + * @return {@code null} when the call may proceed, or the error result to return when it may not + */ + McpSchema.CallToolResult denyFor(Principal caller, Authz.Action action, String target) { + // The enforcement switch lives HERE rather than in the exchange-facing wrapper: any future + // tool that calls this directly must not be able to skip the gate by accident. if (authz == null) { - return null; // legacy: authorization not enforced + return null; // legacy constructor: authorization not enforced } - Principal caller = principal(exchange); if (Authz.permits(caller, action, target)) { if (action != Authz.Action.READ) { - AuditLog.allowed(caller, action, target); + AuditLog.allowed(caller, action, target); // reads would drown the trail } return null; } diff --git a/bridged/src/test/java/dev/ltms/bridged/mcp/BridgeMcpAuthzTest.java b/bridged/src/test/java/dev/ltms/bridged/mcp/BridgeMcpAuthzTest.java new file mode 100644 index 0000000..5dc9a6d --- /dev/null +++ b/bridged/src/test/java/dev/ltms/bridged/mcp/BridgeMcpAuthzTest.java @@ -0,0 +1,168 @@ +package dev.ltms.bridged.mcp; + +import dev.ltms.bridged.auth.Authz; +import dev.ltms.bridged.auth.CallerResolver; +import dev.ltms.bridged.auth.Principal; +import dev.ltms.bridged.auth.Role; +import dev.ltms.bridged.config.BridgedConfig; +import dev.ltms.bridged.guard.SubscriptionGuard; +import dev.ltms.bridged.herdr.AgentControl; +import dev.ltms.bridged.herdr.FakeHerdr; +import dev.ltms.bridged.herdr.PaneLocator; +import dev.ltms.bridged.herdr.WorkspaceControl; +import dev.ltms.bridged.inject.Injector; +import dev.ltms.bridged.metrics.BridgedMetrics; +import dev.ltms.bridged.metrics.Metrics; +import dev.ltms.bridged.msg.InMemoryReplyInbox; +import dev.ltms.bridged.msg.MessageService; +import dev.ltms.bridged.msg.Rendezvous; +import dev.ltms.bridged.session.FakeWorktrees; +import dev.ltms.bridged.session.SessionManager; +import dev.ltms.bridged.worker.ClaudeCodeLauncher; +import io.modelcontextprotocol.spec.McpSchema; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.util.Map; +import java.util.Set; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * CB-513 — the CB-505 authorization gate on the MCP entry path. + * + *
Why this file exists: CB-505 claimed authorization is "enforced on both entry paths", and it + * is — but only REST was ever tested ({@code BridgedAppAuthTest}). Coverage showed + * {@code BridgeMcp.deny()}, {@code principal()} and every tool-registration lambda at zero + * executed lines, because no test had ever constructed a {@code BridgeMcp} — the existing + * {@code BridgeMcpTest} calls only the static handler methods. An unexercised security control is + * a claim, not a control. + * + *
These tests construct a real {@code BridgeMcp} (which also exercises the constructor and the + * tool wiring) and drive the policy half of the gate directly. + */ +class BridgeMcpAuthzTest { + + private final FakeHerdr herdr = new FakeHerdr(); + private final AgentControl agents = new AgentControl(herdr); + private Metrics metrics; + private BridgeMcp mcp; + + @AfterEach + void close() { + if (mcp != null) mcp.close(); + } + + /** A fully wired BridgeMcp on fakes — constructing it is itself part of what is under test. */ + private BridgeMcp mcp(boolean enforce) { + BridgedConfig.Worker cfg = new BridgedConfig.Worker( + "ltms-local", "http://gx00.gw:8000", "coder", null, "BRIDGED_WORKER_TOKEN", null, + "tab", "bridged-workers", "worker: {profile} #{n}", null, null, null); + ClaudeCodeLauncher workers = new ClaudeCodeLauncher(agents, new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), + _ -> "tok"); + SessionManager sessions = new SessionManager(workers, new FakeWorktrees()); + MessageService messages = new MessageService(agents, new Injector(agents), new Rendezvous(), + new InMemoryReplyInbox()); + ConnectionIdentity identity = new ConnectionIdentity(new PaneLocator(herdr), _ -> 999_999); + metrics = BridgedMetrics.create(sessions, new InMemoryReplyInbox()); + + mcp = new BridgeMcp(messages, workers, sessions, identity, sessions.asPresence(), + new PrimaryRegistry(null), + enforce ? new CallerResolver(identity) : null, + metrics); + return mcp; + } + + private static final Principal PRIMARY = Principal.primary(100); + private static final Principal WORKER_A = Principal.worker("term_a", 200); + private static final Principal ANON = Principal.anonymous(); + + // --- the table, enforced on THIS path too --------------------------------------------------- + + @Test + void primaryMayOrchestrate() { + BridgeMcp m = mcp(true); + for (Authz.Action a : new Authz.Action[]{Authz.Action.SPAWN, Authz.Action.STOP, + Authz.Action.SEND, Authz.Action.DRAIN, Authz.Action.READ}) { + assertNull(m.denyFor(PRIMARY, a, "term_a"), a + " is the primary's to perform"); + } + } + + @Test + void aWorkerMayNotOrchestrateOverMcp() { + BridgeMcp m = mcp(true); + for (Authz.Action a : new Authz.Action[]{Authz.Action.SPAWN, Authz.Action.STOP, + Authz.Action.SEND, Authz.Action.DRAIN}) { + McpSchema.CallToolResult denied = m.denyFor(WORKER_A, a, "term_a"); + assertNotNull(denied, a + " must be refused to a worker"); + assertTrue(denied.isError(), "a refusal is returned as an MCP tool error"); + } + } + + @Test + void aWorkerMayReplyAndAskOnlyAsItself() { + BridgeMcp m = mcp(true); + assertNull(m.denyFor(WORKER_A, Authz.Action.REPLY, "term_a"), "its own session is allowed"); + assertNull(m.denyFor(WORKER_A, Authz.Action.ASK, "term_a")); + + assertNotNull(m.denyFor(WORKER_A, Authz.Action.REPLY, "term_b"), + "worker A must not reply on worker B's session"); + assertNotNull(m.denyFor(WORKER_A, Authz.Action.ASK, "term_b")); + } + + @Test + void thePrimaryMayNotForgeAWorkerReplyOverMcp() { + BridgeMcp m = mcp(true); + // A forged reply would resolve the very rendezvous the primary is blocked on. + assertNotNull(m.denyFor(PRIMARY, Authz.Action.REPLY, "term_a")); + assertNotNull(m.denyFor(PRIMARY, Authz.Action.ASK, "term_a")); + } + + @Test + void anonymousIsRefusedEverythingAndCountedAsUnauthenticated() { + BridgeMcp m = mcp(true); + McpSchema.CallToolResult denied = m.denyFor(ANON, Authz.Action.READ, null); + + assertNotNull(denied, "authenticated as nothing ⇒ authorized for nothing"); + assertEquals(1, metrics.count(BridgedMetrics.AUTH_FAILURES, "reason", "unauthenticated")); + assertEquals(0, metrics.count(BridgedMetrics.AUTH_FAILURES, "reason", "forbidden"), + "a missing credential is 401-shaped, not 403-shaped"); + } + + @Test + void aWrongRoleIsCountedAsForbiddenNotUnauthenticated() { + BridgeMcp m = mcp(true); + assertNotNull(m.denyFor(WORKER_A, Authz.Action.SPAWN, null)); + + assertEquals(1, metrics.count(BridgedMetrics.AUTH_FAILURES, "reason", "forbidden")); + assertEquals(0, metrics.count(BridgedMetrics.AUTH_FAILURES, "reason", "unauthenticated"), + "the caller IS authenticated — it is just not the right role"); + } + + @Test + void theLegacyConstructorLeavesTheGateOpen() { + // The 22 pre-existing BridgeMcpTest cases rely on no authorization being enforced. + BridgeMcp m = mcp(false); + assertNull(m.denyFor(ANON, Authz.Action.SPAWN, null), + "no CallerResolver supplied ⇒ authorization not enforced (legacy behaviour)"); + } + + // --- identity reconstruction from the transport context ------------------------------------ + + @Test + void principalIsRebuiltFromTheStashedRole() { + assertEquals(Role.WORKER, BridgeMcp.principalFrom("WORKER", "term_a", 7).role()); + assertEquals("term_a", BridgeMcp.principalFrom("WORKER", "term_a", 7).terminal()); + assertEquals(Role.PRIMARY, BridgeMcp.principalFrom("PRIMARY", null, 7).role()); + assertEquals(Role.ANONYMOUS, BridgeMcp.principalFrom("ANONYMOUS", null, -1).role()); + } + + @Test + void aMissingRoleFallsBackToTheHistoricalInterpretation() { + // Legacy path: no role stashed. A terminal means worker; its absence meant "the primary", + // which is exactly the pre-CB-501 default CB-501 inverted — preserved only here. + assertEquals(Role.WORKER, BridgeMcp.principalFrom(null, "term_a", 7).role()); + assertEquals(Role.PRIMARY, BridgeMcp.principalFrom(null, null, 7).role()); + } +}