History

Fix signed-out OAuth callback recovery

Rhys Sullivan committed 4 months ago

Commit f69332c1 parent bdd7b514 files · +194 −2Browse files

Unified · Split

4 files
  1. apps/cloud/src/auth/return-to.test.ts +13 −1
  2. apps/cloud/src/auth/return-to.ts +7 −1
  3. apps/cloud/src/start.ts +22 −0
  4. e2e/cloud/oauth-callback-unauthenticated.test.ts +152 −0
apps/cloud/src/auth/return-to.test.ts+13−1
@@ -7,7 +7,13 @@ import { isSafeReturnTo, loginPath, safeReturnTo } from "./return-to"; · 6 unmodified lines
77 // boundary, so the validator is what stands between "resume where you were"
88 // and an open redirect.
99 describe("isSafeReturnTo", () => {
10− const safe = ["/", "/tools", "/integrations/sentry?addAccount=1", "/billing/plans"];
10+ const safe = [
11+ "/",
12+ "/tools",
13+ "/integrations/sentry?addAccount=1",
14+ "/billing/plans",
15+ "/api/oauth/callback?state=oauth-state&code=provider-code",
16+ ];
1117 for (const path of safe) {
1218 it(`allows ${path}`, () => {
1319 expect(isSafeReturnTo(path)).toBe(true);
@@ -18,6 +24,7 @@ describe("isSafeReturnTo", () => { · 4 unmodified lines
1824 "https://evil.example", // absolute URL — off-origin redirect
1925 "//evil.example", // protocol-relative — same thing in disguise
2026 "/api/auth/logout", // API endpoints are never a landing page
27+ "/api/oauth/callback/extra?state=oauth-state", // only the exact OAuth callback resumes
2128 "/api", // bare /api too
2229 "javascript:alert(1)", // not a path at all
2330 "tools", // relative paths resolve unpredictably
@@ -56,4 +63,9 @@ describe("loginPath", () => { · 32 unmodified lines
5663 "/login?returnTo=%2Fintegrations%2Fsentry%3FaddAccount%3D1",
5764 );
5865 });
66+ it("carries OAuth callback resumes URI-encoded", () => {
67+ expect(loginPath("/api/oauth/callback?state=oauth-state&code=provider-code")).toBe(
68+ "/login?returnTo=%2Fapi%2Foauth%2Fcallback%3Fstate%3Doauth-state%26code%3Dprovider-code",
69+ );
70+ });
5971 });
apps/cloud/src/auth/return-to.ts+7−1
@@ -11,8 +11,14 @@ · 10 unmodified lines
1111 // Pure string code — imported by server handlers and the login page alike.
1212 // ---------------------------------------------------------------------------
1313
14+const pathPart = (path: string): string => path.split(/[?#]/, 1)[0] ?? "";
15+
16+const isOAuthCallbackReturnTo = (path: string): boolean => pathPart(path) === "/api/oauth/callback";
17+
1418 export const isSafeReturnTo = (path: string): boolean =>
15− path.startsWith("/") && !path.startsWith("//") && !/^\/api(\/|$)/.test(path);
19+ path.startsWith("/") &&
20+ !path.startsWith("//") &&
21+ (!/^\/api(\/|$)/.test(path) || isOAuthCallbackReturnTo(path));
1622
1723 /** The validated returnTo, or null when absent/unsafe. */
1824 export const safeReturnTo = (path: string | null | undefined): string | null =>
apps/cloud/src/start.ts+22−0
@@ -3,6 +3,8 @@ import { createMiddleware, createStart } from "@tanstack/react-start"; · 2 unmodified lines
33 import { cloudApiHandler } from "./app";
44 import { isAppOwnedPath } from "./app-paths";
55 import { authGateMiddleware } from "./auth/ssr-gate";
6+import { parseCookie } from "./auth/cookies";
7+import { loginPath } from "./auth/return-to";
68 import { prepareMcpOrgScope } from "./mcp/mount";
79 import {
810 docsProxyMiddleware,
@@ -34,6 +36,25 @@ import { · 25 unmodified lines
3436 let app: ReturnType<typeof cloudApiHandler> | undefined;
3537 const getApp = () => (app ??= cloudApiHandler());
3638
39+const SESSION_COOKIE = "wos-session";
40+const OAUTH_CALLBACK_PATH = "/api/oauth/callback";
41+
42+const oauthCallbackSignInMiddleware = createMiddleware({ type: "request" }).server(
43+ ({ pathname, request, next }) => {
44+ if (pathname !== OAUTH_CALLBACK_PATH || (request.method !== "GET" && request.method !== "HEAD")) {
45+ return next();
46+ }
47+ const sealed = parseCookie(request.headers.get("cookie"), SESSION_COOKIE);
48+ if (sealed) return next();
49+
50+ const url = new URL(request.url);
51+ return new Response(null, {
52+ status: 302,
53+ headers: { location: loginPath(`${url.pathname}${url.search}`) },
54+ });
55+ },
56+);
57+
3758 // app-owned = anything under `/api/*` (incl. the cloud extension routes) OR an
3859 // MCP/OAuth-discovery path (see `./app-paths`). The app handler serves these at
3960 // their real paths, so we forward unmodified — except `prepareMcpOrgScope`
@@ -63,6 +84,7 @@ export const startInstance = createStart(() => ({ · 23 unmodified lines
6384 docsProxyMiddleware,
6485 sentryTunnelMiddleware,
6586 posthogProxyMiddleware,
87+ oauthCallbackSignInMiddleware,
6688 appRequestMiddleware,
6789 authGateMiddleware,
6890 ],
e2e/cloud/oauth-callback-unauthenticated.test.ts+152−0
@@ -0,0 +1,152 @@
1+import { randomBytes } from "node:crypto";
2+
3+import { expect } from "@effect/vitest";
4+import { Effect } from "effect";
5+import { composePluginApi } from "@executor-js/api/server";
6+import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api";
7+import {
8+ AuthTemplateSlug,
9+ ConnectionName,
10+ IntegrationSlug,
11+ OAuthClientSlug,
12+} from "@executor-js/sdk/shared";
13+import { serveOAuthTestServer } from "@executor-js/sdk/testing";
14+
15+import { scenario } from "../src/scenario";
16+import { Api, Browser, Target } from "../src/services";
17+
18+const api = composePluginApi([openApiHttpPlugin()] as const);
19+
20+const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`;
21+
22+const oauthIntegrationSpec = (oauth: {
23+ readonly authorizationEndpoint: string;
24+ readonly tokenEndpoint: string;
25+}) =>
26+ ({
27+ spec: {
28+ kind: "blob" as const,
29+ value: JSON.stringify({
30+ openapi: "3.0.3",
31+ info: { title: "OAuth-protected API", version: "1.0.0" },
32+ paths: {
33+ "/me": {
34+ get: {
35+ operationId: "getMe",
36+ tags: ["default"],
37+ responses: { "200": { description: "the caller" } },
38+ },
39+ },
40+ },
41+ }),
42+ },
43+ baseUrl: "http://127.0.0.1:59999",
44+ authenticationTemplate: [
45+ {
46+ slug: "oauth",
47+ kind: "oauth2" as const,
48+ authorizationUrl: oauth.authorizationEndpoint,
49+ tokenUrl: oauth.tokenEndpoint,
50+ scopes: ["read"],
51+ },
52+ ],
53+ }) as const;
54+
55+scenario(
56+ "OAuth callback · a signed-out callback uses login returnTo and resumes the connection",
57+ {},
58+ Effect.gen(function* () {
59+ const target = yield* Target;
60+ const { client: makeApiClient } = yield* Api;
61+ const browser = yield* Browser;
62+ const oauth = yield* serveOAuthTestServer();
63+ const identity = yield* target.newIdentity();
64+ const client = yield* makeApiClient(api, identity);
65+
66+ const integration = IntegrationSlug.make(unique("signedoutcb"));
67+ yield* client.openapi.addSpec({
68+ payload: { ...oauthIntegrationSpec(oauth), slug: integration },
69+ });
70+
71+ const clientSlug = OAuthClientSlug.make(unique("signedoutc"));
72+ yield* client.oauth.createClient({
73+ payload: {
74+ owner: "org",
75+ slug: clientSlug,
76+ authorizationUrl: oauth.authorizationEndpoint,
77+ tokenUrl: oauth.tokenEndpoint,
78+ grant: "authorization_code",
79+ clientId: "test-client",
80+ clientSecret: "test-secret",
81+ },
82+ });
83+
84+ const started = yield* client.oauth.start({
85+ payload: {
86+ client: clientSlug,
87+ clientOwner: "org",
88+ owner: "org",
89+ name: ConnectionName.make("main"),
90+ integration,
91+ template: AuthTemplateSlug.make("oauth"),
92+ },
93+ });
94+ expect(started.status, "oauth.start begins at the provider").toBe("redirect");
95+ const authorizationUrl = started.status === "redirect" ? started.authorizationUrl : "";
96+
97+ const authorize = yield* Effect.promise(() => fetch(authorizationUrl, { redirect: "manual" }));
98+ expect(authorize.status, "the provider asks the user to log in").toBe(302);
99+ const consent = yield* Effect.promise(() =>
100+ fetch(authorize.headers.get("location") ?? "", {
101+ method: "POST",
102+ redirect: "manual",
103+ headers: {
104+ authorization: `Basic ${Buffer.from("alice:password").toString("base64")}`,
105+ },
106+ }),
107+ );
108+ expect(consent.status, "provider consent redirects back to Executor").toBe(302);
109+ const callback = new URL(consent.headers.get("location") ?? "");
110+ const callbackPath = `${callback.pathname}${callback.search}`;
111+
112+ // No cookies: this mirrors a provider redirect reaching the callback after
113+ // the user's web session is gone, expired, or otherwise missing an org.
114+ const anonymous = { label: "anonymous" };
115+
116+ yield* browser.session(anonymous, async ({ page, step }) => {
117+ await step("Provider sends a signed-out browser to the OAuth callback", async () => {
118+ const response = await page.goto(callbackPath, { waitUntil: "commit" });
119+ expect(response?.status(), "the callback redirects into the login flow").toBe(200);
120+ await page.getByText("Sign in to manage your tools and sources").waitFor();
121+ });
122+
123+ const loginUrl = new URL(page.url());
124+ expect(loginUrl.pathname, "the signed-out callback lands on the sign-in page").toBe("/login");
125+ expect(
126+ loginUrl.searchParams.get("returnTo"),
127+ "login preserves the callback so it can resume after sign-in",
128+ ).toBe(callbackPath);
129+
130+ await step("Sign in resumes the original OAuth callback", async () => {
131+ await page.getByRole("link", { name: "Sign in" }).click();
132+ await page.getByPlaceholder("new-user@example.com").fill(identity.credentials!.email);
133+ await page.getByRole("button", { name: /Continue/ }).click();
134+ await page.waitForURL((url) => url.pathname === "/api/oauth/callback", {
135+ timeout: 30_000,
136+ });
137+ await page.waitForFunction(() => document.body.innerText.includes("Connected"), null, {
138+ timeout: 30_000,
139+ });
140+ });
141+
142+ const body = (await page.locator("body").textContent())?.trim() ?? "";
143+ expect(new URL(page.url()).pathname, "the login returnTo lands back on the callback").toBe(
144+ "/api/oauth/callback",
145+ );
146+ expect(body, "the callback completes after the sign-in recovery").toContain("Connected");
147+ expect(body, "the raw protected API JSON is not shown").not.toContain(
148+ '"code":"no_organization"',
149+ );
150+ });
151+ }).pipe(Effect.scoped),
152+);