CB-513: test the MCP-side authorization gate (BridgeMcp 27.4% -> 57.4%)
CI / build (push) Successful in 1m13s
CI / build (push) Successful in 1m13s
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).
This commit is contained in:
@@ -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.
|
||||
*
|
||||
* <p>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}.
|
||||
*
|
||||
* <p>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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user