[charlesrhoward]

Browser sign-in only worked for connection presets. prepareOAuthConnection ran discovery and dynamic client registration only when the connection came from a preset, and the add-connection form offered None, Bearer, API key, or Basic. A custom MCP server that expects OAuth, which is most hosted MCP servers, had to be connected with a pasted access token. The public docs already said custom connections support OAuth.

A custom MCP connection can now choose "OAuth (browser sign-in)". It follows the MCP authorization spec through a generic profile in lib/connections/oauth-profile.ts: RFC 9728 protected-resource discovery, dynamic client registration as a public client, PKCE on every request, and the MCP server as the token's resource. The existing authorize route, callback, token refresh, and CLI sync are reused unchanged. Once authorized, the connection reaches the CLI through the same sync as preset OAuth connections.

• Validation: for MCP OAuth, client id, client secret, and endpoint URLs become optional. Left empty, they are discovered on first sign-in. Supplied, they are still validated against the outbound URL rules. REST OAuth still requires its client settings.
• isConnectionMisconfigured treats a custom MCP OAuth row like a dynamic-registration preset before its first sign-in.
• Both add forms (settings and the connections pane) start sign-in right after the connection is created.

Behavior change: a custom MCP connection that was already configured by hand with OAuth client settings now also sends PKCE and the resource parameter, both required by the MCP authorization spec.

A CLI-native sign-in (mogplex mcp login) is a separate follow-up in the CLI repository.

Tests: unit tests cover discovery and registration for a custom server, PKCE, the resource indicator, and exclusion of REST and non-OAuth rows. Validation tests cover URL-only OAuth, rejected supplied endpoints, and unchanged REST requirements. A new e2e spec covers the form starting sign-in, and the existing manual-add spec now expects OAuth to be offered only for MCP servers. The custom-profile tests fail with the profile removed.

---

^{Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.}

[mogplex]

Mogplex PR Review

Status: Attention needed

Solid, well-scoped feature: the oauth-profile extraction, optional validation for custom MCP OAuth rows, and the misconfiguration rule all match the description, and no security or data-integrity hole turned up. Two behavior problems in the new path should be fixed before merge: a stale credential field is submitted as the OAuth client_secret, and the new resource indicator is also sent on refresh for hand-configured custom MCP connections that predate this change.

Warnings

• Stale credential is submitted as the OAuth client secret for MCP browser sign-in (components/connections/add-connection-form.tsx)
  The credential input is hidden once newConn.auth_type === "oauth", but the value stays in form state and addConnection() posts JSON.stringify(newConn) wholesale, so a credential typed under Bearer/API key/Basic is still submitted. normalizeOauthCredentials then wraps it as {"client_secret": "<token>"} (lib/connections/validation.ts), it is stored as the connection's OAuth credentials, and exchangeCodeForTokens sends it as client_secret to the authorization server discovered from the MCP server.

  Two consequences: (1) the sign-in fails with invalid_client for the client we registered with token_endpoint_auth_method: "none", and because the credential persists the row stays broken until it is recreated; (2) a token the user pasted for a different auth mode is transmitted to a third-party authorization server they did not intend to send it to.

  Suggested fix: clear the field when switching modes, e.g. in the MCP auth onChange use credentials: e.target.value === "oauth" ? "" : p.credentials, or drop credentials from the payload when type === "mcp_server" && auth_type === "oauth".

• The resource indicator is now sent on refresh for hand-configured custom MCP OAuth rows (lib/connections/oauth.ts)
  getOAuthResourceIndicator returns mcp_url for every mcp_server row with auth_type: "oauth" and the custom profile sets use_resource_indicator: true, so the value is appended to the authorize URL, the code exchange, and refreshOAuthToken. The connections pane already offered OAuth for MCP before this PR, so rows authorized against manually registered clients exist today; their authorization servers may reject parameters they do not know (invalid_request / unsupported_parameter). That failure would surface only when the stored access token expires, long after this deploy, and it is not covered by the behavior-change note in the description (which mentions sign-in only).

  Was refresh verified against such a server? If not, consider sending resource only for rows whose client we registered (for example when oauth_client_id was written during discovery), or retrying the refresh once without resource when the token endpoint answers 400 invalid_request.

