Add opt-in Enterprise-Managed Authorization (MCP ID-JAG extension)

[jstjoe]

Summary

Implements the MCP Enterprise-Managed Authorization extension (io.modelcontextprotocol/enterprise-managed-authorization), letting organizations gate access to /mcp centrally through their identity provider. Covers both target scenarios:

1. Skyflow-hosted endpoint + Skyflow Okta — run with ENTERPRISE_AUTH_MODE=optional so Okta-authenticated enterprise clients work alongside existing Skyflow-credential and anonymous consumers. (No Okta test credentials exist yet, so this path is validated against a simulated IdP — see Testing.)
2. Self-hosted customers with their own IdP — run with the default ENTERPRISE_AUTH_MODE=required to require SSO-derived tokens on every request, with a server-side SKYFLOW_API_KEY service credential so employees never handle vault credentials.

The feature is entirely opt-in via ENTERPRISE_AUTH_ENABLED=true; without it, server behavior is byte-for-byte unchanged (all new endpoints 404, the new middleware is a no-op).

How it works

The extension profiles the IETF Identity Assertion JWT Authorization Grant (ID-JAG) draft. The client SSO-authenticates with the enterprise IdP, exchanges its identity assertion for an ID-JAG (RFC 8693 token exchange, policy evaluated by the IdP), then presents the ID-JAG to this server. This PR adds the server-side half:

• Built-in Resource Authorization Server — POST /token accepts grant_type=urn:ietf:params:oauth:grant-type:jwt-bearer (RFC 7523) with the ID-JAG as assertion, and issues a short-lived HS256 access token audience-restricted to the MCP resource identifier, as the spec requires. Stateless by design so it works on Vercel/serverless.
• Discovery — GET /.well-known/oauth-authorization-server (RFC 8414) advertises authorization_grant_profiles_supported: ["urn:ietf:params:oauth:grant-profile:id-jag"]; GET /.well-known/oauth-protected-resource[/mcp] (RFC 9728) points clients at the authorization server. Unauthenticated /mcp requests get a WWW-Authenticate: Bearer resource_metadata="..." challenge.
• ID-JAG validation (src/lib/auth/idJag.ts) — typ: oauth-id-jag+jwt header, signature against the IdP's JWKS (explicit ENTERPRISE_IDP_JWKS_URI or OIDC discovery, cached), issuer/audience/expiry with clock tolerance, resource claim matching, optional client_id allowlist, and best-effort in-memory jti replay detection. Asymmetric algorithms only (key-confusion protection).
• /mcp gate (src/lib/middleware/enterpriseAuth.ts) — verifies issued tokens ahead of the existing auth chain and exposes the enterprise identity (sub, email, scope, client_id) as req.enterpriseAuth. Since the Authorization header now carries the enterprise token, Skyflow vault credentials resolve from X-Skyflow-Authorization header → SKYFLOW_API_KEY env var → existing fallbacks (apiKey query param, anonymous mode).
• Fail closed — enabled-but-misconfigured deployments return 500 on /mcp instead of skipping authorization.

Changes

• New: src/lib/auth/{config,idJag,accessTokens,routes}.ts, src/lib/middleware/enterpriseAuth.ts
• src/server.ts: mounts the auth router and middleware; logs enterprise auth status at startup
• src/lib/middleware/authenticateBearer.ts: skips extraction when credentials were already resolved upstream (only behavioral change to existing code; a no-op unless enterprise auth is active)
• New dependency: jose (JWT/JWKS)
• Docs: docs/enterprise-managed-auth.md (flow, env var reference, Okta Cross App Access pointers, curl walkthrough, security notes), plus README/CLAUDE.md/CHANGELOG updates

Testing

• 82 new unit tests (249 total, all passing): config validation, ID-JAG acceptance/rejection matrices (wrong typ/iss/aud/resource, expired, unknown key, HS256 downgrade, replay, allowlist), access token round-trip/tamper/expiry/audience, token endpoint error codes per RFC 6749 §5.2, middleware behavior in both modes, and no-overwrite regression coverage for authenticateBearer.
• End-to-end smoke test: booted the real server with a mock OIDC IdP (discovery + JWKS over HTTP), and drove the full flow — metadata discovery, ID-JAG signing and exchange at /token (including wrong-audience and replay rejection), tools/list through the gated /mcp with the issued token, tampered-token rejection, and optional-mode fall-through for legacy Skyflow credentials. All checks passed.
• Not yet validated against a live Okta org (no credentials provisioned); the Okta setup steps are documented in docs/enterprise-managed-auth.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WsKnZeLzmfh5mQoGaFYd5P

---

Generated by Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
sky-mcp-streamable	Ready	Preview, Comment	Jul 4, 2026 11:23pm

[github-actions]

🔐 Gitleaks Findings: 2 issue(s) detected

🔸 Rule: generic-api-key
📄 File: tests/unit/middleware/enterpriseAuth.test.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/enterpriseAuth.test.ts:null

🔸 Rule: generic-api-key
📄 File: tests/unit/middleware/enterpriseAuth.test.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/enterpriseAuth.test.ts:null

[github-actions]

🔐 Gitleaks Findings: 1 issue(s) detected

🔸 Rule: generic-api-key
📄 File: tests/unit/middleware/enterpriseAuth.test.ts:null
📝 Description: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
🔑 Secret: **********
🔗 Path: tests/unit/middleware/enterpriseAuth.test.ts:null

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG extension)

Thorough, well-structured PR. The security fundamentals are handled correctly and the design decisions (opt-in, fail-closed, stateless) are sound. Findings below, roughly by priority. Nothing here is a blocker; most are hardening suggestions.

Strengths

• Algorithm-confusion protection is correct: ID-JAGs restricted to asymmetric algorithms only (ALLOWED_ID_JAG_ALGORITHMS), issued access tokens pinned to HS256, and verification pins the same. This closes the classic RS↔HS key-confusion attack.
• Fail-closed on misconfiguration (500 rather than skipping auth), and truly opt-in — every new route 404s and the middleware is a genuine no-op when disabled.
• The replay jti check runs after signature verification, so seenJtis can only be populated by legitimately IdP-signed tokens — not attacker-controllable. Good ordering.
• Header hygiene: the enterprise token is deleted from Authorization before authenticateBearer runs, so it can't be mistaken for a Skyflow credential. Verified by tests.
• Excellent test matrix (wrong typ/iss/aud/resource, expiry, unknown key, HS256 downgrade, replay, allowlist, both modes) and thorough docs.

Findings

1. discoverJwksUri fetch has no timeout (reliability) — src/lib/auth/idJag.ts:87
The OIDC discovery fetch(discoveryUrl) has no abort/timeout. A hung IdP discovery endpoint will stall the /token request (and the first request that triggers discovery) indefinitely. Consider fetch(discoveryUrl, { signal: AbortSignal.timeout(5000) }). (jose's createRemoteJWKSet manages its own fetch, so only this discovery call is exposed.)

2. Optional-mode enterprise token can silently downgrade to anonymous mode — src/lib/middleware/enterpriseAuth.ts:102-104
When a valid enterprise token is presented but there is no X-Skyflow-Authorization header and no SKYFLOW_API_KEY, credentials are left unresolved and the request falls through to authenticateBearer, which (with the auth header now deleted) drops to anonymous mode if ANON_MODE_* is configured, or 401 otherwise. An authenticated enterprise user landing in anonymous mode is surprising — worth an explicit decision + doc note, or resolving to a definite outcome.

3. unauthorized() drops the machine-readable OAuth error code from the JSON body — src/lib/middleware/enterpriseAuth.ts:56-60
The /token endpoint returns { error: <code>, error_description: <text> } (RFC 6749 §5.2), but the /mcp 401 body uses { error: <human description> } and puts the code (invalid_token) only in the WWW-Authenticate header. Consider aligning the two shapes so clients get a consistent, machine-parseable error field.

4. ID-JAG resource claim is optional (defense-in-depth) — src/lib/auth/idJag.ts:215-226
An ID-JAG with no resource claim is accepted. Since this server is single-resource and aud is validated against the issuer, this is safe today, but requiring resource (and rejecting when absent) would harden against a future multi-resource authorization server and matches the spirit of the ID-JAG profile. At minimum, document that absence is intentionally permitted.

5. No rate limiting on the public /token endpoint (hardening)
anonymousRateLimiter guards /mcp but /token is unauthenticated and does crypto per request. JWKS caching keeps outbound calls to ~zero after warm-up, so the risk is modest, but a basic limiter on /token would be prudent for a public deployment.

6. Minor perf: per-request config reparse + full replay-map scan
loadEnterpriseAuthConfig(env) is re-parsed/re-validated on every request in both the router handlers and the middleware, and isReplayedJti does an O(n) sweep of seenJtis each call. Both are bounded and fine at current scale; caching the config and using a lazy/periodic sweep would tidy this up if throughput grows.

Nits

• looksLikeEnterpriseToken keys off iss === config.issuer; a Skyflow JWT that happened to carry a matching iss would be routed to enterprise verification (and then rejected). Practically impossible, just noting the assumption.
• CLAUDE.md correctly moves SKYFLOW_API_KEY out of "Removed variables" — nice consistency.

Overall this is a high-quality, well-tested addition. Addressing #1 (fetch timeout) and clarifying #2 (optional-mode downgrade) are the two I'd prioritize before enabling in production.

🤖 Generated with Claude Code

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG)

Reviewed the full diff against CLAUDE.md conventions. This is a well-structured, genuinely opt-in feature: clean separation into config/idJag/accessTokens/routes, dependency injection (env, keyResolver) that makes the code testable without live IdPs, fail-closed behavior on misconfiguration, asymmetric-only signature acceptance for ID-JAGs (key-confusion protection), audience-restricted access tokens, and a strong rejection-matrix test suite. The authenticateBearer no-overwrite guard is the minimal, correct touch to existing code. Nice work.

A few things worth considering before merge.

Security / robustness

• No timeout on the OIDC discovery fetch — can hang /token. src/lib/auth/idJag.ts:87 calls fetch(discoveryUrl) with no AbortSignal/timeout. A slow or hanging IdP discovery endpoint will stall the token request indefinitely (and since the endpoint is unauthenticated, this is externally reachable). Note createRemoteJWKSet does apply jose default timeout to the JWKS fetch, so only the discovery hop is unprotected. Suggest fetch(discoveryUrl, { signal: AbortSignal.timeout(5000) }).

• /token is unauthenticated and unthrottled. Metadata advertises token_endpoint_auth_methods_supported: ["none"] (correct for this profile), but unlike anonymous /mcp there is no rate limiting. Invalid assertions fail fast at jwtVerify, so blast radius is limited, but a caller can drive repeated signature verifications (and, in the discovery path, outbound IdP calls). Consider a lightweight rate limit here, mirroring the anonymous-mode limiter.

• ID-JAG resource claim is optional (src/lib/auth/idJag.ts:215-226). When the claim is absent, validation passes and an access token audience-restricted to this MCP resource is issued for any ID-JAG whose aud/iss match. This is intentional and explicitly tested ("accepts an ID-JAG without a resource claim"), but since the point of the resource claim is to bind the grant to a specific resource, requiring it (or gating the lenience behind config) would be stricter and closer to the extension intent. Please confirm this matches the ID-JAG profile you are targeting.

Minor

• unauthorized() body drops the OAuth error code. In src/lib/middleware/enterpriseAuth.ts:56-60 the JSON body sets error: options.description || default, so the WWW-Authenticate header carries the machine code (invalid_token) but the JSON error field carries human prose. Programmatic clients reading the body get a description where they would expect a code. Consider { error: options.error, error_description: options.description } to match the RFC 6749 5.2 shape you already use on /token.

• Config re-parsed per request. loadEnterpriseAuthConfig() runs on every /mcp (middleware) and every auth-route request. Env does not change at runtime, so this is cheap and correct — just noting it could be memoized if it ever shows up in profiling.

• seenJtis cleanup is O(n) per validation (idJag.ts:142) — full-map scan on each call. Fine at expected volume and already documented as best-effort; flagging only for awareness under load.

Tests

Coverage is excellent — acceptance/rejection matrices for typ/iss/aud/resource/expiry/unknown-key/HS256-downgrade/replay/allowlist, access-token round-trip/tamper/expiry, token-endpoint error codes, and both middleware modes. The one behavior I would add an explicit assertion for is the discovery-timeout item once addressed. (I could not execute pnpm test in this sandbox, so I am going on the PR reported 249-passing plus code inspection.)

Overall: solid, defensible implementation. The three Security/robustness items are the ones I would want addressed or explicitly signed off before merge; the Minor items are polish.

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG extension)

Reviewed the full diff (auth config/idJag/accessTokens/routes, both middlewares, server wiring, and the test suite). This is high-quality work: clean module separation, genuinely opt-in (disabled = no-op / 404s), fail-closed on misconfiguration, and a thorough test matrix. The security fundamentals are right — asymmetric-only algorithms for ID-JAGs (alg-confusion protection), HS256 pinned for the server's own tokens, audience restriction to the MCP resource, https enforcement, a minimum signing-key length, clock tolerance, and secrets-never-logged comments on the credential fields.

A few things worth considering before merge, roughly in priority order.

Potential issues / hardening

1. No timeout on OIDC discovery fetch (src/lib/auth/idJag.ts:87). discoverJwksUri calls await fetch(discoveryUrl) with no AbortSignal.timeout(...). If the configured IdP's discovery endpoint hangs, the /token request hangs with it (and there's no rate limit in front of /token). createRemoteJWKSet has its own default timeout, but this hand-rolled fetch does not. Suggest an explicit timeout (e.g. fetch(url, { signal: AbortSignal.timeout(5000) })).

2. The resource claim is optional, which weakens audience binding in multi-resource deployments (src/lib/auth/idJag.ts:215-226). Validation is skipped entirely when resource is absent — only aud (= idpAudience, defaulting to issuer) is checked. If an org ever runs more than one MCP resource behind the same authorization-server issuer, an ID-JAG minted for resource A without a resource claim could be exchanged for a token scoped to resource B. The extension expects the resource claim to be present; consider requiring it (or at least documenting the "one resource per issuer" assumption in docs/enterprise-managed-auth.md).

3. /token is unauthenticated (by design) and has no rate limiting. The anonymous rate limiter is only wired onto /mcp (src/server.ts:220). Each /token call runs a JWKS signature verification (CPU) and, on cache miss, network I/O. Worth a lightweight limiter or at least a note that a reverse proxy should throttle it.

4. Optional-mode issuer collision (edge case) (src/lib/middleware/enterpriseAuth.ts:153). looksLikeEnterpriseToken routes any bearer JWT whose iss equals this deployment's issuer URL into verifyAccessToken. A legitimate Skyflow bearer JWT that happened to carry the same iss would be intercepted and rejected rather than forwarded. Extremely unlikely (the issuer is this server's own public URL), so more a documented-assumption note than a bug.

Minor / nits

• Config is re-parsed and re-validated on every request (both createEnterpriseAuthMiddleware and each route handler call loadEnterpriseAuthConfig(env)). On the /mcp hot path this re-runs new URL(...), string splits, etc. per request. Reasonable for serverless statelessness, but a memoized load would trim per-request work. Not a correctness issue.
• Replay-cache sweep is O(n) per call (isReplayedJti, idJag.ts:142) and the map is unbounded under burst. Fine given it's explicitly best-effort and jti lifetimes are short, but a size cap or note wouldn't hurt.
• Inconsistent error-body shape. The 401 from unauthorized() puts a human-readable message in a field literally named error (enterpriseAuth.ts:56-60), whereas /token correctly uses the OAuth { error, error_description } split. Aligning the /mcp challenge body would be tidier for clients.

Tests

