diff --git a/workspace-server/internal/handlers/mcp_test.go b/workspace-server/internal/handlers/mcp_test.go index d306fa14..385b03ec 100644 --- a/workspace-server/internal/handlers/mcp_test.go +++ b/workspace-server/internal/handlers/mcp_test.go @@ -417,11 +417,32 @@ func TestMCPHandler_CommitMemory_LocalScope_Success(t *testing.T) { } } -// TestMCPHandler_CommitMemory_GlobalScope_Blocked verifies that C3 is enforced: -// GLOBAL scope is not permitted on the MCP bridge. -func TestMCPHandler_CommitMemory_GlobalScope_Blocked(t *testing.T) { +// TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError verifies +// two contracts at once on the GLOBAL-scope-blocked path: +// +// 1. C3 invariant (commit_memory with scope=GLOBAL aborts on the MCP bridge +// before touching the DB), AND +// 2. OFFSEC-001 / #259 scrub contract (commit 7d1a189f): the JSON-RPC error +// returned to the client is a CONSTANT — code=-32000, message="tool call +// failed" — with the production-internal err.Error() text logged +// server-side, never reflected back to the caller. +// +// Prior to this rename the test asserted that the client-visible message +// CONTAINED the substring "GLOBAL", which was the human-readable internal +// error from toolCommitMemory. mc#664 Class 2 flipped that assertion the +// right way around: now the test FAILS if the scrub regresses (i.e. if the +// internal string is ever reflected back to the wire), and PASSES iff the +// scrubbed constant reaches the client. +// +// Coupling note: the constant string "tool call failed" and the code -32000 +// are the same values asserted by +// TestMCPHandler_dispatchRPC_UnknownTool_ReturnsConstantMessage — both are +// the OFFSEC-001 contract for the dispatch-failure branch in mcp.go (the +// third err.Error() leak that 7d1a189f scrubbed). If those constants ever +// change, both tests must move together. +func TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError(t *testing.T) { h, mock := newMCPHandler(t) - // No DB expectations — handler must abort before touching the DB. + // No DB expectations — handler must abort before touching the DB (C3). w := mcpPost(t, h, "ws-1", map[string]interface{}{ "jsonrpc": "2.0", @@ -436,14 +457,53 @@ func TestMCPHandler_CommitMemory_GlobalScope_Blocked(t *testing.T) { }, }) + // JSON-RPC envelope returns 200 with the error in the body — only + // malformed-JSON-at-the-envelope-layer returns 400 (see Call() in mcp.go). + if w.Code != http.StatusOK { + t.Fatalf("expected 200 (JSON-RPC error in body), got %d: %s", w.Code, w.Body.String()) + } + var resp mcpResponse - json.Unmarshal(w.Body.Bytes(), &resp) + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("response is not valid JSON: %v", err) + } + + // (1) C3: an error must be reported. if resp.Error == nil { - t.Error("expected JSON-RPC error for GLOBAL scope, got nil") + t.Fatal("expected JSON-RPC error for GLOBAL scope, got nil") } - if resp.Error != nil && !bytes.Contains([]byte(resp.Error.Message), []byte("GLOBAL")) { - t.Errorf("error message should mention GLOBAL, got: %s", resp.Error.Message) + + // (2) OFFSEC-001 positive assertions — exact equality on the scrubbed + // constants so any change (re-leak of err.Error(), code mutation) trips + // the test. Substring-match would not catch a partial re-leak. + if resp.Error.Code != -32000 { + t.Errorf("error code should be -32000 (Server error / dispatch-failure), got: %d", resp.Error.Code) } + if resp.Error.Message != "tool call failed" { + t.Errorf("error message should be the OFFSEC-001 constant %q, got: %q", "tool call failed", resp.Error.Message) + } + + // (3) OFFSEC-001 negative assertions — the internal err.Error() text + // from toolCommitMemory ("GLOBAL scope is not permitted via the MCP + // bridge — use LOCAL or TEAM") must NOT appear in the client-visible + // message. Each token below is a distinct substring of that internal + // string; if ANY leaks through, the scrub in mcp.go dispatchRPC has + // regressed and this assertion fires the canary. + leakedTokens := []string{ + "GLOBAL", // scope name + "scope", // policy lexicon + "permitted", // policy verb + "bridge", // internal architecture term + "LOCAL", // alternative scope name + "TEAM", // alternative scope name + } + for _, tok := range leakedTokens { + if bytes.Contains([]byte(resp.Error.Message), []byte(tok)) { + t.Errorf("OFFSEC-001 scrub regression: client-visible error.message leaks internal token %q (got: %q)", tok, resp.Error.Message) + } + } + + // (4) C3 invariant preserved: handler must short-circuit before any DB call. if err := mock.ExpectationsWereMet(); err != nil { t.Errorf("unexpected DB calls on GLOBAL scope block: %v", err) }