fix(core): prevent server name spoofing in policy engine (#12511)

packages/core/src/policy/policy-engine.test.ts

@@ -22,13 +22,13 @@ describe('PolicyEngine', () => {
 
   describe('constructor', () => {
     it('should use default config when none provided', () => {
-      const decision = engine.check({ name: 'test' });
+      const decision = engine.check({ name: 'test' }, undefined);
       expect(decision).toBe(PolicyDecision.ASK_USER);
     });
 
     it('should respect custom default decision', () => {
       engine = new PolicyEngine({ defaultDecision: PolicyDecision.DENY });
-      const decision = engine.check({ name: 'test' });
+      const decision = engine.check({ name: 'test' }, undefined);
       expect(decision).toBe(PolicyDecision.DENY);
     });
 
@@ -57,9 +57,15 @@ describe('PolicyEngine', () => {
 
       engine = new PolicyEngine({ rules });
 
-      expect(engine.check({ name: 'shell' })).toBe(PolicyDecision.ALLOW);
-      expect(engine.check({ name: 'edit' })).toBe(PolicyDecision.DENY);
-      expect(engine.check({ name: 'other' })).toBe(PolicyDecision.ASK_USER);
+      expect(engine.check({ name: 'shell' }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
+      expect(engine.check({ name: 'edit' }, undefined)).toBe(
+        PolicyDecision.DENY,
+      );
+      expect(engine.check({ name: 'other' }, undefined)).toBe(
+        PolicyDecision.ASK_USER,
+      );
     });
 
     it('should match by args pattern', () => {
@@ -87,8 +93,8 @@ describe('PolicyEngine', () => {
         args: { command: 'ls -la' },
       };
 
-      expect(engine.check(dangerousCall)).toBe(PolicyDecision.DENY);
-      expect(engine.check(safeCall)).toBe(PolicyDecision.ALLOW);
+      expect(engine.check(dangerousCall, undefined)).toBe(PolicyDecision.DENY);
+      expect(engine.check(safeCall, undefined)).toBe(PolicyDecision.ALLOW);
     });
 
     it('should apply rules by priority', () => {
@@ -100,7 +106,9 @@ describe('PolicyEngine', () => {
       engine = new PolicyEngine({ rules });
 
       // Higher priority rule (ALLOW) should win
-      expect(engine.check({ name: 'shell' })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'shell' }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
     });
 
     it('should apply wildcard rules (no toolName)', () => {
@@ -111,8 +119,10 @@ describe('PolicyEngine', () => {
 
       engine = new PolicyEngine({ rules });
 
-      expect(engine.check({ name: 'safe-tool' })).toBe(PolicyDecision.ALLOW);
-      expect(engine.check({ name: 'any-other-tool' })).toBe(
+      expect(engine.check({ name: 'safe-tool' }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
+      expect(engine.check({ name: 'any-other-tool' }, undefined)).toBe(
         PolicyDecision.DENY,
       );
     });
@@ -129,13 +139,17 @@ describe('PolicyEngine', () => {
       engine = new PolicyEngine(config);
 
       // ASK_USER should become DENY in non-interactive mode
-      expect(engine.check({ name: 'interactive-tool' })).toBe(
+      expect(engine.check({ name: 'interactive-tool' }, undefined)).toBe(
         PolicyDecision.DENY,
       );
       // ALLOW should remain ALLOW
-      expect(engine.check({ name: 'allowed-tool' })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'allowed-tool' }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
       // Default ASK_USER should also become DENY
-      expect(engine.check({ name: 'unknown-tool' })).toBe(PolicyDecision.DENY);
+      expect(engine.check({ name: 'unknown-tool' }, undefined)).toBe(
+        PolicyDecision.DENY,
+      );
     });
   });
 
@@ -165,11 +179,15 @@ describe('PolicyEngine', () => {
     });
 
     it('should apply newly added rules', () => {
-      expect(engine.check({ name: 'new-tool' })).toBe(PolicyDecision.ASK_USER);
+      expect(engine.check({ name: 'new-tool' }, undefined)).toBe(
+        PolicyDecision.ASK_USER,
+      );
 
       engine.addRule({ toolName: 'new-tool', decision: PolicyDecision.ALLOW });
 
-      expect(engine.check({ name: 'new-tool' })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'new-tool' }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
     });
   });
 
@@ -235,29 +253,31 @@ describe('PolicyEngine', () => {
       engine = new PolicyEngine({ rules });
 
       // Should match my-server tools
-      expect(engine.check({ name: 'my-server__tool1' })).toBe(
+      expect(engine.check({ name: 'my-server__tool1' }, undefined)).toBe(
         PolicyDecision.ALLOW,
       );
-      expect(engine.check({ name: 'my-server__another_tool' })).toBe(
+      expect(engine.check({ name: 'my-server__another_tool' }, undefined)).toBe(
         PolicyDecision.ALLOW,
       );
 
       // Should match blocked-server tools
-      expect(engine.check({ name: 'blocked-server__tool1' })).toBe(
-        PolicyDecision.DENY,
-      );
-      expect(engine.check({ name: 'blocked-server__dangerous' })).toBe(
+      expect(engine.check({ name: 'blocked-server__tool1' }, undefined)).toBe(
         PolicyDecision.DENY,
       );
+      expect(
+        engine.check({ name: 'blocked-server__dangerous' }, undefined),
+      ).toBe(PolicyDecision.DENY);
 
       // Should not match other patterns
-      expect(engine.check({ name: 'other-server__tool' })).toBe(
+      expect(engine.check({ name: 'other-server__tool' }, undefined)).toBe(
         PolicyDecision.ASK_USER,
       );
-      expect(engine.check({ name: 'my-server-tool' })).toBe(
+      expect(engine.check({ name: 'my-server-tool' }, undefined)).toBe(
         PolicyDecision.ASK_USER,
       ); // No __ separator
-      expect(engine.check({ name: 'my-server' })).toBe(PolicyDecision.ASK_USER); // No tool name
+      expect(engine.check({ name: 'my-server' }, undefined)).toBe(
+        PolicyDecision.ASK_USER,
+      ); // No tool name
     });
 
     it('should prioritize specific tool rules over server wildcards', () => {
@@ -277,10 +297,62 @@ describe('PolicyEngine', () => {
       engine = new PolicyEngine({ rules });
 
       // Specific tool deny should override server allow
-      expect(engine.check({ name: 'my-server__dangerous-tool' })).toBe(
-        PolicyDecision.DENY,
+      expect(
+        engine.check({ name: 'my-server__dangerous-tool' }, undefined),
+      ).toBe(PolicyDecision.DENY);
+      expect(engine.check({ name: 'my-server__safe-tool' }, undefined)).toBe(
+        PolicyDecision.ALLOW,
       );
-      expect(engine.check({ name: 'my-server__safe-tool' })).toBe(
+    });
+
+    it('should NOT match spoofed server names when using wildcards', () => {
+      // Vulnerability: A rule for 'prefix__*' matches 'prefix__suffix__tool'
+      // effectively allowing a server named 'prefix__suffix' to spoof 'prefix'.
+      const rules: PolicyRule[] = [
+        {
+          toolName: 'safe_server__*',
+          decision: PolicyDecision.ALLOW,
+        },
+      ];
+      engine = new PolicyEngine({ rules });
+
+      // A tool from a different server 'safe_server__malicious'
+      const spoofedToolCall = { name: 'safe_server__malicious__tool' };
+
+      // CURRENT BEHAVIOR (FIXED): Matches because it starts with 'safe_server__' BUT serverName doesn't match 'safe_server'
+      // We expect this to FAIL matching the ALLOW rule, thus falling back to default (ASK_USER)
+      expect(engine.check(spoofedToolCall, 'safe_server__malicious')).toBe(
+        PolicyDecision.ASK_USER,
+      );
+    });
+
+    it('should verify tool name prefix even if serverName matches', () => {
+      const rules: PolicyRule[] = [
+        {
+          toolName: 'safe_server__*',
+          decision: PolicyDecision.ALLOW,
+        },
+      ];
+      engine = new PolicyEngine({ rules });
+
+      // serverName matches, but tool name does not start with prefix
+      const invalidToolCall = { name: 'other_server__tool' };
+      expect(engine.check(invalidToolCall, 'safe_server')).toBe(
+        PolicyDecision.ASK_USER,
+      );
+    });
+
+    it('should allow when both serverName and tool name prefix match', () => {
+      const rules: PolicyRule[] = [
+        {
+          toolName: 'safe_server__*',
+          decision: PolicyDecision.ALLOW,
+        },
+      ];
+      engine = new PolicyEngine({ rules });
+
+      const validToolCall = { name: 'safe_server__tool' };
+      expect(engine.check(validToolCall, 'safe_server')).toBe(
         PolicyDecision.ALLOW,
       );
     });
@@ -302,17 +374,19 @@ describe('PolicyEngine', () => {
       engine = new PolicyEngine({ rules });
 
       // Matches highest priority rule (ls command)
-      expect(engine.check({ name: 'shell', args: { command: 'ls -la' } })).toBe(
-        PolicyDecision.ALLOW,
-      );
+      expect(
+        engine.check({ name: 'shell', args: { command: 'ls -la' } }, undefined),
+      ).toBe(PolicyDecision.ALLOW);
 
       // Matches middle priority rule (shell without ls)
-      expect(engine.check({ name: 'shell', args: { command: 'pwd' } })).toBe(
-        PolicyDecision.ASK_USER,
-      );
+      expect(
+        engine.check({ name: 'shell', args: { command: 'pwd' } }, undefined),
+      ).toBe(PolicyDecision.ASK_USER);
 
       // Matches lowest priority rule (not shell)
-      expect(engine.check({ name: 'edit' })).toBe(PolicyDecision.DENY);
+      expect(engine.check({ name: 'edit' }, undefined)).toBe(
+        PolicyDecision.DENY,
+      );
     });
 
     it('should handle tools with no args', () => {
@@ -327,17 +401,19 @@ describe('PolicyEngine', () => {
       engine = new PolicyEngine({ rules });
 
       // Tool call without args should not match pattern
-      expect(engine.check({ name: 'read' })).toBe(PolicyDecision.ASK_USER);
-
-      // Tool call with args not matching pattern
-      expect(engine.check({ name: 'read', args: { file: 'public.txt' } })).toBe(
+      expect(engine.check({ name: 'read' }, undefined)).toBe(
         PolicyDecision.ASK_USER,
       );
 
+      // Tool call with args not matching pattern
+      expect(
+        engine.check({ name: 'read', args: { file: 'public.txt' } }, undefined),
+      ).toBe(PolicyDecision.ASK_USER);
+
       // Tool call with args matching pattern
-      expect(engine.check({ name: 'read', args: { file: 'secret.txt' } })).toBe(
-        PolicyDecision.DENY,
-      );
+      expect(
+        engine.check({ name: 'read', args: { file: 'secret.txt' } }, undefined),
+      ).toBe(PolicyDecision.DENY);
     });
 
     it('should match args pattern regardless of property order', () => {
@@ -356,16 +432,16 @@ describe('PolicyEngine', () => {
       const args1 = { command: 'rm -rf /', path: '/home' };
       const args2 = { path: '/home', command: 'rm -rf /' };
 
-      expect(engine.check({ name: 'shell', args: args1 })).toBe(
+      expect(engine.check({ name: 'shell', args: args1 }, undefined)).toBe(
         PolicyDecision.DENY,
       );
-      expect(engine.check({ name: 'shell', args: args2 })).toBe(
+      expect(engine.check({ name: 'shell', args: args2 }, undefined)).toBe(
         PolicyDecision.DENY,
       );
 
       // Verify safe command doesn't match
       const safeArgs = { command: 'ls -la', path: '/home' };
-      expect(engine.check({ name: 'shell', args: safeArgs })).toBe(
+      expect(engine.check({ name: 'shell', args: safeArgs }, undefined)).toBe(
         PolicyDecision.ASK_USER,
       );
     });
@@ -391,10 +467,10 @@ describe('PolicyEngine', () => {
         data: { value: 'secret', sensitive: true },
       };
 
-      expect(engine.check({ name: 'api', args: args1 })).toBe(
+      expect(engine.check({ name: 'api', args: args1 }, undefined)).toBe(
         PolicyDecision.DENY,
       );
-      expect(engine.check({ name: 'api', args: args2 })).toBe(
+      expect(engine.check({ name: 'api', args: args2 }, undefined)).toBe(
         PolicyDecision.DENY,
       );
     });
@@ -424,17 +500,17 @@ describe('PolicyEngine', () => {
 
       // Should not throw stack overflow error
       expect(() =>
-        engine.check({ name: 'test', args: circularArgs }),
+        engine.check({ name: 'test', args: circularArgs }, undefined),
       ).not.toThrow();
 
       // Should detect the circular reference pattern
-      expect(engine.check({ name: 'test', args: circularArgs })).toBe(
-        PolicyDecision.DENY,
-      );
+      expect(
+        engine.check({ name: 'test', args: circularArgs }, undefined),
+      ).toBe(PolicyDecision.DENY);
 
       // Non-circular object should not match
       const normalArgs = { name: 'test', data: { value: 'normal' } };
-      expect(engine.check({ name: 'test', args: normalArgs })).toBe(
+      expect(engine.check({ name: 'test', args: normalArgs }, undefined)).toBe(
         PolicyDecision.ASK_USER,
       );
     });
@@ -471,13 +547,13 @@ describe('PolicyEngine', () => {
 
       // Should handle without stack overflow
       expect(() =>
-        engine.check({ name: 'deep', args: deepCircular }),
+        engine.check({ name: 'deep', args: deepCircular }, undefined),
       ).not.toThrow();
 
       // Should detect the circular reference
-      expect(engine.check({ name: 'deep', args: deepCircular })).toBe(
-        PolicyDecision.DENY,
-      );
+      expect(
+        engine.check({ name: 'deep', args: deepCircular }, undefined),
+      ).toBe(PolicyDecision.DENY);
     });
 
     it('should handle repeated non-circular objects correctly', () => {
@@ -506,7 +582,9 @@ describe('PolicyEngine', () => {
       };
 
       // Should NOT mark repeated objects as circular, and should match the shared value pattern
-      expect(engine.check({ name: 'test', args })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
     });
 
     it('should omit undefined and function values from objects', () => {
@@ -528,7 +606,9 @@ describe('PolicyEngine', () => {
       };
 
       // Should match pattern with defined value, undefined and functions omitted
-      expect(engine.check({ name: 'test', args })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
 
       // Check that the pattern would NOT match if undefined was included
       const rulesWithUndefined: PolicyRule[] = [
@@ -539,7 +619,7 @@ describe('PolicyEngine', () => {
         },
       ];
       engine = new PolicyEngine({ rules: rulesWithUndefined });
-      expect(engine.check({ name: 'test', args })).toBe(
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
         PolicyDecision.ASK_USER,
       );
 
@@ -552,7 +632,7 @@ describe('PolicyEngine', () => {
         },
       ];
       engine = new PolicyEngine({ rules: rulesWithFunction });
-      expect(engine.check({ name: 'test', args })).toBe(
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
         PolicyDecision.ASK_USER,
       );
     });
@@ -573,7 +653,9 @@ describe('PolicyEngine', () => {
       };
 
       // Should match pattern with undefined and functions converted to null
-      expect(engine.check({ name: 'test', args })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
     });
 
     it('should produce valid JSON for all inputs', () => {
@@ -607,10 +689,12 @@ describe('PolicyEngine', () => {
         engine = new PolicyEngine({ rules });
 
         // Should not throw when checking (which internally uses stableStringify)
-        expect(() => engine.check({ name: 'test', args: input })).not.toThrow();
+        expect(() =>
+          engine.check({ name: 'test', args: input }, undefined),
+        ).not.toThrow();
 
         // The check should succeed
-        expect(engine.check({ name: 'test', args: input })).toBe(
+        expect(engine.check({ name: 'test', args: input }, undefined)).toBe(
           PolicyDecision.ALLOW,
         );
       }
@@ -641,7 +725,9 @@ describe('PolicyEngine', () => {
       };
 
       // Should match the sanitized pattern, not the dangerous one
-      expect(engine.check({ name: 'test', args })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
     });
 
     it('should handle toJSON that returns primitives', () => {
@@ -663,7 +749,9 @@ describe('PolicyEngine', () => {
       };
 
       // toJSON returns a string, which should be properly stringified
-      expect(engine.check({ name: 'test', args })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
     });
 
     it('should handle toJSON that throws an error', () => {
@@ -687,7 +775,23 @@ describe('PolicyEngine', () => {
       };
 
       // Should fall back to regular object serialization when toJSON throws
-      expect(engine.check({ name: 'test', args })).toBe(PolicyDecision.ALLOW);
+      expect(engine.check({ name: 'test', args }, undefined)).toBe(
+        PolicyDecision.ALLOW,
+      );
+    });
+  });
+
+  describe('serverName requirement', () => {
+    it('should require serverName for checks', () => {
+      // @ts-expect-error - intentionally testing missing serverName
+      expect(engine.check({ name: 'test' })).toBe(PolicyDecision.ASK_USER);
+      // When serverName is provided (even undefined), it should work
+      expect(engine.check({ name: 'test' }, undefined)).toBe(
+        PolicyDecision.ASK_USER,
+      );
+      expect(engine.check({ name: 'test' }, 'some-server')).toBe(
+        PolicyDecision.ASK_USER,
+      );
     });
   });
 });

packages/core/src/policy/policy-engine.ts

@@ -17,12 +17,21 @@ function ruleMatches(
   rule: PolicyRule,
   toolCall: FunctionCall,
   stringifiedArgs: string | undefined,
+  serverName: string | undefined,
 ): boolean {
   // Check tool name if specified
   if (rule.toolName) {
     // Support wildcard patterns: "serverName__*" matches "serverName__anyTool"
     if (rule.toolName.endsWith('__*')) {
       const prefix = rule.toolName.slice(0, -3); // Remove "__*"
+      if (serverName !== undefined) {
+        // Robust check: if serverName is provided, it MUST match the prefix exactly.
+        // This prevents "malicious-server" from spoofing "trusted-server" by naming itself "trusted-server__malicious".
+        if (serverName !== prefix) {
+          return false;
+        }
+      }
+      // Always verify the prefix, even if serverName matched
       if (!toolCall.name || !toolCall.name.startsWith(prefix + '__')) {
         return false;
       }
@@ -65,7 +74,10 @@ export class PolicyEngine {
   /**
    * Check if a tool call is allowed based on the configured policies.
    */
-  check(toolCall: FunctionCall): PolicyDecision {
+  check(
+    toolCall: FunctionCall,
+    serverName: string | undefined,
+  ): PolicyDecision {
     let stringifiedArgs: string | undefined;
     // Compute stringified args once before the loop
     if (toolCall.args && this.rules.some((rule) => rule.argsPattern)) {
@@ -78,7 +90,7 @@ export class PolicyEngine {
 
     // Find the first matching rule (already sorted by priority)
     for (const rule of this.rules) {
-      if (ruleMatches(rule, toolCall, stringifiedArgs)) {
+      if (ruleMatches(rule, toolCall, stringifiedArgs, serverName)) {
         debugLogger.debug(
           `[PolicyEngine.check] MATCHED rule: toolName=${rule.toolName}, decision=${rule.decision}, priority=${rule.priority}, argsPattern=${rule.argsPattern?.source || 'none'}`,
         );