• New unit test depends on live DNS resolution (tests/unit/connection-oauth-custom.test.ts)
  The spec only stubs global.fetch, but prepareOAuthConnection calls discoverOAuthMetadata("https://mcp.sentry.dev/mcp"), which validates the protected-resource and metadata URLs with assertSafeOutboundHttpUrlWithDns; that helper performs a real dns.lookup (lib/security/outbound-url.ts) before any stubbed fetch runs. On a runner without DNS egress, or when the name resolves to a private address, the test fails with "must resolve to a public host" rather than exercising discovery. Stubbing dns.promises.lookup (already injectable as lookupHostname on the validator) or mocking @/lib/security/outbound-url in this spec would keep it hermetic.

Suggestions

• Registration replaces endpoints a custom connection supplied explicitly (lib/connections/oauth.ts)
  When a custom MCP row has oauth_authorize_url and oauth_token_url filled in but no oauth_client_id, the registration update writes the discovered metadata.authorization_endpoint / metadata.token_endpoint over both the row and the in-memory connection. For presets that is clearly intended; for a hand-configured custom row it silently discards what the user entered. Is that deliberate (the discovered authorization server is authoritative)? If so a one-line comment near the update would save the next reader the same question; otherwise oauth_authorize_url: nextConnection.oauth_authorize_url ?? metadata.authorization_endpoint would preserve explicit configuration.

• Untested branch: supplied client id skips discovery and registration (tests/unit/connection-oauth-custom.test.ts)
  The new tests cover the fully-discovered path only. Two branches of prepareOAuthConnection have no coverage: a custom MCP row that already has oauth_client_id (no fetch at all, PKCE still returned) and one that supplies both endpoints but no client id (discovery plus the endpoint rewrite above). The e2e addition covers the settings form; the connections-pane inline form has its own OAuth branch and its own redirect and is not exercised.

View check run

[mogplex]

Mogplex PR Review

Status: Attention needed

Well-scoped feature with good coverage of discovery, registration, PKCE and the resource indicator, but two correctness gaps should be settled first: custom MCP refreshes drop the resource indicator that authorize/token requests set, and a client secret saved without a client id survives dynamic registration and is later sent with a public client.

Warnings

• Custom MCP refresh drops the resource indicator (lib/connections/oauth.ts)
  getOAuthResourceIndicator(connection, "refresh") returns nothing for a row with no preset, so a custom MCP server that was authorized with resource=<mcp_url> refreshes without it. Everything else about these rows assumes resource indicators work: the authorize request and exchangeCodeForTokens both send it, and the MCP authorization spec requires the parameter on token requests. A preset row with the same registration: "dynamic" / use_resource_indicator: true profile (Sentry) does send it on refresh, so two rows registered through the exact same code path now behave differently. On authorization servers that bind the access token to the requested resource (AuthKit/WorkOS-style servers, Keycloak resource indicators, Auth0 resource servers) the refreshed token can come back without the MCP audience or the request can fail with invalid_target, which surfaces as a 401 from the MCP server and a re-auth roughly every token lifetime. The comment's rationale is real for hand-configured legacy rows on servers that reject resource (Azure AD v2), but it does not describe rows whose client was just registered by this new flow.

  Suggestion: persist a marker when prepareOAuthConnection fills in endpoints/client id from discovery + registration, include resource on refresh for those rows, and keep omitting it only for hand-configured rows. Note that should refresh a custom MCP token without the resource indicator currently locks the omission in.

