fixes from review

Author: taskbot

pkg/vmcp/server/session_management_v2_integration_test.go

@@ -6,6 +6,7 @@ package server_test
 import (
 	"bytes"
 	"context"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"io"
@@ -105,25 +106,53 @@ func newV2FakeFactory(tools []vmcp.Tool) *v2FakeMultiSessionFactory {
 }
 
 func (f *v2FakeMultiSessionFactory) MakeSession(
-	_ context.Context, _ *auth.Identity, _ []*vmcp.Backend,
+	_ context.Context, identity *auth.Identity, _ []*vmcp.Backend,
 ) (vmcpsession.MultiSession, error) {
 	if f.err != nil {
 		return nil, f.err
 	}
 	baseSession := transportsession.NewStreamableSession("auto-id")
+
+	// Populate token hash metadata to match real session factory behavior.
+	allowAnonymous := vmcpsession.ShouldAllowAnonymous(identity)
+	if identity != nil && identity.Token != "" && !allowAnonymous {
+		testSecret := []byte("integration-test-secret")
+		testSalt := []byte("test-salt-123456")
+		tokenHash := vmcpsession.HashToken(identity.Token, testSecret, testSalt)
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, tokenHash)
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenSalt, hex.EncodeToString(testSalt))
+	} else {
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, "")
+	}
+
 	sess := newV2FakeMultiSession(baseSession, f.tools)
 	f.lastCreatedSession = sess
 	return sess, nil
 }
 
 func (f *v2FakeMultiSessionFactory) MakeSessionWithID(
-	_ context.Context, id string, _ *auth.Identity, _ bool, _ []*vmcp.Backend,
+	_ context.Context, id string, identity *auth.Identity, allowAnonymous bool, _ []*vmcp.Backend,
 ) (vmcpsession.MultiSession, error) {
 	f.makeWithIDCalled.Store(true)
 	if f.err != nil {
 		return nil, f.err
 	}
 	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.
+	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 := vmcpsession.HashToken(identity.Token, testSecret, testSalt)
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, tokenHash)
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenSalt, hex.EncodeToString(testSalt))
+	} else {
+		// Anonymous session
+		baseSession.SetMetadata(vmcpsession.MetadataKeyTokenHash, "")
+	}
+
 	sess := newV2FakeMultiSession(baseSession, f.tools)
 	f.lastCreatedSession = sess
 	return sess, nil

pkg/vmcp/server/sessionmanager/session_manager.go

@@ -165,7 +165,7 @@ func (sm *Manager) CreateSession(
 	// Build the fully-formed MultiSession using the SDK-assigned session ID.
 	// Sessions created with an identity are bound to that identity (allowAnonymous=false).
 	// Sessions created without an identity allow anonymous access (allowAnonymous=true).
-	allowAnonymous := identity == nil || identity.Token == ""
+	allowAnonymous := vmcpsession.ShouldAllowAnonymous(identity)
 	sess, err := sm.factory.MakeSessionWithID(ctx, sessionID, identity, allowAnonymous, backends)
 	if err != nil {
 		return nil, fmt.Errorf("Manager.CreateSession: failed to create multi-session: %w", err)

pkg/vmcp/session/factory.go

@@ -270,7 +270,7 @@ func (f *defaultMultiSessionFactory) MakeSession(
 ) (MultiSession, error) {
 	// Sessions created with an identity are bound to that identity (allowAnonymous=false).
 	// Sessions created without an identity allow anonymous access (allowAnonymous=true).
-	allowAnonymous := security.ShouldAllowAnonymous(identity)
+	allowAnonymous := ShouldAllowAnonymous(identity)
 	return f.makeSession(ctx, uuid.New().String(), identity, allowAnonymous, backends)
 }
 

pkg/vmcp/session/internal/security/security.go

@@ -40,17 +40,6 @@ const (
 	MetadataKeyTokenSalt = "vmcp.token.salt" //nolint:gosec // This is a metadata key name, not a credential.
 )
 
-// ShouldAllowAnonymous determines if a session should allow anonymous access
-// based on the creator's identity. Sessions without an identity (nil) or with
-// an empty token are anonymous; sessions with a non-empty bearer token are
-// bound to that token.
-//
-// This helper consolidates the anonymous session logic and aligns with the
-// validation logic in PreventSessionHijacking.
-func ShouldAllowAnonymous(identity *auth.Identity) bool {
-	return identity == nil || identity.Token == ""
-}
-
 // GenerateSalt generates a cryptographically secure random salt for token hashing.
 // Returns 16 bytes of random data from crypto/rand.
 //

pkg/vmcp/session/internal/security/security_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/stacklok/toolhive/pkg/auth"
+	"github.com/stacklok/toolhive/pkg/vmcp/session"
 	"github.com/stacklok/toolhive/pkg/vmcp/session/internal/security"
 )
 
@@ -306,7 +307,7 @@ func TestShouldAllowAnonymous_EdgeCases(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			got := security.ShouldAllowAnonymous(tt.identity)
+			got := session.ShouldAllowAnonymous(tt.identity)
 			assert.Equal(t, tt.want, got)
 		})
 	}

pkg/vmcp/session/session.go

@@ -4,8 +4,10 @@
 package session
 
 import (
+	"github.com/stacklok/toolhive/pkg/auth"
 	transportsession "github.com/stacklok/toolhive/pkg/transport/session"
 	"github.com/stacklok/toolhive/pkg/vmcp"
+	"github.com/stacklok/toolhive/pkg/vmcp/session/internal/security"
 	sessiontypes "github.com/stacklok/toolhive/pkg/vmcp/session/types"
 )
 
@@ -58,3 +60,43 @@ type MultiSession interface {
 	// sessions for debugging and auditing.
 	BackendSessions() map[string]string
 }
+
+const (
+	// MetadataKeyTokenHash is the session metadata key that holds the HMAC-SHA256
+	// hash of the bearer token used to create the session. For authenticated sessions
+	// this is hex(HMAC-SHA256(bearerToken)). For anonymous sessions this is the empty
+	// string sentinel. The raw token is never stored — only the hash.
+	MetadataKeyTokenHash = security.MetadataKeyTokenHash
+
+	// MetadataKeyTokenSalt is the session metadata key that holds the hex-encoded
+	// random salt used for HMAC-SHA256 token hashing. Each session has a unique salt
+	// to prevent attacks across multiple sessions.
+	MetadataKeyTokenSalt = security.MetadataKeyTokenSalt
+)
+
+// ShouldAllowAnonymous determines if a session should allow anonymous access
+// based on the creator's identity. Sessions without an identity (nil) or with
+// an empty token are anonymous; sessions with a non-empty bearer token are
+// bound to that token.
+//
+// This function consolidates the anonymous session logic and aligns with the
+// validation logic in session hijacking prevention.
+func ShouldAllowAnonymous(identity *auth.Identity) bool {
+	return identity == nil || identity.Token == ""
+}
+
+// HashToken returns the hex-encoded HMAC-SHA256 hash of a raw bearer token string.
+// Uses HMAC with a server-managed secret and per-session salt to prevent offline
+// attacks if session storage is compromised.
+//
+// For empty tokens (anonymous sessions) it returns the empty string, which is
+// the sentinel value used to identify sessions created without credentials.
+// The raw token is never stored — only the hash.
+//
+// Parameters:
+//   - token: The bearer token to hash
+//   - secret: Server-managed HMAC secret (should be 32+ bytes)
+//   - salt: Per-session random salt (typically 16 bytes)
+func HashToken(token string, secret, salt []byte) string {
+	return security.HashToken(token, secret, salt)
+}