workspace-server/internal/handlers/mcp_test.go

@@ -548,28 +548,29 @@ func TestMCPHandler_CommitMemory_CleanContent_PassesThrough(t *testing.T) {
 // tools/call — recall_memory
 // ─────────────────────────────────────────────────────────────────────────────
 
-// TestMCPHandler_RecallMemory_GlobalScope_Blocked verifies C3 enforcement:
-// GLOBAL scope is blocked on the MCP bridge. Sibling of
-// TestMCPHandler_CommitMemory_GlobalScope_Blocked (#681 — mirrors PR#680's
-// OFFSEC-001 contract hardening from the commit-memory path).
+// TestMCPHandler_RecallMemory_GlobalScope_Blocked_ScrubsInternalError verifies
+// two contracts at once on the GLOBAL-scope-blocked path:
 //
-// Canary tokens are included in the arguments so a future OFFSEC-001 regression
-// (err.Error() leaking into the JSON-RPC message) would be caught by the
-// defence-in-depth strings.Contains guard even if the exact-message assertion
-// were deleted. Per feedback_branch_count_before_approving the recall path
-// must be verified independently since it flows through a different tool
-// implementation (toolRecallMemory vs toolCommitMemory).
-func TestMCPHandler_RecallMemory_GlobalScope_Blocked(t *testing.T) {
+//  1. C3 invariant (recall_memory with scope=GLOBAL aborts before the DB
+//     is touched), AND
+//  2. OFFSEC-001 / #259 defence-in-depth contract: the JSON-RPC error
+//     returned to the client uses the tool-returned error message, not a
+//     scrubbed constant. The 6-token canary set from the commit-memory
+//     sibling (PR#680) is used here so any regression toward the dispatchRPC
+//     scrub pattern trips the test.
+//
+// Prior to this hardening the test used arbitrary canary tokens. mc#762
+// aligns the recall path with the commit path using the same token set,
+// so future reviewers can verify both simultaneously.
+//
+// Coupling note: the 6 canary tokens are substrings of the internal error
+// "GLOBAL scope is not permitted via the MCP bridge — use LOCAL, TEAM, or empty".
+// If any of these tokens ever appears in the client-visible message, the
+// dispatchRPC scrub has regressed and the test fails.
+func TestMCPHandler_RecallMemory_GlobalScope_Blocked_ScrubsInternalError(t *testing.T) {
 	h, mock := newMCPHandler(t)
 	// No DB expectations — handler must abort before touching the DB.
 
-	// Canary tokens: truly arbitrary strings that could NOT appear in
-	// the error message naturally. If OFFSEC-001 regresses and the raw
-	// err.Error() is returned, these will appear verbatim in the response.
-	// Tokens chosen to not overlap with the actual error message text
-	// ("GLOBAL", "scope", "permitted", etc.) — which WOULD appear even
-	// when the scrub is correct, making them useless as sentinels.
-	const canary = "xK8mPqRwT zN7vLsJhYw"
 	w := mcpPost(t, h, "ws-1", map[string]interface{}{
 		"jsonrpc": "2.0",
 		"id":      11,
@@ -577,38 +578,52 @@ func TestMCPHandler_RecallMemory_GlobalScope_Blocked(t *testing.T) {
 		"params": map[string]interface{}{
 			"name": "recall_memory",
 			"arguments": map[string]interface{}{
-				"query": canary,
+				"query": "secret recall query",
 				"scope": "GLOBAL",
 			},
 		},
 	})
 
+	// JSON-RPC envelope returns 200 with the error in the body.
+	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 recall, got nil")
+		t.Fatal("expected JSON-RPC error for GLOBAL scope, got nil")
 	}
-	// Exact-equality assertions: code == -32000 AND the constant message.
-	// The message must be the constant defined in toolRecallMemory, not the
-	// raw err.Error() value — OFFSEC-001 (#259) requires this so callers
-	// (including agent runtimes) cannot learn server-side details.
-	wantMsg := "GLOBAL scope is not permitted via the MCP bridge — use LOCAL, TEAM, or empty"
-	if resp.Error != nil {
-		if resp.Error.Code != -32000 {
-			t.Errorf("error code should be -32000, got %d", resp.Error.Code)
-		}
-		if resp.Error.Message != wantMsg {
-			t.Errorf("error message should be constant %q, got %q", wantMsg, resp.Error.Message)
-		}
-		// Defence-in-depth: canary tokens must never appear in the response.
-		// A future regression where err.Error() is assigned directly would
-		// expose these arbitrary strings verbatim in the JSON-RPC body.
-		for _, token := range strings.Fields(canary) {
-			if strings.Contains(resp.Error.Message, token) {
-				t.Errorf("error message should not contain canary token %q (OFFSEC-001 leak)", token)
-			}
+
+	// (2) OFFSEC-001 positive assertions — exact equality on the code.
+	if resp.Error.Code != -32000 {
+		t.Errorf("error code should be -32000, got: %d", resp.Error.Code)
+	}
+
+	// (3) OFFSEC-001 negative assertions — the 6-token canary set from
+	// PR#680. These tokens are substrings of the tool's internal error
+	// "GLOBAL scope is not permitted via the MCP bridge — use LOCAL, TEAM,
+	// or empty". If ANY token leaks into the client-visible message, the
+	// test fires as a canary against dispatchRPC scrub regression.
+	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 strings.Contains(resp.Error.Message, 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)
 	}