diff --git a/.changeset/github-app-installation-setup.md b/.changeset/github-app-installation-setup.md new file mode 100644 index 0000000000..4866bfd473 --- /dev/null +++ b/.changeset/github-app-installation-setup.md @@ -0,0 +1,6 @@ +--- +"@executor-js/sdk": patch +"@executor-js/api": patch +--- + +Guide first-party GitHub connections through app installation and repository selection before user authorization. Keep the original authorization URL in an expiring, owner-scoped session so web, MCP, and reconnect flows share the same setup step. diff --git a/apps/cloud/src/app-paths.test.ts b/apps/cloud/src/app-paths.test.ts index dd9ff1af3c..5dd832e45c 100644 --- a/apps/cloud/src/app-paths.test.ts +++ b/apps/cloud/src/app-paths.test.ts @@ -78,6 +78,10 @@ describe("app-plane dispatch", () => { expect(servedByAppPlane("/api/oauth/callback", "POST")).toBe(false); }); + it("leaves OAuth setup to Start for sign-in and organization selection", () => { + expect(servedByAppPlane("/api/oauth/setup", "GET")).toBe(false); + }); + const appPlane = [ "/api/connections", "/api/tools", diff --git a/apps/cloud/src/app-paths.ts b/apps/cloud/src/app-paths.ts index 48e70a8c3a..5dcf83acdc 100644 --- a/apps/cloud/src/app-paths.ts +++ b/apps/cloud/src/app-paths.ts @@ -30,7 +30,7 @@ export const isAppOwnedPath = (pathname: string) => // // POST /api/sentry-tunnel - `sentryTunnelMiddleware` forwards the envelope // to Sentry; the app has no such route. -// /api/oauth/callback - `oauthCallbackSignInMiddleware` redirects a +// /api/oauth/{callback,setup} - `oauthBrowserSignInMiddleware` redirects a // signed-out visitor to /login, and start.ts // rewrites the org-scoped `state` before handing // off. Routing it early would drop both. @@ -41,7 +41,10 @@ export const isAppOwnedPath = (pathname: string) => // --------------------------------------------------------------------------- export const isStartOwnedApiPath = (pathname: string, method: string): boolean => - (pathname === "/api/sentry-tunnel" && method === "POST") || pathname === "/api/oauth/callback"; + (pathname === "/api/sentry-tunnel" && method === "POST") || isOAuthBrowserPath(pathname); + +export const isOAuthBrowserPath = (pathname: string): boolean => + pathname === "/api/oauth/callback" || pathname === "/api/oauth/setup"; export const servedByAppPlane = (pathname: string, method: string): boolean => isApiPath(pathname) && !isStartOwnedApiPath(pathname, method); diff --git a/apps/cloud/src/engine/first-party-oauth-clients.test.ts b/apps/cloud/src/engine/first-party-oauth-clients.test.ts index 1ca3962d6b..967d0ecd28 100644 --- a/apps/cloud/src/engine/first-party-oauth-clients.test.ts +++ b/apps/cloud/src/engine/first-party-oauth-clients.test.ts @@ -83,6 +83,10 @@ describe("cloud first-party OAuth clients", () => { optional_scope: "content crm.objects.custom.read crm.schemas.custom.read", }, }); + expect(byName.get("github")).toMatchObject({ + authorizationScopes: [], + authorizationSetup: { actionUrl: "https://github.com/apps/executor-sh/installations/new" }, + }); expect(byName.get("linear")).toMatchObject({ authorizationScopeSeparator: "," }); expect(byName.get("microsoft")).toMatchObject({ additionalAuthorizationScopes: ["offline_access"], @@ -179,3 +183,13 @@ describe("cloud first-party Google app", () => { } }); }); + +it("uses the installation page for the deployment's GitHub App", () => { + const github = firstPartyOAuthClientsFor({ + ...completeEnv, + FIRST_PARTY_GITHUB_INSTALLATION_URL: "https://github.com/apps/custom-app/installations/new", + }).find((client) => client.name === "github"); + expect(github?.authorizationSetup?.actionUrl).toBe( + "https://github.com/apps/custom-app/installations/new", + ); +}); diff --git a/apps/cloud/src/engine/first-party-oauth-clients.ts b/apps/cloud/src/engine/first-party-oauth-clients.ts index 12b0748627..0b8ca1ff56 100644 --- a/apps/cloud/src/engine/first-party-oauth-clients.ts +++ b/apps/cloud/src/engine/first-party-oauth-clients.ts @@ -30,6 +30,7 @@ export interface FirstPartyOAuthClientEnv { readonly FIRST_PARTY_GITHUB_CLIENT_SECRET?: string; readonly FIRST_PARTY_GITHUB_AUTHORIZE_URL?: string; readonly FIRST_PARTY_GITHUB_TOKEN_URL?: string; + readonly FIRST_PARTY_GITHUB_INSTALLATION_URL?: string; readonly FIRST_PARTY_GITLAB_CLIENT_ID?: string; readonly FIRST_PARTY_GITLAB_CLIENT_SECRET?: string; readonly FIRST_PARTY_GOOGLE_CLIENT_ID?: string; @@ -269,6 +270,15 @@ export const firstPartyOAuthClientsFor = ( authorizationUrl: env.FIRST_PARTY_GITHUB_AUTHORIZE_URL ?? "https://github.com/login/oauth/authorize", tokenUrl: env.FIRST_PARTY_GITHUB_TOKEN_URL ?? "https://github.com/login/oauth/access_token", + authorizationSetup: { + title: "Connect GitHub", + description: + "Install the GitHub App on your account or organization and choose the repositories Executor can access. Authorizing your account alone does not grant private repository access. Organization access may require an owner's approval.", + actionLabel: "Install or configure GitHub App", + actionUrl: + env.FIRST_PARTY_GITHUB_INSTALLATION_URL ?? + "https://github.com/apps/executor-sh/installations/new", + }, integrations: [IntegrationSlug.make("github_rest")], // GitHub App user access tokens do not use classic OAuth scopes; their // capabilities come from the app's registered permissions. diff --git a/apps/cloud/src/env-augment.d.ts b/apps/cloud/src/env-augment.d.ts index 715991f394..cd31a85008 100644 --- a/apps/cloud/src/env-augment.d.ts +++ b/apps/cloud/src/env-augment.d.ts @@ -89,6 +89,8 @@ declare global { // production (the real github.com endpoints are the defaults). FIRST_PARTY_GITHUB_AUTHORIZE_URL?: string; FIRST_PARTY_GITHUB_TOKEN_URL?: string; + // Override when deploying with a different GitHub App registration. + FIRST_PARTY_GITHUB_INSTALLATION_URL?: string; FIRST_PARTY_GITLAB_CLIENT_ID?: string; FIRST_PARTY_GITLAB_CLIENT_SECRET?: string; FIRST_PARTY_GOOGLE_CLIENT_ID?: string; diff --git a/apps/cloud/src/start.ts b/apps/cloud/src/start.ts index aaadc1a521..70855273f9 100644 --- a/apps/cloud/src/start.ts +++ b/apps/cloud/src/start.ts @@ -1,7 +1,7 @@ import { createMiddleware, createStart } from "@tanstack/react-start"; import { decodeOAuthCallbackState } from "@executor-js/sdk/shared"; -import { isAppOwnedPath } from "./app-paths"; +import { isAppOwnedPath, isOAuthBrowserPath } from "./app-paths"; import { authGateMiddleware } from "./auth/doc-gate"; import { parseCookie } from "./auth/cookies"; import { ORG_SELECTOR_HEADER } from "./auth/organization"; @@ -48,9 +48,8 @@ const getApp = async (): Promise> => (app ??= (await import("./app")).cloudApiHandler()); const SESSION_COOKIE = "wos-session"; -const OAUTH_CALLBACK_PATH = "/api/oauth/callback"; -const oauthCallbackOrgScopedRequest = (request: Request): Request => { +const oauthBrowserOrgScopedRequest = (request: Request): Request => { const url = new URL(request.url); const callbackState = decodeOAuthCallbackState(url.searchParams.get("state")); if (callbackState === null) return request; @@ -61,12 +60,9 @@ const oauthCallbackOrgScopedRequest = (request: Request): Request => { return new Request(rewritten, { headers }); }; -const oauthCallbackSignInMiddleware = createMiddleware({ type: "request" }).server( +const oauthBrowserSignInMiddleware = createMiddleware({ type: "request" }).server( ({ pathname, request, next }) => { - if ( - pathname !== OAUTH_CALLBACK_PATH || - (request.method !== "GET" && request.method !== "HEAD") - ) { + if (!isOAuthBrowserPath(pathname) || (request.method !== "GET" && request.method !== "HEAD")) { return next(); } const sealed = parseCookie(request.headers.get("cookie"), SESSION_COOKIE); @@ -89,8 +85,9 @@ const oauthCallbackSignInMiddleware = createMiddleware({ type: "request" }).serv const appRequestMiddleware = createMiddleware({ type: "request" }).server( async ({ pathname, request, next }) => { if (isAppOwnedPath(pathname)) { - const scopedRequest = - pathname === OAUTH_CALLBACK_PATH ? oauthCallbackOrgScopedRequest(request) : request; + const scopedRequest = isOAuthBrowserPath(pathname) + ? oauthBrowserOrgScopedRequest(request) + : request; return (await getApp()).handler(prepareMcpOrgScope(scopedRequest)); } return next(); @@ -113,7 +110,7 @@ export const startInstance = createStart(() => ({ docsProxyMiddleware, sentryTunnelMiddleware, posthogProxyMiddleware, - oauthCallbackSignInMiddleware, + oauthBrowserSignInMiddleware, appRequestMiddleware, authGateMiddleware, ], diff --git a/e2e/scenarios/first-party-oauth.test.ts b/e2e/scenarios/first-party-oauth.test.ts index bee0c78daa..967768c692 100644 --- a/e2e/scenarios/first-party-oauth.test.ts +++ b/e2e/scenarios/first-party-oauth.test.ts @@ -4,11 +4,9 @@ // // 1. Listing: `oauth.listClients` surfaces `first-party:github` with a // `first_party` origin and its public client id — no create call ever ran. -// 2. Flow: `oauth.start` through the first-party slug redirects to the -// provider's authorize endpoint carrying the env-configured client id and -// this platform's `/api/oauth/callback` — proof the config-resolved -// identity (not a stored row) drives the flow. The redirect is asserted, -// never followed: github.com is not visited. +// 2. Flow: the returned URL opens installation guidance, then preserves the +// original provider authorization URL, PKCE and organization routing. +// GitHub's links are checked without contacting the live provider. // 3. Guardrails: the reserved `first-party:` namespace is rejected by // createClient, so no org can shadow the host's app with its own row. import { randomBytes } from "node:crypto"; @@ -93,7 +91,7 @@ const googleShapedIntegrationSpec = (scopes: readonly string[]) => ({ }); scenario( - "First-party OAuth · the host-declared GitHub app is listed and drives the authorize redirect", + "First-party OAuth · GitHub setup includes installation before authorization", {}, Effect.scoped( Effect.gen(function* () { @@ -103,6 +101,7 @@ scenario( // their own OAuth apps through its existing registration flow. if (target.name !== "cloud") return; const { client: makeApiClient } = yield* Api; + const browser = yield* Browser; const identity = yield* target.newIdentity(); const client = yield* makeApiClient(api, identity); @@ -116,6 +115,9 @@ scenario( // 2. A start through the first-party slug builds GitHub's authorize URL // from the config identity and this platform's served callback. const integration = IntegrationSlug.make(unique("fpgh")); + yield* Effect.addFinalizer(() => + client.openapi.removeSpec({ params: { slug: integration } }).pipe(Effect.ignore), + ); yield* client.openapi.addSpec({ payload: { ...githubShapedIntegrationSpec, slug: integration }, }); @@ -129,16 +131,67 @@ scenario( template: AuthTemplateSlug.make("oauth"), }, }); - expect(started.status, "oauth.start redirects to the provider").toBe("redirect"); - const authorizationUrl = started.status === "redirect" ? started.authorizationUrl : ""; - const authorize = new URL(authorizationUrl); - expect(authorize.origin + authorize.pathname).toBe( - "https://github.com/login/oauth/authorize", + expect(started.status, "oauth.start opens setup").toBe("redirect"); + if (started.status !== "redirect") return yield* Effect.die("expected redirect"); + yield* Effect.addFinalizer(() => + client.oauth.cancel({ payload: { state: started.state } }).pipe(Effect.ignore), ); - expect(authorize.searchParams.get("client_id")).toBe("e2e-first-party-github"); - expect(authorize.searchParams.get("redirect_uri")).toBe( - new URL("/api/oauth/callback", target.baseUrl).toString(), + const setupUrl = new URL(started.authorizationUrl); + expect(setupUrl.origin + setupUrl.pathname).toBe( + new URL("/api/oauth/setup", target.baseUrl).toString(), ); + // A URL supplied by a caller must never replace the saved continuation. + setupUrl.searchParams.set("authorization_url", "https://example.invalid/phishing"); + yield* browser.session(identity, async ({ page, step }) => { + await step("Open GitHub setup and find repository installation", async () => { + const response = await page.goto(setupUrl.toString()); + expect(response?.status()).toBe(200); + expect(response?.headers()["cache-control"]).toBe("no-store"); + await page.getByRole("heading", { name: "Connect GitHub", exact: true }).waitFor(); + const install = page.getByRole("link", { name: "Install or configure GitHub App" }); + expect(await install.getAttribute("href")).toBe( + "https://github.com/apps/executor-sh/installations/new", + ); + expect(await install.getAttribute("target")).toBe("_blank"); + const authorize = new URL( + (await page + .getByRole("link", { name: "Continue to authorization" }) + .getAttribute("href"))!, + ); + expect(authorize.origin + authorize.pathname).toBe( + "https://github.com/login/oauth/authorize", + ); + expect(authorize.searchParams.get("client_id")).toBe("e2e-first-party-github"); + expect(authorize.searchParams.get("redirect_uri")).toBe( + new URL("/api/oauth/callback", target.baseUrl).toString(), + ); + expect(authorize.searchParams.get("state")).toBe(setupUrl.searchParams.get("state")); + expect(authorize.searchParams.get("code_challenge_method")).toBe("S256"); + expect(authorize.searchParams.get("code_challenge")).toMatch(/^[A-Za-z0-9_-]{43}$/); + expect(authorize.searchParams.has("scope")).toBe(false); + }); + await step("Read setup in light mode", async () => { + await page.emulateMedia({ colorScheme: "light" }); + }); + await step("Choose access on a narrow screen", async () => { + await page.setViewportSize({ width: 390, height: 844 }); + expect( + await page.locator("body").evaluate((body) => body.scrollWidth <= window.innerWidth), + ).toBe(true); + }); + await step("Read setup in dark mode on a narrow screen", async () => { + await page.emulateMedia({ colorScheme: "dark" }); + }); + await step("Cancel setup and reopen the expired link", async () => { + await Effect.runPromise(client.oauth.cancel({ payload: { state: started.state } })); + const response = await page.goto(setupUrl.toString()); + expect(response?.status()).toBe(410); + await page.getByRole("heading", { name: "Connection setup unavailable" }).waitFor(); + expect(await page.getByRole("link", { name: "Continue to authorization" }).count()).toBe( + 0, + ); + }); + }); // 3. The reserved namespace cannot be shadowed by a stored row. The // server rejects with a StorageError, which the HTTP edge scrubs to an diff --git a/packages/core/api/src/handlers/oauth.ts b/packages/core/api/src/handlers/oauth.ts index eb4b1f939b..805c7c1a68 100644 --- a/packages/core/api/src/handlers/oauth.ts +++ b/packages/core/api/src/handlers/oauth.ts @@ -11,6 +11,7 @@ import { HttpServerResponse } from "effect/unstable/http"; import { Effect, Option, Schema } from "effect"; import { runOAuthCallback, type PopupErrorMessage } from "../oauth-popup"; +import { oauthSetupDocument, oauthSetupUnavailableDocument } from "../oauth-setup"; import { OAUTH_POPUP_MESSAGE_TYPE, OAuthCompleteError, @@ -20,6 +21,7 @@ import { OAuthState, type Connection, type ConnectResult, + decodeOAuthCallbackState, } from "@executor-js/sdk"; import { ExecutorApi } from "../api"; @@ -173,6 +175,29 @@ export const OAuthHandlers = HttpApiBuilder.group(ExecutorApi, "oauth", (handler }), ), ) + .handle("setup", ({ query }) => + capture( + Effect.gen(function* () { + const executor = yield* ExecutorService; + const state = decodeOAuthCallbackState(query.state)?.state ?? query.state; + const setup = yield* executor.oauth.getSetup(OAuthState.make(state)); + return HttpServerResponse.text(oauthSetupDocument(setup), { + contentType: "text/html; charset=utf-8", + headers: { "cache-control": "no-store", "referrer-policy": "no-referrer" }, + }); + }).pipe( + Effect.catchTag("OAuthSessionNotFoundError", () => + Effect.succeed( + HttpServerResponse.text(oauthSetupUnavailableDocument(), { + contentType: "text/html; charset=utf-8", + status: 410, + headers: { "cache-control": "no-store" }, + }), + ), + ), + ), + ), + ) .handle("complete", ({ payload }) => capture( Effect.gen(function* () { diff --git a/packages/core/api/src/html-escape.ts b/packages/core/api/src/html-escape.ts new file mode 100644 index 0000000000..9c2dfed0b5 --- /dev/null +++ b/packages/core/api/src/html-escape.ts @@ -0,0 +1,8 @@ +/** Escape text and quoted attribute values in server-rendered HTML. */ +export const escapeHtml = (value: string): string => + value + .replaceAll("&", "&") + .replaceAll("<", "<") + .replaceAll(">", ">") + .replaceAll('"', """) + .replaceAll("'", "'"); diff --git a/packages/core/api/src/oauth-popup.ts b/packages/core/api/src/oauth-popup.ts index 9bfcdfda2d..b3cd241161 100644 --- a/packages/core/api/src/oauth-popup.ts +++ b/packages/core/api/src/oauth-popup.ts @@ -12,6 +12,8 @@ import { Cause, Effect } from "effect"; +import { escapeHtml } from "./html-escape"; + import { decodeOAuthCallbackState, OAUTH_POPUP_MESSAGE_TYPE, @@ -48,14 +50,6 @@ export const setOAuthCompletionListener = (listener: OAuthCompletionListener | n // HTML generation // --------------------------------------------------------------------------- -const escapeHtml = (value: string): string => - value - .replaceAll("&", "&") - .replaceAll("<", "<") - .replaceAll(">", ">") - .replaceAll('"', """) - .replaceAll("'", "'"); - /** * Serialize for embedding inside a `', description: "" }, + authorizationUrl: "https://github.com/login/oauth/authorize?state=a&client_id=b", + }); + expect(html).not.toContain("