• A client secret saved without a client id survives dynamic registration (lib/connections/validation.ts)
  The MCP OAuth branch accepts credentials while oauth_client_id is optional, and the registration block in prepareOAuthConnection only rewrites credentials when the registration response includes a client_secret:

  const credentials = registration.client_secret
    ? JSON.stringify({ client_secret: registration.client_secret })
    : undefined;
  ...
  ...(credentials ? { credentials } : {}),

  So a row created with a client secret but no client id (both fields are optional in the inline form) registers a fresh public client, keeps the user's secret, and then sends it as client_secret in exchangeCodeForTokens and refreshOAuthToken against a client that was registered with token_endpoint_auth_method: "none". Depending on the authorization server that is invalid_client, or a token for a client the user did not configure.

  Suggestion: reject a client secret without a client id in normalizeConnectionCreateInput, or clear credentials whenever registration issues a new client_id.

Suggestions

• Prefer the discovered resource over the raw mcp_url (lib/connections/oauth-discovery.ts)
  discoverProtectedResourceMetadata already has the server's resource value, but discoverOAuthMetadata returns only the endpoints, so getOAuthResourceIndicator falls back to connection.mcp_url verbatim with no RFC 8707 canonicalization (scheme/host case, trailing slash, fragment). An authorization server that compares the requested resource against its own protected-resource identifier can reject that with invalid_target when a server publishes resource: https://host while its MCP endpoint is https://host/mcp.

  Worth returning resource (and scopes_supported) from discovery, persisting the resource on the row, and using it for both the authorize and token requests. I am not certain this bites the servers you tested, but it is the canonical value per RFC 9728.

• Sign-in silently skipped when the create response has no id (components/connections/add-connection-form.tsx)
  In addConnection, await res.json().catch(() => ({})) swallows a parse failure, and when connection?.id is missing the code falls through to setNewConn(INITIAL_CONNECTION_FORM) and mutateConnections(). The user gets a created oauth row with no sign-in started and no message explaining it. The table's "Connect OAuth" action makes that recoverable, but only if they notice the new row.

  Suggestion: call setConnectionError when the id is missing instead of falling through.

• Settings-form sign-in cannot use a pre-registered client (components/connections/connection-form.ts)
  MCP_CONNECTION_AUTH_OPTIONS adds OAuth to the settings add form, but that form has no client id, client secret or endpoint inputs (the connections-pane form does), and normalizeConnectionSettingsPatch only accepts is_enabled / approval_mode. A custom MCP server that requires a pre-registered client therefore fails at oauth=setup_error for anyone who added it from Settings, and the only fix is deleting the row and re-adding it from the other form.

  Either expose the optional OAuth fields in the settings form, or point users at the connections pane: the current setup-error copy only talks about preset configuration.

View check run

[charlesrhoward]

f56170c addresses the three warnings from the review.

The stale credential is no longer sent. The settings form leaves credentials out of the request when the auth type is OAuth. In the workspace pane, the OAuth client secret now has its own field state, so a token typed under Bearer, API key, or Basic can't turn into the client secret. A secret the user deliberately types in the OAuth field is still sent, because REST OAuth connections require one. connections-custom-mcp-oauth.spec.ts now types a token, switches to OAuth, and checks that the created payload has no credential, in both the settings form and the workspace pane. Both tests failed before the fix.

Refresh no longer sends resource for custom MCP rows. Authorize and code exchange still send it. On refresh it is sent only when a preset declares use_resource_indicator, so Sentry behaves as before. The new unit test in connection-oauth-custom.test.ts failed before the change.

The custom OAuth unit tests point at a public IP literal now. That skips the outbound DNS lookup, so they no longer depend on name resolution. Stubbing node:dns would mean widening the owner-gated mock allowlist, so I avoided it.

On the two suggestions:

Registration replacing hand-entered endpoints is deliberate. When a row has no client id, Mogplex registers a client with the authorization server it discovers from the MCP server. That client id is only valid at that server. If we kept user-entered endpoints that point somewhere else, the next sign-in would fail with an unknown client. If a row already has a client id, discovery and registration are skipped and its endpoints are kept as entered.

On coverage: the workspace pane's OAuth branch and its redirect are now covered by the e2e spec above. The remaining unit gaps are the custom row that already has a client id and the row with endpoints but no client id. Both run the same code paths as the preset tests in connection-oauth.test.ts, which already cover a registered client and a missing-endpoint reconnect, so I left them alone for this PR.

