Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions framework/lrucache/lrucache.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import (
"container/list"
"context"
"fmt"
"strconv"
"strings"
"sync"
)

Expand Down Expand Up @@ -397,3 +399,56 @@ func (c *Cache[V]) Len() int {
defer c.mu.Unlock()
return c.order.Len()
}

// EncodeKey builds a collision-free composite cache key from parts, for
// callers whose key is a tuple of caller-controlled strings (e.g.
// (auth mode, identity, mcp client ID) — identity is frequently a
// caller-asserted string with no charset restriction). A naive
// separator-joined key lets one part's content forge a boundary — e.g.
// join("\x00", "a\x00b", "c") and join("\x00", "a", "b\x00c") would build
// the identical string, aliasing two distinct tuples onto one cache entry
// and skewing any eviction predicate that parses the key back apart.
// Length-prefixing each part makes that forgery impossible regardless of
// what bytes a part contains. Pair with DecodeKey to parse it back.
func EncodeKey(parts ...string) string {
var b strings.Builder
for _, p := range parts {
b.WriteString(strconv.Itoa(len(p)))
b.WriteByte(':')
b.WriteString(p)
}
return b.String()
}

// DecodeKey is EncodeKey's inverse: it parses exactly n length-prefixed
// parts out of key, in the order EncodeKey wrote them. ok is false for a
// key that isn't in the length-prefixed form EncodeKey builds for n parts
// (impossible for keys a well-behaved caller produces with EncodeKey).
func DecodeKey(key string, n int) (parts []string, ok bool) {
if n < 0 {
return nil, false
}
rest := key
parts = make([]string, 0, n)
Comment on lines +427 to +432

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Reject infeasible part counts before allocation.

Every encoded part consumes at least two bytes. A valid key requires n <= len(key)/2.

DecodeKey("", 1<<30) currently attempts a large allocation before it returns false. This can panic or exhaust memory if a caller passes an untrusted count. Reject the count before make, and add a regression test.

Proposed fix
 func DecodeKey(key string, n int) (parts []string, ok bool) {
-	if n < 0 {
+	if n < 0 || n > len(key)/2 {
 		return nil, false
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func DecodeKey(key string, n int) (parts []string, ok bool) {
if n < 0 {
return nil, false
}
rest := key
parts = make([]string, 0, n)
func DecodeKey(key string, n int) (parts []string, ok bool) {
if n < 0 || n > len(key)/2 {
return nil, false
}
rest := key
parts = make([]string, 0, n)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@framework/lrucache/lrucache.go` around lines 427 - 432, Update DecodeKey to
reject any n greater than len(key)/2 before allocating the parts slice, while
preserving the existing negative-count rejection and valid decoding behavior.
Add a regression test covering an oversized count, such as DecodeKey("", 1<<30),
and verify it returns false without attempting a large allocation.

for range n {
i := strings.IndexByte(rest, ':')
if i < 0 {
return nil, false
}
lengthText := rest[:i]
length, err := strconv.Atoi(lengthText)
if err != nil || length < 0 || strconv.Itoa(length) != lengthText {
return nil, false
}
rest = rest[i+1:]
if length > len(rest) {
return nil, false
}
parts = append(parts, rest[:length])
rest = rest[length:]
}
if rest != "" {
return nil, false
}
return parts, true
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
51 changes: 51 additions & 0 deletions framework/lrucache/lrucache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -590,3 +590,54 @@ func TestNew_PanicsOnNonPositiveCapacity(t *testing.T) {
assert.Panics(t, func() { New[string](0) })
assert.Panics(t, func() { New[string](-1) })
}

func TestEncodeDecodeKey_RoundTrip(t *testing.T) {
parts := []string{"user", "alice", "client-1"}
key := EncodeKey(parts...)
got, ok := DecodeKey(key, len(parts))
require.True(t, ok)
assert.Equal(t, parts, got)
}

// TestEncodeDecodeKey_NoCollisionOnEmbeddedDelimiter pins the reason this
// codec exists over a plain separator join: a value that happens to
// contain the separator (or, here, digits and a colon shaped like a length
// prefix) must not let one tuple's key collide with a different tuple's.
func TestEncodeDecodeKey_NoCollisionOnEmbeddedDelimiter(t *testing.T) {
keyA := EncodeKey("user", "a\x00b", "c")
keyB := EncodeKey("user", "a", "b\x00c")
assert.NotEqual(t, keyA, keyB, "distinct tuples must not alias to the same key")

gotA, ok := DecodeKey(keyA, 3)
require.True(t, ok)
assert.Equal(t, []string{"user", "a\x00b", "c"}, gotA)

gotB, ok := DecodeKey(keyB, 3)
require.True(t, ok)
assert.Equal(t, []string{"user", "a", "b\x00c"}, gotB)
}

func TestDecodeKey_RejectsMalformedInput(t *testing.T) {
tests := []struct {
name string
key string
n int
}{
{"empty string", "", 3},
{"no colon", "abc", 1},
{"non-numeric length", "x:abc", 1},
{"negative length", "-1:a", 1},
{"length exceeds remaining bytes", "10:ab", 1},
{"trailing garbage after all parts", "1:a1:btrailing", 2},
{"too few parts", "4:user", 2},
{"negative part count", "1:a", -1},
{"non-canonical length prefix with leading plus", "+1:a", 1},
{"non-canonical length prefix with leading zero", "01:a", 1},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
_, ok := DecodeKey(tc.key, tc.n)
assert.False(t, ok)
})
}
}
165 changes: 165 additions & 0 deletions framework/mcp_headers/credentialcache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
package mcp_headers

import (
"context"

"github.com/maximhq/bifrost/core/schemas"
"github.com/maximhq/bifrost/framework/lrucache"
)

// defaultCredentialCacheCapacity bounds the per-user header credential cache.
// Session-mode identities are caller-asserted strings, so the cache must stay
// bounded no matter what identities callers present; least-recently-used
// entries are dropped once the capacity is reached.
const defaultCredentialCacheCapacity = 4096

// cachedHeaderCredential is the value cached per (auth mode, identity,
// mcp client) binding: the row ID (for targeted eviction) and the parsed
// credential. Header credentials carry no expiry and have no refresh
// machinery, so there is no expiry-as-miss logic; explicit eviction is the
// only way a cached entry stops being served. The credential pointer is
// private to the cache: the provider hands callers a deep copy so a caller
// mutating the returned Headers map can never corrupt the cached value.
type cachedHeaderCredential struct {
credentialID string
credential *schemas.MCPHeadersUserCredential
}

// headerCredentialCache adapts lrucache.Cache to per-user MCP header
// credential lookups: it owns the binding-key scheme, registers each entry
// under its credential row ID for targeted eviction (upsert, delete,
// revoke), and provides the scoped bulk evictions the credential lifecycle
// needs (by MCP client, virtual key, and user). No validator is installed:
// header credentials never expire, so explicit eviction is the only
// invalidation.
type headerCredentialCache struct {
cache *lrucache.Cache[cachedHeaderCredential]
}

// headerCredentialCacheKey builds the cache key for a (mode, identity,
// mcp client) binding. identity is a caller-asserted string with no
// charset restriction, so this goes through lrucache.EncodeKey rather than
// a plain separator join — see its doc comment for why a naive join lets
// an identity value forge a component boundary and alias two distinct
// bindings onto one cache entry. Admin-mode bindings carry an empty
// identity component by design.
func headerCredentialCacheKey(mode schemas.MCPAuthMode, identity, mcpClientID string) string {
return lrucache.EncodeKey(string(mode), identity, mcpClientID)
}

// splitHeaderCredentialCacheKey is headerCredentialCacheKey's inverse, for
// the scoped eviction predicates.
func splitHeaderCredentialCacheKey(key string) (mode, identity, clientID string, ok bool) {
parts, ok := lrucache.DecodeKey(key, 3)
if !ok {
return "", "", "", false
}
return parts[0], parts[1], parts[2], true
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func newHeaderCredentialCache(capacity int) *headerCredentialCache {
if capacity <= 0 {
capacity = defaultCredentialCacheCapacity
}
return &headerCredentialCache{cache: lrucache.New[cachedHeaderCredential](capacity)}
}

// Get returns the cached value for key. Header credentials have no expiry,
// so a hit is served as-is until an eviction removes it.
func (c *headerCredentialCache) Get(key string) (cachedHeaderCredential, bool) {
if c == nil {
return cachedHeaderCredential{}, false
}
return c.cache.Get(key)
}

// Fill runs fill for key with single-flight deduplication: concurrent
// callers for the same key wait for one leader and share its result and
// error, so a tool-call burst for one identity performs a single database
// read instead of a stampede. A successful result is cached under both the
// binding key and its credential row ID; errors are propagated but never
// cached.
func (c *headerCredentialCache) Fill(ctx context.Context, key string, fill func() (cachedHeaderCredential, error)) (cachedHeaderCredential, error) {
if c == nil {
return fill()
}
return c.cache.Fill(ctx, key, func() (cachedHeaderCredential, string, error) {
value, err := fill()
return value, value.credentialID, err
})
}

// Evict removes the entry for an exact binding key, if present.
func (c *headerCredentialCache) Evict(key string) {
if c == nil {
return
}
c.cache.Evict(key)
}

// EvictByCredentialID removes the entry holding the given credential row ID,
// if any.
func (c *headerCredentialCache) EvictByCredentialID(credentialID string) {
if c == nil {
return
}
c.cache.EvictByIndex(credentialID)
}

// EvictByMCPClient removes every cached entry bound to the given MCP client,
// across all auth modes and identities, including the admin-mode binding
// (whose identity component is empty). Used when a client-level change
// invalidates its credential rows as a set, such as a header schema change
// or client deletion. A linear sweep is fine here: these are rare admin
// operations and the cache is bounded.
func (c *headerCredentialCache) EvictByMCPClient(mcpClientID string) {
if c == nil || mcpClientID == "" {
return
}
c.cache.EvictWhere(func(key string) bool {
_, _, clientID, ok := splitHeaderCredentialCacheKey(key)
return ok && clientID == mcpClientID
})
}

// EvictByVirtualKey removes every cached vk-mode entry bound to the given
// virtual key, across all MCP clients. Used when a virtual key change
// orphans or deletes its credential rows as a set.
func (c *headerCredentialCache) EvictByVirtualKey(virtualKeyID string) {
if c == nil || virtualKeyID == "" {
return
}
c.cache.EvictWhere(func(key string) bool {
mode, identity, _, ok := splitHeaderCredentialCacheKey(key)
return ok && mode == string(schemas.MCPAuthModeVK) && identity == virtualKeyID
})
}

// EvictByUser removes every cached user-mode entry bound to the given user,
// across all MCP clients. Used when a user-level change orphans or deletes
// the user's credential rows as a set.
func (c *headerCredentialCache) EvictByUser(userID string) {
if c == nil || userID == "" {
return
}
c.cache.EvictWhere(func(key string) bool {
mode, identity, _, ok := splitHeaderCredentialCacheKey(key)
return ok && mode == string(schemas.MCPAuthModeUser) && identity == userID
})
}

// Flush drops every cached entry.
func (c *headerCredentialCache) Flush() {
if c == nil {
return
}
c.cache.Flush()
}

// Len reports the number of cached entries.
func (c *headerCredentialCache) Len() int {
if c == nil {
return 0
}
return c.cache.Len()
}
Loading
Loading