Refine hosted OAuth chat session flow#1936
Conversation
dosubot
Bot
added
the
size:XXL
✅ 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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c099d68. Configure here.
![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: c099d689aa
ℹ️ 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".
| } else if (isVaultBacked) { | ||
| status = existing?.status === "ready" ? "ready" : "verifying"; | ||
| errorMessage = null; |
There was a problem hiding this comment.
Switching the fallback state to verifying whenever isVaultBacked is true causes guest chatbox sessions (which always have chatboxToken) to skip the initial needs_auth state even before any OAuth consent has happened. That triggers immediate validation attempts, then error states on first load, and also prevents the welcome-first OAuth onboarding path from rendering because useChatboxHostIntroGate only treats all-needs_auth as first-consent idle. In practice, first-time OAuth chatboxes can jump straight to error/“Authorize again” instead of the expected initial consent flow.
Useful? React with 👍 / 👎.
WalkthroughHosted OAuth callback handling now detects guest chatbox session callbacks and, for those, fetches a guest bearer token and forwards it as 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/components/hosted/ChatboxChatPage.tsx (1)
66-80:⚠️ Potential issue | 🟠 MajorDon't silently downgrade signed-in viewers to guest auth.
getHostedBearerHeader()falls back to a guest bearer on anygetAccessToken()failure. That means a transient WorkOS/token-refresh error can bootstrap the chatbox as a guest session, which is likely to flip member-only flows into misleading “guest blocked” or “access denied” states even though the viewer is signed in.Possible fix
-async function getHostedBearerHeader( - getAccessToken: () => Promise<string | undefined | null> -): Promise<string | null> { +async function getHostedBearerHeader( + getAccessToken: () => Promise<string | undefined | null>, + isAuthenticated: boolean +): Promise<string | null> { try { const workOsToken = await getAccessToken(); if (workOsToken) { return `Bearer ${workOsToken}`; } } catch { - // Fall through to guest auth. + if (isAuthenticated) { + throw new Error("Unable to refresh your sign-in session."); + } } const guestToken = await getGuestBearerToken(); return guestToken ? `Bearer ${guestToken}` : null; }-const authorization = await getHostedBearerHeader(getAccessToken); +const authorization = await getHostedBearerHeader( + getAccessToken, + isAuthenticated +);Also applies to: 444-448
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/hosted/ChatboxChatPage.tsx` around lines 66 - 80, getHostedBearerHeader currently treats any exception from getAccessToken as if the user is unauthenticated and falls back to guest auth; change it so only an explicit falsy result from getAccessToken triggers guest fallback. Specifically, remove or rethrow the try/catch around the await getAccessToken() (or in the catch rethrow the error) so exceptions propagate instead of falling through to getGuestBearerToken(), and keep the existing logic that returns `Bearer ${workOsToken}` when getAccessToken() resolves to a token; apply the same change to the related block at lines referenced (444-448) so transient token refresh errors do not silently downgrade signed-in viewers to guest sessions.mcpjam-inspector/client/src/App.tsx (1)
499-552:⚠️ Potential issue | 🟠 MajorGuard against re-spending the single-use OAuth
codewhenisAuthenticatedtransitions during callback processing.When this effect re-runs due to an
isAuthenticatedchange, the cleanup function at line 549 only setscancelled = true, which prevents the result handlers (finalizeHostedOAuth,setHostedOAuthHandling) from executing. However, the in-flight network request itself continues unaborted. If Convex auth resolves whilecompleteHostedOAuthCallbackorhandleOAuthCallbackis mid-request, the effect re-runs, extracts the samecodefrom the URL params, and submits it again. Authorization servers reject reused codes, so the second request fails—the second run's.catch()fires with "Authorization could not be completed," even though the first request succeeded. The first run's success is silently discarded by thecancelledguard.This is a realistic scenario for hosted chatbox guests who authenticate during the callback flow. Both callback functions lack signal support and internal code-reuse guards, making them vulnerable.
A small in-flight sentinel (checking whether the current
codeis already being exchanged) or passing anAbortControllersignal would prevent duplicate submissions.🛡️ Example in-flight guard
+ const oauthCallbackInFlightRef = useRef<string | null>(null); useEffect(() => { const callbackContext = getHostedOAuthCallbackContext(); if (!callbackContext || callbackContext.surface === "workspace") { setHostedOAuthHandling(false); return; } const urlParams = new URLSearchParams(window.location.search); const code = urlParams.get("code"); const error = urlParams.get("error"); const state = urlParams.get("state"); + if (code && oauthCallbackInFlightRef.current === code) { + // Another invocation is already exchanging this code; let it finish. + return; + } + if (code) oauthCallbackInFlightRef.current = code; + let cancelled = false; // ... completeCallback.finally(() => { + oauthCallbackInFlightRef.current = null; if (!cancelled) setHostedOAuthHandling(false); }); }, [isAuthenticated]);
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/App.tsx (1)
505-518: Optional: lift the guest-bearer block out of the IIFE for readability.The
(async () => { ... })()invocation works, but inlining an async IIFE inside a ternary makes the control flow harder to scan than it needs to be. A small named helper would keepcompleteCallbacka flat assignment and put the "fetch guest bearer, then complete" sequence in one place — easier to add tests around, easier to add the in-flight guard suggested separately, and one fewer Promise wrapper.♻️ Sketch
- const completeCallback = shouldUseHostedCompletion - ? (async () => { - const guestAuthorizationHeader = - !isAuthenticated && callbackContext.chatboxToken - ? await getGuestBearerToken() - : null; - return completeHostedOAuthCallback(callbackContext, code, { - callbackState: state, - onTraceUpdate: handleLiveOAuthTrace, - authorizationHeader: guestAuthorizationHeader - ? `Bearer ${guestAuthorizationHeader}` - : undefined, - }); - })() - : handleOAuthCallback(code, { - onTraceUpdate: handleLiveOAuthTrace, - }); + const runHostedCompletion = async () => { + const guestBearer = + !isAuthenticated && callbackContext.chatboxToken + ? await getGuestBearerToken() + : null; + return completeHostedOAuthCallback(callbackContext, code, { + callbackState: state, + onTraceUpdate: handleLiveOAuthTrace, + authorizationHeader: guestBearer ? `Bearer ${guestBearer}` : undefined, + }); + }; + + const completeCallback = shouldUseHostedCompletion + ? runHostedCompletion() + : handleOAuthCallback(code, { onTraceUpdate: handleLiveOAuthTrace });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/App.tsx` around lines 505 - 518, The inline async IIFE assigned to completeCallback makes control flow hard to read; extract the guest-bearer fetch + hosted completion logic into a named async helper (e.g., fetchGuestAndComplete or completeWithGuestIfNeeded) and then set completeCallback = shouldUseHostedCompletion ? fetchGuestAndComplete(callbackContext, code, state) : ...; inside the helper, use isAuthenticated and callbackContext.chatboxToken to call getGuestBearerToken() when needed, build the authorizationHeader string, and call completeHostedOAuthCallback(callbackContext, code, { callbackState: state, onTraceUpdate: handleLiveOAuthTrace, authorizationHeader }); this keeps completeCallback a flat assignment and makes the sequence testable and readable.mcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts (1)
219-242:isVaultBackedcleanly unifies authenticated and chatbox flows.Folding
isAuthenticated || !!chatboxTokeninto a single derived flag and threading it throughbuildHostedOAuthStateMap, the polling effects, and the deps array reads well. One tiny redundancy worth noting (no fix required):authorizeServerlists bothisAuthenticatedandisVaultBackedin its deps (Line 482‑489) — since the latter is derived from the former pluschatboxToken(also listed),isAuthenticatedis effectively dead weight in that array. Harmless, just a touch of noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts` around lines 219 - 242, The derived flag isVaultBacked (isAuthenticated || !!chatboxToken) makes isAuthenticated redundant in authorizeServer's dependency list; update the authorizeServer callback to remove isAuthenticated from its deps and rely on isVaultBacked and chatboxToken (and any other true inputs) instead, ensuring authorizeServer still references buildHostedOAuthStateMap / oauthStateByServerIdRef / processingServerIdsRef as before so behavior doesn't change.mcpjam-inspector/client/src/lib/apis/web/servers-api.ts (1)
34-57: Recommended: tighten the contract soserverNameOrIdandhostedContext.serverIdcannot drift apart.When
hostedContextis supplied,serverNameOrIdis silently discarded — the request keys offhostedContext.serverIdinstead. Today's only caller (validateWithRetryinuse-hosted-oauth-gate.ts) passes the same value through both channels, but a future caller could pass mismatched arguments and have the positional one quietly ignored. Consider either dropping the positional parameter whenhostedContextis present (overloads) or asserting equality, so the API can't lie to its callers.♻️ One possible shape
export async function validateHostedServer( serverNameOrId: string, oauthAccessToken?: string, clientCapabilities?: Record<string, unknown>, hostedContext?: HostedServerValidateContext ): Promise<HostedServerValidateResponse> { + if (hostedContext && hostedContext.serverId !== serverNameOrId) { + // Dev-time guard: keeps the positional arg and the context aligned + // so callers can't silently drop one of them. + console.warn( + "[validateHostedServer] serverNameOrId does not match hostedContext.serverId", + { serverNameOrId, contextServerId: hostedContext.serverId } + ); + } const request: Record<string, unknown> = hostedContext ? {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/apis/web/servers-api.ts` around lines 34 - 57, The function validateHostedServer currently ignores the positional serverNameOrId when hostedContext is provided, which can allow callers to pass mismatched values; update validateHostedServer to enforce consistency by asserting that when hostedContext is present its serverId equals serverNameOrId (throw a clear error if not) or remove the positional parameter in that branch (use overloads) so callers can't supply conflicting values; locate the logic in validateHostedServer (and adjust callers such as validateWithRetry in use-hosted-oauth-gate.ts if you change the signature) and ensure either an equality check between serverNameOrId and hostedContext.serverId or a single-source API so the request is built only from the validated/authoritative value (hostedContext.serverId or buildHostedServerRequest).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mcpjam-inspector/client/src/components/hosted/ChatboxChatPage.tsx`:
- Around line 66-80: getHostedBearerHeader currently treats any exception from
getAccessToken as if the user is unauthenticated and falls back to guest auth;
change it so only an explicit falsy result from getAccessToken triggers guest
fallback. Specifically, remove or rethrow the try/catch around the await
getAccessToken() (or in the catch rethrow the error) so exceptions propagate
instead of falling through to getGuestBearerToken(), and keep the existing logic
that returns `Bearer ${workOsToken}` when getAccessToken() resolves to a token;
apply the same change to the related block at lines referenced (444-448) so
transient token refresh errors do not silently downgrade signed-in viewers to
guest sessions.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 505-518: The inline async IIFE assigned to completeCallback makes
control flow hard to read; extract the guest-bearer fetch + hosted completion
logic into a named async helper (e.g., fetchGuestAndComplete or
completeWithGuestIfNeeded) and then set completeCallback =
shouldUseHostedCompletion ? fetchGuestAndComplete(callbackContext, code, state)
: ...; inside the helper, use isAuthenticated and callbackContext.chatboxToken
to call getGuestBearerToken() when needed, build the authorizationHeader string,
and call completeHostedOAuthCallback(callbackContext, code, { callbackState:
state, onTraceUpdate: handleLiveOAuthTrace, authorizationHeader }); this keeps
completeCallback a flat assignment and makes the sequence testable and readable.
In `@mcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts`:
- Around line 219-242: The derived flag isVaultBacked (isAuthenticated ||
!!chatboxToken) makes isAuthenticated redundant in authorizeServer's dependency
list; update the authorizeServer callback to remove isAuthenticated from its
deps and rely on isVaultBacked and chatboxToken (and any other true inputs)
instead, ensuring authorizeServer still references buildHostedOAuthStateMap /
oauthStateByServerIdRef / processingServerIdsRef as before so behavior doesn't
change.
In `@mcpjam-inspector/client/src/lib/apis/web/servers-api.ts`:
- Around line 34-57: The function validateHostedServer currently ignores the
positional serverNameOrId when hostedContext is provided, which can allow
callers to pass mismatched values; update validateHostedServer to enforce
consistency by asserting that when hostedContext is present its serverId equals
serverNameOrId (throw a clear error if not) or remove the positional parameter
in that branch (use overloads) so callers can't supply conflicting values;
locate the logic in validateHostedServer (and adjust callers such as
validateWithRetry in use-hosted-oauth-gate.ts if you change the signature) and
ensure either an equality check between serverNameOrId and
hostedContext.serverId or a single-source API so the request is built only from
the validated/authoritative value (hostedContext.serverId or
buildHostedServerRequest).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b9a691e-bbff-4df9-95eb-191f50a8d389
📒 Files selected for processing (14)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/ChatTabV2.tsxmcpjam-inspector/client/src/components/__tests__/ChatTabV2.history-sync.test.tsxmcpjam-inspector/client/src/components/chatboxes/ChatboxEditor.tsxmcpjam-inspector/client/src/components/chatboxes/__tests__/ChatboxEditor.test.tsxmcpjam-inspector/client/src/components/chatboxes/builder/ChatboxBuilderView.tsxmcpjam-inspector/client/src/components/hosted/ChatboxChatPage.tsxmcpjam-inspector/client/src/components/hosted/__tests__/ChatboxChatPage.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-chat-session.hosted.test.tsxmcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.tsmcpjam-inspector/client/src/hooks/use-chat-session.tsmcpjam-inspector/client/src/lib/apis/web/servers-api.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
mcpjam-inspector/client/src/App.tsx (2)
509-532: Guard the awaited bearer fetch against unexpected throws.
getGuestBearerTokencurrently swallows fetch failures and resolves tonull, so thenullbranch covers most realities. Still, a straylocalStorage.getItemthrow (e.g., disabled storage in private modes) could escape and slip into the outer.catch, where it would surface as the generic"Authorization could not be completed. Try again."rather than the curated guest-expired copy. A smalltry/catcharound the await preserves the precise UX for that fringe path.🛡️ Optional defensive wrap
const completeCallback = shouldUseHostedCompletion ? (async () => { let authorizationHeader: string | undefined; if (isGuestChatboxSessionCallback) { - const guestBearerToken = await getGuestBearerToken(); + let guestBearerToken: string | null = null; + try { + guestBearerToken = await getGuestBearerToken(); + } catch { + guestBearerToken = null; + } if (!guestBearerToken) { return { success: false, error: "Your guest session expired. Reopen the chatbox link and try again.", }; } authorizationHeader = `Bearer ${guestBearerToken}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/App.tsx` around lines 509 - 532, The guest-token fetch in the completeCallback branch can throw (e.g., localStorage access), so wrap the await getGuestBearerToken() call in a try/catch inside the isGuestChatboxSessionCallback branch (within completeCallback) and treat any thrown error as a missing token: set guestBearerToken to null or directly return the same { success: false, error: "Your guest session expired. Reopen the chatbox link and try again." } response so the UX remains the curated guest-expired message; keep the rest of the authorizationHeader logic and subsequent call to completeHostedOAuthCallback unchanged.
514-520: Add a test for guest session error preservation. The sanitizer will pass"Your guest session expired. Reopen the chatbox link and try again."through verbatim—it matches no substitution patterns (lines 127–167). The existing test"preserves already-safe plain-language errors"validates this behavior generically. A dedicated test case would document explicitly that this message survives intact and guard against future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/App.tsx` around lines 514 - 520, Add a dedicated unit test that verifies the guest session error string "Your guest session expired. Reopen the chatbox link and try again." is preserved verbatim by the sanitizer; locate the sanitizer usage (the function tested by the existing "preserves already-safe plain-language errors" test) and add a new test case that passes this exact message through the sanitizer and asserts the output equals the original string so future changes to substitution patterns cannot alter it.mcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts (1)
499-507: Redundant but harmless dependency entry.Since
isVaultBacked = isAuthenticated || !!chatboxToken, listing bothisAuthenticatedandisVaultBackedin theuseCallbackdeps is redundant. Not a bug — React will simply re-evaluate equivalently — but trimmingisAuthenticated(it isn't read directly insideauthorizeServeroutside the markeraccessScopebranch, which already uses it) would reduce noise. Optional polish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts` around lines 499 - 507, The dependency list for the useCallback that defines authorizeServer includes a redundant isAuthenticated entry because isVaultBacked is derived as isAuthenticated || !!chatboxToken; remove isAuthenticated from the dependency array so the deps become [isVaultBacked, pendingKey, chatboxToken, shareToken, surface, workspaceId], leaving authorizeServer, isVaultBacked, pendingKey, chatboxToken, shareToken, surface, and workspaceId referenced as before to preserve correct re-evaluation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 509-532: The guest-token fetch in the completeCallback branch can
throw (e.g., localStorage access), so wrap the await getGuestBearerToken() call
in a try/catch inside the isGuestChatboxSessionCallback branch (within
completeCallback) and treat any thrown error as a missing token: set
guestBearerToken to null or directly return the same { success: false, error:
"Your guest session expired. Reopen the chatbox link and try again." } response
so the UX remains the curated guest-expired message; keep the rest of the
authorizationHeader logic and subsequent call to completeHostedOAuthCallback
unchanged.
- Around line 514-520: Add a dedicated unit test that verifies the guest session
error string "Your guest session expired. Reopen the chatbox link and try
again." is preserved verbatim by the sanitizer; locate the sanitizer usage (the
function tested by the existing "preserves already-safe plain-language errors"
test) and add a new test case that passes this exact message through the
sanitizer and asserts the output equals the original string so future changes to
substitution patterns cannot alter it.
In `@mcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts`:
- Around line 499-507: The dependency list for the useCallback that defines
authorizeServer includes a redundant isAuthenticated entry because isVaultBacked
is derived as isAuthenticated || !!chatboxToken; remove isAuthenticated from the
dependency array so the deps become [isVaultBacked, pendingKey, chatboxToken,
shareToken, surface, workspaceId], leaving authorizeServer, isVaultBacked,
pendingKey, chatboxToken, shareToken, surface, and workspaceId referenced as
before to preserve correct re-evaluation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 803f96a9-ece4-4a82-9853-51f052b86f2e
📒 Files selected for processing (5)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/hosted/ChatboxChatPage.tsxmcpjam-inspector/client/src/components/hosted/__tests__/ChatboxChatPage.test.tsxmcpjam-inspector/client/src/hooks/hosted/use-hosted-oauth-gate.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/components/hosted/ChatboxChatPage.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad02595b03
ℹ️ 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".
| const isGuestChatboxSessionCallback = | ||
| !isAuthenticated && | ||
| !!callbackContext.chatboxToken && | ||
| !!callbackContext.sessionId; | ||
| const shouldUseHostedCompletion = | ||
| hasHostedServerContext && | ||
| (isAuthenticated || isGuestChatboxSessionCallback); |
There was a problem hiding this comment.
This branch now requires callbackContext.sessionId (and chatboxToken) before using completeHostedOAuthCallback, so guest chatbox callbacks that arrive after the hosted pending marker expires (10-minute TTL in hosted-oauth-callback) fall back to handleOAuthCallback. In this commit, chatbox chat requests no longer send browser oauthTokens when chatboxToken is present, so a local-only callback completion cannot satisfy later vault validation and users can get stuck in an authorize-again loop after a long consent flow. Guest chatbox callbacks should still use hosted completion when possible, even if the session marker is missing.
Useful? React with 👍 / 👎.
…zooming-teapot # Conflicts: # mcpjam-inspector/client/src/components/hosted/ChatboxChatPage.tsx
dosubot
Bot
added
size:XL
chelojimenez
temporarily deployed
to
preview-pr-1936
GitHub Actions
Inactive
Internal previewPreview URL: https://mcp-inspector-pr-1936.up.railway.app |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 349d60cf45
ℹ️ 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".
| [servers] | ||
| ); | ||
| const isVaultBacked = isAuthenticated || !!chatboxToken; | ||
| const verifyVaultCredentialOnLoad = isAuthenticated; |
There was a problem hiding this comment.
By setting verifyVaultCredentialOnLoad to isAuthenticated, guest chatbox sessions (chatboxToken present, not signed in) always initialize OAuth servers as needs_auth unless there is an active resume marker, even when a valid OAuth credential already exists in the hosted vault. After a successful guest OAuth callback, reloading the chatbox page will therefore block the composer and show authorize-again UI every time, because the gate never attempts a validation pass to transition back to ready.
Useful? React with 👍 / 👎.
chelojimenez
had a problem deploying
to
preview-pr-1936
GitHub Actions
Error
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/hosted/__tests__/SharedServerChatPage.test.tsx (1)
256-262: Signature pin reads cleanly — a touch brittle, but defensible.The assertion faithfully mirrors the widened
validateHostedServer(serverId, context?, options?, …)shape introduced elsewhere in this PR, and the explicitundefinedtrio doubles as living documentation of the new signature. The only quiet concern: every future positional arg added tovalidateHostedServerwill now ripple through this expectation. If you'd rather decouple the test from arity creep, a focused match works just as well:♻️ Optional: assert only what this test cares about
- expect(mockValidateHostedServer).toHaveBeenCalledWith( - "srv_asana", - undefined, - undefined, - undefined - ); - expect(mockValidateHostedServer).toHaveBeenCalledTimes(1); + expect(mockValidateHostedServer).toHaveBeenCalledTimes(1); + expect(mockValidateHostedServer).toHaveBeenNthCalledWith(1, "srv_asana");Note:
toHaveBeenNthCalledWithin Vitest, like Jest, treats trailingundefinedarguments as matching, so the shorter form still guards the meaningful contract — that the guest/shared path invokes validation without a hosted context, options, or auth header.Happy to leave as-is if you prefer the explicit signature snapshot.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/hosted/__tests__/SharedServerChatPage.test.tsx` around lines 256 - 262, The test currently asserts the full positional signature of validateHostedServer by calling expect(mockValidateHostedServer).toHaveBeenCalledWith("srv_asana", undefined, undefined, undefined), which is brittle to future added parameters; replace that assertion with a focused check that only verifies the meaningful argument(s) (e.g., use expect(mockValidateHostedServer).toHaveBeenNthCalledWith(1, "srv_asana") or an equivalent one-arg assertion) so the test continues to confirm the guest/shared path calls validateHostedServer with the serverId without coupling to extra positional params.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/hosted/__tests__/SharedServerChatPage.test.tsx`:
- Around line 256-262: The test currently asserts the full positional signature
of validateHostedServer by calling
expect(mockValidateHostedServer).toHaveBeenCalledWith("srv_asana", undefined,
undefined, undefined), which is brittle to future added parameters; replace that
assertion with a focused check that only verifies the meaningful argument(s)
(e.g., use expect(mockValidateHostedServer).toHaveBeenNthCalledWith(1,
"srv_asana") or an equivalent one-arg assertion) so the test continues to
confirm the guest/shared path calls validateHostedServer with the serverId
without coupling to extra positional params.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b424623-870c-4487-b494-babf02d3cd0d
📒 Files selected for processing (1)
mcpjam-inspector/client/src/components/hosted/__tests__/SharedServerChatPage.test.tsx
There was a problem hiding this comment.
💡 Codex Review
https://github.com/MCPJam/inspector/blob/54ea6f1b00e1b81fcf3ecb773fd645e6b5274f74/mcpjam-inspector/mcpjam-inspector/client/src/App.tsx#L505-L507
If OAuth consent takes longer than the hosted pending TTL (10 minutes), getHostedOAuthCallbackContext reconstructs chatbox callbacks without workspaceId/serverId; this branch then forces handleOAuthCallback even for authenticated users. In this commit chatbox requests no longer include browser oauthTokens (use-chat-session suppresses them when chatboxToken is present), so a local-only completion cannot restore vault credentials and users are redirected back into an immediate re-authorize flow after callback. This should stay on hosted completion (or surface a hard resume-expired error) instead of silently falling back.
ℹ️ 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".
1 participant
Summary
Testing
Note
Medium Risk
Changes hosted OAuth completion and token handling for chatbox/guest flows, which can affect login/authorization success paths and session recovery behavior.
Overview
Refines hosted OAuth handling so chatbox callbacks can be completed via
completeHostedOAuthCallbackeven when the viewer is unauthenticated, using a guest bearer token (and emitting a clear “guest session expired” error when unavailable).Removes reliance on locally stored OAuth access tokens for hosted chatbox sessions (editor/builder/chat page) by treating chatbox contexts as vault-backed:
useHostedOAuthGatenow validates using hosted context (workspaceId,serverId,chatboxToken) andChatTabV2suppresseshostedOAuthTokenswhenever achatboxTokenis present.Updates and expands unit tests to cover guest chatbox callback completion, missing guest bearer handling, chatbox-token validation context passed into
validateHostedServer, and the new “no oauthTokens in chatbox contexts” behavior.Reviewed by Cursor Bugbot for commit 54ea6f1. Bugbot is set up for automated code reviews on this repo. Configure here.