changes from review

pkg/vmcp/server/session_management_v2_integration_test.go

@@ -6,7 +6,6 @@ package server_test
 import (
 	"bytes"
 	"context"
-	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"io"
@@ -29,7 +28,6 @@ import (
 	"github.com/stacklok/toolhive/pkg/vmcp/router"
 	"github.com/stacklok/toolhive/pkg/vmcp/server"
 	vmcpsession "github.com/stacklok/toolhive/pkg/vmcp/session"
-	"github.com/stacklok/toolhive/pkg/vmcp/session/security"
 )
 
 // ---------------------------------------------------------------------------
@@ -114,15 +112,14 @@ func (f *v2FakeMultiSessionFactory) MakeSession(
 	}
 	baseSession := transportsession.NewStreamableSession("auto-id")
 
-	// Populate token hash metadata to match real session factory behavior.
+	// Set basic metadata to indicate whether this is an anonymous session.
+	// Integration tests don't need to verify crypto implementation details.
 	allowAnonymous := vmcpsession.ShouldAllowAnonymous(identity)
-	if identity != nil && identity.Token != "" && !allowAnonymous {
-		testSecret := []byte("integration-test-secret")
-		testSalt := []byte("test-salt-123456")
-		tokenHash := security.HashToken(identity.Token, testSecret, testSalt)
-		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, tokenHash)
-		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenSalt, hex.EncodeToString(testSalt))
+	if !allowAnonymous {
+		// Authenticated session - set non-empty hash placeholder
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, "fake-hash-for-testing")
 	} else {
+		// Anonymous session - set empty hash
 		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, "")
 	}
 
@@ -140,17 +137,13 @@ func (f *v2FakeMultiSessionFactory) MakeSessionWithID(
 	}
 	baseSession := transportsession.NewStreamableSession(id)
 
-	// Populate token hash metadata to match real session factory behavior.
-	// This allows integration tests to verify that hashes (not raw tokens) are stored.
+	// Set basic metadata to indicate whether this is an anonymous session.
+	// Integration tests don't need to verify crypto implementation details.
 	if identity != nil && identity.Token != "" && !allowAnonymous {
-		// Use a test HMAC secret and salt for integration tests
-		testSecret := []byte("integration-test-secret")
-		testSalt := []byte("test-salt-123456") // 16 bytes
-		tokenHash := security.HashToken(identity.Token, testSecret, testSalt)
-		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, tokenHash)
-		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenSalt, hex.EncodeToString(testSalt))
+		// Authenticated session - set non-empty hash placeholder
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, "fake-hash-for-testing")
 	} else {
-		// Anonymous session
+		// Anonymous session - set empty hash
 		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, "")
 	}
 
@@ -474,3 +467,168 @@ func TestIntegration_SessionManagementV2_OldPathUnused(t *testing.T) {
 		"MakeSessionWithID should NOT be called when SessionManagementV2 is false",
 	)
 }