[mogplex]

Mogplex PR Review

Status: Attention needed

Solid, well-tested implementation of custom-MCP browser sign-in, but I'd want one design question answered before merge: the resource parameter is sent on the sign-in exchange for every custom row yet deliberately omitted on refresh for hand-configured rows, which can leave a refreshed token bound to the wrong resource. Secondary: discovered authorization endpoints are persisted and used as a browser redirect target without the outbound-URL validation that user-supplied endpoints must pass.

Warnings

• resource is sent at sign-in but dropped on refresh for hand-configured custom clients (lib/connections/oauth.ts)
  getOAuthResourceIndicator(connection) (no refresh argument) makes the authorize URL and the code exchange carry resource=<mcp_url> for every custom MCP row, including rows where the user supplied oauth_client_id/endpoints by hand. On refresh, refreshOAuthToken passes { registeredClient }, so those same rows call it without resource because they never went through dynamic registration and therefore never get the registered_client marker in the encrypted credentials.

  Why it matters: per RFC 8707 the authorization server binds the issued token to the requested resource. Refreshing without a resource can mint a token for the authorization server's default resource instead of the MCP server, which typically surfaces as 401s from the MCP server roughly an hour after sign-in — the kind of failure that is hard to trace back.

  It also makes the in-code rationale self-defeating: the comment says hand-configured clients may talk to servers that reject resource, but the sign-in exchange for exactly those rows already sends it, so such a server would fail at sign-in rather than at refresh.

  Suggestion: derive the refresh decision from the same evidence as the exchange, e.g. persist whether the exchange was resource-bound (a marker next to registered_client, or the resource itself) when exchangeCodeForTokens runs, and reuse it in refreshOAuthToken. If the current split is deliberate, a test plus a comment explaining why sign-in and refresh differ would help. Explicitly: is the asymmetry intended?

• Discovered OAuth endpoints are persisted and redirect the browser without the outbound-URL checks applied to supplied endpoints (lib/connections/oauth-discovery.ts)
  discoverOAuthMetadata returns authorization_endpoint/token_endpoint straight from the fetched authorization-server document; only the well-known URL and registration_endpoint go through assertSafeOutboundHttpUrlWithDns. prepareOAuthConnection then writes those values into oauth_authorize_url/oauth_token_url unchanged, while the same fields supplied by a user go through optionalHttpUrl -> assertSafeOutboundHttpUrl in lib/connections/validation.ts. The two paths now disagree.

  Why it matters here and not before: discovery used to run only against pinned preset mcp_url values; this PR makes it run against any user-typed MCP URL, so the metadata is effectively third-party input. oauth_token_url is re-validated with a DNS check at use, but oauth_authorize_url is only ever used as a redirect target (buildAuthorizeUrl -> NextResponse.redirect), so a server's metadata can send the user's browser to an arbitrary absolute URL — including a non-http(s) scheme — carrying state, client_id, redirect_uri, and code_challenge.

  Suggestion: validate both discovered endpoints with assertSafeOutboundHttpUrl (and require https:) inside discoverOAuthMetadata or before the updateConnection in prepareOAuthConnection, so discovered and supplied configuration follow the same rules. I'm flagging this as hardening rather than a confirmed exploit: the MCP spec does have the client trust the server's advertised authorization server.

Suggestions

• Partially supplied OAuth endpoints are silently replaced by discovery (lib/connections/validation.ts)
  normalizeConnectionCreateInput now treats oauth_client_id, oauth_authorize_url, and oauth_token_url as independently optional for MCP OAuth, but prepareOAuthConnection runs discovery when either endpoint is missing and then overwrites both from the metadata. A user who supplies a correct oauth_authorize_url and leaves the token URL empty gets their value replaced with no feedback.

  Suggestion: make the endpoint pair all-or-nothing in validation, or fill only the missing endpoint in prepareOAuthConnection. Related note: servers without dynamic client registration can only be connected from the pane form, since AddConnectionForm has no fields for client id/endpoints — from settings they fail through a generic ?oauth=setup_error redirect.

