feat(mcp): add Basic Auth support for custom MCP server connectors#3187
feat(mcp): add Basic Auth support for custom MCP server connectors#3187jericopingul wants to merge 3 commits into
Conversation
Custom MCP servers that require HTTP Basic Auth (e.g. DataForSEO's remote MCP server) had no way to connect — the only non-OAuth option was "API Key", which sends `Authorization: Bearer <key>` and 401s against Basic-auth-only upstreams. Adds a `"basic"` auth type (username/password) across the install form, install flow, and mobile's custom-install screen. Client-side only: the actual `Authorization: Basic ...` header is built server-side by the PostHog backend proxy, which needs a matching change (tracked in PostHog/posthog#68653) before this is functional end-to-end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Reviews (1): Last reviewed commit: "Merge branch 'main' into jp/dataforseo-m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| username: | ||
| authType === "basic" && username.trim() ? username.trim() : undefined, | ||
| password: authType === "basic" && password ? password : undefined, |
There was a problem hiding this comment.
Mobile sends username and password independently — a user who fills in a username but leaves the password blank will submit a request with auth_type: "basic" and username but no password. The web path (buildCustomServerRequest) requires both to be non-empty before including either, so it would silently drop both fields for the same partial input. Neither path validates that both are present before allowing submit, so in both cases the user gets a confusing backend error rather than clear client-side feedback. Consider adding a validation check (mirroring the name/url guards above) that requires both username and password to be non-empty when authType === "basic" before calling installCustomWithOAuth.
There was a problem hiding this comment.
Confirmed — mobile submitted username/password independently, and web's buildCustomServerRequest silently dropped both on a partial pair, so either path could install a non-functional basic-auth server with only an opaque backend 401 later. Fixed by making canSubmitCustomServer() reject a lone username or password for authType === "basic" (wired into the web form's disabled-submit state), and adding the equivalent guard before installCustomWithOAuth on mobile.
| it("includes username/password only for basic auth when both are present", () => { | ||
| const req = buildCustomServerRequest( | ||
| values({ authType: "basic", username: " user ", password: "pass" }), | ||
| ); | ||
| expect(req.username).toBe("user"); | ||
| expect(req.password).toBe("pass"); | ||
|
|
||
| expect( | ||
| buildCustomServerRequest( | ||
| values({ authType: "oauth", username: "user", password: "pass" }), | ||
| ).username, | ||
| ).toBeUndefined(); | ||
| expect( | ||
| buildCustomServerRequest( | ||
| values({ authType: "basic", username: "", password: "pass" }), | ||
| ).username, | ||
| ).toBeUndefined(); | ||
| expect( | ||
| buildCustomServerRequest( | ||
| values({ authType: "basic", username: "user", password: "" }), | ||
| ).password, | ||
| ).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
This it() block tests four independent scenarios (both present, wrong auth type, empty username, empty password) as sequential assertions. Per project convention, these should use it.each (or equivalent) so that each scenario is reported and fails independently. The installFlow.test.ts basic-auth test is a single case and is fine, but this block is the one that tests multiple distinct inputs.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed — split into an it.each covering the exclude cases (wrong auth type, empty username, empty password) plus one plain it for the happy path, per the project's parameterised-test convention.
Web and mobile both let a user submit auth_type=basic with only one of username/password filled in — web silently dropped both fields, mobile sent the partial pair — so the install would succeed but the connection would fail later with an opaque backend 401 instead of clear client-side feedback. canSubmitCustomServer() now treats a lone username or password as incomplete; api_key/oauth are unaffected since they have no pairing requirement. Also splits the buildCustomServerRequest basic-auth test into it.each per the project's parameterised-test convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
"basic"auth type (username/password) to the custom MCP server install form, install flow, and mobile's custom-install screen, alongside the existing OAuth/API Key options.Authorization: Basic base64(username:password)), which the existingapi_keytype can't produce (it always sendsBearer <key>).Authorizationheader is built server-side by the PostHog backend's MCP Store proxy. This PR hand-patches the generated API client types ahead of a realpnpm typed-openapiregen, and is inert until the backend recognizes"basic".Depends on
"basic"toAUTH_TYPE_CHOICES, acceptsusername/passwordon the custom-install endpoint, and builds the Basic auth header in the MCP Store proxy. This PR should land and deploy before merging this one (selecting "Basic Auth" before that ships would 400).Changes
packages/api-client: widenMcpAuthType/InstallCustomAuthTypeEnumto include"basic"; addusername/passwordto the install request shape.packages/core/src/mcp-servers: threadusername/passwordthroughcustomServerForm.tsandinstallFlow.ts, with new test coverage.packages/ui/src/features/mcp-server-managerandmcp-servers: add the "Basic Auth" option with Username/Password fields to the Add Custom Server form, wired through both places it's consumed.apps/mobile: mirror the same auth type and fields in the native "Add Custom Server" screen.Test plan
pnpm --filter @posthog/core test— 196 files / 1982 tests passingpnpm --filter @posthog/core typecheckpnpm --filter @posthog/ui typecheckpnpm --filter @posthog/api-client typecheckpnpm --filter mobile test— 52 files / 394 tests passingpnpm biome checkon all touched files🤖 Generated with Claude Code