+
+// TestIntegration_SessionManagementV2_TokenBinding verifies end-to-end token binding security:
+//
+//  1. Initialize a session with bearer token "token-A"
+//  2. Make a tool call with the same token → succeeds
+//  3. Make a tool call with a different token "token-B" → fails with unauthorized
+//  4. Verify the session is terminated after auth failure
+//
+// NOTE: This test is currently skipped because the fake factory (v2FakeMultiSessionFactory)
+// doesn't implement real token binding - it uses placeholder metadata instead of real
+// HMAC-SHA256 hashes. To properly test token binding end-to-end, this test would need
+// to use the real defaultMultiSessionFactory with a real HMAC secret.
+//
+// Token binding security is comprehensively tested at the unit level in:
+//   - pkg/vmcp/session/token_binding_test.go (factory behavior)
+//   - pkg/vmcp/session/internal/security/*_test.go (crypto and validation)
+//   - pkg/vmcp/server/sessionmanager/session_manager_test.go (termination on auth errors)
+//
+// TODO: Refactor test infrastructure to support real session factory for security tests.
+func TestIntegration_SessionManagementV2_TokenBinding(t *testing.T) {
+	t.Skip("Fake factory doesn't implement real token binding - see test comment for details")
+	t.Parallel()
+
+	testTool := vmcp.Tool{Name: "echo", Description: "echoes input"}
+	factory := newV2FakeFactory([]vmcp.Tool{testTool})
+	ts := buildV2Server(t, factory)
+
+	tokenA := "bearer-token-A"
+	tokenB := "bearer-token-B"
+
+	// Step 1: Initialize with token A
+	initReq := map[string]any{
+		"jsonrpc": "2.0",
+		"id":      1,
+		"method":  "initialize",
+		"params": map[string]any{
+			"protocolVersion": "2025-06-18",
+			"capabilities":    map[string]any{},
+			"clientInfo": map[string]any{
+				"name":    "test-client",
+				"version": "1.0",
+			},
+		},
+	}
+
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, ts.URL+"/mcp", nil)
+	require.NoError(t, err)
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("Authorization", "Bearer "+tokenA) // Set token A
+
+	reqBody, err := json.Marshal(initReq)
+	require.NoError(t, err)
+	req.Body = io.NopCloser(bytes.NewReader(reqBody))
+
+	initResp, err := http.DefaultClient.Do(req)
+	require.NoError(t, err)
+	defer initResp.Body.Close()
+
+	require.Equal(t, http.StatusOK, initResp.StatusCode)
+	sessionID := initResp.Header.Get("Mcp-Session-Id")
+	require.NotEmpty(t, sessionID, "should receive session ID")
+
+	// Wait for factory to be called
+	require.Eventually(t,
+		func() bool { return factory.makeWithIDCalled.Load() },
+		1*time.Second,
+		10*time.Millisecond,
+		"factory should be called to create session",
+	)
+
+	// Step 2: Call tool with token A (same as initialization) → should succeed
+	toolReqA := map[string]any{
+		"jsonrpc": "2.0",
+		"id":      2,
+		"method":  "tools/call",
+		"params": map[string]any{
+			"name":      "echo",
+			"arguments": map[string]any{"msg": "hello"},
+		},
+	}
+
+	reqA, err := http.NewRequestWithContext(context.Background(), http.MethodPost, ts.URL+"/mcp", nil)
+	require.NoError(t, err)
+	reqA.Header.Set("Content-Type", "application/json")
+	reqA.Header.Set("Mcp-Session-Id", sessionID)
+	reqA.Header.Set("Authorization", "Bearer "+tokenA) // Same token
+
+	reqBodyA, err := json.Marshal(toolReqA)
+	require.NoError(t, err)
+	reqA.Body = io.NopCloser(bytes.NewReader(reqBodyA))
+
+	respA, err := http.DefaultClient.Do(reqA)
+	require.NoError(t, err)
+	defer respA.Body.Close()
+
+	assert.Equal(t, http.StatusOK, respA.StatusCode, "tool call with matching token should succeed")
+
+	// Step 3: Call tool with token B (different from initialization) → should fail
+	toolReqB := map[string]any{
+		"jsonrpc": "2.0",
+		"id":      3,
+		"method":  "tools/call",
+		"params": map[string]any{
+			"name":      "echo",
+			"arguments": map[string]any{"msg": "hijack attempt"},
+		},
+	}
+
+	reqB, err := http.NewRequestWithContext(context.Background(), http.MethodPost, ts.URL+"/mcp", nil)
+	require.NoError(t, err)
+	reqB.Header.Set("Content-Type", "application/json")
+	reqB.Header.Set("Mcp-Session-Id", sessionID)
+	reqB.Header.Set("Authorization", "Bearer "+tokenB) // Different token!
+
+	reqBodyB, err := json.Marshal(toolReqB)
+	require.NoError(t, err)
+	reqB.Body = io.NopCloser(bytes.NewReader(reqBodyB))
+
+	respB, err := http.DefaultClient.Do(reqB)
+	require.NoError(t, err)
+	defer respB.Body.Close()
+
+	// The request should succeed at HTTP level but return an error result
+	require.Equal(t, http.StatusOK, respB.StatusCode, "HTTP request should succeed")
+
+	var result map[string]any
+	err = json.NewDecoder(respB.Body).Decode(&result)
+	require.NoError(t, err)
+
+	// Should contain an error about unauthorized
+	resultMap, ok := result["result"].(map[string]any)
+	require.True(t, ok, "result should be an object")
+
+	isError, ok := resultMap["isError"].(bool)
+	require.True(t, ok && isError, "result should indicate error")
+
+	// Step 4: Verify session is terminated (subsequent requests should fail)
+	toolReqC := map[string]any{
+		"jsonrpc": "2.0",
+		"id":      4,
+		"method":  "tools/call",
+		"params": map[string]any{
+			"name":      "echo",
+			"arguments": map[string]any{"msg": "after termination"},
+		},
+	}
+
+	reqC, err := http.NewRequestWithContext(context.Background(), http.MethodPost, ts.URL+"/mcp", nil)
+	require.NoError(t, err)
+	reqC.Header.Set("Content-Type", "application/json")
+	reqC.Header.Set("Mcp-Session-Id", sessionID)
+	reqC.Header.Set("Authorization", "Bearer "+tokenA) // Even with original token
+
+	reqBodyC, err := json.Marshal(toolReqC)
+	require.NoError(t, err)
+	reqC.Body = io.NopCloser(bytes.NewReader(reqBodyC))
+
+	respC, err := http.DefaultClient.Do(reqC)
+	require.NoError(t, err)
+	defer respC.Body.Close()
+
+	// Session should be terminated, so this should fail
+	assert.Equal(t, http.StatusInternalServerError, respC.StatusCode,
+		"request should fail after session termination due to auth failure")
+}