• Pane form keeps OAuth selected when switching to REST API (components/panes/connections-pane/inline-add-form.tsx)
  The REST API / MCP Server buttons only call setType, so an OAuth selection survives the switch to rest_api. Submitting then sends type: "rest_api" with auth_type: "oauth" and empty client settings, and validation rejects it with oauth_client_id is required.

  The settings form gained exactly this guard in this PR (nextType === "rest_api" && p.auth_type === "oauth" ? "none"); mirroring it in the pane form would keep the two add surfaces consistent.

• Custom MCP sign-in never requests a scope (lib/connections/oauth-profile.ts)
  CUSTOM_MCP_OAUTH_PROFILE carries no scopes, discoverOAuthMetadata drops scopes_supported from the protected-resource and authorization-server metadata, and AddConnectionForm has no scopes input — so for a custom server reached from settings the authorize request has no scope parameter at all.

  Suggestion: feed the server's advertised scopes_supported into oauth_scopes during discovery (the MCP authorization spec expects clients to use it), or at minimum surface the oauth_scopes field that the pane form already has.

• Authorize params are still read from the preset only (app/api/connections/oauth/route.ts)
  The route builds the authorize URL with authorizeParams: preset?.oauth_config?.authorize_params, while everything else about the flow now goes through getOAuthProfile. OAuthProfile includes authorize_params, so adding any to CUSTOM_MCP_OAUTH_PROFILE would silently do nothing.

  Suggestion: use getOAuthProfile(resolvedConnection)?.authorize_params so both profile sources are honored.

View check run

[charlesrhoward]

eb5e52d settles the two points from the previous review.

When Mogplex registers a custom MCP server's client, the registration write now replaces the row's credentials with the issued secret, if there is one, plus a registered_client marker. A secret saved without a client id is no longer sent for the new public client. Refresh reads the marker, so registered custom clients keep sending resource and hand-configured custom clients refresh without it. Preset rows are unchanged: they keep their stored credentials until the token write, so a legacy row with a working bearer credential isn't broken by an abandoned reconnect. Each branch has a unit test, and I broke each branch in turn to confirm its test fails.

Replies to the latest review:

The sign-in/refresh asymmetry for hand-configured rows is intended. Those rows existed before this PR, and some were authorized against clients that were set up by hand. Their stored refresh tokens predate the indicator, so refresh is where adding a new parameter could break a working row long after deploy. You're right that a fresh sign-in on such a row now sends resource at authorize and exchange. If the server rejects it, the user sees that at sign-in and can act on it, unlike a silent refresh failure an hour later. If we want refresh to follow whatever the exchange actually sent, recording that at exchange time is a reasonable follow-up.

Validating discovered endpoints is fair hardening. oauth_token_url is checked with DNS at every use, but the discovered authorize URL is only used as a redirect target and doesn't pass through assertSafeOutboundHttpUrl. I'd rather do that as its own small change, with https-only checks on both discovered endpoints, than grow this PR further.

Partially supplied endpoints, the pane form keeping OAuth selected after a switch to REST, scopes from scopes_supported, and reading authorize_params from the profile are all reasonable follow-ups. None of them changes the behavior this PR ships, so I've left them out to keep its scope fixed.

[mogplex]

Mogplex PR Review

Status: Attention needed

Approve-ready: the custom MCP OAuth path reuses the existing authorize/callback/refresh machinery correctly, and CUSTOM_MCP_OAUTH_PROFILE, PKCE, the resource indicator, isConnectionMisconfigured and the CLI sync path (listConnectionsForCli → buildCliMcpRecord) all line up with the unchanged callers. One non-blocking correctness concern: validation now accepts each OAuth endpoint independently, but prepareOAuthConnection re-discovers and overwrites both whenever either is missing, so a supplied endpoint can be silently discarded.

