fix(mcp): fail-closed on unknown channel + signing-key self-heal (#471)

Adversarial review of PR #464 found that classify() promoted an absent
channelType to the 'authenticated' trust branch, stamping an untrusted
ThreadLocal username (e.g. stale value on a reused thread, or internal
tasks like SkillConsolidation/Reflection that carry no channel) with
authenticated trust — contradicting the fail-closed contract the service
documents.

- classify(): channel==null/blank now resolves to NONE (no injection);
  only the explicit 'web' channel may yield authenticated. Unrecognised
  non-web channels downgrade to external, never authenticated.
- signingKey(): replace the one-shot keyParseAttempted latch with
  lastAttemptedPem so a corrected/hot-reloaded PEM re-parses on the next
  call without an app restart. Still fail-closed when PEM is unchanged.
- Tests: 4 new cases lock the regression (null+dirty-ThreadLocal->NONE,
  blank->NONE, novel channel->external, self-heal after config fix).
- .gitignore: exclude local .codebase-memory/ agent index.

MCP+identity suite: 93/93 green.

.gitignore

@@ -105,6 +105,9 @@ CLAUDE.md
 # Codex CLI local artifacts
 .codex/
 
+# Codebase memory (local agent index / graph artifact; do not commit)
+.codebase-memory/
+
 # Sync tooling local state (generated each run; report is intentionally tracked)
 scripts/.*-sync-state.json
 

mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardService.java

@@ -64,9 +64,21 @@ public class McpIdentityForwardService {
 
     private final McpIdentityForwardProperties properties;
 
-    /** Lazily parsed signing key; {@code null} until first use / when unavailable. */
+    /**
+     * Lazily parsed signing key; {@code null} until first successful parse or
+     * while a parse is pending. Guarded by {@code this} (the parse block) and
+     * safe to read outside the lock because the field is volatile and the
+     * {@link PrivateKey} is published safely after construction.
+     */
     private volatile PrivateKey signingKey;
-    private volatile boolean keyParseAttempted;
+
+    /**
+     * PEM content last handed to the parser. Lets the cache self-heal when the
+     * operator fixes the config (or a config reload pushes a new key) without an
+     * app restart: if the PEM changed since the last attempt we retry instead of
+     * sticking with a permanent null. {@code null} = never attempted.
+     */
+    private volatile String lastAttemptedPem;
 
     public McpIdentityForwardService(McpIdentityForwardProperties properties) {
         this.properties = properties;
@@ -94,10 +106,13 @@ public class McpIdentityForwardService {
      * {@link ResolvedIdentity#NONE} for cron / system / unattributed origins
      * (never assert identity on behalf of a non-user).
      *
-     * <p>Source channel is read from {@link ChatOrigin} (carried on the
-     * {@link ToolContext}); when the context carries no origin it falls back to
-     * the legacy {@link ToolExecutionContext} username (treated as web/authenticated
-     * only if non-blank — preserves the original contract for the ThreadLocal path).
+     * <p>Trust is keyed off {@link ChatOrigin#channelType()}: only the explicit
+     * {@code "web"} channel (built by {@code ChatOrigin.web()}) may resolve to
+     * {@code authenticated} — every other channel is typed as {@code anonymous}
+     * / {@code external}, and an <em>unknown</em> (null/blank) channel resolves
+     * to {@link ResolvedIdentity#NONE} (fail-closed). The ThreadLocal
+     * {@link ToolExecutionContext} username is consulted only inside the web
+     * branch and only when no immutable user id is present.
      */
     ResolvedIdentity classify(ToolContext ctx) {
         ChatOrigin origin = ChatOrigin.from(ctx);
@@ -106,10 +121,19 @@ public class McpIdentityForwardService {
             return ResolvedIdentity.NONE;
         }
         String channel = origin.channelType();
+        // Unknown / unattributed channel: never assert identity. An absent
+        // channelType must NOT be promoted to authenticated — only the explicit
+        // "web" channel (built by ChatOrigin.web()) carries MateClaw's assertion
+        // that a real account backs this request. Treating null/blank as web
+        // would silently stamp an untrusted ThreadLocal value with authenticated
+        // trust, violating the fail-closed contract this service guarantees.
+        if (channel == null || channel.isBlank()) {
+            return ResolvedIdentity.NONE;
+        }
         // Authenticated web account: MateClaw vouches for this user. Prefer the
         // immutable numeric id when available (web-console login); fall back to
         // the username when only the ThreadLocal path supplied identity.
-        if (channel == null || channel.isBlank() || "web".equals(channel)) {
+        if ("web".equals(channel)) {
             if (origin.requesterUserId() != null) {
                 return new ResolvedIdentity(String.valueOf(origin.requesterUserId()),
                         TRUST_AUTHENTICATED, "web");
@@ -180,20 +204,35 @@ public class McpIdentityForwardService {
         }
     }
 
+    /**
+     * Resolve the signing key, parsing it lazily. Self-heals when the configured
+     * PEM changes (e.g. an operator fixes a malformed key or a config reload
+     * pushes a new one) — a prior failed parse is retried on the next call once
+     * {@code private-key-pem} differs from what was last attempted, so recovery
+     * no longer needs an app restart. Stays fail-closed otherwise.
+     */
     private PrivateKey signingKey() {
         PrivateKey k = signingKey;
         if (k != null) {
             return k;
         }
-        if (keyParseAttempted) {
-            return null;   // already tried and failed; don't spam parsing
+        String pem = properties.getToken().getPrivateKeyPem();
+        // Skip only while the PEM is unchanged since the last attempt — that
+        // avoids re-parsing (and re-logging) on every call. A changed PEM clears
+        // the way for a fresh parse, which is the self-healing path.
+        if (lastAttemptedPem != null && lastAttemptedPem.equals(pem)) {
+            return null;
         }
         synchronized (this) {
             if (signingKey != null) {
                 return signingKey;
             }
-            keyParseAttempted = true;
-            String pem = properties.getToken().getPrivateKeyPem();
+            // Re-check under the lock: another thread may have just attempted
+            // the same (unchanged) PEM.
+            if (lastAttemptedPem != null && lastAttemptedPem.equals(pem)) {
+                return null;
+            }
+            lastAttemptedPem = pem;
             if (pem == null || pem.isBlank()) {
                 log.error("[McpIdentity] token mode enabled but mateclaw.mcp.identity-forward.token.private-key-pem is empty; "
                         + "identity tokens will NOT be issued (fail-closed)");

mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpIdentityForwardServiceTest.java

@@ -129,6 +129,51 @@ class McpIdentityForwardServiceTest {
             assertThat(id.subject()).isEqualTo("alice");
             assertThat(id.trust()).isEqualTo(McpIdentityForwardService.TRUST_AUTHENTICATED);
         }
+
+        // ---- Fail-closed: an unknown / unattributed channel must NEVER be
+        //      promoted to authenticated, even when a stale ThreadLocal username
+        //      is present. This is the regression for the privilege-escalation
+        //      gap where channel==null used to fall into the web branch and
+        //      stamp an untrusted identifier with authenticated trust. ----
+
+        @Test
+        @DisplayName("unknown channelType (null) → NONE, even with a polluted ThreadLocal (no privilege escalation)")
+        void unknownChannelIsFailClosedEvenWithThreadLocal() {
+            // A stale username from a prior request reused this thread.
+            ToolExecutionContext.set("c1", "attacker");
+            // System-style origin built with no channelType (e.g. SkillConsolidation
+            // / SkillReflection internal tasks): requesterId="", channelType=null.
+            ChatOrigin origin = new ChatOrigin(null, "c1", "", 1L, null,
+                    null, null, false, null, null, null, null, null);
+            assertThat(svc(new McpIdentityForwardProperties()).classify(ctx(origin)))
+                    .isEqualTo(McpIdentityForwardService.ResolvedIdentity.NONE);
+        }
+
+        @Test
+        @DisplayName("blank channelType → NONE (fail-closed, not authenticated)")
+        void blankChannelIsFailClosed() {
+            ToolExecutionContext.set("c1", "attacker");
+            ChatOrigin origin = new ChatOrigin(null, "c1", "someone", 1L, null,
+                    null, null, false, null, "  ", null, null, null);
+            assertThat(svc(new McpIdentityForwardProperties()).classify(ctx(origin)))
+                    .isEqualTo(McpIdentityForwardService.ResolvedIdentity.NONE);
+        }
+
+        @Test
+        @DisplayName("unrecognised non-web channel → downgraded to external (never authenticated)")
+        void novelChannelIsDowngradedNotAuthenticated() {
+            // A channel the classifier doesn't recognise is treated as external
+            // (explicit trust downgrade), never as authenticated. The backend sees
+            // an untrusted id and decides for itself — this is the key guarantee:
+            // no unknown channel can ever acquire authenticated trust.
+            ChatOrigin origin = new ChatOrigin(null, "c1", "u1", 1L, null,
+                    null, null, false, null, "future-net", null, null, null);
+            McpIdentityForwardService.ResolvedIdentity id =
+                    svc(new McpIdentityForwardProperties()).classify(ctx(origin));
+            assertThat(id.subject()).isEqualTo("u1");
+            assertThat(id.trust()).isEqualTo(McpIdentityForwardService.TRUST_EXTERNAL);
+            assertThat(id.channelType()).isEqualTo("future-net");
+        }
     }
 
     // ==================== Resolution: plaintext & token modes ====================
@@ -170,6 +215,41 @@ class McpIdentityForwardServiceTest {
         assertThat(svc(p).resolve(ctx(origin), "my-api")).isEmpty();
     }
 
+    @Test
+    @DisplayName("signing key self-heals after a bad PEM is corrected (no restart needed)")
+    void signingKeySelfHealsAfterConfigFix() {
+        KeyPair kp = rsaKeyPair();
+        String goodPem = Base64.getEncoder().encodeToString(kp.getPrivate().getEncoded());
+        ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L);
+
+        var p = new McpIdentityForwardProperties();
+        p.getToken().setEnabled(true);
+        p.getToken().setIssuer("mateclaw");
+        p.getToken().setTtlSeconds(60);
+
+        // 1. Malformed key → fail-closed.
+        p.getToken().setPrivateKeyPem("not-a-valid-pem");
+        McpIdentityForwardService service = svc(p);
+        assertThat(service.resolve(ctx(origin), "my-api")).isEmpty();
+
+        // 2. Operator fixes the config (or a reload pushes a good key) → next
+        //    call re-parses and issues a token, without needing an app restart.
+        p.getToken().setPrivateKeyPem(goodPem);
+        Optional<McpIdentityForwardService.Injection> inj = service.resolve(ctx(origin), "my-api");
+        assertThat(inj).isPresent();
+        assertThat(inj.get().key()).isEqualTo(McpIdentityForwardProperties.TOKEN_ARG);
+
+        // The minted token verifies against the matching public key.
+        Claims claims = Jwts.parser()
+                .verifyWith(kp.getPublic())
+                .requireIssuer("mateclaw")
+                .requireAudience("my-api")
+                .build()
+                .parseSignedClaims(inj.get().value())
+                .getPayload();
+        assertThat(claims.getSubject()).isEqualTo("42");
+    }
+
     @Test
     @DisplayName("token mode: mints RS256 JWT that verifies with trust/channel_type claims")
     void tokenMintAndVerify() throws Exception {