-
-
Notifications
You must be signed in to change notification settings - Fork 11.2k
feat(ui): connect-flow interlude banner on the MCP apps grid for gateway DCR sign-in #33192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4a04e6c
bc924dd
54bbd0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { fireEvent, render, screen } from "@testing-library/react"; | ||
| import ConnectFlowBanner from "./ConnectFlowBanner"; | ||
| import { PERSERVER_CONNECTING_KEY } from "@/hooks/mcpOAuthUtils"; | ||
|
|
||
| vi.mock("@/components/networking", () => ({ | ||
| getProxyBaseUrl: () => "https://gateway.example.com", | ||
| })); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| sessionStorage.clear(); | ||
| }); | ||
|
|
||
| describe("ConnectFlowBanner", () => { | ||
| it("posts the flow handle to the proxy /authorize/complete as a full-page form", () => { | ||
| const { container } = render(<ConnectFlowBanner flowHandle="flow-handle-123" clientOrigin="https://claude.ai" />); | ||
|
|
||
| const form = container.querySelector("form")!; | ||
| expect(form.getAttribute("method")).toBe("POST"); | ||
| expect(form.getAttribute("action")).toBe("https://gateway.example.com/authorize/complete"); | ||
|
|
||
| const hidden = form.querySelector('input[name="flow"]') as HTMLInputElement; | ||
| expect(hidden.value).toBe("flow-handle-123"); | ||
| // No token, code, or secret is ever placed in the form; the sealed cookie carries them. | ||
| expect(form.innerHTML).not.toContain("token"); | ||
| }); | ||
|
|
||
| it("shows the client origin so the user knows what they are connecting to", () => { | ||
| render(<ConnectFlowBanner flowHandle="h" clientOrigin="https://claude.ai" />); | ||
| expect(screen.getAllByText(/claude\.ai/).length).toBeGreaterThan(0); | ||
| expect(screen.getByRole("button", { name: /finish connecting/i })).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("falls back to a generic label when the client origin is unknown", () => { | ||
| render(<ConnectFlowBanner flowHandle="h" clientOrigin={null} />); | ||
| expect(screen.getAllByText(/the application/).length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| it("best-effort auto-finishes on pagehide (closing the tab)", () => { | ||
| const beaconMock = vi.fn(() => true); | ||
| vi.stubGlobal("navigator", { ...navigator, sendBeacon: beaconMock }); | ||
| render(<ConnectFlowBanner flowHandle="flow-xyz" clientOrigin="https://claude.ai" />); | ||
|
|
||
| window.dispatchEvent(new Event("pagehide")); | ||
|
|
||
| expect(beaconMock).toHaveBeenCalledTimes(1); | ||
| const [url, body] = beaconMock.mock.calls[0] as unknown as [string, URLSearchParams]; | ||
| expect(url).toBe("https://gateway.example.com/authorize/complete"); | ||
| expect(body.toString()).toContain("flow=flow-xyz"); | ||
| }); | ||
|
|
||
| it("does NOT auto-finish while a per-server connect is navigating away", () => { | ||
| const beaconMock = vi.fn(() => true); | ||
| vi.stubGlobal("navigator", { ...navigator, sendBeacon: beaconMock }); | ||
| render(<ConnectFlowBanner flowHandle="flow-xyz" clientOrigin="https://claude.ai" />); | ||
|
|
||
| // the per-server connect flow sets this right before it navigates to the upstream IdP | ||
| sessionStorage.setItem(PERSERVER_CONNECTING_KEY, "1"); | ||
| window.dispatchEvent(new Event("pagehide")); | ||
|
|
||
| expect(beaconMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("does NOT double-fire the auto-finish after the button was pressed", () => { | ||
| const beaconMock = vi.fn(() => true); | ||
| vi.stubGlobal("navigator", { ...navigator, sendBeacon: beaconMock }); | ||
| const { container } = render(<ConnectFlowBanner flowHandle="flow-xyz" clientOrigin="https://claude.ai" />); | ||
|
|
||
| // jsdom does not submit forms; fire the form's submit so onSubmit marks it finished | ||
| fireEvent.submit(container.querySelector("form")!); | ||
| window.dispatchEvent(new Event("pagehide")); | ||
|
|
||
| expect(beaconMock).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,77 @@ | ||||||||||||||||||||||
| "use client"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import React, { useEffect, useRef } from "react"; | ||||||||||||||||||||||
| import { CheckCircle } from "lucide-react"; | ||||||||||||||||||||||
| import { getProxyBaseUrl } from "@/components/networking"; | ||||||||||||||||||||||
| import { PERSERVER_CONNECTING_KEY } from "@/hooks/mcpOAuthUtils"; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| interface Props { | ||||||||||||||||||||||
| flowHandle: string; | ||||||||||||||||||||||
| clientOrigin: string | null; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * The interlude shown when a DCR client (Claude Desktop, MCP Inspector) sends the user | ||||||||||||||||||||||
| * through the gateway sign-in and lands them on the apps grid to authorize servers. The | ||||||||||||||||||||||
| * grid below authorizes individual servers into the per-user vault; this banner is the | ||||||||||||||||||||||
| * finish step that returns the user to the client. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * Finishing happens two ways, both hitting the proxy's /authorize/complete, which mints the | ||||||||||||||||||||||
| * gateway authorization code and 303-redirects to the DCR client's own redirect URI: | ||||||||||||||||||||||
| * - The explicit "Finish connecting" button is a native form POST, so the full-page | ||||||||||||||||||||||
| * navigation carries the HttpOnly per-flow cookie and follows the cross-origin redirect | ||||||||||||||||||||||
| * to the client's loopback. This is the reliable path. | ||||||||||||||||||||||
| * - Closing (or navigating away from) the tab fires a best-effort navigator.sendBeacon to the | ||||||||||||||||||||||
| * same endpoint. The browser follows the 303 to the client's loopback, so in most browsers | ||||||||||||||||||||||
| * the code still reaches the client without an explicit click. This is a convenience, not a | ||||||||||||||||||||||
| * consent gate: consent already happened at sign-in, so returning the user is safe. It is | ||||||||||||||||||||||
| * skipped while a per-server connect is navigating away (that is not leaving the flow), | ||||||||||||||||||||||
| * and after the button was pressed (which already delivers the code). | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| const ConnectFlowBanner: React.FC<Props> = ({ flowHandle, clientOrigin }) => { | ||||||||||||||||||||||
| const action = `${getProxyBaseUrl()}/authorize/complete`; | ||||||||||||||||||||||
| const clientLabel = clientOrigin ?? "the application"; | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The If the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium: Misleading OAuth client identity A malicious DCR client can register a redirect such as
Suggested change
|
||||||||||||||||||||||
| const finishedRef = useRef(false); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||
| sessionStorage.removeItem(PERSERVER_CONNECTING_KEY); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const autoFinishOnLeave = () => { | ||||||||||||||||||||||
| if (finishedRef.current) return; | ||||||||||||||||||||||
| if (sessionStorage.getItem(PERSERVER_CONNECTING_KEY) === "1") return; | ||||||||||||||||||||||
| if (typeof navigator.sendBeacon === "function") { | ||||||||||||||||||||||
| navigator.sendBeacon(action, new URLSearchParams({ flow: flowHandle })); | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High: OAuth flow completes without user confirmation A malicious DCR client can send an already-authenticated user through its authorization URL and then receive a redeemable code when the user merely closes or navigates away from this page. Because the client controls the PKCE verifier, it can exchange that code for access and refresh tokens representing the user; only an explicit finish action should call |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
| window.addEventListener("pagehide", autoFinishOnLeave); | ||||||||||||||||||||||
| return () => window.removeEventListener("pagehide", autoFinishOnLeave); | ||||||||||||||||||||||
| }, [action, flowHandle]); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return ( | ||||||||||||||||||||||
| <div className="mb-6 rounded-lg border border-primary/30 bg-primary/5 px-5 py-4"> | ||||||||||||||||||||||
| <div className="flex items-start justify-between gap-4 flex-wrap"> | ||||||||||||||||||||||
| <div className="flex items-start gap-3 min-w-0"> | ||||||||||||||||||||||
| <CheckCircle className="h-5 w-5 text-primary shrink-0 mt-0.5" /> | ||||||||||||||||||||||
| <div className="min-w-0"> | ||||||||||||||||||||||
| <p className="text-sm font-semibold text-foreground">Connect your MCP servers to {clientLabel}</p> | ||||||||||||||||||||||
| <p className="text-[13px] text-muted-foreground mt-0.5"> | ||||||||||||||||||||||
| Authorize the servers you want to use below, then finish connecting to return to {clientLabel}. Closing | ||||||||||||||||||||||
| this tab finishes for you. | ||||||||||||||||||||||
| </p> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| <form method="POST" action={action} className="shrink-0" onSubmit={() => (finishedRef.current = true)}> | ||||||||||||||||||||||
| <input type="hidden" name="flow" value={flowHandle} /> | ||||||||||||||||||||||
| <button | ||||||||||||||||||||||
| type="submit" | ||||||||||||||||||||||
| className="h-[38px] rounded-md bg-primary px-4 text-sm font-semibold text-primary-foreground hover:bg-primary/90" | ||||||||||||||||||||||
| > | ||||||||||||||||||||||
| Finish connecting | ||||||||||||||||||||||
| </button> | ||||||||||||||||||||||
| </form> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export default ConnectFlowBanner; | ||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(form.innerHTML).not.toContain("token")passes as long as the literal string"token"never appears anywhere in the rendered HTML — including class names, aria labels, or comments. A hidden field named"auth_token"would be caught, but"access_code"or"secret"would not. Enumerating all<input>elements byquerySelectorAll('input')and asserting exactly onename="flow"field would give the same intent with a tighter, name-based assertion.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!