Warnings

• Supplied OAuth endpoint is silently overwritten when only one of the pair is configured (lib/connections/validation.ts)
  The new MCP-OAuth branch of normalizeConnectionCreateInput accepts oauth_client_id, oauth_authorize_url and oauth_token_url independently ("validated only when supplied"), but prepareOAuthConnection in lib/connections/oauth.ts treats the endpoint pair as all-or-nothing: if (!nextConnection.oauth_authorize_url || !nextConnection.oauth_token_url) runs discoverOAuthMetadata(mcp_url) and writes both endpoints back, replacing whatever the user supplied. Two consequences:

  • A row created with only oauth_authorize_url (or only oauth_token_url) has the supplied value replaced on first sign-in, which contradicts the validation contract that supplied endpoints are kept and only missing ones are discovered.
  • When oauth_client_id is also supplied, the registration block is skipped (if (!nextConnection.oauth_client_id)), so the kept client id is paired with endpoints discovered from the server's metadata. If the user's client id belongs to a different authorization server, sign-in fails at the provider (typically invalid_client) with no hint that the pair was mismatched.

  Suggestion: either require the two endpoints together (mirroring the REST branch, which requires all four fields) or only fill in the missing endpoint instead of overwriting the supplied one, and add a unit test for a custom row that carries just one of the two URLs.

Suggestions

• Discovered registration_endpoint bypasses the https check applied to the other endpoints (lib/connections/oauth-discovery.ts)
  assertDiscoveredEndpoint is applied to authorization_endpoint and token_endpoint, but discoverOAuthMetadata copies registration_endpoint straight through, so the dynamic client registration POST is only checked by assertSafeOutboundHttpUrlWithDns and may go out over plain http. Impact is limited (the DCR body carries only client name, redirect URIs and scopes), but the response supplies the client_id that is then persisted on the row, so an unauthenticated registration response can influence stored config. Running the same helper on registration_endpoint (which would also reject non-string values from the parsed metadata) keeps the invariant the helper's doc comment states.

• Inline add form duplicates the auth option list and disagrees with the settings form (components/panes/connections-pane/inline-add-form.tsx)
  AUTH_OPTIONS is a local copy of the option list that components/connections/connection-form.ts now owns (CONNECTION_AUTH_OPTIONS / MCP_CONNECTION_AUTH_OPTIONS). Besides the duplicate label for the same value ("OAuth" here versus "OAuth (browser sign-in)" in the settings form), the two add surfaces now diverge: the settings form only offers OAuth for mcp_server and resets the selection when switching back to rest_api, while this pane still offers OAuth for REST connections, where it fails with the "OAuth client … is required" validation error unless all four fields are filled. Importing the shared constants and filtering by connection type would remove both the drift and the copy.

View check run

[charlesrhoward]

a3d38f4 makes the last round of changes.

Refresh now follows what the code exchange actually sent. The exchange records in the encrypted credentials whether it named the MCP resource, and refresh sends resource exactly when that marker is present or a preset declares it. That covers both registered and hand-configured custom clients that signed in through this flow. Tokens from exchanges that predate the marker keep refreshing without it, and a later exchange that doesn't name a resource clears it. This replaces the registered_client marker from the previous push. Registration still replaces a stale client secret on custom rows.

The authorization and token endpoints returned by discovery now go through the same assertSafeOutboundHttpUrl check as user-entered OAuth URLs, and must use https. A server that advertises a private, loopback, non-http, or plain-http endpoint fails sign-in with an error that names the field but not the URL. Credential parsing moved to lib/connections/oauth-credentials.ts to keep oauth.ts under the line cap.

The remaining review notes are tracked in #724:

• the partly supplied endpoint pair being overwritten by discovery (the warning in the latest review)
• the pane form keeping OAuth selected when switching to REST
• requesting scopes_supported
• reading authorize_params from the profile
• applying the https check to registration_endpoint
• having the pane form use the shared auth option lists

None of these changes what this PR ships, so I've kept them out of it.