feat: unify local and hosted authorization paths#2028
Conversation
Brings local Inspector (CLI / --ui) onto the same Convex-backed
authorization path that hosted uses, so server configs (including STDIO
+ env) and OAuth credentials live in Convex instead of being trusted
from the client / localStorage.
Server (Hono /api/mcp/*):
* Add server/utils/local-server-resolver.ts — calls Convex's
/web/authorize-batch-local with the user's bearer, maps the response
into an MCPServerConfig (including OAuth header injection), threads
the server-side log context, strips internalLogContext.
* /api/mcp/connect and /api/mcp/servers/reconnect accept the new
{projectId, serverId} shape and resolve config server-side. Legacy
{serverConfig, serverId} body is retained as a fallback during the
client migration; can be dropped once all clients are flipped.
Client:
* Drop HOSTED_MODE short-circuits in shouldRetryHostedAuth401,
hasHostedGuestAccess, getHostedAuthorizationHeader, authFetch's 401
retry, and use-hosted-api-context. Bearer / 401 retry / form-save
flows are now mode-agnostic.
* Add tryResolveProjectServer — opt into the new {projectId, serverId}
request shape when the API context has both, fall back to legacy
otherwise (handles the post-migration window and brand-new servers
not yet synced to Convex).
* Unify the form-save Convex sync branch so both modes await the
sync; OAuth in either mode now requires a Convex serverId.
* testConnection / reconnectServer accept optional {projectId,
serverName} and emit the new body shape when present.
* Include `env` when serializing servers for sharing.
Migration:
* One-time localStorage → Convex migration for legacy local installs:
reads mcp-inspector-projects / mcp-inspector-workspaces /
mcp-inspector-state, pushes each project + servers to Convex via
projects:createProject, clears legacy keys + per-server OAuth keys.
OAuth tokens are NOT migrated — users re-auth on first connect
post-migration. Gated by mcp-inspector-migrated-to-convex flag,
skipped in HOSTED_MODE.
Pairs with mcpjam-backend PR #215 (/web/authorize-batch-local).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chelojimenez
temporarily deployed
to
preview-pr-2028
GitHub Actions
Inactive
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. |
![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: d42f31ec7a
ℹ️ 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 (resolved) { | ||
| return testConnection(serverConfig, resolved.serverId, { | ||
| projectId: resolved.projectId, | ||
| serverName, |
There was a problem hiding this comment.
When local mode has a populated Convex mapping (for migrated/existing synced servers), this connects the MCP client manager under the Convex serverId, while the local UI still calls /api/mcp/tools/* with the display name via listTools({ serverId: serverName }) in components like ToolsTab. Since the local tools routes only look up connected clients by the provided id/name, the connect succeeds but listing/executing tools for that server reports it is not connected. The resolver request can still carry the Convex id, but the local manager key needs to remain the UI server name or all subsequent local API calls must be translated too.
Useful? React with 👍 / 👎.
Internal previewPreview URL: https://mcp-inspector-pr-2028.up.railway.app |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
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:
WalkthroughIntroduces a one‑time localStorage→Convex migration module and a client hook ( 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/server/routes/mcp/servers.ts (2)
228-255:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRestore hosted-mode transport checks on the legacy reconnect fallback.
This branch now reconnects whatever
serverConfigthe client posts. Because the legacy{ serverConfig, serverId }shape is still accepted during migration, an older client—or any direct caller—can bypass hosted protections here and reconnect a STDIO or plain-HTTP server even thoughmcpjam-inspector/server/routes/mcp/connect.tsstill blocks both.Please mirror the hosted-mode STDIO and HTTPS guards in this fallback path before calling
connectToServer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/servers.ts` around lines 228 - 255, The legacy fallback currently accepts posted serverConfig without the hosted-mode protections; before assigning normalizedConfig and before calling connectToServer, replicate the hosted-mode guards from connect.ts: check the parsed cfg (the cloned serverConfig) for a disallowed stdio transport (e.g., cfg.transport === "stdio") and reject with the same error/status used in connect.ts, and validate cfg.url to ensure it uses HTTPS (reject plain HTTP/unencrypted URLs the same way connect.ts does). Use the same error messages and response codes as the existing hosted-mode checks so older clients cannot bypass the STDIO/HTTP(S) protections when normalizedConfig is set and connectToServer is invoked.
257-286:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the server entry when reconnect setup throws.
mcpjam-inspector/server/routes/mcp/connect.tsalready cleans up withremoveServer(serverId)after a failed connect, but this reconnect path only logs and returns 500. IfconnectToServer()registered partial state before throwing, the stale entry survives and subsequent status/reconnect calls start from a corrupted manager state.Suggested parity with the connect route
} catch (error) { + try { + await mcpClientManager.removeServer(serverId); + } catch (cleanupError) { + logger.debug("Failed to remove server after reconnect failure", { + serverId, + error: + cleanupError instanceof Error + ? cleanupError.message + : String(cleanupError), + }); + } logger.error("Error reconnecting server", error, { serverId }); return c.json( { success: false, error: error instanceof Error ? error.message : "Unknown error",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/servers.ts` around lines 257 - 286, The reconnect path currently only logs and returns 500 on errors, leaving partial state in the mcpClientManager; update the catch block in the reconnect handler to call mcpClientManager.removeServer(serverId) (same cleanup used in connect route) before returning the error response so any stale/partially-registered state is removed; ensure you still log the original error with logger.error and then return the existing 500 JSON response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/hooks/use-local-state-migration.ts`:
- Around line 39-77: This effect must be gated on user/org bootstrap completion
to avoid racing projects:createProject against users:ensureUser; add a check
before starting runLocalStateMigration (e.g., if (!isUserBootstrapComplete)
return) and include that flag in the effect dependencies so the effect only runs
after bootstrap completes; update the useEffect to reference the existing
bootstrap completion signal (or a new selector/prop like isUserBootstrapComplete
/ hasUserBootstrapCompleted that reflects users:ensureUser finishing), and keep
doneRef/inFlightRef logic unchanged so migration will run once bootstrap is done
and will retry only when the completion flag changes.
In `@mcpjam-inspector/client/src/lib/local-state-migration.ts`:
- Around line 278-285: The migration currently drops the legacy project
visibility because the call to deps.createProject(...) does not forward
project.visibility; update the createProject invocation in the migration to
include the visibility field (using the project.visibility value returned by
readLegacyMigrationPayload()) so MigrationDeps.createProject(...) receives and
preserves the legacy public/private flag.
- Around line 255-308: The migration loop currently creates projects one-by-one
via deps.createProject using payload.projects and only clears all legacy keys on
complete success, so a partial failure will cause already-migrated projects to
be recreated on the next retry; modify the logic to make per-project progress
persistent and idempotent: after a successful deps.createProject for a given
project (refer to project.name, projectServers, projectsMigrated, errors),
immediately record that project as migrated in durable storage (e.g., a
migratedNames list or remove that project's entry from the legacy payload via
the same deps storage API you read from) so subsequent runs skip
already-migrated projects, and adjust the final
clearLegacyKeys/markMigrationComplete behavior to only remove or mark remaining
legacy items once all per-project records are accounted for; alternatively
implement an idempotent create (check for existing project by name/organization
via deps before create) or switch to fail-fast by validating all creates would
succeed before writing any state—pick one approach and apply it to the block
that calls deps.createProject and the subsequent success/error handling.
In `@mcpjam-inspector/client/src/lib/project-serialization.ts`:
- Around line 28-29: serializeServersForSharing currently copies
server.config.env into the shared payload (via config.env = (server.config as
any).env) which can leak secrets; change the function so it does NOT include
config.env in the sharing serializer (remove or guard the assignment), and
instead add a separate persistence/migration serializer (e.g.,
serializeServersForPersistence or a boolean redactSecrets parameter on
serializeServersForSharing) that will include config.env for internal
storage/migrations only; update calls to use the appropriate serializer and
ensure serializeServersForSharing never passes through server.config.env while
the new persistence serializer preserves it.
In `@mcpjam-inspector/client/src/state/mcp-api.ts`:
- Around line 130-147: The POST body built for the resolver path currently omits
clientCapabilities so reconnectServer()/testConnection() lose runtime-derived
capabilities; update the body construction in the conditional that creates body
(the object passed to authFetchWithTimeout for "/api/mcp/connect") to include
clientCapabilities: options.clientCapabilities (or the variable used by
withProjectConnectionDefaults()) when present, matching the other branch that
sends { serverConfig, serverId, clientCapabilities }, ensuring reconnectServer
and the server resolver receive the computed clientCapabilities.
In `@mcpjam-inspector/server/utils/local-server-resolver.ts`:
- Around line 138-153: The fetch to `${convexUrl}/web/authorize-batch-local` in
local-server-resolver.ts currently can hang indefinitely; wrap the request in an
AbortController with a configurable timeout (e.g., 5–10s), pass signal to fetch,
start a setTimeout that calls controller.abort() after the timeout and clear
that timer after fetch completes, and in the catch distinguish an abort/timeout
(throw a WebRouteError with an appropriate status like 504 and
ErrorCode.SERVER_UNREACHABLE) from other errors so the existing error handling
around response and parseErrorMessage still works; locate the fetch call and the
surrounding try/catch where `response` is declared to insert the controller,
timer, signal usage and cleanup.
---
Outside diff comments:
In `@mcpjam-inspector/server/routes/mcp/servers.ts`:
- Around line 228-255: The legacy fallback currently accepts posted serverConfig
without the hosted-mode protections; before assigning normalizedConfig and
before calling connectToServer, replicate the hosted-mode guards from
connect.ts: check the parsed cfg (the cloned serverConfig) for a disallowed
stdio transport (e.g., cfg.transport === "stdio") and reject with the same
error/status used in connect.ts, and validate cfg.url to ensure it uses HTTPS
(reject plain HTTP/unencrypted URLs the same way connect.ts does). Use the same
error messages and response codes as the existing hosted-mode checks so older
clients cannot bypass the STDIO/HTTP(S) protections when normalizedConfig is set
and connectToServer is invoked.
- Around line 257-286: The reconnect path currently only logs and returns 500 on
errors, leaving partial state in the mcpClientManager; update the catch block in
the reconnect handler to call mcpClientManager.removeServer(serverId) (same
cleanup used in connect route) before returning the error response so any
stale/partially-registered state is removed; ensure you still log the original
error with logger.error and then return the existing 500 JSON response.
🪄 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: 62ec2a3b-5add-4ef1-92cc-8ea5b731b874
📒 Files selected for processing (17)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.hosted-oauth.test.tsxmcpjam-inspector/client/src/hooks/__tests__/use-server-state.test.tsxmcpjam-inspector/client/src/hooks/hosted/use-hosted-api-context.tsmcpjam-inspector/client/src/hooks/use-local-state-migration.tsmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/lib/__tests__/local-state-migration.test.tsmcpjam-inspector/client/src/lib/apis/web/context.tsmcpjam-inspector/client/src/lib/local-state-migration.tsmcpjam-inspector/client/src/lib/project-serialization.tsmcpjam-inspector/client/src/lib/session-token.tsmcpjam-inspector/client/src/state/mcp-api.tsmcpjam-inspector/server/routes/mcp/__tests__/connect.test.tsmcpjam-inspector/server/routes/mcp/__tests__/servers.test.tsmcpjam-inspector/server/routes/mcp/connect.tsmcpjam-inspector/server/routes/mcp/servers.tsmcpjam-inspector/server/utils/local-server-resolver.ts
💤 Files with no reviewable changes (1)
- mcpjam-inspector/client/src/hooks/hosted/use-hosted-api-context.ts
- Resolver path keys MCP client manager by display name, not Convex serverId. Without this, connect succeeds but tools list/execute (which pass display names from the UI) report "not connected" for synced servers. Requires `serverName` in the body when `projectId` is present. - `injectHostedServerMapping` no longer no-ops in local mode, so the immediate post-save connect uses the resolver path instead of the legacy fallback for one tick. - Restored HOSTED_MODE STDIO/HTTPS guards on the legacy reconnect path to keep defense-in-depth aligned with connect.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
chelojimenez
temporarily deployed
to
preview-pr-2028
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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/lib/apis/web/context.ts`:
- Around line 147-160: The helper tryResolveProjectServer should preserve the
bootstrap-not-ready signal instead of silently returning null: when
hostedApiContext.projectId is missing, throw (or rethrow) the same
BootstrapNotReadyError used by buildHostedServerRequest so callers can detect
bootstrap-not-ready; keep the existing null return for the case where projectId
exists but the server cannot be resolved. Update tryResolveProjectServer to
reference hostedApiContext and BootstrapNotReadyError and ensure callers that
expect {projectId, serverId} still get that on success.
In `@mcpjam-inspector/server/routes/mcp/servers.ts`:
- Around line 277-280: The code constructs urlForCheck via new
URL(String(cfg.url)) without handling parse errors, so malformed legacy
serverConfig.url will throw a 500; wrap the URL parsing for cfg.url (the
urlForCheck creation) in a try/catch (or use a safe URL-parse helper) and on
failure treat it as a validation error (return the existing 400 validation
response path) instead of letting the exception propagate; ensure this change
references cfg.url and urlForCheck in the HOSTED_MODE branch so the subsequent
protocol check (urlForCheck.protocol !== "https:") only runs for successfully
parsed URLs.
- Around line 295-300: The reconnect route currently calls
mcpClientManager.disconnectServer(managerKey) and if that throws (e.g., server
already disconnected) it prevents the subsequent connectToServer call; wrap the
disconnectServer call in its own try/catch and treat “already disconnected/not
found” errors as non-fatal (log/debug and continue) so that
mcpClientManager.connectToServer(managerKey, normalizedConfig) always runs;
reference the disconnectServer and connectToServer calls and mirror the
non-fatal handling used by the DELETE handler for consistency.
🪄 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: ff278d40-a39a-4ae5-a525-b26a3f2325ae
📒 Files selected for processing (5)
mcpjam-inspector/client/src/lib/apis/web/context.tsmcpjam-inspector/server/routes/mcp/__tests__/connect.test.tsmcpjam-inspector/server/routes/mcp/__tests__/servers.test.tsmcpjam-inspector/server/routes/mcp/connect.tsmcpjam-inspector/server/routes/mcp/servers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/server/routes/mcp/connect.ts
| export function tryResolveProjectServer( | ||
| serverNameOrId: string, | ||
| ): { projectId: string; serverId: string } | null { | ||
| const projectId = hostedApiContext.projectId; | ||
| if (!projectId) return null; | ||
| const direct = hostedApiContext.serverIdsByName[serverNameOrId]; | ||
| if (direct) return { projectId, serverId: direct }; | ||
| if ( | ||
| Object.values(hostedApiContext.serverIdsByName).includes(serverNameOrId) | ||
| ) { | ||
| return { projectId, serverId: serverNameOrId }; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Preserve the bootstrap signal in this resolver API.
Line 151 currently turns “project context isn’t ready yet” into null, so callers can silently fall back to the legacy body instead of getting the same BootstrapNotReadyError that buildHostedServerRequest() preserves. Since the project-scoped path now also depends on serverName, returning only { projectId, serverId } makes this helper easy to misuse and easy to regress later.
Possible tightening
export function tryResolveProjectServer(
serverNameOrId: string,
-): { projectId: string; serverId: string } | null {
+): { projectId: string; serverId: string; serverName: string } | null {
const projectId = hostedApiContext.projectId;
- if (!projectId) return null;
- const direct = hostedApiContext.serverIdsByName[serverNameOrId];
- if (direct) return { projectId, serverId: direct };
- if (
- Object.values(hostedApiContext.serverIdsByName).includes(serverNameOrId)
- ) {
- return { projectId, serverId: serverNameOrId };
+ if (!projectId) {
+ throw new BootstrapNotReadyError(
+ "provisioning-project",
+ "hosted projectId is not in the API context yet",
+ );
+ }
+
+ const mappedServerId = hostedApiContext.serverIdsByName[serverNameOrId];
+ if (mappedServerId) {
+ return {
+ projectId,
+ serverId: mappedServerId,
+ serverName: serverNameOrId,
+ };
+ }
+
+ const serverName = findHostedServerName(serverNameOrId);
+ if (serverName) {
+ return { projectId, serverId: serverNameOrId, serverName };
}
return null;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/lib/apis/web/context.ts` around lines 147 - 160,
The helper tryResolveProjectServer should preserve the bootstrap-not-ready
signal instead of silently returning null: when hostedApiContext.projectId is
missing, throw (or rethrow) the same BootstrapNotReadyError used by
buildHostedServerRequest so callers can detect bootstrap-not-ready; keep the
existing null return for the case where projectId exists but the server cannot
be resolved. Update tryResolveProjectServer to reference hostedApiContext and
BootstrapNotReadyError and ensure callers that expect {projectId, serverId}
still get that on success.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765afdfb23
ℹ️ 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 resolved = tryResolveProjectServer(serverName); | ||
| if (resolved) { | ||
| return testConnection(serverConfig, resolved.serverId, { |
There was a problem hiding this comment.
When local mode has a Convex mapping, this resolver branch is also taken for OAuth servers. The callers pass a serverConfig that contains the local Authorization header from stored tokens (including the existing-token and OAuth-callback paths), but testConnection switches to a {projectId, serverId} body and drops that config; the local resolver then rejects useOAuth servers unless Convex returns oauthAccessToken. Since the local OAuth provider still saves tokens only in localStorage for !HOSTED_MODE, synced local OAuth servers will fail to connect with oauthRequired despite having valid local tokens. Keep the legacy body for local OAuth or forward/persist the token into the resolver path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/server/routes/mcp/servers.ts (1)
257-264:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCatch malformed
hrefURLs during legacy normalization.
new URL((urlValue as { href: string }).href)can still throw here, so a bad legacy payload becomes a 500 before the later 400 validation path runs.Proposed fix
} else if ( typeof urlValue === "object" && urlValue !== null && "href" in (urlValue as Record<string, unknown>) && typeof (urlValue as { href?: unknown }).href === "string" ) { - cfg.url = new URL((urlValue as { href: string }).href).toString(); + try { + cfg.url = new URL((urlValue as { href: string }).href).toString(); + } catch { + return c.json( + { success: false, error: "Invalid server URL" }, + 400, + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/servers.ts` around lines 257 - 264, The legacy normalization branch that reads urlValue.href can throw when constructing a URL, causing a 500; wrap the URL construction in a try/catch and only assign to cfg.url if new URL(...) succeeds (or otherwise leave cfg.url unset/invalid so the existing validation path returns a 400). Specifically, inside the branch that checks urlValue and "href" (the block that assigns cfg.url = new URL(...).toString()), catch any errors from new URL((urlValue as { href: string }).href) and avoid throwing—log or ignore the error and do not set cfg.url so downstream validation (the later 400 path) handles the malformed value.
♻️ Duplicate comments (1)
mcpjam-inspector/client/src/lib/local-state-migration.ts (1)
333-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward
project.visibilitywhen creating the Convex project.You preserve legacy visibility on read, and
MigrationDeps.createProjectaccepts it, but this call drops it. Migrated public/private projects will silently fall back to the backend default.💡 Minimal fix
await deps.createProject({ name: project.name, description: project.description, icon: project.icon, clientConfig: project.clientConfig, servers: serializedServers, organizationId: deps.organizationId, + visibility: project.visibility, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/local-state-migration.ts` around lines 333 - 340, The call to deps.createProject when migrating projects omits project.visibility, so migrated projects lose their original public/private setting; update the object passed to deps.createProject in local-state-migration.ts to include visibility: project.visibility so the MigrationDeps.createProject receives and preserves the legacy visibility value (ensure the key name matches the createProject parameter expected by the MigrationDeps implementation).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/lib/local-state-migration.ts`:
- Around line 292-296: The current branch treats any null from
readLegacyMigrationPayload() the same; update the logic in the function handling
migration start to distinguish "no legacy data" vs "failed to read/malformed
payload": call readLegacyMigrationPayload(), detect whether the payload is
absent (e.g., storage key missing) and only then set the MIGRATION_FLAG_KEY,
call clearMigrationProgress(), and markMigrationComplete(); if the payload
exists but is unreadable/malformed, do NOT mark migration complete — instead
clear transient progress (clearMigrationProgress()), preserve or explicitly
remove orphaned keys matching mcp-tokens-* and mcp-env-* as appropriate, and
return an error/result that allows retry rather than permanently suppressing
retries; locate and change the conditional around readLegacyMigrationPayload(),
clearMigrationProgress(), and markMigrationComplete() to implement these two
distinct branches.
- Around line 301-341: The migration loop is only guarded per-process
(inFlightRef) and can race across browser tabs causing duplicate
deps.createProject calls for the same project.id; replace the in-memory guard by
an atomic, storage-backed lease or make the creation idempotent: before calling
createProject in the payload.projects loop, acquire a short-lived lock in
localStorage/sessionStorage (or write a “migrating:<project.id>” flag) and only
proceed if the lock was obtained, then on success persist the migrated marker
via recordMigratedProjectId(project.id) and release the lock; alternatively,
modify deps.createProject to accept an idempotency key (project.id) so the
server deduplicates concurrent calls — update references to
readMigratedProjectIds, recordMigratedProjectId, and deps.createProject
accordingly and ensure lock acquisition/release is safe across tabs and robust
to crashes.
In `@mcpjam-inspector/server/routes/mcp/connect.ts`:
- Around line 102-105: The route currently calls
mcpClientManager.disconnectServer(serverDisplayName) unconditionally which can
cause an early failure when a server isn't connected; change the logic to guard
that call (e.g., check mcpClientManager.isConnected(serverDisplayName) or wrap
disconnectServer in a targeted try/catch that ignores "not connected" errors)
before calling disconnectServer, and apply the same guard to the other
occurrence that calls disconnectServer prior to connectToServer; ensure
connectToServer(serverDisplayName, resolved.config) still runs when there was no
prior connection.
---
Outside diff comments:
In `@mcpjam-inspector/server/routes/mcp/servers.ts`:
- Around line 257-264: The legacy normalization branch that reads urlValue.href
can throw when constructing a URL, causing a 500; wrap the URL construction in a
try/catch and only assign to cfg.url if new URL(...) succeeds (or otherwise
leave cfg.url unset/invalid so the existing validation path returns a 400).
Specifically, inside the branch that checks urlValue and "href" (the block that
assigns cfg.url = new URL(...).toString()), catch any errors from new
URL((urlValue as { href: string }).href) and avoid throwing—log or ignore the
error and do not set cfg.url so downstream validation (the later 400 path)
handles the malformed value.
---
Duplicate comments:
In `@mcpjam-inspector/client/src/lib/local-state-migration.ts`:
- Around line 333-340: The call to deps.createProject when migrating projects
omits project.visibility, so migrated projects lose their original
public/private setting; update the object passed to deps.createProject in
local-state-migration.ts to include visibility: project.visibility so the
MigrationDeps.createProject receives and preserves the legacy visibility value
(ensure the key name matches the createProject parameter expected by the
MigrationDeps implementation).
🪄 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: 9cad5e2e-5e44-48d9-bc16-6e4c379c0c5e
📒 Files selected for processing (4)
mcpjam-inspector/client/src/lib/__tests__/local-state-migration.test.tsmcpjam-inspector/client/src/lib/local-state-migration.tsmcpjam-inspector/server/routes/mcp/connect.tsmcpjam-inspector/server/routes/mcp/servers.ts
| const alreadyMigrated = readMigratedProjectIds(); | ||
|
|
||
| for (const project of payload.projects) { | ||
| if (alreadyMigrated.has(project.id)) { | ||
| deps.logger?.info("Skipping already-migrated project", { | ||
| name: project.name, | ||
| id: project.id, | ||
| }); | ||
| continue; | ||
| } | ||
| // Merge mcp-env-${name} into per-server config so syncProjectServers picks | ||
| // it up. STDIO servers may have env in either place; the merge is | ||
| // last-write-wins favoring the localStorage env entry (which is what the | ||
| // user actually configured most recently). | ||
| const projectServers: Record<string, ServerWithName> = {}; | ||
| for (const [name, server] of Object.entries(project.servers)) { | ||
| const env = payload.envByName[name]; | ||
| if (env && env !== undefined && (server.config as any)?.command) { | ||
| projectServers[name] = { | ||
| ...server, | ||
| config: { | ||
| ...(server.config as any), | ||
| env: { ...((server.config as any).env ?? {}), ...env }, | ||
| } as any, | ||
| }; | ||
| } else { | ||
| projectServers[name] = server; | ||
| } | ||
| } | ||
|
|
||
| const serializedServers = serializeServersForSharing(projectServers); | ||
| try { | ||
| await deps.createProject({ | ||
| name: project.name, | ||
| description: project.description, | ||
| icon: project.icon, | ||
| clientConfig: project.clientConfig, | ||
| servers: serializedServers, | ||
| organizationId: deps.organizationId, | ||
| }); | ||
| recordMigratedProjectId(project.id); |
There was a problem hiding this comment.
This is only idempotent within a single tab.
use-local-state-migration.ts suppresses re-entry with inFlightRef, but that's process-local. Two tabs opened before either writes the completion flag can both pass this loop and call createProject, duplicating projects in Convex. A storage-backed lease or an idempotent backend create is needed here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/lib/local-state-migration.ts` around lines 301 -
341, The migration loop is only guarded per-process (inFlightRef) and can race
across browser tabs causing duplicate deps.createProject calls for the same
project.id; replace the in-memory guard by an atomic, storage-backed lease or
make the creation idempotent: before calling createProject in the
payload.projects loop, acquire a short-lived lock in localStorage/sessionStorage
(or write a “migrating:<project.id>” flag) and only proceed if the lock was
obtained, then on success persist the migrated marker via
recordMigratedProjectId(project.id) and release the lock; alternatively, modify
deps.createProject to accept an idempotency key (project.id) so the server
deduplicates concurrent calls — update references to readMigratedProjectIds,
recordMigratedProjectId, and deps.createProject accordingly and ensure lock
acquisition/release is safe across tabs and robust to crashes.
Five fixes from PR review of the resolver-path migration: 1. STATE-format migration fallback. `local-state-migration` now lifts servers out of the pre-projects `mcp-inspector-state` shape into a synthetic default project when no `mcp-inspector-projects` / `mcp-inspector-workspaces` is present, mirroring the in-process lift in `state/storage.ts`. Without this, users on the older format would have the migration mark itself complete with nothing pushed to Convex and then have STATE cleared on the next boot. 2. OAuth discovery key cleanup. The legacy-key sweep was looking for `mcp-oauth-discovery-*` but the live OAuth code in `lib/oauth/mcp-oauth.ts` writes `mcp-discovery-*`. Added the current prefix (and `mcp-oauth-flow-state-*`) so post-migration discovery state is actually cleared. 3. Hosted-strips-env defense in depth. Added `stripStdioFieldsFromHostedConfig` in `routes/web/auth.ts` which deletes `command/args/env` from any `serverConfig` returned through `/web/authorize-batch` (hosted), even if a backend regression lets them through. Local mode continues to carry STDIO fields via `/web/authorize-batch-local` + `local-server-resolver.ts`. Added a regression test that feeds STDIO fields into a mocked Convex response and asserts they're absent on the wrapper return. 4. Resolver path now forwards project connection defaults. `testConnection` / `reconnectServer` accept a `connectionDefaults` option (headers, timeoutMs, clientCapabilities); `guardedTestConnection` / `guardedReconnectServer` extract these from the runtime serverConfig that `withProjectConnectionDefaults` produced. The local Hono `/api/mcp/connect` and `/api/mcp/servers/reconnect` parse and forward them via `parseConnectionDefaults` to `resolveLocalServerForConnect`, which merges them into the resolved `MCPServerConfig` (project headers overlay Convex-stored server headers, OAuth Authorization always wins). Without this, project-level headers/timeout/capabilities applied client-side were lost when the resolver fetched per-server config from Convex. Added a connect.test.ts assertion covering the merge. 5. Narrowed `authFetch` hosted-bearer attachment. `getHostedAuthorizationHeader` is now only awaited (and the bearer only attached) for paths that actually call Convex on the user's behalf — `/api/web/*`, `/api/mcp/connect`, `/api/mcp/servers/reconnect`. Unrelated local paths (`/api/session-token`, `/api/health`, local-only MCP read paths) no longer block on minting a guest session at cold boot, and 401s from those paths no longer trigger guest-session refresh. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chelojimenez
temporarily deployed
to
preview-pr-2028
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d86d7449c9
ℹ️ 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 HOSTED_AUTH_PATH_PREFIXES = [ | ||
| "/api/web/", | ||
| // Local resolver path that calls Convex /web/authorize-batch-local. | ||
| "/api/mcp/connect", | ||
| "/api/mcp/servers/reconnect", | ||
| ]; |
There was a problem hiding this comment.
With this allow-list, authFetch no longer attaches a hosted/guest bearer to absolute Convex site HTTP actions such as https://...convex.site/web/registry/catalog in client/src/lib/apis/registry-http.ts or /web/oauth/complete in client/src/lib/oauth/mcp-oauth.ts, because their pathnames are /web/... or /registry/..., not /api/web/.... Those calls previously received getHostedAuthorizationHeader() unconditionally, so hosted registry and OAuth-completion flows that rely on Convex HTTP actions will start returning unauthorized unless every caller supplies its own Authorization header.
Useful? React with 👍 / 👎.
| if ((server.config as any).env) | ||
| config.env = (server.config as any).env; |
There was a problem hiding this comment.
This serializer is also used by the share/invite path (ShareProjectDialog calls serializeServersForSharing before createProject), so adding config.env here sends every STDIO environment variable to the shared Convex project and makes secrets such as API keys available outside the local machine. The migration path needs envs, but the sharing serializer previously stripped sensitive HTTP Authorization headers and should not blindly include local env secrets for project sharing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/server/routes/mcp/servers.ts (1)
252-267:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCatch invalid
href-style URLs before normalizing.The
hrefbranch callsnew URL(...)outside any validation guard. A malformed legacy payload here still throws a 500 instead of returning the 400 “Invalid server URL” response you already use elsewhere in this route.Suggested patch
} else if ( typeof urlValue === "object" && urlValue !== null && "href" in (urlValue as Record<string, unknown>) && typeof (urlValue as { href?: unknown }).href === "string" ) { - cfg.url = new URL((urlValue as { href: string }).href).toString(); + try { + cfg.url = new URL((urlValue as { href: string }).href).toString(); + } catch { + return c.json( + { success: false, error: "Invalid server URL" }, + 400, + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/servers.ts` around lines 252 - 267, The href branch normalizes cfg.url by calling new URL(...) without guarding against malformed hrefs; wrap the URL construction in a validation step (try/catch) and if new URL(...) throws, propagate the existing 400 "Invalid server URL" response instead of allowing a 500; specifically update the branch that checks typeof urlValue === "object" && "href" in urlValue to validate (urlValue as { href?: unknown }).href is a string and then attempt to construct new URL(href) inside a try/catch, setting cfg.url only on success and returning the route's 400 error on failure.
♻️ Duplicate comments (4)
mcpjam-inspector/client/src/lib/local-state-migration.ts (2)
388-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve project visibility during migration.
The legacy reader keeps
project.visibility, andMigrationDeps.createProject()accepts it, but the migration drops it here. Public/private projects will silently fall back to the backend default after migration.Suggested patch
await deps.createProject({ name: project.name, description: project.description, icon: project.icon, clientConfig: project.clientConfig, servers: serializedServers, organizationId: deps.organizationId, + visibility: project.visibility, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/local-state-migration.ts` around lines 388 - 395, The migration is dropping project.visibility when calling deps.createProject, so preserve visibility by passing project.visibility into the createProject call; update the call site where deps.createProject(...) is invoked (the code that currently sets name, description, icon, clientConfig, servers, organizationId) to include visibility: project.visibility so MigrationDeps.createProject / createProject receives and sets the legacy visibility value.
347-351:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mark unreadable legacy state as fully migrated.
This branch treats
readLegacyMigrationPayload() === nullas “nothing to do”, but that function also returnsnullwhen the legacy payload exists and is malformed. In that case we permanently setMIGRATION_FLAG_KEYand suppress any future retry, even though user data may still be sitting in localStorage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/local-state-migration.ts` around lines 347 - 351, The current branch treats readLegacyMigrationPayload() === null as “nothing to do” and calls markMigrationComplete(), which hides cases where the legacy payload exists but is malformed; instead, detect whether the legacy payload key actually exists in storage (e.g. by checking localStorage.getItem for the legacy payload key or adding an isPayloadPresent helper) and only call markMigrationComplete() when there truly is no payload key; if the key exists but readLegacyMigrationPayload() returned null (malformed), call clearMigrationProgress() and return an error/result indicating migration failed without setting the MIGRATION_FLAG_KEY via markMigrationComplete() so the migration can be retried. Ensure this change references readLegacyMigrationPayload(), clearMigrationProgress(), and markMigrationComplete() so it’s applied in the same function.mcpjam-inspector/server/utils/local-server-resolver.ts (1)
138-153:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to the Convex authorization fetch.
This call sits directly on the
/api/mcp/connectand/api/mcp/servers/reconnectrequest path. If Convex accepts the connection and never responds, this handler hangs indefinitely and burns a server worker until the client gives up.Suggested hardening
- let response: Response; + let response: Response; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10_000); try { response = await fetch(`${convexUrl}/web/authorize-batch-local`, { method: "POST", headers: { "Content-Type": "application/json", Authorization: `Bearer ${bearerToken}`, }, body: JSON.stringify({ projectId, serverIds }), + signal: controller.signal, }); } catch (error) { throw new WebRouteError( - 502, + error instanceof Error && error.name === "AbortError" ? 504 : 502, ErrorCode.SERVER_UNREACHABLE, - `Failed to reach authorization service: ${parseErrorMessage(error)}` + error instanceof Error && error.name === "AbortError" + ? "Authorization service timed out" + : `Failed to reach authorization service: ${parseErrorMessage(error)}` ); + } finally { + clearTimeout(timeoutId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/utils/local-server-resolver.ts` around lines 138 - 153, The Convex authorization fetch in local-server-resolver.ts can hang indefinitely; wrap the fetch call that posts to `${convexUrl}/web/authorize-batch-local` with an AbortController and a timeout (e.g., 5s) so the request is aborted if Convex doesn’t respond; ensure you clear the timeout on success, catch the abort/timeout and throw a WebRouteError (use ErrorCode.SERVER_UNREACHABLE or an appropriate code) with a descriptive message that includes parseErrorMessage(error), and keep the existing error handling for other failures—adjust the existing try/catch around the fetch (which uses response, bearerToken, projectId, serverIds) to handle the abort case.mcpjam-inspector/server/routes/mcp/connect.ts (1)
104-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the disconnect before connecting.
Both code paths still call
disconnectServer(...)unconditionally. If that key is stale or has never been connected, the request can fail beforeconnectToServer(...)ever runs, which turns a first connect into a 500.Suggested patch
- await mcpClientManager.disconnectServer(serverDisplayName); + if (mcpClientManager.getClient(serverDisplayName)) { + await mcpClientManager.disconnectServer(serverDisplayName); + } await mcpClientManager.connectToServer(serverDisplayName, resolved.config);- await mcpClientManager.disconnectServer(serverId); + if (mcpClientManager.getClient(serverId)) { + await mcpClientManager.disconnectServer(serverId); + } await mcpClientManager.connectToServer(serverId, serverConfig);Also applies to: 180-182
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/connect.ts` around lines 104 - 106, The code currently calls mcpClientManager.disconnectServer(serverDisplayName) unconditionally which can fail for stale/absent keys and prevent connectToServer from running; change this to only call disconnectServer when a connection exists (e.g., use an existing predicate like mcpClientManager.hasConnection(serverDisplayName) or mcpClientManager.getClient(serverDisplayName) to check) or, if no predicate exists, catch and ignore the specific "not found" error from disconnectServer so it doesn't abort the flow; apply the same guard to the other occurrence (the block referenced at lines 180-182) and keep connectToServer(serverDisplayName, resolved.config) unconditional after the guarded disconnect.
🧹 Nitpick comments (2)
mcpjam-inspector/server/routes/mcp/__tests__/connect.test.ts (2)
243-291: ⚡ Quick winAssert a server-only header survives the overlay.
This still passes if the runtime defaults replace the stored header map instead of merging it, because every asserted key comes from
connectionDefaults. Add one header that exists only in the Convex-resolved config and assert it remains after the overlay.Proposed test tightening
serverConfig: { transportType: "http", url: "http://localhost:3000/mcp", - headers: { "X-Server-Default": "from-convex" }, + headers: { + "X-Server-Default": "from-convex", + "X-Server-Only": "kept-from-convex", + }, useOAuth: false, }, @@ expect(cfg.requestInit.headers).toMatchObject({ "X-Project-Default": "from-runtime", "X-Server-Default": "overridden-by-runtime", + "X-Server-Only": "kept-from-convex", });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/__tests__/connect.test.ts` around lines 243 - 291, The test currently only asserts headers that come from connectionDefaults so it won't catch a bug where the runtime headers replace rather than merge serverConfig.headers; update the test around the mcpClientManager.connectToServer call (inspect callArgs[1] -> cfg.requestInit.headers) to include an assertion that a header present only in the Convex-resolved serverConfig (e.g. "X-Server-Only" or add one to serverConfig.headers in the mocked batch authorize response) still exists and retains its value after the overlay, while keeping the existing assertions for overridden keys, timeout (cfg.timeout) and clientCapabilities (cfg.clientCapabilities).
405-434: ⚡ Quick winVerify reconnect waits for
disconnectServerto settle.
invocationCallOrderproves call order, not await order. A regression that invokesdisconnectServerand immediately startsconnectToServerbefore the disconnect promise resolves would still satisfy this test. HolddisconnectServeropen and assertconnectToServerstays untouched until you release it.Proposed async-order assertion
+ let releaseDisconnect!: () => void; + const disconnectPending = new Promise<void>((resolve) => { + releaseDisconnect = resolve; + }); + mcpClientManager.disconnectServer.mockImplementationOnce( + () => disconnectPending + ); + - const res = await app.request("/api/mcp/connect", { + const responsePromise = app.request("/api/mcp/connect", { method: "POST", headers: authHeaders(), body: JSON.stringify({ projectId: PROJECT_ID, serverId: SERVER_ID, serverName: SERVER_NAME }), }); + await Promise.resolve(); + expect(mcpClientManager.connectToServer).not.toHaveBeenCalled(); + + releaseDisconnect(); + const res = await responsePromise; expect(res.status).toBe(200); - const disconnectOrder = - mcpClientManager.disconnectServer.mock.invocationCallOrder[0]; - const connectOrder = - mcpClientManager.connectToServer.mock.invocationCallOrder[0]; - expect(disconnectOrder).toBeLessThan(connectOrder); + expect(mcpClientManager.connectToServer).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/__tests__/connect.test.ts` around lines 405 - 434, The test currently only checks call order via invocationCallOrder but should assert that connectToServer does not start until disconnectServer's promise resolves; update the test for "disconnects existing connection before reconnecting" to replace the simple mock with a controllable/deferred promise for mcpClientManager.disconnectServer, verify mcpClientManager.connectToServer has not been called while that promise is pending, then resolve the deferred promise and assert connectToServer is called afterwards; reference mcpClientManager.disconnectServer and mcpClientManager.connectToServer and ensure the request/response flow awaits the connect only after the disconnect promise is settled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/lib/session-token.ts`:
- Around line 262-270: shouldAttachHostedAuthorization currently only inspects
the pathname, allowing absolute foreign URLs to receive hosted bearer tokens;
change it to validate the request origin first: use resolveRequestUrl(input)
(and treat raw relative paths starting with "/" as same-origin) and only proceed
if parsed.origin equals the app origin (e.g., window.location.origin) or the
parsed hostname is a loopback host (localhost, 127.0.0.1, ::1); only after that
check evaluate HOSTED_AUTH_PATH_PREFIXES against pathname so authFetch cannot
attach credentials to cross-origin URLs.
In `@mcpjam-inspector/server/routes/mcp/connect.ts`:
- Around line 188-191: Replace the direct console.debug call in the cleanup
branch with the centralized logger: import logger from "@/utils/logger" if not
already present, then call logger.debug with a structured message and include
the serverId and cleanupError so the log is consistent with other server logs
(e.g., logger.debug("Failed to remove MCP server after connection failure", {
serverId, error: cleanupError })). Ensure any leftover console.* usage in this
file is removed.
In `@mcpjam-inspector/server/routes/mcp/servers.ts`:
- Around line 308-314: The catch block for the reconnect cleanup is logging via
console.debug; replace that with the centralized logger by calling
logger.debug(...) instead and import the shared logger from "@/utils/logger" if
not already present. Update the catch handling around the call to
mcpClientManager.disconnectServer(managerKey) so it uses logger.debug with the
same message and the caught disconnectError (e.g., logger.debug("Failed to
disconnect MCP server %s before reconnect", managerKey, disconnectError)) to
keep logs structured and consistent with the rest of the server code.
---
Outside diff comments:
In `@mcpjam-inspector/server/routes/mcp/servers.ts`:
- Around line 252-267: The href branch normalizes cfg.url by calling new
URL(...) without guarding against malformed hrefs; wrap the URL construction in
a validation step (try/catch) and if new URL(...) throws, propagate the existing
400 "Invalid server URL" response instead of allowing a 500; specifically update
the branch that checks typeof urlValue === "object" && "href" in urlValue to
validate (urlValue as { href?: unknown }).href is a string and then attempt to
construct new URL(href) inside a try/catch, setting cfg.url only on success and
returning the route's 400 error on failure.
---
Duplicate comments:
In `@mcpjam-inspector/client/src/lib/local-state-migration.ts`:
- Around line 388-395: The migration is dropping project.visibility when calling
deps.createProject, so preserve visibility by passing project.visibility into
the createProject call; update the call site where deps.createProject(...) is
invoked (the code that currently sets name, description, icon, clientConfig,
servers, organizationId) to include visibility: project.visibility so
MigrationDeps.createProject / createProject receives and sets the legacy
visibility value.
- Around line 347-351: The current branch treats readLegacyMigrationPayload()
=== null as “nothing to do” and calls markMigrationComplete(), which hides cases
where the legacy payload exists but is malformed; instead, detect whether the
legacy payload key actually exists in storage (e.g. by checking
localStorage.getItem for the legacy payload key or adding an isPayloadPresent
helper) and only call markMigrationComplete() when there truly is no payload
key; if the key exists but readLegacyMigrationPayload() returned null
(malformed), call clearMigrationProgress() and return an error/result indicating
migration failed without setting the MIGRATION_FLAG_KEY via
markMigrationComplete() so the migration can be retried. Ensure this change
references readLegacyMigrationPayload(), clearMigrationProgress(), and
markMigrationComplete() so it’s applied in the same function.
In `@mcpjam-inspector/server/routes/mcp/connect.ts`:
- Around line 104-106: The code currently calls
mcpClientManager.disconnectServer(serverDisplayName) unconditionally which can
fail for stale/absent keys and prevent connectToServer from running; change this
to only call disconnectServer when a connection exists (e.g., use an existing
predicate like mcpClientManager.hasConnection(serverDisplayName) or
mcpClientManager.getClient(serverDisplayName) to check) or, if no predicate
exists, catch and ignore the specific "not found" error from disconnectServer so
it doesn't abort the flow; apply the same guard to the other occurrence (the
block referenced at lines 180-182) and keep connectToServer(serverDisplayName,
resolved.config) unconditional after the guarded disconnect.
In `@mcpjam-inspector/server/utils/local-server-resolver.ts`:
- Around line 138-153: The Convex authorization fetch in
local-server-resolver.ts can hang indefinitely; wrap the fetch call that posts
to `${convexUrl}/web/authorize-batch-local` with an AbortController and a
timeout (e.g., 5s) so the request is aborted if Convex doesn’t respond; ensure
you clear the timeout on success, catch the abort/timeout and throw a
WebRouteError (use ErrorCode.SERVER_UNREACHABLE or an appropriate code) with a
descriptive message that includes parseErrorMessage(error), and keep the
existing error handling for other failures—adjust the existing try/catch around
the fetch (which uses response, bearerToken, projectId, serverIds) to handle the
abort case.
---
Nitpick comments:
In `@mcpjam-inspector/server/routes/mcp/__tests__/connect.test.ts`:
- Around line 243-291: The test currently only asserts headers that come from
connectionDefaults so it won't catch a bug where the runtime headers replace
rather than merge serverConfig.headers; update the test around the
mcpClientManager.connectToServer call (inspect callArgs[1] ->
cfg.requestInit.headers) to include an assertion that a header present only in
the Convex-resolved serverConfig (e.g. "X-Server-Only" or add one to
serverConfig.headers in the mocked batch authorize response) still exists and
retains its value after the overlay, while keeping the existing assertions for
overridden keys, timeout (cfg.timeout) and clientCapabilities
(cfg.clientCapabilities).
- Around line 405-434: The test currently only checks call order via
invocationCallOrder but should assert that connectToServer does not start until
disconnectServer's promise resolves; update the test for "disconnects existing
connection before reconnecting" to replace the simple mock with a
controllable/deferred promise for mcpClientManager.disconnectServer, verify
mcpClientManager.connectToServer has not been called while that promise is
pending, then resolve the deferred promise and assert connectToServer is called
afterwards; reference mcpClientManager.disconnectServer and
mcpClientManager.connectToServer and ensure the request/response flow awaits the
connect only after the disconnect promise is settled.
🪄 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: 89cb5cc0-44b6-49de-9d4c-49b2ba279254
📒 Files selected for processing (11)
mcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/lib/__tests__/local-state-migration.test.tsmcpjam-inspector/client/src/lib/local-state-migration.tsmcpjam-inspector/client/src/lib/session-token.tsmcpjam-inspector/client/src/state/mcp-api.tsmcpjam-inspector/server/routes/mcp/__tests__/connect.test.tsmcpjam-inspector/server/routes/mcp/connect.tsmcpjam-inspector/server/routes/mcp/servers.tsmcpjam-inspector/server/routes/web/__tests__/authorize-batch.test.tsmcpjam-inspector/server/routes/web/auth.tsmcpjam-inspector/server/utils/local-server-resolver.ts
Additional fixes from inline review (skipping the registry path per
follow-up direction; that surface is being refactored separately):
Security / regression:
* `session-token.shouldAttachHostedAuthorization` now requires same-origin,
loopback, or the configured `*.convex.site` hostname before attaching the
hosted bearer. Without this an absolute foreign URL (`https://evil/api/web/...`)
matching a path prefix could exfiltrate the bearer.
* Added `/web/oauth/` to the path allowlist so the hosted OAuth flow
(`https://*.convex.site/web/oauth/...`) keeps working — the previous
narrowing inadvertently dropped the bearer for these calls.
Secret leak:
* Split `serializeServersForSharing` into two named exports:
`serializeServersForSharing` (redacts STDIO `env`, used by share/invite
flows incl. `ShareProjectDialog`) and `serializeServersForPersistence`
(preserves `env`, used by the legacy localStorage→Convex migration).
Three reviewers flagged that the migration's env-preserving change had
also opened up share payloads to leak STDIO secrets.
Local OAuth resilience:
* `testConnection`/`reconnectServer` now fall back to the legacy
`{serverConfig, serverId}` body when the runtime config carries a local
OAuth `Authorization` bearer (local OAuth tokens still live in
localStorage; the resolver path would reject `useOAuth` servers without
a Convex-stored `oauthAccessToken`).
Resolver hardening:
* Added a 10s `AbortController` timeout on the inline
`/web/authorize-batch-local` fetch in `local-server-resolver.ts`. A hung
Convex would otherwise tie up the inspector worker indefinitely on
`/api/mcp/connect` and `/api/mcp/servers/reconnect`.
* Guarded the pre-connect `disconnectServer` call in `connect.ts` (both
resolver and legacy paths) so a first-connect with nothing to disconnect
doesn't 500 before `connectToServer` runs. Mirrors the existing tolerance
in `/servers/reconnect`.
Migration robustness:
* `readLegacyMigrationPayload` now returns a tri-state
(`empty | unreadable | payload`) so the runner only marks the migration
complete when storage is genuinely empty. An unreadable/malformed legacy
payload no longer permanently suppresses retries.
* `useLocalStateMigration` waits for `users:ensureUser` (`isEnsuringUser`)
before firing — fixes the race where `projects:createProject` could land
before the actor's row + default org existed.
* Added a backoff retry: a `setTimeout` driven `retryTick` re-runs the
effect after partial failures. Previously `useMutation` returns a stable
reference and the gate inputs rarely change, so "retry on next render"
effectively waited for a page reload.
* Added a localStorage-backed migration lease (TTL 5 min) so two tabs
opened pre-migration don't both call `createProject` for the same legacy
project. A crashed-mid-migration tab is superseded after the TTL.
* Migration now forwards `project.visibility` to `createProject`, so
legacy public/private settings survive.
Logging guideline:
* Replaced the remaining `console.debug` calls in `connect.ts` and
`servers.ts` with `logger.debug` per server-side coding guideline.
All 3603 tests still pass; no new typecheck errors introduced (only
pre-existing ones on main).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chelojimenez
temporarily deployed
to
preview-pr-2028
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 356fc2ac38
ℹ️ 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".
| for (const project of payload.projects) { | ||
| if (alreadyMigrated.has(project.id)) { | ||
| deps.logger?.info("Skipping already-migrated project", { | ||
| name: project.name, | ||
| id: project.id, | ||
| }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
For existing local installs that have already used cloud sync, mcp-inspector-projects contains projects with sharedProjectId persisted by saveAppState, but this new migration loops over every legacy project and calls createProject unless only the new progress key says it already ran. On first boot after this change, those already-linked projects are created again in Convex, duplicating projects/servers before the migration flag is set. The older in-app migration explicitly filtered out project.sharedProjectId; this one needs the same skip.
Useful? React with 👍 / 👎.
| if (HOSTED_MODE) return; | ||
| if (!isAuthenticated) return; | ||
| if (isUserBootstrapping) return; | ||
| if (doneRef.current) return; | ||
| if (inFlightRef.current) return; | ||
| if (hasMigrationCompleted()) { | ||
| doneRef.current = true; |
There was a problem hiding this comment.
In signed-in local sessions, this effect can run as soon as Convex auth and ensureUser finish, while useOrganizationQueries is still loading and activeOrganizationId is still undefined. In that window runLocalStateMigration calls createProject with organizationId: undefined, so Convex falls back to the actor's default org, then clears the legacy keys and sets the migration flag; users with a stored/route-selected different org have their local projects migrated to the wrong organization permanently. Gate this on organization loading/selection resolution for signed-in users before acquiring the migration lease.
Useful? React with 👍 / 👎.
The retry mechanism added in the previous fix was unconditional — any `ok: false` result scheduled another attempt 30s later. When the failure is a permanent backend rejection (e.g. `billing_limit_reached` from the free-plan workspace cap), the loop just spams the console every 30s for the lifetime of the tab. Two new gates: * `MAX_RETRY_ATTEMPTS` (3): hard cap on automatic retries. After this many attempts the hook marks itself done and stops scheduling. The user can reload to retry. * `PERMANENT_ERROR_CODES`: error-message substring match against `billing_limit_reached`, `FORBIDDEN`, `UNAUTHORIZED`, `VALIDATION_ERROR`. Hits any of these and the migration stops immediately — no retry, no log noise. Also threads the current `attempt` count into the warn/error logs so the source of multiple retries is obvious in the console. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chelojimenez
temporarily deployed
to
preview-pr-2028
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/lib/project-serialization.ts (1)
55-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
Authorizationheaders in the persistence path.
serializeServersForPersistence()also flows through this branch during the localStorage → Convex migration, so strippingrequestInit.headers.Authorizationhere drops user-configured HTTP credentials from legacy servers. After migration, those servers reconnect without the header they were using before.💡 Suggested patch
if ((server.config as any).requestInit.headers) { const headers: Record<string, string> = {}; for (const [key, value] of Object.entries( (server.config as any).requestInit.headers, )) { - if (key.toLowerCase() !== "authorization") { + if (!options.redactSecrets || key.toLowerCase() !== "authorization") { headers[key] = value as string; } } requestInit.headers = headers; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/project-serialization.ts` around lines 55 - 67, The serialization currently removes Authorization from requestInit.headers when building requestInit in serializeServersForPersistence (or the function that iterates server.config.requestInit), which drops user HTTP credentials during migration; change the logic so it preserves the Authorization header (i.e., do not skip keys where key.toLowerCase() === "authorization") and copy all header entries from (server.config as any).requestInit.headers into requestInit.headers, ensuring existing behavior for other headers remains unchanged.
♻️ Duplicate comments (2)
mcpjam-inspector/server/routes/mcp/servers.ts (1)
252-266:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe malformed legacy URL case is still reachable through
hrefnormalization.
new URL((urlValue as { href: string }).href)can still throw before the later 400 validation path runs, so a bad{ url: { href: ... } }body becomes a 500 again.Patch sketch
} else if ( typeof urlValue === "object" && urlValue !== null && "href" in (urlValue as Record<string, unknown>) && typeof (urlValue as { href?: unknown }).href === "string" ) { - cfg.url = new URL((urlValue as { href: string }).href).toString(); + try { + cfg.url = new URL((urlValue as { href: string }).href).toString(); + } catch { + return c.json( + { success: false, error: "Invalid server URL" }, + 400, + ); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/server/routes/mcp/servers.ts` around lines 252 - 266, The current normalization branch uses new URL((urlValue as { href: string }).href).toString() which can throw and cause a 500; change this to avoid parsing here so malformed hrefs fall through to the existing 400 validation. Replace the new URL(...) call in the servers.ts block handling cfg.url (the urlValue / cfg variables) with a safe assignment of the raw href string ((urlValue as { href?: unknown }).href as string) or, if you must parse, wrap new URL(...) in a try/catch and on error assign the raw href string to cfg.url so the later validation returns a 400 instead of throwing.mcpjam-inspector/client/src/lib/local-state-migration.ts (1)
221-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not fall back to
mcp-inspector-statewhen the newer store is unreadable.If
mcp-inspector-projectsormcp-inspector-workspacesexists but cannot be parsed, this branch still migrates the oldermcp-inspector-statepayload. A later successful run clears all legacy keys, so the unreadable higher-priority data is effectively discarded in favor of stale fallback state.💡 Suggested patch
const projectsRead = readProjectsFromLegacyStorage(); let projects: Project[] = projectsRead ?? []; let unreadable: string | null = null; if (projectsRead === null) { - unreadable = "legacy projects/workspaces store is unreadable"; + return { + kind: "unreadable", + reason: "legacy projects/workspaces store is unreadable", + }; } if (projects.length === 0) { const stateResult = readProjectFromLegacyStateOnly(); if (stateResult === "unreadable") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mcpjam-inspector/client/src/lib/local-state-migration.ts` around lines 221 - 236, The code currently treats a null return from readProjectsFromLegacyStorage() (which signals an unreadable newer store) the same as no data and then falls back to readProjectFromLegacyStateOnly(); change the fallback logic so we only attempt readProjectFromLegacyStateOnly() when the newer store was absent (undefined/empty) not when it was unreadable (null). Concretely, keep the unreadable assignment when projectsRead === null, but only run the fallback branch (the call to readProjectFromLegacyStateOnly and subsequent assignment to projects) when projectsRead !== null (or explicitly projectsRead === undefined) — reference readProjectsFromLegacyStorage, projectsRead, projects, unreadable, and readProjectFromLegacyStateOnly. Ensure unreadable remains set and you do not overwrite/clear legacy keys when projectsRead was null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mcpjam-inspector/client/src/hooks/use-local-state-migration.ts`:
- Around line 48-74: The lease is just a timestamp and not owner-aware, so
concurrent tabs can overwrite each other and release other tabs' leases; update
tryAcquireMigrationLease and releaseMigrationLease to use an owner id: generate
a unique ownerId per tab (e.g., UUID stored in-memory), store the lease value as
a JSON string { ts, owner } under MIGRATION_LEASE_KEY, and when acquiring
perform the same TTL check but write the JSON and then re-read the key to
confirm the stored owner matches our ownerId (treat mismatch as acquisition
failure); when releasing, only remove the key if the current stored owner equals
our ownerId; preserve existing try/catch fallbacks for localStorage errors and
keep behavior of returning booleans/void as before, and reference
MIGRATION_LEASE_TTL_MS for TTL logic.
In `@mcpjam-inspector/client/src/lib/session-token.ts`:
- Around line 278-295: The allowlist currently lets any loopback hostname and
any URL sharing the Convex hostname (even with different scheme/port) through;
tighten isHostedAuthAllowedOrigin by (1) only allowing loopback origins when the
origin exactly matches the current page origin (i.e., require parsed.origin ===
window.location.origin in addition to isLoopbackHostname(parsed.hostname)), and
(2) when checking the configured convexSite use the full origin comparison
(compare parsed.origin to new URL(convexSite).origin) instead of comparing
hostnames so scheme and port must match; keep the try/catch for malformed
convexSite.
In `@mcpjam-inspector/client/src/state/mcp-api.ts`:
- Around line 163-176: hasLocalOAuthBearer currently treats any Authorization:
Bearer header as “local OAuth” which is unsafe; change it to avoid sniffing the
header value and instead require an explicit local-OAuth signal. Update the
function (hasLocalOAuthBearer) to stop checking
value.toLowerCase().startsWith("bearer ") and instead look for a dedicated
explicit flag on serverConfig (e.g. serverConfig.localOAuth === true) or a
dedicated header key (e.g. requestInit.headers['x-local-oauth'] === '1') and
return true only when that explicit signal is present; keep using
requestInit?.headers to locate the explicit header if you choose the header
approach and ensure other codepaths still treat missing signal as false.
In `@mcpjam-inspector/server/routes/mcp/connect.ts`:
- Around line 67-83: resolveLocalServerForConnect currently returns local-mode
config verbatim and is used to connect without the HOSTED_MODE transport checks;
update the code that calls resolveLocalServerForConnect (the callsite around the
variables projectId/serverId) to validate the returned config's transport mode
exactly like the legacy path does (enforce HOSTED_MODE STDIO/HTTPS restrictions
and reject or sanitize disallowed transports), and throw an appropriate error
when a hosted-mode transport is attempted; apply the same validation logic to
the other resolveLocalServerForConnect usage (the similar block later in the
file) so both resolver paths enforce the hosted transport checks consistently.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/lib/project-serialization.ts`:
- Around line 55-67: The serialization currently removes Authorization from
requestInit.headers when building requestInit in serializeServersForPersistence
(or the function that iterates server.config.requestInit), which drops user HTTP
credentials during migration; change the logic so it preserves the Authorization
header (i.e., do not skip keys where key.toLowerCase() === "authorization") and
copy all header entries from (server.config as any).requestInit.headers into
requestInit.headers, ensuring existing behavior for other headers remains
unchanged.
---
Duplicate comments:
In `@mcpjam-inspector/client/src/lib/local-state-migration.ts`:
- Around line 221-236: The code currently treats a null return from
readProjectsFromLegacyStorage() (which signals an unreadable newer store) the
same as no data and then falls back to readProjectFromLegacyStateOnly(); change
the fallback logic so we only attempt readProjectFromLegacyStateOnly() when the
newer store was absent (undefined/empty) not when it was unreadable (null).
Concretely, keep the unreadable assignment when projectsRead === null, but only
run the fallback branch (the call to readProjectFromLegacyStateOnly and
subsequent assignment to projects) when projectsRead !== null (or explicitly
projectsRead === undefined) — reference readProjectsFromLegacyStorage,
projectsRead, projects, unreadable, and readProjectFromLegacyStateOnly. Ensure
unreadable remains set and you do not overwrite/clear legacy keys when
projectsRead was null.
In `@mcpjam-inspector/server/routes/mcp/servers.ts`:
- Around line 252-266: The current normalization branch uses new URL((urlValue
as { href: string }).href).toString() which can throw and cause a 500; change
this to avoid parsing here so malformed hrefs fall through to the existing 400
validation. Replace the new URL(...) call in the servers.ts block handling
cfg.url (the urlValue / cfg variables) with a safe assignment of the raw href
string ((urlValue as { href?: unknown }).href as string) or, if you must parse,
wrap new URL(...) in a try/catch and on error assign the raw href string to
cfg.url so the later validation returns a 400 instead of throwing.
🪄 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: b18a2fbc-7dd8-41b4-ae33-12bae2a7d6d8
📒 Files selected for processing (9)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/hooks/use-local-state-migration.tsmcpjam-inspector/client/src/lib/local-state-migration.tsmcpjam-inspector/client/src/lib/project-serialization.tsmcpjam-inspector/client/src/lib/session-token.tsmcpjam-inspector/client/src/state/mcp-api.tsmcpjam-inspector/server/routes/mcp/connect.tsmcpjam-inspector/server/routes/mcp/servers.tsmcpjam-inspector/server/utils/local-server-resolver.ts
| function tryAcquireMigrationLease(): boolean { | ||
| if (typeof window === "undefined") return false; | ||
| try { | ||
| const now = Date.now(); | ||
| const existing = localStorage.getItem(MIGRATION_LEASE_KEY); | ||
| if (existing) { | ||
| const ts = Number(existing); | ||
| if (Number.isFinite(ts) && now - ts < MIGRATION_LEASE_TTL_MS) { | ||
| return false; | ||
| } | ||
| } | ||
| localStorage.setItem(MIGRATION_LEASE_KEY, String(now)); | ||
| return true; | ||
| } catch { | ||
| // localStorage blocked — caller falls back to in-memory ref guard. | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| function releaseMigrationLease(): void { | ||
| if (typeof window === "undefined") return; | ||
| try { | ||
| localStorage.removeItem(MIGRATION_LEASE_KEY); | ||
| } catch { | ||
| // best-effort | ||
| } | ||
| } |
There was a problem hiding this comment.
Make the migration lease owner-aware.
This lease is only a timestamp. Two tabs can both read “no lease”, both set it, and both continue. releaseMigrationLease() then removes the key unconditionally, so one tab can clear another tab’s active lease. That leaves the duplicate-project race intact.
💡 Suggested direction
+type MigrationLease = { token: string; ts: number };
+
-function tryAcquireMigrationLease(): boolean {
+function tryAcquireMigrationLease(): string | null {
if (typeof window === "undefined") return false;
try {
const now = Date.now();
- const existing = localStorage.getItem(MIGRATION_LEASE_KEY);
- if (existing) {
- const ts = Number(existing);
- if (Number.isFinite(ts) && now - ts < MIGRATION_LEASE_TTL_MS) {
+ const existingRaw = localStorage.getItem(MIGRATION_LEASE_KEY);
+ if (existingRaw) {
+ const existing = JSON.parse(existingRaw) as MigrationLease;
+ if (Number.isFinite(existing.ts) && now - existing.ts < MIGRATION_LEASE_TTL_MS) {
return false;
}
}
- localStorage.setItem(MIGRATION_LEASE_KEY, String(now));
- return true;
+ const lease = { token: crypto.randomUUID(), ts: now };
+ const serialized = JSON.stringify(lease);
+ localStorage.setItem(MIGRATION_LEASE_KEY, serialized);
+ return localStorage.getItem(MIGRATION_LEASE_KEY) === serialized
+ ? lease.token
+ : null;
} catch {
- return true;
+ return "in-memory-fallback";
}
}
-function releaseMigrationLease(): void {
+function releaseMigrationLease(token: string | null): void {
if (typeof window === "undefined") return;
try {
- localStorage.removeItem(MIGRATION_LEASE_KEY);
+ const currentRaw = localStorage.getItem(MIGRATION_LEASE_KEY);
+ if (!currentRaw) return;
+ const current = JSON.parse(currentRaw) as MigrationLease;
+ if (current.token === token) {
+ localStorage.removeItem(MIGRATION_LEASE_KEY);
+ }
} catch {
// best-effort
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/hooks/use-local-state-migration.ts` around lines
48 - 74, The lease is just a timestamp and not owner-aware, so concurrent tabs
can overwrite each other and release other tabs' leases; update
tryAcquireMigrationLease and releaseMigrationLease to use an owner id: generate
a unique ownerId per tab (e.g., UUID stored in-memory), store the lease value as
a JSON string { ts, owner } under MIGRATION_LEASE_KEY, and when acquiring
perform the same TTL check but write the JSON and then re-read the key to
confirm the stored owner matches our ownerId (treat mismatch as acquisition
failure); when releasing, only remove the key if the current stored owner equals
our ownerId; preserve existing try/catch fallbacks for localStorage errors and
keep behavior of returning booleans/void as before, and reference
MIGRATION_LEASE_TTL_MS for TTL logic.
| function hasLocalOAuthBearer(serverConfig: MCPServerConfig): boolean { | ||
| const headers = (serverConfig as { requestInit?: { headers?: unknown } }) | ||
| ?.requestInit?.headers; | ||
| if (!headers || typeof headers !== "object") return false; | ||
| for (const [key, value] of Object.entries(headers as Record<string, unknown>)) { | ||
| if ( | ||
| key.toLowerCase() === "authorization" && | ||
| typeof value === "string" && | ||
| value.toLowerCase().startsWith("bearer ") | ||
| ) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Don't use a generic Bearer header as the legacy-path switch.
hasLocalOAuthBearer() treats every Authorization: Bearer … header as “local OAuth”. That means any server with a normal static bearer token will bypass { projectId, serverId } and fall back to client-trusted serverConfig, which reopens the trust gap this PR is meant to close.
Please gate this fallback on an explicit local-OAuth signal instead of sniffing the header value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/client/src/state/mcp-api.ts` around lines 163 - 176,
hasLocalOAuthBearer currently treats any Authorization: Bearer header as “local
OAuth” which is unsafe; change it to avoid sniffing the header value and instead
require an explicit local-OAuth signal. Update the function
(hasLocalOAuthBearer) to stop checking value.toLowerCase().startsWith("bearer ")
and instead look for a dedicated explicit flag on serverConfig (e.g.
serverConfig.localOAuth === true) or a dedicated header key (e.g.
requestInit.headers['x-local-oauth'] === '1') and return true only when that
explicit signal is present; keep using requestInit?.headers to locate the
explicit header if you choose the header approach and ensure other codepaths
still treat missing signal as false.
| let resolved; | ||
| try { | ||
| resolved = await resolveLocalServerForConnect( | ||
| c, | ||
| bearer, | ||
| projectId, | ||
| serverId, | ||
| { | ||
| serverDisplayName, | ||
| clientCapabilities: | ||
| typeof body?.clientCapabilities === "object" && | ||
| body.clientCapabilities !== null | ||
| ? body.clientCapabilities | ||
| : undefined, | ||
| defaults: parseConnectionDefaults(body?.connectionDefaults), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Apply hosted transport validation to the resolver path.
resolveLocalServerForConnect() returns the local-mode config verbatim, but this branch connects it without the HOSTED_MODE STDIO/HTTPS checks the legacy path still enforces. That makes the new { projectId, serverId } shape a bypass for hosted-mode RCE/SSRF protections.
Patch sketch
try {
resolved = await resolveLocalServerForConnect(
c,
bearer,
projectId,
serverId,
{
serverDisplayName,
clientCapabilities:
typeof body?.clientCapabilities === "object" &&
body.clientCapabilities !== null
? body.clientCapabilities
: undefined,
defaults: parseConnectionDefaults(body?.connectionDefaults),
},
);
} catch (error) {
if (error instanceof WebRouteError) {
return c.json(
{
success: false,
error: error.message,
...(error.details ?? {}),
},
error.status as any,
);
}
return c.json(
{
success: false,
error: "Failed to resolve server config",
details: error instanceof Error ? error.message : "Unknown error",
},
500,
);
}
+
+ if (HOSTED_MODE && "command" in resolved.config) {
+ return c.json(
+ { success: false, error: "STDIO transport is disabled in the web app" },
+ 403,
+ );
+ }
+ if (HOSTED_MODE && "url" in resolved.config) {
+ const urlForCheck =
+ resolved.config.url instanceof URL
+ ? resolved.config.url
+ : new URL(String(resolved.config.url));
+ if (urlForCheck.protocol !== "https:") {
+ return c.json(
+ {
+ success: false,
+ error:
+ "HTTPS is required in the web app. Please use an https:// URL.",
+ },
+ 400,
+ );
+ }
+ }
try {Also applies to: 105-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mcpjam-inspector/server/routes/mcp/connect.ts` around lines 67 - 83,
resolveLocalServerForConnect currently returns local-mode config verbatim and is
used to connect without the HOSTED_MODE transport checks; update the code that
calls resolveLocalServerForConnect (the callsite around the variables
projectId/serverId) to validate the returned config's transport mode exactly
like the legacy path does (enforce HOSTED_MODE STDIO/HTTPS restrictions and
reject or sanitize disallowed transports), and throw an appropriate error when a
hosted-mode transport is attempted; apply the same validation logic to the other
resolveLocalServerForConnect usage (the similar block later in the file) so both
resolver paths enforce the hosted transport checks consistently.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcf33d9d49
ℹ️ 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".
| useLocalStateMigration({ | ||
| isAuthenticated, | ||
| isUserBootstrapping: isEnsuringUser, | ||
| organizationId: activeOrganizationId, | ||
| }); |
There was a problem hiding this comment.
When a local signed-in or guest session has legacy local projects and Convex initially returns no remote projects, this new migration hook runs in the same App mount as the existing use-project-state migration effect (client/src/hooks/use-project-state.ts:760-823). That older effect still calls projects:createProject for every unshared local project and has no awareness of MIGRATION_FLAG_KEY, while this hook also calls runLocalStateMigration from localStorage, so the first boot after this change can create each local project twice before either subscription observes the new remote rows. Gate or remove the old effect before enabling this one to avoid duplicate Convex projects/servers.
Useful? React with 👍 / 👎.
chelojimenez
temporarily deployed
to
preview-pr-2028
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c8cbbaa76
ℹ️ 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 serializedServers = serializeServersForPersistence(projectServers); |
There was a problem hiding this comment.
For local users who configured a non-OAuth HTTP server with a custom Authorization header, this migration sends the project through serializeServersForPersistence, but that serializer still unconditionally drops Authorization from requestInit.headers before createProject receives the payload. After the migration succeeds, clearLegacyKeys() removes the local project data, so the Convex copy is missing the API key/bearer and reconnects fail even though this path is meant to preserve in-account credentials. This is distinct from the existing STDIO env sharing concern: the affected case is the new persistence migration, not outbound sharing.
Useful? React with 👍 / 👎.
chelojimenez
had a problem deploying
to
preview-pr-2028
GitHub Actions
Error
chelojimenez
temporarily deployed
to
preview-pr-2028
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50cb19f504
ℹ️ 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 (projects.length > 0) { | ||
| return { kind: "payload", payload: { projects, envByName } }; |
There was a problem hiding this comment.
If mcp-inspector-projects/workspaces is present but cannot be parsed and the older mcp-inspector-state key still contains servers, this branch returns a migratable payload from the older state instead of preserving the unreadable failure. A successful migration then calls clearLegacyKeys(), deleting the unreadable newer project store even though none of its projects were migrated. Treat the unreadable newer store as terminal before falling back to state-only data, or avoid clearing it in this mixed case.
Useful? React with 👍 / 👎.
…Jam/inspector into feat/unify-local-hosted-auth
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 937ed9c340
ℹ️ 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 resolved = tryResolveProjectServer(serverName); | ||
| if (resolved) { | ||
| return testConnection(serverConfig, resolved.serverId, { | ||
| projectId: resolved.projectId, | ||
| serverName, | ||
| connectionDefaults: buildResolverConnectionDefaults(serverConfig), |
There was a problem hiding this comment.
When a signed-in local user connects a STDIO server that has env vars, this new branch switches to {projectId, serverId} as soon as syncServerToConvex returns an id, but that sync payload still only persists command/args (not config.env). The local resolver then rebuilds the STDIO config from Convex and falls back to env: {}, so servers that need API keys or PATH-like env fail to start even though the legacy body still has the env from the form/localStorage. Either keep these STDIO-env connects on the legacy path or persist/forward the env before resolving from Convex.
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 937ed9c. Configure here.
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing timer cleanup in lease-contention effect cleanup
Low Severity
When tryAcquireMigrationLease() returns false (another tab holds the lease), scheduleRetry() is called which sets retryTimerRef.current via setTimeout. However, the cleanup function returned from this branch only sets cancelled = true and does not clear the retry timer. The main async path's cleanup correctly clears the timer with clearTimeout(retryTimerRef.current). This inconsistency means a dep-change or unmount during the lease-contention wait leaves an orphaned 30-second timer that fires setRetryTick after the effect instance is stale.
Reviewed by Cursor Bugbot for commit 937ed9c. Configure here.
1 participant
Summary
Brings local Inspector (CLI /
--ui) onto the same Convex-backed authorization path hosted uses, so server configs (STDIO + env included) and OAuth credentials live in Convex instead of being trusted from the client / localStorage.Server (
/api/mcp/*)server/utils/local-server-resolver.ts— calls Convex/web/authorize-batch-localwith the user's bearer, maps the response to anMCPServerConfig(with OAuth header injection), threads the server-side log context./api/mcp/connectand/api/mcp/servers/reconnectaccept the new{projectId, serverId}shape and resolve config server-side. Legacy{serverConfig, serverId}body is retained as a fallback during the client migration.Client
HOSTED_MODEshort-circuits inshouldRetryHostedAuth401,hasHostedGuestAccess,getHostedAuthorizationHeader,authFetch's 401 retry, anduse-hosted-api-context. Bearer / 401 retry / form-save are now mode-agnostic.tryResolveProjectServer— opt into the new request shape when the API context has bothprojectIdand a ConvexserverId, fall back to legacy otherwise.awaitthe sync; OAuth in either mode now requires a ConvexserverId.testConnection/reconnectServeraccept optional{projectId, serverName}and emit the new body shape when present.envinserializeServersForSharing.Migration
mcp-inspector-projects/mcp-inspector-workspaces/mcp-inspector-state, pushes each project + servers to Convex viaprojects:createProject, clears legacy keys + per-server OAuth keys. OAuth tokens are not migrated — users re-auth on first connect post-migration. Gated bymcp-inspector-migrated-to-convexflag; skipped inHOSTED_MODE.Pairs with mcpjam-backend PR #215 which adds
/web/authorize-batch-local. Do not merge before that PR is deployed.Test plan
bun testpasses (mcpjam-inspector/server/routes/mcp/__tests__/{connect,servers}.test.tsandmcpjam-inspector/client/src/lib/__tests__/local-state-migration.test.tsupdated)bun run typecheckbun run dev+--ui):envvars, connect — env vars reach the spawned process/web/guest-sessionflow)HOSTED_MODE):/web/authorize-batch-localmcp-inspector-projectspopulated — projects + servers appear in Convex, legacy keys cleared, OAuth keys cleared, OAuth servers prompt for re-auth on first connect🤖 Generated with Claude Code
Note
High Risk
Changes core auth/token-handling and the
/api/mcp/*connection flow to resolve server configs via Convex, plus adds a one-time localStorage→Convex migration; mistakes could break connectivity or leak/withhold credentials. Also tightens when/where the Convex bearer is attached, so regressions could cause unexpected 401/403s or failed OAuth flows.Overview
Unifies local and hosted Inspector connection/auth flows around Convex-resolved server config. Local
/api/mcp/connectand/api/mcp/servers/reconnectnow accept a new{projectId, serverId, serverName}request shape and resolve full server config (including STDIO and OAuth bearer injection) server-side via a newlocal-server-resolver, while keeping the legacy{serverConfig, serverId}body as a transitional fallback.Makes bearer/session handling mode-agnostic and safer.
getHostedAuthorizationHeader/401-retry logic is no longer gated byHOSTED_MODE,authFetchonly attaches the Convex bearer to an allowlisted set of paths and trusted origins (incl.*.convex.site), and 401 retries are skipped when responses are flaggedX-MCP-Auth-Required: oauth.Adds a one-time migration from legacy localStorage state to Convex. A new
useLocalStateMigrationhook runs on app boot (non-hosted only) to push legacy projects/servers into Convex, with cross-tab lease + bounded retries, progress tracking for partial failures, and updated serialization that preserves secrets for persistence while still redacting them for sharing.Reviewed by Cursor Bugbot for commit 937ed9c. Bugbot is set up for automated code reviews on this repo. Configure here.