Coverage is strong — the ID-JAG rejection matrix (wrong typ/iss/aud/resource, expired, unknown key, HS256 downgrade, replay, allowlist), token round-trip/tamper/expiry, RFC 6749 §5.2 error codes, and both middleware modes are all exercised, including the important "invalid token that still claims this issuer" case and the no-overwrite regression on authenticateBearer. Gaps mirroring the findings above: no test for a missing resource claim being accepted (would document behavior in #2), and none for the discovery-fetch timeout/failure path (#1).

Note: I couldn't execute pnpm build:server / vitest in this review environment (command execution was blocked), so the "249 passing / clean tsc" claims are unverified here — worth confirming CI is green.

Overall: solid, safe-by-default implementation. The discovery timeout (#1) and the optional-resource audience-binding question (#2) are the two I'd want addressed or explicitly acknowledged before merge; the rest are polish.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed the review feedback in 1bf441e (one reply for all three reviews, since the findings converge):

Fixed

• OIDC discovery timeout — the discovery fetch now uses AbortSignal.timeout(5000), so a hung IdP can't stall /token (src/lib/auth/idJag.ts).
• /token rate limiting — the endpoint is now rate-limited per client IP (default 30 req/60s, configurable via ENTERPRISE_TOKEN_RATE_LIMIT_REQUESTS/_WINDOW_MS). Implemented by extracting a shared IP limiter that anonymous mode also uses, with tests.
• 401/500 body shape — the /mcp middleware now returns the same RFC 6749 §5.2 {error, error_description} shape as /token (invalid_token, unauthorized, server_error), with the code also asserted in tests.

Intentional, now documented

• Optional resource claim — kept as-is: the extension makes the token-exchange resource parameter OPTIONAL and constrains the claim only "if present" (spec §4), so requiring it would reject spec-compliant IdPs. The aud check still binds every grant to this authorization server, which serves exactly one resource; the one-resource-per-issuer assumption is now called out in the code comment and in docs/enterprise-managed-auth.md security notes.
• Optional-mode anonymous fallback — an enterprise-authenticated request with no Skyflow credentials deliberately degrades to anonymous mode (responses carry anonymousMode: true) rather than failing, as a demo path before vault credentials are provisioned. Documented in the middleware and the guide, including how to opt out (SKYFLOW_API_KEY for a real vault, or leave ANON_MODE_* unset for a hard 401).

Noted, no change — per-request config parse and the O(n) jti sweep are bounded and fine at current scale, as the reviews acknowledged. One correction: the "missing resource claim accepted" test the third review flagged as absent already exists (idJag.test.ts → "accepts an ID-JAG without a resource claim").

254 tests passing; the end-to-end smoke flow (mock IdP → discovery → ID-JAG exchange → gated tools/list) re-verified after the changes.

---

Generated by Claude Code

[claude]

Review: Enterprise-Managed Authorization (MCP ID-JAG extension)

Thorough, well-structured PR. The opt-in / fail-closed design is sound, the RFC alignment (7523/8414/9728/6749 §5.2) is careful, and the code carries genuinely useful security comments. Strengths:

• Algorithm-confusion protection is correct: ID-JAGs are restricted to asymmetric algs and verified against the IdP JWKS; issued access tokens are HS256-only against the shared secret. No key-type crossover.
• typ header enforcement on both the ID-JAG (oauth-id-jag+jwt) and the issued token (at+jwt).
• Fail-closed on misconfiguration (500 rather than skipping auth) and byte-for-byte no-op when disabled are the right defaults.
• Test coverage is excellent — the rejection matrices in idJag.test.ts (wrong typ/iss/aud/resource, expired, unknown key, HS256 downgrade, replay, allowlist) and the mode/credential-resolution cases in enterpriseAuth.test.ts cover the important edges. (I reviewed statically; could not run pnpm test in this env due to command approvals, so I trust the stated 249-passing result.)

Findings

1. Scopes are captured but never enforced (design gap — please confirm intent). validateIdJag and verifyAccessToken extract scope and store it on req.enterpriseAuth, but nothing downstream reads it (req.enterpriseAuth is only ever written, never read). An enterprise token minted with scope "de-identify" can still call re-identify. If the model is meant to gate which tools a grant permits, this is a hole; if gating is intentionally connection-level only, worth a one-line note in docs/enterprise-managed-auth.md. (src/lib/middleware/enterpriseAuth.ts:166)

2. Token-endpoint rate-limit config is validated at startup even when the feature is disabled (low). createEnterpriseAuthRouter() is mounted unconditionally (server.ts:185) and eagerly calls getTokenEndpointRateLimitConfig(), which throws on an invalid ENTERPRISE_TOKEN_RATE_LIMIT_REQUESTS/WINDOW_MS. So a bad value crashes startup for a deployment that never set ENTERPRISE_AUTH_ENABLED=true — a small dent in the "disabled implies unchanged behavior" guarantee. Consider deferring that read until enterprise auth is known-enabled. (src/lib/auth/routes.ts:184)

3. 5 MB global JSON parse happens before the /token rate limiter (low). app.use(express.json({ limit: "5mb" })) (server.ts:176) runs before the router, so an unauthenticated caller can make the server parse up to 5 MB on /token before the per-IP limiter engages. The endpoint needs only a tiny body; a tighter limit scoped to /token would shrink the pre-auth work.

4. Discovered jwks_uri is not scheme-validated (low / hardening). Config URLs are validated https (or localhost http), but the jwks_uri pulled from the IdP discovery document (idJag.ts:110) is passed straight into createRemoteJWKSet. idpIssuer is operator-configured so risk is low, but applying the same https check to the discovered URI would harden against a misconfigured/compromised discovery doc.

5. Signing-key check is length-only (low / doc). MIN_SIGNING_KEY_LENGTH = 32 guards length, not entropy — forgery is possible with a long-but-weak HS256 secret. Worth documenting that this must be a high-entropy random value (e.g. openssl rand -base64 32).

Nits (non-blocking)

• Per-request config reload: loadEnterpriseAuthConfig(env) re-parses env (incl. new URL(...)) on every /mcp request and route hit. Fine and good for testability, but memoizing the process.env case would avoid repeated parsing on the hot path.
• isReplayedJti is O(n) per call: it sweeps the whole seenJtis map on every validation. Bounded by the token-endpoint rate limit + short ID-JAG TTL, so acceptable — a periodic sweep (like rateLimiter.ts setInterval) would keep validation O(1).
• decodeJwt in looksLikeEnterpriseToken is correctly unsigned/structural-only, and a forged iss merely forces the verify path (which then 401s) — no abuse there.

Overall high-quality work. The only item worth resolving before merge is confirming intent on #1 (scope enforcement); the rest are low-severity hardening suggestions.

Reviewed with Claude Code

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed the latest review in 1af5c4a:

1. Scope enforcement (was the design gap) — now implemented rather than just documented. When the enterprise access token carries a scope claim, each scope names a permitted tool (de-identify, re-identify); denied invocations return an insufficient_scope isError result. Tokens without a scope claim stay unrestricted (connection-level gating), so IdPs that don't configure scopes keep working. New src/lib/auth/scopes.ts + unit tests, and the end-to-end smoke run verifies a de-identify-only token is denied on re-identify.
2. Rate-limit config no longer read at startup — deferred until enterprise auth is known-enabled, so invalid ENTERPRISE_TOKEN_RATE_LIMIT_* values can't crash a disabled deployment (regression test added).
3. JSON parser ordering — the auth router now mounts before the global 5MB express.json, so /token only parses its own form-urlencoded bodies (100kb limit).
4. Discovered jwks_uri scheme-validated — https required (localhost http excepted), matching the configured-URL policy, with tests.
5. Signing-key entropy — docs now state the key must be high-entropy random (openssl rand -base64 32), not merely long.

266 tests passing; typecheck and the full smoke flow re-verified.

---

Generated by Claude Code

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG extension)

Reviewed the full auth surface (config, idJag, accessTokens, routes, scopes, both middleware, and the server.ts wiring). This is high-quality, well-documented work: cleanly opt-in, fails closed, dependency-injectable (env/keyResolver) for testing, and it follows the repo's established patterns (pure functions in src/lib, thin middleware, per-request wiring in server.ts). Security fundamentals are handled correctly — asymmetric-only algorithms with key-confusion protection, https enforcement (including on the discovered jwks_uri), Cache-Control: no-store on /token, IP rate limiting on the unauthenticated endpoint, RFC-shaped error bodies, and the WWW-Authenticate challenge. The X-Skyflow-Authorization → SKYFLOW_API_KEY → fallback credential resolution and the header-deletion-after-consume step are both correct.

Note: I couldn't run the suite (deps aren't installable in the review sandbox), so the findings below are from static analysis; the PR reports 249 tests passing.

Correctness

1. Empty-scope ID-JAG fails open (low–medium). accessTokens.ts:55 uses ...(identity.scope && { scope }), so an ID-JAG carrying scope: "" is treated as falsy and the scope claim is dropped from the issued token → the token becomes unrestricted. But scopes.ts establishes the opposite convention: parseGrantedScopes("") → [] → isToolPermitted denies everything (explicitly tested at scopes.test.ts:38). So an IdP that emits an empty scope string to mean "no tools" gets inverted into "all tools." Unusual IdP behavior and low severity, but it fails in the permissive direction. Consider normalizing an empty/whitespace-only scope at validation time to an explicit deny-all (or documenting that empty == unrestricted). No test covers the ID-JAG→issuance path for this case.

2. Config re-parsed several times per request (perf, low). loadEnterpriseAuthConfig() runs in createEnterpriseAuthMiddleware, again in createLazyTokenRateLimiter, and again in every route handler via configForRequest — each call re-runs new URL(...) validation on multiple env vars. Env is static per process, so this is pure overhead on the hot /mcp path. Since the "config can crash startup vs. per-request 500" behavior is intentional, a memoized loader (parse once, cache the result/error) would preserve semantics while dropping the repeated work.

Security

3. /token rate limiting depends on X-Forwarded-For handling (worth confirming for the target platform). getClientId (rateLimiter.ts:29-38) keys off the rightmost XFF entry. That's the right call to resist spoofing behind a single trusted proxy, but on some serverless/multi-hop setups the rightmost hop is a shared provider proxy — in which case many distinct callers collapse into one bucket and the default 30/60s becomes an effectively global cap (self-inflicted DoS on the token endpoint). This is pre-existing logic (shared with anonymous mode) but it's now guarding a new unauthenticated endpoint, so it's worth validating against the actual Vercel deployment (and note trust proxy / req.ip is not configured on the Express app).

4. Replay cache does a full-map scan per validation (scale, low). isReplayedJti (idJag.ts:164) iterates all of seenJtis on every /token call to expire entries. Bounded by the short ID-JAG TTL, and the "best-effort, per-instance" framing is honest, but under a burst of unique jtis each request is O(n). A lazy/periodic sweep (like rateLimiter.ts's setInterval cleanup) would avoid the per-request cost.

Test coverage

5. Strong unit matrices, but no automated end-to-end test. The acceptance/rejection matrices (typ/iss/aud/resource/expiry/HS256-downgrade/replay/allowlist), token round-trip/tamper, RFC 6749 §5.2 error codes, and the authenticateBearer no-overwrite regression are all excellent. The full HTTP flow (discovery → /token → gated /mcp, header-deletion, middleware ordering) is only validated by the manual smoke test described in the PR. A supertest-based integration test would lock in the wiring in server.ts (which is exactly where an ordering regression would silently reintroduce a security gap) and could also cover the empty-scope case from finding #1.

Nits

6. unauthorized() (enterpriseAuth.ts:46-51) interpolates error/description into the WWW-Authenticate header unescaped. All current call sites pass constants, so there's no injection today — a one-line comment noting the invariant would guard against a future caller passing dynamic text.

Overall: solid, defensible design with careful attention to failing closed and to backward compatibility. #1 and #3 are the two I'd want confirmed before relying on scope-based gating or the /token rate limit in production; the rest are polish.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed the latest review in 15539f0:

1. Empty-scope fail-open — confirmed and fixed. Good catch: issueAccessToken used a truthiness spread, so scope: "" was dropped from the issued token, inverting the IdP's deny-all into unrestricted. Empty scope claims now round-trip through both the issued JWT and the token response body, and deny all tools per the scopes.ts convention. Covered at three levels: unit (accessTokens), token-endpoint (routes), and end-to-end HTTP.
2. Config re-parse — leaving as-is (per-request parse keeps env-injection semantics simple and it's a few string ops + new URL on a bounded set of vars); acknowledged as pure-overhead-but-correct in the review.
3. XFF/rate-limit bucketing — on Vercel (the current deployment target) the rightmost XFF entry is the platform-appended client IP, so per-client bucketing works there. For self-hosted multi-hop setups this is now called out in docs/enterprise-managed-auth.md (verify your outermost proxy's XFF semantics; raise the limit or throttle at the proxy if callers collapse into one bucket).
4. Replay-cache sweep — moved to a 60s setInterval (unref'd, matching rateLimiter.ts), so validation is O(1); lingering expired entries are harmless since expired tokens fail jwtVerify before the replay check.
5. Automated end-to-end coverage — added tests/integration/enterpriseAuth.integration.test.ts: boots the real Express app over HTTP against a mock OIDC IdP and drives discovery → /token → gated /mcp, including scope denial, the empty-scope case, replay, tampered tokens, and optional-mode fall-through. This locks in the server.ts middleware ordering and Authorization-header consumption in CI (no supertest needed — plain fetch against app.listen(0); the gitignored src/generated/ui-html.js is stubbed via a vitest alias so no UI build is required).
6. WWW-Authenticate interpolation — added the constants-only invariant comment.

275 tests passing (7 new integration + 2 empty-scope unit tests), typecheck clean, and the suite verified to pass with the generated UI module absent (CI conditions).

---

Generated by Claude Code

[claude]

Review: Enterprise-Managed Authorization (MCP ID-JAG extension)

Thanks for this — it's a large, genuinely well-executed PR. The design is sound and defensively minded, the docs are excellent, and the test coverage (unit matrices + a real over-the-wire integration test) is a model for how to land security-sensitive code. Below is feedback, ordered by importance. Nothing here is a blocker; the strengths dominate.

Strengths worth calling out

• Fail-closed everywhere — misconfiguration → 500 rather than silently skipping auth, and the feature is a true no-op (404s, no-op middleware) when disabled.
• Token-confusion resistance — asymmetric-only algorithm pinning for ID-JAGs (blocks HS/key-confusion), HS256 pinning for issued tokens, and distinct typ values (oauth-id-jag+jwt vs at+jwt) so an ID-JAG can't be replayed as an access token and vice-versa.
• Audience restriction on issued tokens, jti replay detection with a self-sweeping unref()'d cache, and the careful ""-scope-vs-absent-scope distinction (deny-all vs unrestricted) — subtle and correctly tested end-to-end.
• Attack-surface hygiene on the unauthenticated /token: per-IP rate limiting, a 100kb urlencoded cap, the router mounted before the 5MB JSON parser, and a discovery timeout so a hung IdP can't stall requests.
• The authenticateBearer no-overwrite guard is the minimal, correct way to compose with the existing chain.

Suggestions (all minor)

1. X-Skyflow-Authorization prefix handling is case-sensitive and untrimmed (enterpriseAuth.ts, resolveSkyflowCredentials). The docs say the value may be sent "with or without a Bearer prefix", but headerValue.startsWith("Bearer ") only matches that exact casing. A client sending bearer <key> (lowercase) gets double-prefixed to Bearer bearer <key>, so the literal bearer ends up embedded in the API key and Skyflow silently rejects it. Leading whitespace has a similar effect (substring(7) isn't trimmed downstream). Consider a case-insensitive prefix strip + .trim() before handing off to extractCredentials.

2. loadEnterpriseAuthConfig() is re-run on every request — once per /mcp request in the middleware, again in the lazy /token rate limiter, and per /token call. Each invocation re-parses/validates URLs and re-splits the client-ID allowlist, though the result is a process-lifetime constant. The per-request read is only really needed for test env injection; consider memoizing the production path (e.g. cache keyed on the enabled flag) to avoid the repeated new URL() / string work on the hot path.

3. JWKS fetch timeout relies on the jose default. You added an explicit 5s timeout to the OIDC discovery fetch (nice), but the actual key retrieval via createRemoteJWKSet(new URL(uri)) uses jose's default timeoutDuration/cooldownDuration. It happens to default to 5s, so behavior is fine — but making it explicit (or a one-line comment noting the reliance) keeps the hardening intent visible right next to the discovery timeout, rather than implicit.

4. Replay jti is burned during validation, before issuance (idJag.ts → isReplayedJti, called before issueAccessToken in the route). Given local HS256 signing this virtually never fails, but if issuance did fail transiently the ID-JAG would already be marked used and the client would have to re-exchange at the IdP. That's acceptable for single-use semantics — a short comment noting the ordering choice would help future readers.

5. RFC 9728 metadata pointer. The WWW-Authenticate challenge points at the bare ${issuer}/.well-known/oauth-protected-resource, but the protected resource has a path component (/mcp), for which RFC 9728 prescribes the path-suffixed URL (/.well-known/oauth-protected-resource/mcp). You serve both, so clients work either way; pointing the challenge at the suffixed variant would be strictly more conformant.

6. Latent header-injection invariant (unauthorized()). error/description are interpolated into the WWW-Authenticate header unescaped. Every current caller passes constants and there's a good INVARIANT: comment — but since this is one refactor away from carrying request-derived text, a defensive escape (or a typed enum of allowed codes) would make the invariant enforced rather than documented.

Not blocking / non-issues

• Startup loadEnterpriseAuthConfig() reads env at import time for the log line only; per-request reads make this harmless (and is what the integration test leans on).
• Optional-mode fall-through via looksLikeEnterpriseToken is correctly safe: a token whose iss matches but whose signature is invalid is rejected (401) rather than falling through, and non-JWT/foreign-issuer bearers pass through to the existing Skyflow chain. 👍

Overall: solid, ship-worthy work once the case-insensitive header fix (#1) is in — that's the only one with a plausible real-world failure mode. The rest are polish.

🤖 Generated with Claude Code

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed in afd7aeb:

1. Case-insensitive X-Skyflow-Authorization prefix — fixed. The value is now trimmed and the Bearer prefix stripped case-insensitively (RFC 7235), so bearer <key> no longer double-prefixes into a corrupted API key. Tests added for lowercase prefix and surrounding whitespace; the existing malformed-header 401 case still holds.
2. Config memoization — declining (third ask). Per-request parsing keeps env-injection semantics simple and is a handful of string ops on a bounded var set; both prior reviews and this one agree it's correct, so I'm leaving it.
3. JWKS timeout — now explicit. createRemoteJWKSet gets timeoutDuration: 5000 pinned next to the discovery timeout (shared IDP_HTTP_TIMEOUT_MS constant), so the hardening intent is visible and version-proof.
4. jti burn ordering — commented. Noted at the call site that burning before issuance is deliberate: a transient issuance failure forces a fresh ID-JAG, which beats leaving a validated grant replayable.
5. RFC 9728 pointer — fixed. The WWW-Authenticate challenge now points at the path-suffixed metadata URL derived from the resource identifier (.../oauth-protected-resource/mcp), and the metadata route serves any resource-path suffix so custom ENTERPRISE_MCP_RESOURCE paths work too. Both bare and suffixed URLs remain served.
6. Header invariant — enforced. WWW-Authenticate parameter values are now defensively escaped (quotes/backslashes/CRLF stripped) rather than relying on the constants-only comment.

277 tests passing (3 new), typecheck clean; the integration suite covers the suffixed challenge URL and metadata route over real HTTP.

---

Generated by Claude Code

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed in f9d3505:

• Add Claude Code GitHub Workflow #2 Scope naming vs. real IdPs — documented. Fair point given the Okta path is still simulated: the scope-enforcement docs now carry a prominent "known assumption to verify" callout — scope values must match tool names exactly, namespaced strings like mcp:de-identify will deny tools, and the workaround (omit the scope grant until a mapping layer exists) is spelled out as a live-Okta validation item.
• Jstjoe/2025 10 09 #1 Replay residual risk — already surfaced where you asked for it: docs/enterprise-managed-auth.md → Security notes has a dedicated bullet stating replay detection is in-memory/best-effort on serverless/multi-instance deployments, that the short ID-JAG lifetime bounds the window, and that a shared store is the path for strict single-use. (Same section also covers the per-instance rate limiter.)
• add simple authentication and a sample .env #3 Config memoization — declining (ninth ask, standing rationale).
• rename tools #4 Strict parsing for the anonymous limiter — declining deliberately: getAnonymousRateLimitConfig() runs at startup, so tightening it would turn existing deployments that today tolerate a sloppy value into boot crashes — a backward-compat break outside this PR's scope. The new enterprise vars are strict because they're new, with no installed base to break.

286 tests passing.

---

Generated by Claude Code

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG extension)

Reviewed the auth modules (config, idJag, accessTokens, routes, scopes), the enterpriseAuth/authenticateBearer middleware, the rate limiter, and the /mcp + startup wiring in server.ts. This is a genuinely strong piece of work — the commit history shows the security-sensitive surface has already been hardened iteratively, and it shows.

What is done well

• Fail-closed posture throughout. Misconfiguration → 500 on /mcp and /token rather than silently skipping auth; the feature is a true no-op when disabled (all new routes 404, authenticateBearer early-return only triggers when creds are already resolved upstream).
• JWT hardening is correct. Asymmetric-only algorithm allowlist for ID-JAGs, HS256-only for issued tokens (blocks alg-confusion), requiredClaims [exp, iat, sub, jti] closes the no-exp=eternal-token gap that jose leaves open by default, and aud/iss/typ are all constrained.
• HTTPS enforcement on both configured and discovered jwks_uri, timeouts on discovery + JWKS fetches, WWW-Authenticate header value escaping, and the empty-scope fail-open fix (scope:"" → deny-all rather than unrestricted) are exactly the subtle things that usually get missed.
• Broad test coverage — ~92 unit tests across the auth surface plus an HTTP-level integration test driving the real app against a mock OIDC IdP. (I could not execute the suite in this sandbox — network-restricted — so I am relying on reading the tests, not a green run. Worth confirming CI is green.)

Discussion points (non-blocking)

1. required mode can silently degrade an enterprise user to the demo/anonymous vault. In resolveSkyflowCredentials, when a valid enterprise token carries no X-Skyflow-Authorization and SKYFLOW_API_KEY is unset, the request falls through to anonymous mode if ANON_MODE_* is configured (enterpriseAuth.ts:119-125). The startup warning covers this, but for a deployment that chose required for a strict security posture, silently serving via the demo vault is a surprising outcome — a hard 401 might match operator intent better. At minimum, consider making the required + no-server-credential + anon-configured combination louder (or configurable), since the two features can be enabled independently.

2. loadEnterpriseAuthConfig() re-parses env on every request — once per /mcp request in the middleware, and 2-3x per /token request (lazy limiter, then configForRequest, plus express.urlencoded). Env is effectively immutable after boot, so this re-validates URLs and re-splits the client-ID list needlessly. Memoizing the parsed config (you already compute it once at startup for the log line) would also let a genuine misconfiguration fail fast at boot rather than only surfacing as a per-request 500. Minor, but cheap.

3. /.well-known/ metadata endpoints are unauthenticated and un-rate-limited*, and each hit re-runs loadEnterpriseAuthConfig. Low risk (no crypto work, no secrets returned), but noting it alongside the /token limiter for completeness.

4. Per-instance replay + rate-limit state. The jti cache and IP rate limiter are in-memory singletons — correctly documented as best-effort on serverless/multi-instance, with the short ID-JAG TTL bounding the replay window. The X-Forwarded-For rightmost-IP handling is the right call and the direct-exposure caveat is documented. Flagging only so reviewers know these are deliberate trade-offs.

5. Scope naming assumption. Scopes must be bare tool names (de-identify); namespaced IdP scopes (mcp:de-identify) will deny everything. Already called out in the setup guide as a live-IdP validation item — the highest-risk works-in-tests-breaks-against-real-Okta item to verify before GA.

Nothing here blocks merge from a correctness standpoint; items 1 and 2 are the ones I would most want a maintainer to weigh in on. Nice work on the RFC conformance and the adversarial test matrix.

Review by Claude (Opus 4.8). Test suite not executed locally — sandbox network restrictions prevented pnpm test; please confirm CI.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed in 7f0df24:

• Jstjoe/2025 10 09 #1 Required-mode anonymous demotion — behavior changed, agreeing with your framing. required mode is the explicitly strict posture, and since the feature is unreleased there's no installed base to preserve: an enterprise-authenticated request with no Skyflow credentials from any source (X-Skyflow-Authorization, SKYFLOW_API_KEY, apiKey query param) now returns a hard 401 missing_skyflow_credentials — never the anonymous demo vault, even when ANON_MODE_* is configured. optional mode keeps the documented anonymous fallback as the demo path for mixed deployments. Docs, unit tests (three new/updated cases), and the integration test all reflect the mode-aware matrix.
• Add Claude Code GitHub Workflow #2 Config memoization — declining (tenth ask, standing rationale). On the fail-fast point specifically: startup already surfaces misconfiguration loudly via the boot-time load/log, and the per-request 500 is the fail-closed backstop, so boot visibility isn't lost.
• add simple authentication and a sample .env #3 Metadata endpoints — agreed low-risk (static public JSON, no crypto); leaving un-rate-limited.
• rename tools #4/readme updats: title, try it out section #5 — both already documented as deliberate trade-offs / validation items, as you noted.
• CI status — confirming since your sandbox couldn't run the suite: all checks are green on head (unit tests, coverage, gitleaks, Vercel), 288 tests passing locally with typecheck clean.

---

Generated by Claude Code

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG extension)

Reviewed the new src/lib/auth/*, src/lib/middleware/enterpriseAuth.ts, the authenticateBearer/rateLimiter changes, and the /mcp + router integration in src/server.ts. Overall this is a high-quality, defensively-written PR. The opt-in gating is clean (loadEnterpriseAuthConfig returns null so the middleware is a no-op and every route 404s), the fail-closed posture is consistent, and spec conformance (RFC 8414/9728/7523/6749 5.2, typ/aud/iss/resource checks, asymmetric-only algorithms to block key confusion, requiredClaims to stop no-exp grants) is careful and well-commented. The 14 review-response commits clearly hardened many edge cases already. Nice work.

A few observations, roughly in priority order — most are confirmations rather than blockers:

Security / correctness

1. Replay cache + /token rate limiter are per-instance in-memory, and the stated deploy target is serverless/Vercel. This is honestly documented in docs/enterprise-managed-auth.md and mitigated by the short ID-JAG lifetime, but worth flagging prominently: on Vercel each invocation may be a cold instance, so jti single-use is effectively NOT enforced there. If strict single-use matters for the threat model, a shared store (Redis/Upstash) is really required, not just recommended. Consider making this louder in the README so operators do not assume replay protection is active on serverless.

2. HS256 shared-secret verification implies all instances must share ENTERPRISE_AUTH_SIGNING_KEY. Since the same server both issues and verifies, a token minted by instance A must verify on instance B. Works as long as the key is identical everywhere, but I did not see it called out explicitly. Worth a one-line note in the env-var docs so a rotation or per-instance-secret setup does not silently 401 valid tokens.

3. jti replay check has a benign TOCTOU window (isReplayedJti does has then set non-atomically) — two concurrent requests with the same jti could both pass. Already best-effort by design; noting for completeness, no action needed.

4. Optional-mode issuer collision: in optional mode, a legitimate non-enterprise bearer whose iss happens to equal config.issuer would be routed to verifyAccessToken and rejected (401) rather than falling through. Extremely unlikely given issuer is this server own base URL, and the spoofed-issuer-must-not-demote comment shows it is intentional — just confirming the trade-off is understood.

Quality / performance

5. loadEnterpriseAuthConfig(env) is re-parsed multiple times per request — in createEnterpriseAuthMiddleware, in createLazyTokenRateLimiter, and per route handler via configForRequest. Each call re-runs new URL() validation. Cheap, but memoizing the parsed config (env-derived, only invalidated on process restart) would remove redundant work on the hot /mcp path.

6. configForRequest returns not_found as an untyped 404 body while other errors use the OAuth {error, error_description} shape — minor inconsistency, fine for a disabled-feature 404.

Tests

Coverage is strong: unit suites for config validation, the ID-JAG accept/reject matrix (wrong typ/iss/aud/resource, expired, unknown key, HS256 downgrade, replay, allowlist), access-token round-trip/tamper/expiry, /token error codes, scope enforcement including the empty-scope deny-all regression, plus an HTTP-level integration test driving the real Express app against a mock OIDC IdP. That end-to-end test locking in middleware ordering and Authorization-header handling is exactly the right call.

Two small additions worth considering:

• A test asserting the X-Forwarded-For rightmost-IP selection under a repeated-header + comma-list combination (the getClientId normalization is subtle and security-relevant).
• A test that a token issued by one instance verifies against config loaded fresh (guards the shared-secret assumption in item 2).

Verdict

No blocking bugs found. The main things I would want before production reliance are operational, not code: make the serverless replay/rate-limit caveat and the shared-signing-key requirement impossible to miss for operators. Solid, spec-aware implementation.

Reviewed with Claude Code

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed in ad0ccea:

• Jstjoe/2025 10 09 #1 Serverless caveat — now in the README too, as a call-out box in the enterprise auth section (replay detection and /token rate limiting are per-instance best-effort on serverless including Vercel; shared store or edge enforcement for strict guarantees), linking to the guide's security notes.
• Add Claude Code GitHub Workflow #2 Shared signing key — already documented: the env-var table entry for ENTERPRISE_AUTH_SIGNING_KEY states "Must be identical across instances of the same deployment," and the security notes cover rotation semantics (rotation invalidates outstanding tokens → silent re-exchange).
• add API key auth, update docs #6 404 body — aligned to the {error, error_description} shape.
• Suggested test (XFF repeated-header + comma-list) — already exists: rateLimiter.test.ts → "should comma-split repeated X-Forwarded-For headers (array form)" uses exactly that combination (["1.1.1.1, 2.2.2.2", "3.3.3.3, 4.4.4.4"] → 4.4.4.4).
• Suggested test (cross-instance verification) — effectively covered: the routes test verifies tokens issued via env-loaded config against an independently constructed config with the same key, and every integration-test request loads config fresh (issue and verify happen in separate requests).
• add simple authentication and a sample .env #3 TOCTOU / rename tools #4 issuer collision — confirmed understood and intentional, per your own assessment. readme updats: title, try it out section #5 memoization — declining (eleventh ask, standing rationale).

288 tests passing, typecheck clean, CI green on head.

---

Generated by Claude Code

[claude]

Review — Enterprise-Managed Authorization (ID-JAG extension)

Thorough read of src/lib/auth/*, src/lib/middleware/{enterpriseAuth,authenticateBearer,rateLimiter}.ts, and src/server.ts. This is a high-quality, security-conscious PR: the opt-in gating is clean, the fail-closed posture is consistent, and the test matrix (HS256 downgrade, replay, resource/audience/issuer mismatch, empty-scope deny-all, tampered tokens, optional-mode fall-through) covers the paths that matter most. No blocking issues found.

Note: pnpm install is blocked in my review sandbox, so I reviewed by reading rather than executing — I could not independently confirm the "249 passing" claim.

Things done well

• Algorithm confusion protection — ID-JAGs restricted to asymmetric algs; issued access tokens verified HS256-only with an explicit algorithms allowlist. typ header pinning on both sides is a nice touch.
• requiredClaims on both verify paths so a token missing exp/iat/sub/jti can't slip through as eternally valid.
• Validation ordering in validateIdJag burns the jti last, only after signature/resource/client checks pass — a token that fails those checks doesn't waste its replay slot. Correct.
• looksLikeEnterpriseToken fall-through deliberately rejects (not falls through) a token whose iss spoofs this server but fails verification — the comment calls this out and the integration test confirms it. This is the subtle bit most implementations get wrong.
• Empty scope = deny-all preserved end-to-end (issueAccessToken keeps "", parseGrantedScopes("") → []), with an explicit test.
• Header-injection hardening in unauthorized() (headerParam escaping) even though inputs are constants.
• X-Forwarded-For rightmost-IP selection for rate limiting — correct anti-spoofing choice.

Non-blocking suggestions

1. Config re-parsed from env on every request — loadEnterpriseAuthConfig(env) runs in the middleware, in each route handler via configForRequest, and again in createLazyTokenRateLimiter. Each call re-runs URL parsing/validation. Env is static after boot, so consider memoizing the loaded config to avoid repeated work and guarantee all call sites see an identical config within a request.

2. discoverJwksUri caches the discovered jwks_uri for the process lifetime (discoveredJwksUris never expires). If an IdP rotates its jwks_uri, a restart is required — createRemoteJWKSet refreshes keys but not the URI. Worth a comment or a TTL.

3. Replay detection is per-instance/in-memory — acknowledged in the code, but since it's the one materially weaker guarantee on serverless (a jti can be replayed up to N times across N instances within the ID-JAG lifetime), consider surfacing it in docs/enterprise-managed-auth.md's security notes with a pointer to a shared store (Redis) for hardened multi-instance deployments.

4. jti burned before token issuance (intentional): a transient failure in issueAccessToken forces the client back to the IdP for a fresh ID-JAG. Safe default; just flagging the UX tradeoff combined with add simple authentication and a sample .env #3.

5. Minor: when the feature is disabled, POST /token still passes through express.urlencoded before returning 404, and the well-known//token paths now return JSON {error:"not_found"} 404s instead of Express's default. Harmless, but slightly softens the "byte-for-byte unchanged when disabled" claim.

Test coverage

Excellent for the auth logic — the rejection matrix in idJag.test.ts and the E2E enterpriseAuth.integration.test.ts are exactly the right shape. One gap worth considering: an explicit test for the required mode + apiKey query-param path in resolveSkyflowCredentials (returns true without setting req.skyflowCredentials, relying on authenticateBearer downstream) — a non-obvious control-flow handoff that currently seems only indirectly exercised.

Overall: solid, well-scoped, and safe to merge after considering the config-memoization and the multi-instance-replay doc note. Nice work.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed in 1ae5946:

• Add Claude Code GitHub Workflow #2 Discovery-cache lifetime — documented at the cache site: key rotation happens at a stable jwks_uri (jose refetches keys), while relocating the URI itself is a rare IdP reconfiguration — set ENTERPRISE_IDP_JWKS_URI explicitly or restart in that case; serverless recycling bounds staleness in practice.
• Test gap (required mode + apiKey param) — the unit test already exists (enterpriseAuth.test.ts → "lets the apiKey query parameter through in required mode", added a few rounds back), and I've now added the end-to-end version too: enterprise token + ?apiKey=... with no SKYFLOW_API_KEY → 200 through the real app, locking in the middleware→authenticateBearer handoff over the wire.
• add simple authentication and a sample .env #3 Replay doc note — already in place: the guide's security notes state per-instance/best-effort semantics with the shared-store (Redis) pointer, and the README now carries a serverless call-out box linking to it (added last round).
• Jstjoe/2025 10 09 #1 Memoization — declining (twelfth ask, standing rationale). rename tools #4 jti-burn UX — intentional and commented at the call site. readme updats: title, try it out section #5 disabled-state 404s — fair nuance: those paths didn't exist before this PR (they 404'd with Express's default body), so "unchanged when disabled" refers to pre-existing routes; the JSON 404 on new paths is deliberate.

289 tests passing, typecheck clean.

---

Generated by Claude Code

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG extension)

Thorough review of the auth stack (config, idJag, accessTokens, routes, scopes, enterpriseAuth/authenticateBearer middleware, rate limiter, and server.ts wiring). Overall this is high-quality, security-conscious work — clean separation of concerns, pure/injectable functions that are genuinely unit-testable, careful RFC alignment, and thoughtful inline rationale for non-obvious decisions. I found no blocking bugs. Notes below are mostly hardening/documentation suggestions.

Strengths

• Correct crypto hygiene: ID-JAGs restricted to asymmetric algs only (ALLOWED_ID_JAG_ALGORITHMS) and access tokens to HS256 — closes the alg-confusion / key-substitution class. requiredClaims forces exp/iat/sub/jti so a claimless JWT cannot pass as eternally valid.
• Genuinely opt-in & fail-closed: disabled → all routes 404 and middleware is a no-op; enabled-but-misconfigured → 500 per request rather than silently skipping authorization.
• Good token-boundary handling: the enterprise token is consumed and the Authorization header is deleted before authenticateBearer runs, so it cannot be mistaken for a Skyflow credential; authenticateBearer respects pre-resolved req.skyflowCredentials.
• Spoofed-issuer safety in optional mode: a token whose iss claims to be this server but fails verification is rejected (401) instead of falling through to the Skyflow path (enterpriseAuth.ts:193-200).
• Router mounted before the 5MB JSON parser, with /token scoped to a 100kb urlencoded body — nice attack-surface reduction on the unauthenticated endpoint.
• Strict numeric env parsing (digits-only regex instead of parseInt, which would silently accept 900abc), empty-scope preservation (empty string = deny-all, not unrestricted), and WWW-Authenticate header-injection escaping are all careful touches.
• Broad test coverage: ID-JAG acceptance/rejection matrices (wrong typ/iss/aud/resource, expired, unknown key, HS256 downgrade, replay, allowlist), token round-trip/tamper/expiry, RFC 6749 error codes, both middleware modes, and a real E2E flow with a mock OIDC IdP.

Suggestions (non-blocking)

1. Distributed deployments weaken replay + rate limiting (highest-value item). Both seenJtis (idJag.ts) and rateLimitStore (rateLimiter.ts) are per-process in-memory Maps. The code acknowledges this, but the security implication deserves to be stated loudly in the docs: on Vercel/serverless or any multi-instance deploy, jti replay detection is effectively bypassable (a replayed ID-JAG landing on a different instance is not seen) and the /token per-IP rate limit is multiplied by the instance count. The short ID-JAG TTL bounds the replay window, but consider documenting a shared store (e.g. Redis/Upstash) as the production-hardening path in docs/enterprise-managed-auth.md.

2. X-Forwarded-For rightmost-IP trust is topology-dependent. getClientId takes the rightmost XFF entry, correct behind exactly one trusted proxy. Behind zero or multiple proxies the rate-limit key becomes spoofable or wrong. Worth a one-line note that this assumes a single trusted proxy hop (true for Vercel, but self-hosters vary).

3. loadEnterpriseAuthConfig re-parses/re-validates URLs on every request — once in the middleware, again in the lazy /token rate limiter, again in configForRequest. Cheap, but trivially memoizable since config is process-static. Minor.

4. usesEnvVaultFallback edge case (server.ts). When an enterprise-authenticated request supplies its own vault via X-Skyflow-Authorization but leaves placeholder vaultId/vaultUrl query params, the code overrides with process.env.VAULT_ID/VAULT_URL, pairing the server vault id with the user credentials. Narrow/misconfigured case, but a mismatched cluster/vault could produce confusing Skyflow errors. Consider applying the env-vault fallback only when credentials also came from the server side (SKYFLOW_API_KEY), or document the precedence.

5. Minor: a /token request with a JSON content type (instead of form-urlencoded) yields unsupported_grant_type rather than a clearer content-type hint. RFC 6749 mandates form-urlencoded so this is technically correct, just a possible integrator footgun worth a doc callout.

Nice work — the design faithfully implements the extension and the fail-closed posture is exactly right for an auth feature.

Automated review; not executed in this environment (build/test run was sandbox-blocked), so I relied on static analysis plus the PR reported 249-passing-tests + E2E results.

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed in aca737e:

• rename tools #4 Vault/credential pairing — confirmed and fixed (good catch). The env-vault placeholder fallback now applies only when credentials came from the server's SKYFLOW_API_KEY — the deployment's credential and vault belong together. Per-user credentials from X-Skyflow-Authorization are never paired with a guessed vault; and tracing your scenario further exposed that such requests could still demote to the anonymous vault via the placeholder branch, inconsistent with the required-mode strictness decision — enterprise-authenticated requests now get a clear 400 (unsubstituted placeholders + fix guidance) on that path instead. The middleware records the credential source, docs describe the precedence, and an integration test covers the per-user + placeholder 400.
• readme updats: title, try it out section #5 /token content type — doc'd: the curl walkthrough now notes RFC 6749 requires form-urlencoded and JSON bodies are not parsed.
• Jstjoe/2025 10 09 #1 Distributed replay/rate-limit caveat — already documented twice: the guide's security notes (per-instance best-effort, shared store/Redis as the hardening path) and a README call-out box added specifically to make it impossible to miss.
• Add Claude Code GitHub Workflow #2 XFF topology note — already documented: the "Rate limiting and proxies" security note covers the single-trusted-proxy assumption (correct on Vercel), multi-hop bucketing, and the direct-exposure spoofing caveat.
• add simple authentication and a sample .env #3 Memoization — declining (thirteenth ask, standing rationale).

290 tests passing, typecheck clean.

---

Generated by Claude Code

[claude]

PR Review: Enterprise-Managed Authorization (MCP ID-JAG extension)

Reviewed the full auth surface (config, idJag, accessTokens, routes, scopes, enterpriseAuth/authenticateBearer middleware) plus the server.ts wiring. This is a high-quality, security-conscious implementation with genuinely thorough tests. Notes below are mostly polish and a couple of things worth confirming — nothing I would consider a blocker.

🔒 Security — strong

The fundamentals are done right, worth calling out because these are the things most implementations get wrong:

• Algorithm confinement: ID-JAGs restricted to asymmetric algs (ALLOWED_ID_JAG_ALGORITHMS), issued access tokens pinned to HS256. Closes the classic key-confusion / alg downgrade attack.
• requiredClaims on both verifies — a grant without exp/iat/sub/jti cannot pass as eternally valid.
• https-only enforcement for issuer/JWKS URIs (localhost exception for dev), applied even to the JWKS URI pulled from OIDC discovery — good defense against a tampered discovery doc.
• Fail-closed everywhere: misconfigured-but-enabled returns 500 on /mcp and /token, never a silent skip.
• WWW-Authenticate header injection escaping (headerParam) even though inputs are currently constants — nice defensive habit.
• Cache-Control: no-store on /token, rightmost-XFF for rate-limit keying, jti replay burned before issuance. All correct.
• Optional-mode fall-through is subtle but correct: a token whose iss claims to be this server but fails verification is rejected (401), never demoted into the Skyflow credential path.

🐛 Worth confirming

1. Scope claim semantics can silently deny all tools (scopes.ts + server.ts). The convention is "each scope value names a permitted tool," and a present scope claim restricts to exactly those names. So if the IdP ID-JAG carries conventional OIDC scopes (e.g. scope: "openid profile email"), isToolPermitted("de-identify", ["openid","profile","email"]) is false and every tool is denied — with no obvious signal that the scope vocabulary was wrong. Since the scope claim comes from the enterprise IdP policy engine (outside this repo control), this is an easy footgun. Consider (a) documenting explicitly in docs/enterprise-managed-auth.md that IdP policy must emit tool-name scopes and nothing else, or (b) namespacing tool scopes (e.g. mcp:tool:de-identify) so unrelated scopes are ignored rather than causing a deny-all. The empty-scope="deny all" case itself is correct and nicely handled — this is specifically about mixed/standard scopes.

2. Protected-resource wildcard route ignores the suffix (routes.ts:184-187). The *resourcePath route returns identical metadata for any path suffix. RFC 9728 expects the suffix to correspond to the resource path; a wrong suffix returns 200 with the /mcp metadata rather than 404. Harmless today (single resource) — minor spec-conformance nit.

⚡ Performance / minor

3. loadEnterpriseAuthConfig(env) runs on every request — re-parsing env and re-running new URL(...) on each /mcp hit (and twice per /token: lazy limiter enabled-check plus handler). Env is static after boot; the JWKS resolver is already cached, so consider memoizing the parsed config the same way (with a test-only reset mirroring resetIdJagCaches). Not hot-path-critical, just cheap to fix.

4. IdP-down case re-runs discovery every request — discoveredJwksUris only caches on success, so an unreachable discovery endpoint means each /token call pays a fresh 5s-timeout fetch. The /token rate limiter bounds the blast radius, so acceptable — just flagging.

✅ Tests

Coverage is excellent — 82 new tests spanning config validation, the ID-JAG accept/reject matrix (wrong typ/iss/aud/resource, expired, unknown key, HS256 downgrade, replay, allowlist), access-token round-trip/tamper/expiry, /token RFC 6749 error codes, both middleware modes, and the authenticateBearer no-overwrite regression. The mock-OIDC end-to-end smoke test is a nice touch. The one gap (no live Okta validation) is called out and reasonable.

Note: I reviewed statically — dependency install was blocked in my environment, so I did not execute vitest/tsc myself.

Summary

Clean separation of concerns, opt-in with byte-for-byte unchanged default behavior, and careful attention to the auth details that matter. The scope-vocabulary footgun (item 1) is the one I would most want addressed (docs or namespacing) before this meets an IdP whose policies emit standard OIDC scopes. Nice work.

Generated with Claude Code

[github-actions]

✅ Gitleaks Findings: No secrets detected. Safe to proceed!

[jstjoe]

Addressed in fa8dff2:

1. Scope-vocabulary footgun — fixed with a middle path between your (a) and (b). Standard OIDC identity scopes (openid, profile, email, address, phone, offline_access) are now filtered out before tool enforcement — they describe identity data, not tool policy, so an IdP's default openid profile email grant no longer denies every tool (a claim of only identity scopes behaves like no claim). Mixed claims keep just the tool scopes. Crucially, unrecognized custom scopes still fail closed (mcp:de-identify → deny, with the granted scopes named in the insufficient_scope error) — blanket ignore-unrecognized would have flipped misconfigured namespacing into fail-open. Docs updated; 5 new unit tests cover identity-only, mixed, and custom-scope claims. Note the denial was never fully silent — the error result names the tool and the granted scopes — but the default-OIDC-scopes case deserved to work, not just be diagnosable.
2. PRM wildcard suffix — fixed. The metadata route now 404s for suffixes that don't match the configured resource path, per RFC 9728, instead of answering any path. Unit tests added for the valid suffix and a wrong suffix.
3. Memoization — declining (fourteenth ask, standing rationale).
4. Discovery re-fetch when the IdP is down — leaving per your own assessment (bounded by the /token rate limiter and 5s timeout).

295 tests passing, typecheck clean.

---

Generated by Claude Code

[claude]

Code Review — Enterprise-Managed Authorization (ID-JAG)

Reviewed the full diff (src/lib/auth/*, enterpriseAuth.ts, rateLimiter.ts, authenticateBearer.ts, server.ts, docs, and tests). This is a genuinely high-quality PR — the security posture is careful, it is cleanly opt-in, and the 19-commit iteration history shows most of the sharp edges were already found and addressed. Overall: approve with minor, non-blocking suggestions. 🎯

Strengths

• Fails closed everywhere it matters. Misconfig gives 500 (never skips auth), missing exp/iat guarded via requiredClaims, algorithm allowlists on both sides.
• Key-confusion protection is correct. ID-JAGs restricted to asymmetric algs (ALLOWED_ID_JAG_ALGORITHMS), issued access tokens restricted to HS256 — so the IdP public JWKS material cannot be abused as an HMAC secret.
• Scheme validation on the discovered jwks_uri, not just configured URLs — closes a real hole where a compromised discovery doc could point keys at plaintext http.
• Empty-scope round-trip (scope of "" means deny-all, not unrestricted) and the OIDC-identity-scope filtering are subtle and correct.
• No behavior change when disabled is well-preserved: router 404s, middleware no-ops, authenticateBearer guarded by if (req.skyflowCredentials).
• Docs + CLAUDE.md are thorough, including correctly un-listing SKYFLOW_API_KEY from "Removed variables" now that it is reintroduced with new semantics, and documenting the middleware-ordering invariant.
• Defensive WWW-Authenticate value escaping, no-store on /token, jti burned before issuance, per-instance caveats documented.

Suggestions (non-blocking)

1. Config is re-parsed per request. loadEnterpriseAuthConfig() runs on every /mcp request and up to ~2x per /token request (lazy limiter + configForRequest), each doing env reads and several new URL() parses. Since it is derived purely from process.env (static after boot), consider memoizing the parsed result once. Pure micro-optimization — correctness is fine, and re-reading keeps tests injectable.

2. tools/list is unfiltered by scope. Acknowledged in the commit history and code comments — flagging only so it is a conscious product decision: a token scoped to de-identify still sees re-identify in discovery (enforcement is at invocation). Fine for this shared-registry design; just worth a line in the user-facing docs so clients do not treat listing as authorization.

3. Rate limiter / replay cache are per-instance in-memory. Well-documented as best-effort on serverless/multi-instance, which is the right call for now. If a hard limit is ever needed, the createIpRateLimiter seam makes a shared-store backend easy to slot in.

4. looksLikeEnterpriseToken in optional mode routes on an unverified iss match. Correct and safe (spoofed-issuer tokens are rejected, never demoted to the Skyflow path), but it means a legitimate Skyflow bearer JWT that happened to carry iss === config.issuer would be forced down the enterprise path. Practically impossible; noting for completeness.

Testing

• 82 new unit tests + an HTTP integration test covering the acceptance/rejection matrices, RFC 6749 error codes, scope enforcement, replay, tampered tokens, and middleware ordering — coverage looks excellent by inspection, including the tricky empty-scope and placeholder-vault fall-through paths.
• I was unable to execute pnpm test in this review environment (sandbox approval), so this assessment is from code reading rather than a green run — recommend confirming CI is green before merge.

Nice work — the fail-closed discipline and the attention to the ID-JAG / RFC details are the standout parts here.

Generated with Claude Code

[jstjoe]

Thanks — all four items are already settled in earlier rounds, so no changes this time:

1. Config memoization — declining (fifteenth ask; standing rationale: per-request reads are load-bearing for the integration test's runtime env manipulation, and the cost is dwarfed by per-request Skyflow SDK construction).
2. tools/list unfiltered — the doc line you suggest already exists: docs/enterprise-managed-auth.md → Scope enforcement → "tools/list is not filtered by scope … enforcement happens at invocation time."
3. Per-instance state — documented in the guide's security notes and a README call-out; agreed the createIpRateLimiter seam is where a shared store would slot in.
4. Issuer collision in optional mode — intentional and commented (spoofed issuers must reject, never demote), as you noted.

On CI: green on the current head — unit tests, coverage, gitleaks, and the Vercel preview all pass; 295 tests locally with typecheck clean.

---

Generated by Claude Code