Guest sessions#2003
Conversation
dosubot
Bot
added
the
size:XL
chelojimenez
temporarily deployed
to
preview-pr-2003
GitHub Actions
Inactive
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Internal previewPreview URL: https://mcp-inspector-pr-2003.up.railway.app |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e5d15e696
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "lookup_or_create", | ||
| legacyToken, | ||
| ); | ||
| if (session) cachedSession = session; |
There was a problem hiding this comment.
A forceRefreshGuestSession() call no longer protects against an older getOrCreateGuestSession() request finishing later and writing a stale token back into cachedSession. forceRefreshGuestSession() only nulls inFlightRequest, which does not cancel the original fetch, and this unconditional assignment lets the late response overwrite the freshly refreshed JWT; subsequent requests can reuse that stale token and re-trigger 401/retry loops.
Useful? React with 👍 / 👎.
| if (response.status === 204 || response.status === 404) { | ||
| return { kind: "miss", setCookies }; |
There was a problem hiding this comment.
Mapping all upstream 404 responses to kind: "miss" causes /api/web/guest-session to return 204 instead of an error, which silently disables guest auth when the upstream session endpoint is missing/misconfigured. For lookup_or_create callers this is not a normal miss condition, so the current behavior hides operational failures and makes client behavior look like “no guest session” rather than surfacing a recoverable server error.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughGuest-session handling was refactored from localStorage persistence to an in-memory cache with cookie-backed identity and deduplicated requests. The client introduces Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/lib/guest-session.ts`:
- Around line 121-128: The async result of requestGuestSession can overwrite
cachedSession after clearGuestSession()/forceRefreshGuestSession(); add a
generation/epoch guard or abort logic so stale responses are ignored before
committing. Concretely: introduce a numeric epoch (e.g., guestEpoch) incremented
inside clearGuestSession and forceRefreshGuestSession, capture the current epoch
at the start of getOrCreateGuestSession/getExistingGuestBearerToken (or when
creating inFlightRequest), and before assigning cachedSession or returning set
the check "if (capturedEpoch !== guestEpoch) then abort/return undefined" to
prevent resurrecting old tokens; alternatively use an AbortController captured
and signalled on clear/forceRefresh and reject older requests before writing
cachedSession. Apply this guard consistently where inFlightRequest,
cachedSession, getOrCreateGuestSession, and getExistingGuestBearerToken are
mutated.
In `@mcpjam-inspector/server/routes/web/guest-session.ts`:
- Around line 115-120: The current GuestSessionFetchContext construction
forwards the entire cookie header (c.req.header("cookie")) which leaks unrelated
cookies; extract only the __Host-mcpjam_guest_session cookie value and set
context.cookie to either "__Host-mcpjam_guest_session=<value>" or null if
missing. Locate where GuestSessionFetchContext is built (the context object
creation) and replace the cookie assignment to parse c.req.header("cookie") for
the __Host-mcpjam_guest_session pair (or use a small helper to
getCookieValue("cookieHeader", "__Host-mcpjam_guest_session")) so only that
single cookie is forwarded upstream.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a3d6d59-9f5d-4f79-bed1-c01c13b5e919
📒 Files selected for processing (11)
mcpjam-inspector/client/src/hooks/__tests__/useRegistryServers.guest-merge.test.tsxmcpjam-inspector/client/src/hooks/__tests__/useRegistryServers.test.tsxmcpjam-inspector/client/src/hooks/useRegistryServers.tsmcpjam-inspector/client/src/lib/__tests__/guest-session.test.tsmcpjam-inspector/client/src/lib/guest-session.tsmcpjam-inspector/server/routes/web/__tests__/guest-session.test.tsmcpjam-inspector/server/routes/web/guest-session.tsmcpjam-inspector/server/utils/__tests__/guest-session-source.test.tsmcpjam-inspector/server/utils/convex-guest-auth-sync.tsmcpjam-inspector/server/utils/guest-session-pepper.tsmcpjam-inspector/server/utils/guest-session-source.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
chelojimenez
temporarily deployed
to
preview-pr-2003
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/hooks/useRegistryServers.ts`:
- Around line 419-443: The effect currently uses refs (mergeRanRef,
mergeInFlightRef) so a transient failure leaves mergeRanRef false? actually
comment says transient failure becomes permanent — fix by only marking the merge
as done on success and/or surface completion via React state so the effect can
re-run: remove or delay setting mergeRanRef.current = true until after await
loadCatalog() (i.e., only set after successful mergeGuestRegistryStars and
loadCatalog), and/or replace mergeRanRef with a useState flag (e.g., [mergeRan,
setMergeRan]) included in the effect dependencies so the effect will retry on
subsequent renders when enabled/isAuthenticated/loadCatalog change; ensure
mergeInFlightRef is still reset in finally and do not set mergeRan on error.
Reference getExistingGuestBearerToken, mergeGuestRegistryStars,
clearGuestSession, resetTokenCache, loadCatalog, mergeRanRef, mergeInFlightRef.
In `@mcpjam-inspector/client/src/lib/guest-session.ts`:
- Around line 141-153: Multiple callers are independently calling
consumeLegacyToken() and racing to forward the same legacy cookie; change the
flow so consumeLegacyToken() is called exactly once per logical operation and
that same legacyToken value is passed into all requestGuestSession(...) calls
(e.g., the lookup_or_create, lookup_only, and forceRefresh branches), and mark
legacyMigrationConsumed as true immediately when you consume the token (or
before awaiting the outbound request) to prevent a concurrent caller from
reading the same token; use the existing currentInFlight / sessionGeneration
logic to serialize callers and ensure all code paths (the blocks around
requestGuestSession(...) at the current snippet and the other spots noted)
accept an externally captured legacyToken instead of calling
consumeLegacyToken() themselves.
In `@mcpjam-inspector/server/utils/local-secret-store.ts`:
- Around line 62-83: The current getOrCreateLocalSecret function falls through
to local-file behavior for any NODE_ENV that's not "production" or "test";
change this to an explicit allowlist so only "development" uses
loadPersistedLocalSecret/persistLocalSecret: after checking env var
(spec.envVar), if NODE_ENV === "development" return
loadPersistedLocalSecret(spec) || persistLocalSecret(spec), otherwise throw new
Error(spec.productionErrorMessage) so non-dev environments
(staging/preview/ci/unset) surface an error instead of silently regenerating
secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 79768e3f-bcc5-493d-8233-99c82fb82125
📒 Files selected for processing (10)
mcpjam-inspector/client/src/hooks/useRegistryServers.tsmcpjam-inspector/client/src/lib/__tests__/guest-session.test.tsmcpjam-inspector/client/src/lib/guest-session.tsmcpjam-inspector/server/routes/web/__tests__/guest-session.test.tsmcpjam-inspector/server/routes/web/guest-session.tsmcpjam-inspector/server/utils/__tests__/guest-session-source.test.tsmcpjam-inspector/server/utils/guest-session-pepper.tsmcpjam-inspector/server/utils/guest-session-secret.tsmcpjam-inspector/server/utils/guest-session-source.tsmcpjam-inspector/server/utils/local-secret-store.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/server/routes/web/tests/guest-session.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- mcpjam-inspector/server/routes/web/guest-session.ts
- mcpjam-inspector/client/src/lib/tests/guest-session.test.ts
- mcpjam-inspector/server/utils/tests/guest-session-source.test.ts
- mcpjam-inspector/server/utils/guest-session-source.ts
| const legacyToken = consumeLegacyToken(); | ||
| const generation = sessionGeneration; | ||
| inFlightRequest = (async () => { | ||
| try { | ||
| const response = await fetch("/api/web/guest-session", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| console.error( | ||
| "Failed to create guest session:", | ||
| response.status, | ||
| response.statusText, | ||
| ); | ||
| return null; | ||
| } | ||
| // Initialized to a placeholder so TS sees a definite assignment; the | ||
| // real value is assigned synchronously below before the IIFE's finally | ||
| // (which references currentInFlight via closure) can run. | ||
| let currentInFlight: Promise<GuestSession | null> = Promise.resolve(null); | ||
|
|
||
| const session: GuestSession = await response.json(); | ||
| // Only write if no force-refresh has invalidated this generation | ||
| if (sessionGeneration === generation) { | ||
| writeToStorage(session); | ||
| currentInFlight = (async () => { | ||
| try { | ||
| const session = await requestGuestSession( | ||
| "lookup_or_create", | ||
| legacyToken, | ||
| ); |
There was a problem hiding this comment.
Serialize legacy-token forwarding across request modes.
lookup_or_create, lookup_only, and forceRefresh each call consumeLegacyToken() independently, and legacyMigrationConsumed is only flipped after the response. Two concurrent callers can therefore capture and send the same legacyToken before the first Set-Cookie lands, which makes the migration path race itself on a cold browser.
Also applies to: 199-205, 258-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/guest-session.ts` around lines 141 - 153,
Multiple callers are independently calling consumeLegacyToken() and racing to
forward the same legacy cookie; change the flow so consumeLegacyToken() is
called exactly once per logical operation and that same legacyToken value is
passed into all requestGuestSession(...) calls (e.g., the lookup_or_create,
lookup_only, and forceRefresh branches), and mark legacyMigrationConsumed as
true immediately when you consume the token (or before awaiting the outbound
request) to prevent a concurrent caller from reading the same token; use the
existing currentInFlight / sessionGeneration logic to serialize callers and
ensure all code paths (the blocks around requestGuestSession(...) at the current
snippet and the other spots noted) accept an externally captured legacyToken
instead of calling consumeLegacyToken() themselves.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 943e95f809
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (response.status === 204 || response.status === 404) { | ||
| if (legacyToken) deleteLegacyToken(); | ||
| return null; |
There was a problem hiding this comment.
requestGuestSession() treats both 204 and 404 as definitive misses and immediately calls deleteLegacyToken(). A 404 in lookup_or_create is not definitive (it can come from a temporarily missing/misrouted /api/web/guest-session endpoint), so this path permanently discards the only migration token and prevents retrying migration after the server issue is fixed. Keep the legacy token for unexpected 404 responses (or only clear it for confirmed success / explicit lookup-only misses).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 943e95f. Configure here.
1 participant
Note
High Risk
Touches security-sensitive session plumbing (cookie forwarding, token caching, and guest migration) across both client and server, so mistakes could break guest continuity or inadvertently leak/strip cookies. Also adds retry/backoff behavior that could change request volume and edge-case timing.
Overview
Guest sessions are now cookie-backed. The client
guest-sessionmodule stops persisting JWTs tolocalStorage, keeps tokens in memory with deduped in-flight requests, addsgetExistingGuestBearerToken()(lookup_only) to avoid minting new guests, and implements one-time legacylocalStoragetoken migration vialegacyTokenplus a generation guard to prevent stale in-flight responses from resurrecting cleared tokens.Server guest-session proxying is tightened and expanded.
POST /api/web/guest-sessionnow parsesmode/legacyToken, forwards browser context (but only the__Host-mcpjam_guest_sessioncookie) to upstream, passes through upstreamSet-Cookie, returns204on lookup misses, and distinguishes403revocations from other upstream errors. Convex provisioning now also setsGUEST_SESSION_HASH_PEPPER, with new reusablelocal-secret-storefor dev-only secret persistence.Registry guest-star merge is safer.
useRegistryServersswitches tolookup_onlybefore merging guest stars after sign-in, adds bounded retries with backoff, and includes new tests for the merge/no-guest cases.Reviewed by Cursor Bugbot for commit 3bac0c6. Bugbot is set up for automated code reviews on this repo. Configure here.