[codex] Support stored preregistered OAuth secrets#1993
Conversation
chelojimenez
temporarily deployed
to
preview-pr-1993
GitHub Actions
Inactive
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
dosubot
Bot
added
size:XL
![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:
WalkthroughThis pull request adds OAuth client secret storage and management across the client and SDK. It introduces UI controls for revealing, clearing, and undoing client secret modifications; adds an API endpoint to retrieve secrets in hosted environments; implements secret redaction in error reporting; and enhances authorization plan resolution to handle preregistered credentials using metadata-driven authentication method selection. The changes support both explicit credential entry and preregistered configurations where secrets may be stored separately from the client code. 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 (1)
mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts (1)
561-597:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent conflicting secret intent in submitted preregistered payload.
If a replacement
clientSecretis entered,clearClientSecretshould be suppressed andhasClientSecretshould remain true for that submission.💡 Suggested fix
const normalizedClientSecret = clientSecret.trim(); - const nextHasClientSecret = - shouldUsePreregisteredCredentials && - !clearClientSecret && - (hasStoredClientSecret || normalizedClientSecret.length > 0); + const hasReplacementClientSecret = normalizedClientSecret.length > 0; + const nextHasClientSecret = + shouldUsePreregisteredCredentials && + (hasReplacementClientSecret || + (hasStoredClientSecret && !clearClientSecret)); + const nextClearClientSecret = + shouldUsePreregisteredCredentials && + clearClientSecret && + !hasReplacementClientSecret; @@ hasClientSecret: shouldUsePreregisteredCredentials ? nextHasClientSecret : undefined, clearClientSecret: shouldUsePreregisteredCredentials - ? clearClientSecret + ? nextClearClientSecret : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts` around lines 561 - 597, The submitted payload can claim both a new clientSecret and request clearing it; update the return object for preregistered credentials so that if normalizedClientSecret has content we force clearClientSecret to false and keep hasClientSecret true. Concretely, in the object returned by use-server-form.ts (the block that computes clientId, clientSecret, hasClientSecret, clearClientSecret), change the clearClientSecret assignment to something like: clearClientSecret: shouldUsePreregisteredCredentials ? (normalizedClientSecret.length > 0 ? false : clearClientSecret) : undefined, and keep hasClientSecret using nextHasClientSecret so it remains true when a replacement secret is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/client/src/App.tsx`:
- Around line 2187-2192: The current call to
navigator.clipboard?.writeText(details) may return undefined and cause a
TypeError when chaining .then(); update the copy logic to first test that
navigator.clipboard and navigator.clipboard.writeText are available (or wrap the
result with Promise.resolve) before calling .then/.catch, and if unavailable
immediately call toast.error("Could not copy OAuth debugger error"); locate the
snippet using navigator.clipboard?.writeText(details) and the toast.success /
toast.error calls to implement the guard and safe async handling.
In
`@mcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsx`:
- Around line 145-147: The dynamic headersWarning paragraph in
AdvancedConnectionSettingsSection.tsx should expose alert semantics so screen
readers announce it; update the element that renders {headersWarning} (the <p>
currently using className "text-xs text-amber-700") to include accessible
attributes such as role="alert" or aria-live="assertive" (and optionally
aria-atomic="true") so the warning is announced when it appears.
In
`@mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx`:
- Around line 105-107: The props object currently sets hasClientSecret to
hasStoredClientSecret whenever showClientCredentials is true, but it must also
respect clearClientSecret; update the logic that builds hasClientSecret in
AuthenticationSection (the place where hasClientSecret, showClientCredentials
and hasStoredClientSecret are used) so that if clearClientSecret is true the
planner does NOT receive a true value (i.e., set hasClientSecret to false or
undefined when clearClientSecret is set), for example by gating
hasStoredClientSecret with !clearClientSecret before assigning it.
In `@mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts`:
- Around line 2071-2081: The JSON.parse of existingClientInfo (used to build
existingJsonRaw/existingJson inside initiateOAuth) can throw on malformed
mcp-client-* payloads; wrap the parse in a try/catch and fall back to an empty
object {} on error, then continue the HOSTED_MODE filtering logic as before
(i.e., compute existingJson from existingJsonRaw after safe parse). Ensure you
reference existingClientInfo, existingJsonRaw, existingJson, and initiateOAuth
when making the change.
In `@mcpjam-inspector/client/src/lib/workspace-serialization.ts`:
- Around line 246-259: The serversHaveChanged comparison omits a top-level check
for hasClientSecret so toggling that flag alone (localServer.hasClientSecret vs
remoteServer.hasClientSecret) can be ignored; update the comparison in
serversHaveChanged to explicitly include hasClientSecret alongside the existing
OAuth-derived fields (ensure you compare localServer.hasClientSecret !==
remoteServer.hasClientSecret) so that changes to hasClientSecret force the state
refresh and preserve correct UI secret state (refer to the serversHaveChanged
function and the localServer/remoteServer.hasClientSecret properties).
In `@sdk/src/oauth/state-machines/shared/client-auth.ts`:
- Around line 22-27: The normalization currently returns an empty array if a
non-empty but invalid `raw` array contains no string entries; update the logic
in the function in client-auth.ts so after computing `const result =
raw.filter((method): method is string => typeof method === "string")` you return
`["client_secret_basic"]` when `result.length === 0`, otherwise return `result`,
preserving the existing type guard and behavior when `raw` is absent or empty.
---
Outside diff comments:
In `@mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts`:
- Around line 561-597: The submitted payload can claim both a new clientSecret
and request clearing it; update the return object for preregistered credentials
so that if normalizedClientSecret has content we force clearClientSecret to
false and keep hasClientSecret true. Concretely, in the object returned by
use-server-form.ts (the block that computes clientId, clientSecret,
hasClientSecret, clearClientSecret), change the clearClientSecret assignment to
something like: clearClientSecret: shouldUsePreregisteredCredentials ?
(normalizedClientSecret.length > 0 ? false : clearClientSecret) : undefined, and
keep hasClientSecret using nextHasClientSecret so it remains true when a
replacement secret is provided.
🪄 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: 8d6d43ef-bb43-403a-915a-d3b033fc7178
📒 Files selected for processing (26)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/__tests__/App.hosted-oauth.test.tsxmcpjam-inspector/client/src/components/connection/AddServerModal.tsxmcpjam-inspector/client/src/components/connection/EditServerFormContent.tsxmcpjam-inspector/client/src/components/connection/__tests__/AuthenticationSection.test.tsxmcpjam-inspector/client/src/components/connection/hooks/__tests__/use-server-form.test.tsmcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsxmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/components/evals/ErrorBoundary.tsxmcpjam-inspector/client/src/hooks/use-server-state.tsmcpjam-inspector/client/src/hooks/useWorkspaces.tsmcpjam-inspector/client/src/lib/oauth/__tests__/mcp-oauth.test.tsmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/lib/workspace-serialization.tsmcpjam-inspector/client/src/state/app-types.tsmcpjam-inspector/shared/types.tssdk/src/oauth/authorization-plan.tssdk/src/oauth/state-machines/debug-oauth-2025-03-26.tssdk/src/oauth/state-machines/debug-oauth-2025-06-18.tssdk/src/oauth/state-machines/debug-oauth-2025-11-25.tssdk/src/oauth/state-machines/shared/client-auth.tssdk/src/oauth/state-machines/types.tssdk/tests/oauth-conformance/runner.test.tssdk/tests/oauth/authorization-plan.test.tssdk/tests/oauth/state-machine-regressions.test.ts
| void navigator.clipboard | ||
| ?.writeText(details) | ||
| .then(() => toast.success("Copied OAuth debugger error")) | ||
| .catch(() => | ||
| toast.error("Could not copy OAuth debugger error"), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its basic info
git ls-files | grep -E "mcpjam-inspector/client.*App\.tsx"Repository: MCPJam/inspector
Length of output: 96
🏁 Script executed:
# Get the specific lines mentioned in the review (2187-2192)
if [ -f "mcpjam-inspector/client/src/App.tsx" ]; then
echo "=== Lines 2187-2192 ==="
sed -n '2185,2195p' "mcpjam-inspector/client/src/App.tsx"
fiRepository: MCPJam/inspector
Length of output: 539
🏁 Script executed:
# Get broader context around copyDetails function
if [ -f "mcpjam-inspector/client/src/App.tsx" ]; then
echo "=== Searching for copyDetails function ==="
grep -n "copyDetails" "mcpjam-inspector/client/src/App.tsx" | head -20
fiRepository: MCPJam/inspector
Length of output: 230
Prevent unhandled errors when Clipboard API is unavailable.
navigator.clipboard?.writeText() returns undefined when the API is unavailable, and chaining .then() on undefined throws a TypeError, breaking the copy action in restricted environments.
🛠️ Safe async handling
- const copyDetails = () => {
+ const copyDetails = async () => {
const details = formatOAuthDebuggerErrorDetails(error);
- void navigator.clipboard
- ?.writeText(details)
- .then(() => toast.success("Copied OAuth debugger error"))
- .catch(() =>
- toast.error("Could not copy OAuth debugger error"),
- );
+ try {
+ if (!navigator.clipboard?.writeText) {
+ throw new Error("Clipboard API unavailable");
+ }
+ await navigator.clipboard.writeText(details);
+ toast.success("Copied OAuth debugger error");
+ } catch {
+ toast.error("Could not copy OAuth debugger error");
+ }
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/App.tsx` around lines 2187 - 2192, The current
call to navigator.clipboard?.writeText(details) may return undefined and cause a
TypeError when chaining .then(); update the copy logic to first test that
navigator.clipboard and navigator.clipboard.writeText are available (or wrap the
result with Promise.resolve) before calling .then/.catch, and if unavailable
immediately call toast.error("Could not copy OAuth debugger error"); locate the
snippet using navigator.clipboard?.writeText(details) and the toast.success /
toast.error calls to implement the guard and safe async handling.
![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: fc4cd1d9a8
ℹ️ 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 (options.clientId || (!HOSTED_MODE && options.clientSecret)) { | ||
| const existingClientInfo = localStorage.getItem( |
There was a problem hiding this comment.
The new HOSTED_MODE guard prevents saving client_secret before redirect, but handleOAuthCallback rebuilds the OAuth client from mcp-client-* after reload and reads client_secret from that storage. In hosted unauthenticated flows (e.g., chatbox/shared paths that still use handleOAuthCallback instead of workspace callback completion), confidential pre-registered OAuth will now fail at token exchange because the callback no longer has the secret. This is a functional regression for any hosted non-workspace flow that uses a confidential client.
Useful? React with 👍 / 👎.
chelojimenez
temporarily deployed
to
preview-pr-1993
GitHub Actions
Inactive
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
mcpjam-inspector/client/src/lib/workspace-serialization.ts (2)
9-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
hasClientSecretin the serialized payload.This serializer drops the new flag entirely, so a workspace round-trip loses the masked/replace/clear state even though the secret itself is still hidden.
💡 Suggested fix
const serializedServer: Record<string, unknown> = { name: server.name, enabled: server.enabled, useOAuth: server.useOAuth, + hasClientSecret: server.hasClientSecret === true, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/workspace-serialization.ts` around lines 9 - 13, The serializer currently omits the hasClientSecret flag causing loss of masked/replace/clear state; update the serializedServer object in workspace-serialization.ts to include hasClientSecret: server.hasClientSecret so the serialized payload preserves the secret state for the workspace round-trip (refer to the serializedServer constant to locate where to add this property).
251-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep flat OAuth synthesis from discarding nested fields.
When this branch is entered because only
hasClientSecretis present,scopesandclientIdare overwritten withundefinedinstead of falling back to the nested profile. That can make an unchanged server look dirty on every comparison.💡 Suggested fix
const remoteOAuthProfile = remoteServer.oauthScopes || remoteServer.clientId || remoteServer.hasClientSecret || remoteServer.oauthResourceUrl ? { ...(remoteServer.oauthFlowProfile ?? {}), scopes: Array.isArray(remoteServer.oauthScopes) ? remoteServer.oauthScopes.join(",") - : remoteServer.oauthScopes, - clientId: remoteServer.clientId, + : remoteServer.oauthScopes ?? + remoteServer.oauthFlowProfile?.scopes, + clientId: + remoteServer.clientId ?? + remoteServer.oauthFlowProfile?.clientId, clientSecret: remoteServer.hasClientSecret === true ? "" : remoteServer.oauthFlowProfile?.clientSecret,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/workspace-serialization.ts` around lines 251 - 270, The flat OAuth synthesis for remoteOAuthProfile overwrites scopes and clientId with undefined when only hasClientSecret is present; update the construction of remoteOAuthProfile so each top-level field (scopes, clientId, clientSecret) explicitly falls back to the nested oauthFlowProfile values if the top-level value is undefined (e.g., compute scopes by preferring remoteServer.oauthScopes (joined if array) and if that is null/undefined use remoteServer.oauthFlowProfile?.scopes; compute clientId by using remoteServer.clientId ?? remoteServer.oauthFlowProfile?.clientId; compute clientSecret by keeping the hasClientSecret logic but otherwise use remoteServer.oauthFlowProfile?.clientSecret when remoteServer.oauthFlowProfile exists). Ensure you only change the object keys in the remoteOAuthProfile expression and preserve existing resourceUrl fallback logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@mcpjam-inspector/client/src/lib/workspace-serialization.ts`:
- Around line 9-13: The serializer currently omits the hasClientSecret flag
causing loss of masked/replace/clear state; update the serializedServer object
in workspace-serialization.ts to include hasClientSecret: server.hasClientSecret
so the serialized payload preserves the secret state for the workspace
round-trip (refer to the serializedServer constant to locate where to add this
property).
- Around line 251-270: The flat OAuth synthesis for remoteOAuthProfile
overwrites scopes and clientId with undefined when only hasClientSecret is
present; update the construction of remoteOAuthProfile so each top-level field
(scopes, clientId, clientSecret) explicitly falls back to the nested
oauthFlowProfile values if the top-level value is undefined (e.g., compute
scopes by preferring remoteServer.oauthScopes (joined if array) and if that is
null/undefined use remoteServer.oauthFlowProfile?.scopes; compute clientId by
using remoteServer.clientId ?? remoteServer.oauthFlowProfile?.clientId; compute
clientSecret by keeping the hasClientSecret logic but otherwise use
remoteServer.oauthFlowProfile?.clientSecret when remoteServer.oauthFlowProfile
exists). Ensure you only change the object keys in the remoteOAuthProfile
expression and preserve existing resourceUrl fallback logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 871314fb-7131-45b1-a750-b21c80544ddd
📒 Files selected for processing (8)
mcpjam-inspector/client/src/components/connection/hooks/use-server-form.tsmcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsxmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/lib/oauth/mcp-oauth.tsmcpjam-inspector/client/src/lib/workspace-serialization.tssdk/src/oauth/state-machines/shared/client-auth.tssdk/tests/oauth/authorization-plan.test.tssdk/tests/oauth/state-machine-regressions.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- mcpjam-inspector/client/src/components/connection/shared/AdvancedConnectionSettingsSection.tsx
- mcpjam-inspector/client/src/components/connection/hooks/use-server-form.ts
- mcpjam-inspector/client/src/lib/oauth/mcp-oauth.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx (1)
163-172: 💤 Low valueConsider cleanup for the copy-feedback timeout.
The
setTimeoutat line 168 may attempt a state update after unmount. While React 18+ handles this gracefully, a cleanup pattern prevents the edge case entirely.🧹 Optional: useEffect-based timeout cleanup
+ const [copyTimeoutId, setCopyTimeoutId] = useState<ReturnType<typeof setTimeout> | null>(null); + + useEffect(() => { + return () => { + if (copyTimeoutId) clearTimeout(copyTimeoutId); + }; + }, [copyTimeoutId]); const handleCopyRevealedSecret = async () => { if (!revealedClientSecret) return; try { await navigator.clipboard.writeText(revealedClientSecret); setDidCopyRevealedSecret(true); - setTimeout(() => setDidCopyRevealedSecret(false), 2000); + const id = setTimeout(() => setDidCopyRevealedSecret(false), 2000); + setCopyTimeoutId(id); } catch { // Clipboard failures are non-fatal } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx` around lines 163 - 172, The copy-feedback timeout in handleCopyRevealedSecret can call setDidCopyRevealedSecret after the component unmounts; store the timeout id (e.g., via a ref like copyTimeoutRef) when you call setTimeout and clear it in a useEffect cleanup (or clearTimeout in a cleanup function) to prevent state updates after unmount; update references to setDidCopyRevealedSecret and the timeout management in AuthenticationSection so the timer is cleared on unmount and any previous timer is cleared before setting a new one.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@mcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsx`:
- Around line 163-172: The copy-feedback timeout in handleCopyRevealedSecret can
call setDidCopyRevealedSecret after the component unmounts; store the timeout id
(e.g., via a ref like copyTimeoutRef) when you call setTimeout and clear it in a
useEffect cleanup (or clearTimeout in a cleanup function) to prevent state
updates after unmount; update references to setDidCopyRevealedSecret and the
timeout management in AuthenticationSection so the timer is cleared on unmount
and any previous timer is cleared before setting a new one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4590dc35-cf8c-46a3-8ee9-57f29afe80ac
📒 Files selected for processing (6)
mcpjam-inspector/client/src/components/connection/EditServerFormContent.tsxmcpjam-inspector/client/src/components/connection/ServerDetailModal.tsxmcpjam-inspector/client/src/components/connection/__tests__/AuthenticationSection.test.tsxmcpjam-inspector/client/src/components/connection/shared/AuthenticationSection.tsxmcpjam-inspector/client/src/lib/apis/hosted-oauth-client-secret-api.tsmcpjam-inspector/server/routes/web/oauth.ts
✅ Files skipped from review due to trivial changes (1)
- mcpjam-inspector/client/src/components/connection/tests/AuthenticationSection.test.tsx
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a2a195d. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2a195d1bb
ℹ️ 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 ( | ||
| basePlan.status !== "discovery_required" && | ||
| basePlan.registrationStrategy !== "preregistered" | ||
| ) { |
There was a problem hiding this comment.
This guard now forces loadCallbackDiscoveryState(...) for every pre-registered plan, including plans that are already blocked (for example, missing client_id). In those cases we should return the blocker immediately; otherwise a discovery/network failure can mask the real validation error and turn a local form-validation case into a transport failure. This regression was introduced by adding the registrationStrategy !== "preregistered" condition to the early return.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47fcf7b89a
ℹ️ 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".
| // but only surface credential fields from saved client configuration. | ||
| clientIdValue = storedTokens?.client_id || savedClientId; | ||
| clientSecretValue = savedClientSecret; | ||
| clientSecretValue = hasStoredClientSecretValue ? "" : savedClientSecret; |
There was a problem hiding this comment.
When an existing OAuth server has hasClientSecret=true, the form now force-initializes clientSecret to an empty string instead of the saved value. In non-hosted/local flows this means buildFormData() later emits clientSecret: undefined, and saving any unrelated edit overwrites mcp-client-* without the secret, silently deleting credentials needed for refresh/reconnect/token exchange. This is a regression introduced by the new masking logic because local mode has no backend reveal path to recover the secret.
Useful? React with 👍 / 👎.
…auth-secrets-main
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/components/ui/error-boundary.tsx (1)
7-9: ⚡ Quick win
error: Error | nullin the callback overstates the null surface.Within the
hasError: truebranch,this.state.erroris invariably a realError—getDerivedStateFromErroralways supplies one andhandleResetreturns the boundary tohasError: falsebeforeerroris nulled. Consumers of the function fallback are forced to null-guard against a case that never materialises at the call site.✨ Tighter callback signature
fallback?: | React.ReactNode - | ((input: { error: Error | null; reset: () => void }) => React.ReactNode); + | ((input: { error: Error; reset: () => void }) => React.ReactNode);And in
render, assert the invariant once:if (typeof this.props.fallback === "function") { return this.props.fallback({ - error: this.state.error, + error: this.state.error!, reset: this.handleReset, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/components/ui/error-boundary.tsx` around lines 7 - 9, The fallback callback type currently accepts error: Error | null which is too wide; change the type to receive a non-null Error (i.e., (input: { error: Error; reset: () => void }) => React.ReactNode) and update the fallback union accordingly; in the ErrorBoundary class (methods getDerivedStateFromError, handleReset, render) assert the invariant in render before invoking fallback by using the existing hasError flag and passing this.state.error as a non-null Error (you can add a short runtime check or TypeScript non-null assertion where fallback is called) so consumers no longer need to guard for null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcpjam-inspector/client/src/components/ui/error-boundary.tsx`:
- Around line 7-9: The fallback callback type currently accepts error: Error |
null which is too wide; change the type to receive a non-null Error (i.e.,
(input: { error: Error; reset: () => void }) => React.ReactNode) and update the
fallback union accordingly; in the ErrorBoundary class (methods
getDerivedStateFromError, handleReset, render) assert the invariant in render
before invoking fallback by using the existing hasError flag and passing
this.state.error as a non-null Error (you can add a short runtime check or
TypeScript non-null assertion where fallback is called) so consumers no longer
need to guard for null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 793af287-acde-4b74-a32b-9f07a4576a0e
📒 Files selected for processing (2)
mcpjam-inspector/client/src/App.tsxmcpjam-inspector/client/src/components/ui/error-boundary.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/App.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93f64beb72
ℹ️ 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".
| void navigator.clipboard | ||
| ?.writeText(details) | ||
| .then(() => toast.success("Copied OAuth debugger error")) | ||
| .catch(() => |
There was a problem hiding this comment.
When navigator.clipboard is unavailable (e.g., insecure context or restricted browser), navigator.clipboard?.writeText(details) evaluates to undefined, and the following .then(...) access throws a TypeError. This makes the “Copy details” action crash instead of gracefully reporting that copy is unavailable.
Useful? React with 👍 / 👎.
| if (options.clientId || (!HOSTED_MODE && options.clientSecret)) { | ||
| const existingClientInfo = localStorage.getItem( |
There was a problem hiding this comment.
This hosted-mode guard skips persisting client_secret entirely, but downstream reauth/refresh code rebuilds OAuth client credentials from mcp-client-* local storage (for example in refreshOAuthTokens and legacy callback restoration). For confidential preregistered servers in hosted mode, token refresh/reauth will lose client authentication and fail with invalid_client once a secret is required.
Useful? React with 👍 / 👎.
chelojimenez
had a problem deploying
to
preview-pr-1993
GitHub Actions
Error
1 participant
Summary
Validation
Note
High Risk
Changes OAuth credential handling and storage semantics (including client secret reveal/clear flows and new backend endpoints), which is security-sensitive and easy to regress across hosted/local modes. Also alters OAuth plan/state-machine validation logic, so misconfigurations could block sign-ins or choose the wrong token auth method.
Overview
Adds stored preregistered OAuth client secret support across the Inspector: forms now track
hasClientSecret/clearClientSecret, avoid overwriting secrets on unrelated edits, warn when customAuthorizationheaders may conflict, and in hosted mode can reveal a Vault-stored secret via a new/api/web/oauth/client-secretfetch.Updates save/sync paths to send secret operations through dedicated Convex actions, stops persisting hosted-mode client secrets to
localStorage, and propagateshasClientSecretthrough shared types/serialization.Hardens OAuth Debugger reliability by adding a function-style
ErrorBoundaryfallback that sanitizes tokens/secrets before PostHog capture and clipboard copy, and updates SDK OAuth planning/state machines to account forhasClientSecret, select token endpoint auth method via new shared logic, and add conformance/regression tests for public preregistered clients and unsupported auth methods.Reviewed by Cursor Bugbot for commit 5bf9e09. Bugbot is set up for automated code reviews on this repo. Configure here.