fix: mcp auth session reconciliation fixes

Author: Pratham-Mishra04

core/mcp/credstore/per_user_headers.go

@@ -108,9 +108,14 @@ func (r *perUserHeadersResolver) buildAuthRequiredError(ctx *schemas.BifrostCont
 }
 
 // missingRequiredHeaderKeys returns the names of any required header key
-// that's absent or whose stored value is empty in storedHeaders. Comparison
-// is case-insensitive at the wire level but the schema is the source of
-// truth — we look up by the exact key the admin declared.
+// that's absent or whose stored value is empty in storedHeaders.
+//
+// Both inputs are assumed to be in canonical form (lowercase + trimmed) —
+// see the invariant doc on mcputils.CanonicalizeHeaderKey. All write
+// boundaries (HTTP create/update, flow submit, config.json load) run
+// the inputs through that helper, so exact map lookup here is correct.
+// Do NOT add defensive case-folding inside this function: it would mask
+// a missed write-side canonicalization rather than catching it.
 func missingRequiredHeaderKeys(required []string, storedHeaders map[string]string) []string {
 	if len(storedHeaders) == 0 {
 		return append([]string(nil), required...)
@@ -128,6 +133,12 @@ func missingRequiredHeaderKeys(required []string, storedHeaders map[string]strin
 // user-submitted credential values for the required keys. Keys not declared
 // by the current schema are dropped on purpose so a stale row that still
 // stores a deprecated key cannot leak it onto the wire.
+//
+// Required keys and storedHeaders keys are both canonical (lowercase +
+// trimmed) by the write-side invariant — see missingRequiredHeaderKeys
+// above and mcputils.CanonicalizeHeaderKey. http.Header.Set runs its own
+// MIME canonicalization on the way out (so "authorization" becomes
+// "Authorization" on the wire), which is what upstream servers expect.
 func buildPerUserHeaderValues(required []string, storedHeaders map[string]string) http.Header {
 	out := http.Header{}
 	for _, key := range required {

core/mcp/utils/utils.go

@@ -25,9 +25,16 @@ func FlattenHeaders(h http.Header) map[string]string {
 // BuildMCPCallbackBaseURL extracts the base URL set on the BifrostContext by
 // the HTTP middleware (e.g. "https://host"). Per-user OAuth and per-user
 // headers resolvers append their respective paths on top.
+//
+// Trailing slashes are stripped defensively. The sole writer today
+// (lib/ctx.go BuildBaseURL) already normalizes, but OAuth providers match
+// redirect URIs exactly — a `https://host//api/oauth/callback` produced by
+// a future writer that forgets to trim would silently break every per-user
+// OAuth flow. Guarding once on the read side keeps that invariant local
+// to this function rather than spread across every potential writer.
 func BuildMCPCallbackBaseURL(ctx *schemas.BifrostContext) string {
 	if base, ok := ctx.Value(schemas.BifrostContextKeyMCPCallbackBaseURL).(string); ok && base != "" {
-		return base
+		return strings.TrimRight(base, "/")
 	}
 	return ""
 }
@@ -91,6 +98,69 @@ func matchesPerUserHeaderKey(name string, perUserKeys []string) bool {
 	return false
 }
 
+// Canonical-form invariant for per-user-headers data
+// =====================================================
+// HTTP header names are case-insensitive on the wire (RFC 7230 §3.2),
+// so anywhere the per-user-headers feature compares a schema key against
+// a stored or submitted header name we'd need EqualFold lookups. Doing
+// that defensively at every read site is fragile — a single missed call
+// site re-introduces the bug (stored `authorization` looking missing
+// against schema `Authorization`, etc.).
+//
+// Instead we enforce a write-time invariant: every external boundary
+// that accepts a header key (or a credential header map) lowercases and
+// trims via the helpers below before persisting. Downstream code can
+// then assume canonical form and use plain map lookups.
+//
+// Write boundaries that MUST call these:
+//   - createMCPClient / updateMCPClient / resolvePerUserHeaderKeys
+//     (handlers/mcp.go) for MCPClientConfig.PerUserHeaderKeys
+//   - flowSubmit (handlers/mcp_per_user_headers.go) for the
+//     user-submitted credential.Headers map
+//   - loadMCPClientConfigFromFile (lib/config.go) for the config.json
+//     load path
+//
+// New write paths added in the future must canonicalize too — there is
+// no defensive case-folding on the read side anymore.
+
+// CanonicalizeHeaderKey returns the canonical lowercase + trimmed form
+// of a single header key. Empty input returns empty.
+func CanonicalizeHeaderKey(key string) string {
+	return strings.ToLower(strings.TrimSpace(key))
+}
+
+// CanonicalizeHeaderKeys returns a new slice with every entry passed
+// through CanonicalizeHeaderKey. Nil in → nil out so a caller that
+// uses "nil means preserve existing" semantics (e.g.
+// resolvePerUserHeaderKeys, UpdateMCPClientConfig) keeps that signal.
+// The input slice is not mutated.
+func CanonicalizeHeaderKeys(keys []string) []string {
+	if keys == nil {
+		return nil
+	}
+	out := make([]string, len(keys))
+	for i, k := range keys {
+		out[i] = CanonicalizeHeaderKey(k)
+	}
+	return out
+}
+
+// CanonicalizeHeaderMap returns a new map whose keys are passed through
+// CanonicalizeHeaderKey. On collision (e.g. "Authorization" and
+// "authorization" both present in the input), the last value wins —
+// callers that need duplicate detection should run it on the raw input
+// before calling this. Nil in → nil out.
+func CanonicalizeHeaderMap(m map[string]string) map[string]string {
+	if m == nil {
+		return nil
+	}
+	out := make(map[string]string, len(m))
+	for k, v := range m {
+		out[CanonicalizeHeaderKey(k)] = v
+	}
+	return out
+}
+
 // ExtractFilteredExtras returns just the per-request "extra" headers carried
 // in the BifrostContext (BifrostContextKeyMCPExtraHeaders), scoped by the
 // client's AllowedExtraHeaders. Static config headers are NOT included here —