Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions apps/mobile/src/app/mcp-servers/add-custom.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const AUTH_OPTIONS: { value: McpAuthType; label: string }[] = [
{ value: "none", label: "None" },
{ value: "api_key", label: "API key" },
{ value: "oauth", label: "OAuth" },
{ value: "basic", label: "Basic Auth" },
];

export default function AddCustomMcpServerScreen() {
Expand All @@ -39,6 +40,8 @@ export default function AddCustomMcpServerScreen() {
const [apiKey, setApiKey] = useState("");
const [clientId, setClientId] = useState("");
const [clientSecret, setClientSecret] = useState("");
const [username, setUsername] = useState("");
const [password, setPassword] = useState("");

const [submitting, setSubmitting] = useState(false);
const [error, setError] = useState<string | null>(null);
Expand All @@ -47,6 +50,12 @@ export default function AddCustomMcpServerScreen() {
setError(null);
if (!name.trim()) return setError("Name is required");
if (!url.trim()) return setError("URL is required");
if (
authType === "basic" &&
Boolean(username.trim()) !== Boolean(password)
) {
return setError("Username and password are both required");
}

setSubmitting(true);
try {
Expand All @@ -63,6 +72,9 @@ export default function AddCustomMcpServerScreen() {
authType === "oauth" && clientSecret.trim()
? clientSecret.trim()
: undefined,
username:
authType === "basic" && username.trim() ? username.trim() : undefined,
password: authType === "basic" && password ? password : undefined,
Comment on lines +75 to +77

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Credential fields sent independently, inconsistent with web behavior

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
await installations.refetch();
router.back();
Expand Down Expand Up @@ -178,6 +190,28 @@ export default function AddCustomMcpServerScreen() {
</>
) : null}

{authType === "basic" ? (
<>
<FieldLabel>Username</FieldLabel>
<FieldInput
value={username}
onChangeText={setUsername}
placeholder="Enter username"
autoCapitalize="none"
autoCorrect={false}
/>
<FieldLabel>Password</FieldLabel>
<FieldInput
value={password}
onChangeText={setPassword}
placeholder="Enter password"
secureTextEntry
autoCapitalize="none"
autoCorrect={false}
/>
</>
) : null}

{error ? (
<Text className="mb-3 text-[13px] text-status-error">{error}</Text>
) : null}
Expand Down
5 changes: 3 additions & 2 deletions apps/mobile/src/features/mcp/components/McpServerRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ interface McpServerRowProps {
title: string;
subtitle?: string;
description?: string;
authType?: "api_key" | "oauth" | "none";
authType?: "api_key" | "oauth" | "basic" | "none";
badge?: ReactNode;
isStdio?: boolean;
needsReauth?: boolean;
Expand All @@ -20,9 +20,10 @@ interface McpServerRowProps {
onPress: () => void;
}

function authBadge(auth?: "api_key" | "oauth" | "none") {
function authBadge(auth?: "api_key" | "oauth" | "basic" | "none") {
if (auth === "oauth") return "OAuth";
if (auth === "api_key") return "API key";
if (auth === "basic") return "Basic Auth";
return null;
}

Expand Down
4 changes: 3 additions & 1 deletion apps/mobile/src/features/mcp/types.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Shared types for MCP server installations and marketplace templates.
// Mirrors the PostHog cloud REST schema (see `apps/code/src/renderer/api/generated.ts`).

export type McpAuthType = "api_key" | "oauth" | "none";
export type McpAuthType = "api_key" | "oauth" | "basic" | "none";

export type McpApprovalState = "approved" | "needs_approval" | "do_not_use";

Expand Down Expand Up @@ -81,6 +81,8 @@ export interface InstallCustomMcpServerOptions {
description?: string;
client_id?: string;
client_secret?: string;
username?: string;
password?: string;
install_source?: McpInstallSource;
posthog_code_callback_url?: string;
}
Expand Down
10 changes: 8 additions & 2 deletions packages/api-client/src/generated.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9348,7 +9348,9 @@ export namespace Schemas {
values?: (unknown | null) | undefined;
};
export type InsightsToolCall = { query: string; insight_type: InsightTypeEnum };
export type InstallCustomAuthTypeEnum = "api_key" | "oauth";
// TODO: hand-patched ahead of a real `pnpm typed-openapi` regen — the
// posthog/posthog backend PR adding "basic" hasn't deployed yet.
export type InstallCustomAuthTypeEnum = "api_key" | "oauth" | "basic";
export type InstallSourceEnum = "posthog" | "posthog-code";
export type InstallCustom = {
name: string;
Expand All @@ -9358,6 +9360,8 @@ export namespace Schemas {
description?: string | undefined;
client_id?: string | undefined;
client_secret?: string | undefined;
username?: string | undefined;
password?: string | undefined;
install_source?: (InstallSourceEnum & unknown) | undefined;
posthog_code_callback_url?: string | undefined;
};
Expand Down Expand Up @@ -9706,7 +9710,9 @@ export namespace Schemas {
created_by: UserBasic & unknown;
updated_at: string | null;
};
export type MCPAuthTypeEnum = "api_key" | "oauth";
// TODO: hand-patched ahead of a real `pnpm typed-openapi` regen — the
// posthog/posthog backend PR adding "basic" hasn't deployed yet.
export type MCPAuthTypeEnum = "api_key" | "oauth" | "basic";
export type MCPServerInstallation = {
id: string;
template_id: string | null;
Expand Down
2 changes: 2 additions & 0 deletions packages/api-client/src/posthog-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3884,6 +3884,8 @@ export class PostHogAPIClient {
description?: string;
client_id?: string;
client_secret?: string;
username?: string;
password?: string;
install_source?: "posthog" | "posthog-code";
posthog_code_callback_url?: string;
}): Promise<McpServerInstallation | Schemas.OAuthRedirectResponse> {
Expand Down
57 changes: 57 additions & 0 deletions packages/core/src/mcp-servers/customServerForm.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ function values(
apiKey: "",
clientId: "",
clientSecret: "",
username: "",
password: "",
...overrides,
};
}
Expand Down Expand Up @@ -48,6 +50,38 @@ describe("canSubmitCustomServer", () => {
);
expect(canSubmitCustomServer({ name: "X", url: "nope" })).toBe(false);
});

it.each([
["both empty", "", "", true],
["both present", "user", "pass", true],
["username only", "user", "", false],
["password only", "", "pass", false],
])(
"for basic auth with %s, returns %s",
(_label, username, password, expected) => {
expect(
canSubmitCustomServer({
name: "X",
url: "https://x.com",
authType: "basic",
username,
password,
}),
).toBe(expected);
},
);

it("ignores username/password pairing for non-basic auth types", () => {
expect(
canSubmitCustomServer({
name: "X",
url: "https://x.com",
authType: "oauth",
username: "user",
password: "",
}),
).toBe(true);
});
});

describe("buildCustomServerRequest", () => {
Expand Down Expand Up @@ -88,4 +122,27 @@ describe("buildCustomServerRequest", () => {
expect(apiKeyReq.client_id).toBeUndefined();
expect(apiKeyReq.client_secret).toBeUndefined();
});

it("includes username/password 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");
});

it.each([
["oauth", "user", "pass"],
["basic", "", "pass"],
["basic", "user", ""],
] as const)(
"excludes username/password when authType is %s with username=%j password=%j",
(authType, username, password) => {
const req = buildCustomServerRequest(
values({ authType, username, password }),
);
expect(req.username).toBeUndefined();
expect(req.password).toBeUndefined();
},
);
});
18 changes: 16 additions & 2 deletions packages/core/src/mcp-servers/customServerForm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ export interface CustomServerFormValues {
apiKey: string;
clientId: string;
clientSecret: string;
username: string;
password: string;
}

export interface CustomServerRequest {
Expand All @@ -18,16 +20,25 @@ export interface CustomServerRequest {
api_key?: string;
client_id?: string;
client_secret?: string;
username?: string;
password?: string;
}

export function isValidMcpUrl(url: string): boolean {
return /^https?:\/\/.+/i.test(url.trim());
}

export function canSubmitCustomServer(
values: Pick<CustomServerFormValues, "name" | "url">,
values: Pick<CustomServerFormValues, "name" | "url"> &
Partial<Pick<CustomServerFormValues, "authType" | "username" | "password">>,
): boolean {
return values.name.trim() !== "" && isValidMcpUrl(values.url);
if (values.name.trim() === "" || !isValidMcpUrl(values.url)) return false;
if (values.authType === "basic") {
const hasUsername = Boolean(values.username?.trim());
const hasPassword = Boolean(values.password);
if (hasUsername !== hasPassword) return false;
}
return true;
}

export function buildCustomServerRequest(
Expand All @@ -47,5 +58,8 @@ export function buildCustomServerRequest(
...(values.authType === "oauth" && values.clientSecret.trim()
? { client_secret: values.clientSecret.trim() }
: {}),
...(values.authType === "basic" && values.username.trim() && values.password
? { username: values.username.trim(), password: values.password }
: {}),
};
}
29 changes: 29 additions & 0 deletions packages/core/src/mcp-servers/installFlow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,35 @@ describe("installCustomWithOAuth", () => {
});
expect(result).toEqual({ error: "denied" });
});

it("forwards username/password for basic auth", async () => {
const oauth = makeOAuth();
const client: InstallFlowClient = {
installMcpTemplate: vi.fn(),
installCustomMcpServer: vi.fn().mockResolvedValue(installedInstallation),
authorizeMcpInstallation: vi.fn(),
};

await installCustomWithOAuth(client, oauth, {
name: "N",
url: "https://x",
description: "d",
auth_type: "basic",
username: "user",
password: "pass",
});

expect(client.installCustomMcpServer).toHaveBeenCalledWith({
name: "N",
url: "https://x",
description: "d",
auth_type: "basic",
username: "user",
password: "pass",
install_source: "posthog-code",
posthog_code_callback_url: "cb://here",
});
});
});

describe("reauthorizeWithOAuth", () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/mcp-servers/installFlow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ export interface InstallFlowClient {
api_key?: string;
client_id?: string;
client_secret?: string;
username?: string;
password?: string;
install_source?: "posthog" | "posthog-code";
posthog_code_callback_url?: string;
}): Promise<InstallResult>;
Expand Down Expand Up @@ -80,6 +82,8 @@ export async function installCustomWithOAuth(
api_key?: string;
client_id?: string;
client_secret?: string;
username?: string;
password?: string;
},
): Promise<OAuthCallbackResult> {
const { callbackUrl } = await oauth.getCallbackUrl();
Expand Down
Loading