Electron Sign in redirects to localhost#1647
Open
ignaciojimenezr wants to merge 14 commits intomainfrom
Open
Conversation
|
🚅 Environment inspector-pr-1647 in triumphant-alignment has no services deployed. 1 service not affected by this PR
|
Contributor
✅ 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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds an Electron-aware hosted-auth hook and replaces usages of the previous auth hook across UI components. It centralizes WorkOS client config into a new module, exposes an async 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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
mcpjam-inspector/client/src/lib/__tests__/workos-config.test.ts (1)
33-39: Consider adding fallback-branch tests forgetWorkosDevMode().A case for implicit localhost fallback (when
VITE_WORKOS_DEV_MODEis unset) would harden this suite against subtle env regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/__tests__/workos-config.test.ts` around lines 33 - 39, Add tests that cover the implicit localhost fallback for getWorkosDevMode(): when VITE_WORKOS_DEV_MODE is unset (vi.unstubEnv or vi.stubEnv("VITE_WORKOS_DEV_MODE", undefined)), assert getWorkosDevMode() returns true if window.location.hostname is "localhost" (stub window.location via Object.defineProperty or vi.stubGlobal) and returns false for a non-local hostname like "example.com"; add descriptive test cases such as "returns true on implicit localhost when env unset" and "returns false on implicit non-localhost when env unset" to harden the suite.mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx (1)
1-77: Prefer shared client mock presets over bespoke test scaffolding.This setup uses custom inline mocks instead of the repository’s shared client presets, which can drift from common test behavior and fixtures.
As per coding guidelines
mcpjam-inspector/client/**/__tests__/*.test.{ts,tsx}: Use mock presets fromclient/src/test/mocks/includingmcpApiPresetsandstorePresetsin client tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx` around lines 1 - 77, Replace the inline bespoke mocks with the repository's shared test presets: import the shared mcpApiPresets and storePresets from client/src/test/mocks and apply them in this test (e.g., call the preset setup functions in a top-level beforeEach or via vi.mock hooks) instead of individually mocking testConnection, deleteServer, listServers, reconnectServer, getInitializationInfo, sonner toast (toastError/toastSuccess), handleOAuthCallback/getStoredTokens, useUIPlaygroundStore, and useWorkspaces/useServerMutations; remove the corresponding vi.mock blocks and only add small, targeted overrides via the presets when you need to change behavior for this test (use the preset's override API or replace individual functions like handleOAuthCallbackMock/getStoredTokensMock via the preset).mcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsx (1)
1-33: Align this client test setup with shared mock presets.The suite currently hand-rolls mocks; switching to repository presets will keep behavior consistent with the rest of the client test surface.
As per coding guidelines
mcpjam-inspector/client/**/__tests__/*.test.{ts,tsx}: Use mock presets fromclient/src/test/mocks/includingmcpApiPresetsandstorePresetsin client tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsx` around lines 1 - 33, Replace the hand-rolled mock setup (the vi.hoisted block and the individual vi.mock calls that define useAuthMock, createClientMock, getWorkosClientIdMock, getWorkosClientOptionsMock, getWorkosDevModeMock and getWorkosRedirectUriMock) with the shared client test mock presets: import and apply the mcpApiPresets and storePresets from client/src/test/mocks and use the repository’s mock setup helper (the same pattern used in other client tests) so the WorkOS and store behavior is provided by those presets instead of bespoke mocks; ensure the test uses the common setup helper before rendering the hook (so symbols like useAuth and createClient are resolved by the presets).mcpjam-inspector/client/src/components/oauth/OAuthDebugCallback.tsx (1)
8-18: Potentialflowparameter duplication.The function sets
flow=debugat line 10, then appends all existing search params at lines 13-15. Should the source URL already contain aflowparameter, the result will carry both values. WhileURLSearchParams.get()returns only the first, this creates ambiguity.Exclude `flow` when copying params
for (const [key, value] of params.entries()) { + if (key === "flow") continue; callbackUrl.searchParams.append(key, value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/oauth/OAuthDebugCallback.tsx` around lines 8 - 18, The buildElectronDebugCallbackUrl function currently sets callbackUrl.searchParams.set("flow","debug") and then appends all params from window.location.search which can duplicate flow; change the loop in buildElectronDebugCallbackUrl to skip any incoming param with key === "flow" (or remove existing "flow" from params) before appending so only the canonical flow=debug remains in callbackUrl.searchParams.mcpjam-inspector/client/src/hooks/use-server-state.ts (1)
110-131: Error classification relies on fragile string matching.The retry predicate depends on exact error substrings (e.g.,
"sse error: sse error: non-200 status code (404)"). Changes in upstream error formatting could silently break retry behavior.Consider documenting these patterns or extracting them into named constants to ease future maintenance.
Extract error patterns for clarity
+const RETRYABLE_ERROR_PATTERNS = [ + "request timed out", + "streamable http error", + "sse error: sse error: non-200 status code (404)", +] as const; + export function shouldRetryOAuthConnectionFailure( errorMessage?: string, ): boolean { if (!errorMessage) { return false; } const normalized = errorMessage.toLowerCase(); if ( normalized.includes("authentication failed") || normalized.includes("invalid_client") || normalized.includes("unauthorized_client") ) { return false; } - return ( - normalized.includes("request timed out") || - normalized.includes("streamable http error") || - normalized.includes("sse error: sse error: non-200 status code (404)") - ); + return RETRYABLE_ERROR_PATTERNS.some((pattern) => + normalized.includes(pattern) + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 110 - 131, The retry predicate in shouldRetryOAuthConnectionFailure relies on fragile hard-coded substrings; extract those patterns into clearly named constants (e.g., RETRYABLE_PATTERNS and NON_RETRYABLE_PATTERNS) and replace the inline includes checks with iteration over those arrays (or a helper like matchesAnyPattern) so future formatting changes are easier to update and document; update the function to normalize the message once, reference the new constants from shouldRetryOAuthConnectionFailure, and add a short comment above each constant explaining the intent of the pattern group.
🤖 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/oauth/mcp-oauth.ts`:
- Around line 272-275: The electron external-open call can reject and abort the
OAuth flow; wrap the call to
window.electronAPI.app.openExternal(authorizationUrl.toString()) in a try-catch
inside the current OAuth redirect logic (the block where window.isElectron is
checked) and on catch fall back to setting window.location.href =
authorizationUrl.toString(); ensure the function that invokes this (the OAuth
redirect flow around authorizationUrl) awaits the try and still returns after
success so the fallback executes only on error.
In `@mcpjam-inspector/client/src/lib/workos-config.ts`:
- Around line 9-16: The function getWorkosDevMode() uses global
location.hostname without guarding for non-browser environments; update it to
check for a browser context (e.g. typeof window !== "undefined" and typeof
location !== "undefined") before accessing location.hostname and return a safe
default (false) when not running in a browser; keep the existing
environment-variable and import.meta.env.DEV logic, but replace the final return
that reads location.hostname with a guarded check so the function never throws
when imported on the server or in non-window contexts.
In `@mcpjam-inspector/src/ipc/app/app-listeners.ts`:
- Around line 15-30: The app:open-external IPC handler currently trusts all
senders; change its signature to accept the IPC event (replace _event with
event) and validate event.sender.id against the same allowed-origin check used
by other handlers (e.g., compare to the known main window webContents.id or an
allowlist) before proceeding; if the sender is not authorized, log and throw an
Error, otherwise continue with the existing URL parsing/protocol checks and
shell.openExternal call in the open-external handler.
In `@mcpjam-inspector/src/preload.ts`:
- Line 15: The app:open-external IPC handler currently allows the renderer to
request shell.openExternal without verifying the caller; update the
app:open-external listener to validate event.senderFrame (or
event.senderFrame.url/origin) against your trusted origins or the app window’s
expected URL before calling shell.openExternal, and early-return or log+reject
if the senderFrame is missing or untrusted; specifically, modify the
app:open-external handler code that receives (event, url) to check
event.senderFrame and its origin/url and only call shell.openExternal(url) when
that verification passes.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/components/oauth/OAuthDebugCallback.tsx`:
- Around line 8-18: The buildElectronDebugCallbackUrl function currently sets
callbackUrl.searchParams.set("flow","debug") and then appends all params from
window.location.search which can duplicate flow; change the loop in
buildElectronDebugCallbackUrl to skip any incoming param with key === "flow" (or
remove existing "flow" from params) before appending so only the canonical
flow=debug remains in callbackUrl.searchParams.
In `@mcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsx`:
- Around line 1-77: Replace the inline bespoke mocks with the repository's
shared test presets: import the shared mcpApiPresets and storePresets from
client/src/test/mocks and apply them in this test (e.g., call the preset setup
functions in a top-level beforeEach or via vi.mock hooks) instead of
individually mocking testConnection, deleteServer, listServers, reconnectServer,
getInitializationInfo, sonner toast (toastError/toastSuccess),
handleOAuthCallback/getStoredTokens, useUIPlaygroundStore, and
useWorkspaces/useServerMutations; remove the corresponding vi.mock blocks and
only add small, targeted overrides via the presets when you need to change
behavior for this test (use the preset's override API or replace individual
functions like handleOAuthCallbackMock/getStoredTokensMock via the preset).
In `@mcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsx`:
- Around line 1-33: Replace the hand-rolled mock setup (the vi.hoisted block and
the individual vi.mock calls that define useAuthMock, createClientMock,
getWorkosClientIdMock, getWorkosClientOptionsMock, getWorkosDevModeMock and
getWorkosRedirectUriMock) with the shared client test mock presets: import and
apply the mcpApiPresets and storePresets from client/src/test/mocks and use the
repository’s mock setup helper (the same pattern used in other client tests) so
the WorkOS and store behavior is provided by those presets instead of bespoke
mocks; ensure the test uses the common setup helper before rendering the hook
(so symbols like useAuth and createClient are resolved by the presets).
In `@mcpjam-inspector/client/src/hooks/use-server-state.ts`:
- Around line 110-131: The retry predicate in shouldRetryOAuthConnectionFailure
relies on fragile hard-coded substrings; extract those patterns into clearly
named constants (e.g., RETRYABLE_PATTERNS and NON_RETRYABLE_PATTERNS) and
replace the inline includes checks with iteration over those arrays (or a helper
like matchesAnyPattern) so future formatting changes are easier to update and
document; update the function to normalize the message once, reference the new
constants from shouldRetryOAuthConnectionFailure, and add a short comment above
each constant explaining the intent of the pattern group.
In `@mcpjam-inspector/client/src/lib/__tests__/workos-config.test.ts`:
- Around line 33-39: Add tests that cover the implicit localhost fallback for
getWorkosDevMode(): when VITE_WORKOS_DEV_MODE is unset (vi.unstubEnv or
vi.stubEnv("VITE_WORKOS_DEV_MODE", undefined)), assert getWorkosDevMode()
returns true if window.location.hostname is "localhost" (stub window.location
via Object.defineProperty or vi.stubGlobal) and returns false for a non-local
hostname like "example.com"; add descriptive test cases such as "returns true on
implicit localhost when env unset" and "returns false on implicit non-localhost
when env unset" to harden the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b916712a-26fc-4d75-a7b2-d049b05c561d
📒 Files selected for processing (30)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ChatTabV2.tsxmcpjam-inspector/client/src/components/OAuthFlowTab.tsxmcpjam-inspector/client/src/components/OrganizationsTab.tsxmcpjam-inspector/client/src/components/ProfileTab.tsxmcpjam-inspector/client/src/components/auth/auth-upper-area.tsxmcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsxmcpjam-inspector/client/src/components/oauth/OAuthAuthorizationModal.tsxmcpjam-inspector/client/src/components/oauth/OAuthDebugCallback.tsxmcpjam-inspector/client/src/components/oauth/OAuthFlowProgressSimple.tsxmcpjam-inspector/client/src/components/oauth/__tests__/OAuthDebugCallback.test.tsxmcpjam-inspector/client/src/components/setting/AccountApiKeySection.tsxmcpjam-inspector/client/src/components/sidebar/sidebar-user.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsxmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/useElectronHostedAuth.tsmcpjam-inspector/client/src/hooks/useElectronOAuth.tsmcpjam-inspector/client/src/lib/__tests__/guest-session.test.tsmcpjam-inspector/client/src/lib/__tests__/workos-config.test.tsmcpjam-inspector/client/src/lib/oauth/constants.tsmcpjam-inspector/client/src/lib/oauth/debug-oauth-provider.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/lib/workos-config.tsmcpjam-inspector/client/src/main.tsxmcpjam-inspector/client/src/types/electron.d.tsmcpjam-inspector/src/ipc/app/app-listeners.tsmcpjam-inspector/src/main.tsmcpjam-inspector/src/preload.ts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/client/src/lib/oauth/debug-oauth-provider.ts
Comment on lines
+9
to
+16
| export function getWorkosDevMode(): boolean { | ||
| const explicit = import.meta.env.VITE_WORKOS_DEV_MODE as string | undefined; | ||
| if (explicit === "true") return true; | ||
| if (explicit === "false") return false; | ||
| if (import.meta.env.DEV) return true; | ||
|
|
||
| return location.hostname === "localhost" || location.hostname === "127.0.0.1"; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where getWorkosDevMode is used and whether calls are strictly browser-only.
rg -nP '\bgetWorkosDevMode\s*\(' -C3 --type=ts --type=tsx
rg -nP 'from ["'\''].*workos-config["'\'']' -C2 --type=ts --type=tsxRepository: MCPJam/inspector
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Find usages of getWorkosDevMode (correct rg syntax for TypeScript)
rg -nP '\bgetWorkosDevMode\s*\(' -C3 --type=typescript
# Find imports from workos-config
rg -nP 'from ["'"'"'].*workos-config["'"'"']' -C2 --type=typescriptRepository: MCPJam/inspector
Length of output: 129
🏁 Script executed:
#!/bin/bash
# Find usages of getWorkosDevMode
rg '\bgetWorkosDevMode\s*\(' -C3 mcpjam-inspector/client
# Find imports from workos-config
rg 'from.*workos-config' -C2 mcpjam-inspector/clientRepository: MCPJam/inspector
Length of output: 3776
🏁 Script executed:
#!/bin/bash
# View the test file to see how location is tested
cat -n mcpjam-inspector/client/src/lib/__tests__/workos-config.test.tsRepository: MCPJam/inspector
Length of output: 1531
Guard non-browser access in getWorkosDevMode.
Line 15 accesses global location without a window guard. While this code currently runs only in browser contexts (entry point, React hooks), adding a guard improves robustness if the function is ever imported elsewhere.
Suggested fix
export function getWorkosDevMode(): boolean {
const explicit = import.meta.env.VITE_WORKOS_DEV_MODE as string | undefined;
if (explicit === "true") return true;
if (explicit === "false") return false;
if (import.meta.env.DEV) return true;
-
- return location.hostname === "localhost" || location.hostname === "127.0.0.1";
+ if (typeof window === "undefined") return false;
+ return (
+ window.location.hostname === "localhost" ||
+ window.location.hostname === "127.0.0.1"
+ );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/workos-config.ts` around lines 9 - 16, The
function getWorkosDevMode() uses global location.hostname without guarding for
non-browser environments; update it to check for a browser context (e.g. typeof
window !== "undefined" and typeof location !== "undefined") before accessing
location.hostname and return a safe default (false) when not running in a
browser; keep the existing environment-variable and import.meta.env.DEV logic,
but replace the final return that reads location.hostname with a guarded check
so the function never throws when imported on the server or in non-window
contexts.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mcpjam-inspector/client/src/hooks/useElectronHostedAuth.ts (1)
121-134: Consider error handling forclient.signOutfailure.If
client.signOut()rejects, thefinallyblock disposes the client, but the user remains in a potentially inconsistent state—history updated, events dispatched, yet sign-out incomplete.💡 Proposed defensive adjustment
try { await client.signOut({ returnTo: window.location.origin, navigate: false, }); + } catch (err) { + console.warn("[auth] WorkOS sign-out failed; proceeding with local cleanup.", err); } finally { client.dispose(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/useElectronHostedAuth.ts` around lines 121 - 134, The current flow always updates history and dispatches events even if client.signOut rejects; change the sign-out sequence in the function using client.signOut so that you await it inside a try/catch/finally where client.dispose() remains in finally, but only proceed to call resolveElectronSignOutPath, window.history.replaceState, and dispatch the PopStateEvent/hashchange/electron-auth-reset events when signOut succeeds; on catch, log or emit an error (e.g., via console.error or dispatch a custom error event) and return early so the app doesn’t transition to the signed-out UI when signOut failed.mcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsx (1)
123-164: Consider testing the sign-out path resolution logic.The sign-out test passes
returnTo: "http://localhost:8080/callback", which should resolve to/perresolveElectronSignOutPath. You verifypathname === "/", implicitly covering this—but an explicit test for non-callback paths (e.g.,/profile) preserving the path would strengthen coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsx` around lines 123 - 164, Add an explicit test that verifies resolveElectronSignOutPath preserves non-callback paths: create a new it block similar to the existing sign-out test but pass returnTo: "http://localhost:8080/profile" (or set window.history to "/profile?tab=account#settings"), call useElectronHostedAuth().signOut and assert that window.location.pathname remains "/profile" (and search/hash are preserved/cleared as expected), assert createClientMock and clientSignOut were called as in the existing test, and ensure you still clean up the electron-auth-reset listener and dispose mock; this will exercise resolveElectronSignOutPath and confirm non-callback paths are preserved.
🤖 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/hooks/__tests__/useElectronHostedAuth.test.tsx`:
- Around line 123-164: Add an explicit test that verifies
resolveElectronSignOutPath preserves non-callback paths: create a new it block
similar to the existing sign-out test but pass returnTo:
"http://localhost:8080/profile" (or set window.history to
"/profile?tab=account#settings"), call useElectronHostedAuth().signOut and
assert that window.location.pathname remains "/profile" (and search/hash are
preserved/cleared as expected), assert createClientMock and clientSignOut were
called as in the existing test, and ensure you still clean up the
electron-auth-reset listener and dispose mock; this will exercise
resolveElectronSignOutPath and confirm non-callback paths are preserved.
In `@mcpjam-inspector/client/src/hooks/useElectronHostedAuth.ts`:
- Around line 121-134: The current flow always updates history and dispatches
events even if client.signOut rejects; change the sign-out sequence in the
function using client.signOut so that you await it inside a try/catch/finally
where client.dispose() remains in finally, but only proceed to call
resolveElectronSignOutPath, window.history.replaceState, and dispatch the
PopStateEvent/hashchange/electron-auth-reset events when signOut succeeds; on
catch, log or emit an error (e.g., via console.error or dispatch a custom error
event) and return early so the app doesn’t transition to the signed-out UI when
signOut failed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a02d0aa-73ba-4933-a48d-d97b21031641
📒 Files selected for processing (5)
mcpjam-inspector/client/src/components/sidebar/sidebar-user.tsxmcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsxmcpjam-inspector/client/src/hooks/useElectronHostedAuth.tsmcpjam-inspector/client/src/main.tsxmcpjam-inspector/vite.renderer.config.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/main.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts (1)
272-285:⚠️ Potential issue | 🟠 MajorDon’t let the fallback bypass the new URL policy.
This
catchtreats everyopenExternalfailure as recoverable. If the main process rejects a non-HTTP URL, we still fall back towindow.location.assign(...)and load it inside the Electron renderer, which undoes the hardening added inapp:open-external.Possible fix
+ const isHttpUrl = + authorizationUrl.protocol === "http:" || + authorizationUrl.protocol === "https:"; + if (window.isElectron && window.electronAPI?.app?.openExternal) { + if (!isHttpUrl) { + throw new Error("Refusing non-HTTP OAuth URL in Electron"); + } + try { await window.electronAPI.app.openExternal(authorizationUrl.toString()); return; } catch (error) { console.error( "Failed to open system browser for MCP OAuth, falling back to in-app navigation:", error, ); } } + if (window.isElectron && !isHttpUrl) { + throw new Error("Refusing non-HTTP OAuth URL in Electron"); + } window.location.assign(authorizationUrl.toString());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 272 - 285, The catch block for window.electronAPI.app.openExternal treats all failures as recoverable and may cause non-HTTP(s) schemes to be loaded in the renderer; update the catch in the function that calls window.electronAPI.app.openExternal(authorizationUrl.toString()) so it only falls back to window.location.assign when the authorizationUrl uses a safe scheme (e.g., protocol is "http:" or "https:"); for non-HTTP(s) schemes do not call window.location.assign (instead log the error and abort/propagate), and use the authorizationUrl object (authorizationUrl.protocol or authorizationUrl.toString()) to determine allowed schemes before deciding to navigate from the renderer.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts (1)
166-200: Please cover the Electron callback URI as well.This exercises the
openExternalfallback, but it never asserts which redirect URI the provider registers in Electron. A regression back tolocalhost/file://would still pass this test suite, so I’d add one assertion formcpjam://oauth/callbackunderwindow.isElectron = true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts` around lines 166 - 200, Add an assertion that the Electron callback URI "mcpjam://oauth/callback" is what the provider registers when window.isElectron = true; after calling MCPOAuthProvider.redirectToAuthorization, verify that the Electron registration was performed by checking the appropriate symbol used in the codebase—either a property on the provider (e.g., provider.redirectUri === "mcpjam://oauth/callback") or a call on window.electronAPI.app (e.g., setAsDefaultProtocolClient/registerProtocol was invoked with "mcpjam://oauth/callback")—and add a corresponding expect(...) that the registration happened with that exact URI.
🤖 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/oauth/mcp-oauth.ts`:
- Around line 24-30: The redirect URI should use the Electron deep-link
(mcpjam://oauth/callback) when running inside Electron; update
getMCPOAuthRedirectUri to detect Electron (e.g., check
process?.versions?.electron or navigator.userAgent for "Electron") and return
"mcpjam://oauth/callback" in that case, otherwise keep the existing behavior of
using window.location.origin or the localhost fallback; modify
getMCPOAuthRedirectUri accordingly so the provider registers the correct
deep-link redirect and avoids browser/localhost mismatch.
---
Duplicate comments:
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 272-285: The catch block for window.electronAPI.app.openExternal
treats all failures as recoverable and may cause non-HTTP(s) schemes to be
loaded in the renderer; update the catch in the function that calls
window.electronAPI.app.openExternal(authorizationUrl.toString()) so it only
falls back to window.location.assign when the authorizationUrl uses a safe
scheme (e.g., protocol is "http:" or "https:"); for non-HTTP(s) schemes do not
call window.location.assign (instead log the error and abort/propagate), and use
the authorizationUrl object (authorizationUrl.protocol or
authorizationUrl.toString()) to determine allowed schemes before deciding to
navigate from the renderer.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.ts`:
- Around line 166-200: Add an assertion that the Electron callback URI
"mcpjam://oauth/callback" is what the provider registers when window.isElectron
= true; after calling MCPOAuthProvider.redirectToAuthorization, verify that the
Electron registration was performed by checking the appropriate symbol used in
the codebase—either a property on the provider (e.g., provider.redirectUri ===
"mcpjam://oauth/callback") or a call on window.electronAPI.app (e.g.,
setAsDefaultProtocolClient/registerProtocol was invoked with
"mcpjam://oauth/callback")—and add a corresponding expect(...) that the
registration happened with that exact URI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5e1a964-10c4-4077-8742-9f06a6d229e1
📒 Files selected for processing (5)
mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/src/ipc/app/app-listeners.ts
✅ Files skipped from review due to trivial changes (2)
- mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx
- mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts
Comment on lines
+24
to
+30
| function getMCPOAuthRedirectUri(): string { | ||
| if (typeof window !== "undefined") { | ||
| return `${window.location.origin}/oauth/callback`; | ||
| } | ||
|
|
||
| return "http://localhost:6274/oauth/callback"; | ||
| } |
Contributor
There was a problem hiding this comment.
Use the Electron deep-link callback here.
In Electron, this still derives redirectUri from window.location.origin, so the provider registers the renderer URL instead of mcpjam://oauth/callback. That sends the system browser back to localhost/file:// rather than back into the app, and it can also break token exchange with a redirect URI mismatch.
Possible fix
function getMCPOAuthRedirectUri(): string {
if (typeof window !== "undefined") {
+ if (window.isElectron) {
+ return "mcpjam://oauth/callback";
+ }
return `${window.location.origin}/oauth/callback`;
}
return "http://localhost:6274/oauth/callback";
}Also applies to: 153-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts` around lines 24 - 30, The
redirect URI should use the Electron deep-link (mcpjam://oauth/callback) when
running inside Electron; update getMCPOAuthRedirectUri to detect Electron (e.g.,
check process?.versions?.electron or navigator.userAgent for "Electron") and
return "mcpjam://oauth/callback" in that case, otherwise keep the existing
behavior of using window.location.origin or the localhost fallback; modify
getMCPOAuthRedirectUri accordingly so the provider registers the correct
deep-link redirect and avoids browser/localhost mismatch.
ignaciojimenezr
force-pushed
the
electron-oauth
branch
from
5224a8f to
37a745c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/hooks/use-server-state.ts (1)
126-131: Brittle exact-match on SSE error message.The string
"sse error: sse error: non-200 status code (404)"is highly specific. If the upstream library changes this message format, retry logic silently breaks. Consider a more resilient pattern:return ( normalized.includes("request timed out") || normalized.includes("streamable http error") || - normalized.includes("sse error: sse error: non-200 status code (404)") + (normalized.includes("sse error") && normalized.includes("non-200")) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/hooks/use-server-state.ts` around lines 126 - 131, The current return expression uses a brittle exact string match for SSE errors; update the check that currently tests normalized.includes("sse error: sse error: non-200 status code (404)") to a more resilient pattern (e.g., use a case-insensitive regex like /sse error.*non-200.*\(\d{3}\)/i or a compound check normalized.includes("sse error") && normalized.includes("non-200")) so the retry logic tolerates upstream message format changes—modify the return statement that references normalized accordingly.
🤖 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/hooks/use-server-state.ts`:
- Around line 126-131: The current return expression uses a brittle exact string
match for SSE errors; update the check that currently tests
normalized.includes("sse error: sse error: non-200 status code (404)") to a more
resilient pattern (e.g., use a case-insensitive regex like /sse
error.*non-200.*\(\d{3}\)/i or a compound check normalized.includes("sse error")
&& normalized.includes("non-200")) so the retry logic tolerates upstream message
format changes—modify the return statement that references normalized
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 129446f8-6cc3-4641-ac28-abeb77b4a701
📒 Files selected for processing (34)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ChatTabV2.tsxmcpjam-inspector/client/src/components/OAuthFlowTab.tsxmcpjam-inspector/client/src/components/OrganizationsTab.tsxmcpjam-inspector/client/src/components/ProfileTab.tsxmcpjam-inspector/client/src/components/auth/auth-upper-area.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsxmcpjam-inspector/client/src/components/oauth/OAuthAuthorizationModal.tsxmcpjam-inspector/client/src/components/oauth/OAuthDebugCallback.tsxmcpjam-inspector/client/src/components/oauth/OAuthFlowProgressSimple.tsxmcpjam-inspector/client/src/components/oauth/__tests__/OAuthDebugCallback.test.tsxmcpjam-inspector/client/src/components/setting/AccountApiKeySection.tsxmcpjam-inspector/client/src/components/sidebar/sidebar-user.tsxmcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/__tests__/useElectronHostedAuth.test.tsxmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/useElectronHostedAuth.tsmcpjam-inspector/client/src/hooks/useElectronOAuth.tsmcpjam-inspector/client/src/lib/__tests__/guest-session.test.tsmcpjam-inspector/client/src/lib/__tests__/workos-config.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/constants.tsmcpjam-inspector/client/src/lib/oauth/debug-oauth-provider.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/lib/workos-config.tsmcpjam-inspector/client/src/main.tsxmcpjam-inspector/client/src/types/electron.d.tsmcpjam-inspector/src/ipc/app/app-listeners.tsmcpjam-inspector/src/main.tsmcpjam-inspector/src/preload.tsmcpjam-inspector/vite.renderer.config.mts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/client/src/lib/oauth/debug-oauth-provider.ts
✅ Files skipped from review due to trivial changes (12)
- mcpjam-inspector/client/src/components/connection/ServerDetailModal.tsx
- mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts
- mcpjam-inspector/client/src/components/ProfileTab.tsx
- mcpjam-inspector/client/src/components/hosted/SandboxChatPage.tsx
- mcpjam-inspector/vite.renderer.config.mts
- mcpjam-inspector/client/src/components/OAuthFlowTab.tsx
- mcpjam-inspector/src/preload.ts
- mcpjam-inspector/client/src/components/ui-playground/PlaygroundMain.tsx
- mcpjam-inspector/client/src/components/auth/auth-upper-area.tsx
- mcpjam-inspector/client/src/lib/tests/workos-config.test.ts
- mcpjam-inspector/client/src/hooks/useElectronOAuth.ts
- mcpjam-inspector/client/src/hooks/tests/useElectronHostedAuth.test.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- mcpjam-inspector/client/src/components/oauth/tests/OAuthDebugCallback.test.tsx
- mcpjam-inspector/client/src/components/ChatTabV2.tsx
- mcpjam-inspector/client/src/types/electron.d.ts
- mcpjam-inspector/client/src/lib/tests/guest-session.test.ts
- mcpjam-inspector/client/src/components/oauth/OAuthAuthorizationModal.tsx
- mcpjam-inspector/client/src/components/OrganizationsTab.tsx
- mcpjam-inspector/client/src/hooks/tests/use-server-state.test.tsx
- mcpjam-inspector/client/src/main.tsx
- mcpjam-inspector/client/src/lib/oauth/tests/mcp-oauth.test.ts
- mcpjam-inspector/client/src/components/sidebar/sidebar-user.tsx
- mcpjam-inspector/client/src/lib/workos-config.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR moves Electron OAuth out of Electron’s in-app browser and into the user’s real browser, then routes the result back into the app through mcpjam://oauth/callback.
It fixes the main Electron auth problems: