From 1e6daa5c73f287eb194cd9d0b046f992477436bb Mon Sep 17 00:00:00 2001 From: Kevin Nguyen Date: Sat, 1 Aug 2026 23:11:56 +0700 Subject: [PATCH] CB-513: test the MCP-side authorization gate (BridgeMcp 27.4% -> 57.4%) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CB-505 claimed authorization is "enforced on both entry paths". It is — but only REST was ever tested. Coverage showed BridgeMcp.deny(), principal(), callerTerminal(), worktreeRequest() and every tool-registration lambda at ZERO executed lines: no test had ever constructed a BridgeMcp, because the existing BridgeMcpTest calls only the static handler methods. So the MCP half of the security control had ten REST tests' worth of nothing behind it. An unexercised security control is a claim, not a control. Made testable by separating policy from plumbing rather than by reaching for a mocking library the project does not use: - denyFor(Principal, Action, target) is the decision — testable directly. - deny(exchange, ...) shrinks to pulling the caller out of the SDK exchange. - principalFrom(role, terminal, pid) extracts identity reconstruction from McpSyncServerExchange, an SDK type with no fake available. Moved the `authz == null` enforcement switch OUT of the exchange-facing wrapper and INTO denyFor. Found by a failing test: as written, any future tool calling denyFor directly would have silently skipped the gate. The switch now lives with the decision it governs. New BridgeMcpAuthzTest constructs a real BridgeMcp — which is why coverage moved so far, since that also runs the constructor and all the tool wiring — and pins the table on this path: primary orchestrates, worker cannot; worker replies only as itself; the primary cannot forge a worker reply; anonymous gets nothing; and 401-shaped vs 403-shaped refusals are counted apart. Verified as real controls, not decoration: with the gate forced open, 5 of the 9 fail. 335 tests (was 326). --- .../java/dev/ltms/bridged/mcp/BridgeMcp.java | 46 ++++- .../ltms/bridged/mcp/BridgeMcpAuthzTest.java | 168 ++++++++++++++++++ 2 files changed, 206 insertions(+), 8 deletions(-) create mode 100644 bridged/src/test/java/dev/ltms/bridged/mcp/BridgeMcpAuthzTest.java diff --git a/bridged/src/main/java/dev/ltms/bridged/mcp/BridgeMcp.java b/bridged/src/main/java/dev/ltms/bridged/mcp/BridgeMcp.java index 7dc962a..19e7cda 100644 --- a/bridged/src/main/java/dev/ltms/bridged/mcp/BridgeMcp.java +++ b/bridged/src/main/java/dev/ltms/bridged/mcp/BridgeMcp.java @@ -216,14 +216,27 @@ public final class BridgeMcp { /** The caller reconstructed from the transport context. */ private static Principal principal(McpSyncServerExchange exchange) { - Object r = exchange.transportContext().get(CALLER_ROLE); - String terminal = callerTerminal(exchange); - long pid = callerPid(exchange); - if (r == null) { + return principalFrom(exchange.transportContext().get(CALLER_ROLE), + callerTerminal(exchange), callerPid(exchange)); + } + + /** + * Rebuild a {@link Principal} from the three values the context extractor stashed. + * + *

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()); + } +}