Skip to content

Commit 6191411

Browse files
authored
fix: Fix builder logout CORS (#5752)
## Description 1. What is this PR about (link the issue and add a short description) ## Steps for reproduction 1. click button 2. expect xyz ## Code Review - [ ] hi @kof, I need you to do - conceptual review (architecture, feature-correctness) - detailed review (read every line) - test it on preview ## Before requesting a review - [ ] made a self-review - [ ] added inline comments where things may be not obvious (the "why", not "what") ## Before merging - [ ] tested locally and on preview environment (preview dev login: 0000) - [ ] updated [test cases](https://github.com/webstudio-is/webstudio/blob/main/apps/builder/docs/test-cases.md) document - [ ] added tests - [ ] if any new env variables are added, added them to `.env` file
1 parent 9c2116a commit 6191411

2 files changed

Lines changed: 195 additions & 18 deletions

File tree

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
import { beforeEach, describe, expect, test, vi } from "vitest";
2+
3+
const logoutMock = vi.hoisted(() => vi.fn());
4+
5+
vi.mock("~/services/builder-auth.server", () => ({
6+
builderAuthenticator: {
7+
logout: logoutMock,
8+
},
9+
}));
10+
11+
import { action } from "../routes/builder-logout";
12+
13+
const projectOrigin =
14+
"https://p-042d35b0-718e-4c5f-a5dd-8568282366e5.apps.webstudio.is";
15+
const dashboardOrigin = "https://apps.webstudio.is";
16+
17+
const callAction = (request: Request) =>
18+
action({
19+
request,
20+
params: {},
21+
context: {},
22+
});
23+
24+
describe("builder logout route", () => {
25+
beforeEach(() => {
26+
logoutMock.mockReset();
27+
});
28+
29+
test("responds to authorized preflight with CORS headers", async () => {
30+
const response = await callAction(
31+
new Request(`${projectOrigin}/builder-logout`, {
32+
method: "OPTIONS",
33+
headers: {
34+
Origin: dashboardOrigin,
35+
},
36+
})
37+
);
38+
39+
expect(response.status).toBe(204);
40+
expect(response.headers.get("Access-Control-Allow-Origin")).toBe(
41+
dashboardOrigin
42+
);
43+
expect(response.headers.get("Access-Control-Allow-Credentials")).toBe(
44+
"true"
45+
);
46+
expect(response.headers.get("Access-Control-Allow-Methods")).toContain(
47+
"POST"
48+
);
49+
expect(response.headers.get("Access-Control-Allow-Headers")).toContain(
50+
"Content-Type"
51+
);
52+
});
53+
54+
test("adds CORS headers to successful logout response", async () => {
55+
logoutMock.mockRejectedValueOnce(
56+
new Response(null, {
57+
status: 302,
58+
headers: {
59+
Location: `${dashboardOrigin}/login`,
60+
"Set-Cookie": "__Host-_session_builder_session_3=; Max-Age=0",
61+
},
62+
})
63+
);
64+
65+
const response = await callAction(
66+
new Request(`${projectOrigin}/builder-logout`, {
67+
method: "POST",
68+
headers: {
69+
Origin: dashboardOrigin,
70+
"Content-Type": "application/json",
71+
"Sec-Fetch-Site": "same-site",
72+
},
73+
})
74+
);
75+
76+
expect(response.status).toBe(204);
77+
expect(response.headers.get("Set-Cookie")).toContain("Max-Age=0");
78+
expect(response.headers.get("Access-Control-Allow-Origin")).toBe(
79+
dashboardOrigin
80+
);
81+
expect(response.headers.get("Access-Control-Allow-Credentials")).toBe(
82+
"true"
83+
);
84+
});
85+
86+
test("rejects preflight from a different origin", async () => {
87+
const response = await callAction(
88+
new Request(`${projectOrigin}/builder-logout`, {
89+
method: "OPTIONS",
90+
headers: {
91+
Origin: "https://evil.example",
92+
},
93+
})
94+
);
95+
96+
expect(response.status).toBe(403);
97+
expect(response.headers.get("Access-Control-Allow-Origin")).toBeNull();
98+
});
99+
100+
test("rejects same-origin builder requests without logging out", async () => {
101+
const response = await callAction(
102+
new Request(`${projectOrigin}/builder-logout`, {
103+
method: "POST",
104+
headers: {
105+
Origin: projectOrigin,
106+
"Content-Type": "application/json",
107+
"Sec-Fetch-Site": "same-origin",
108+
},
109+
})
110+
);
111+
112+
expect(response.status).toBe(403);
113+
expect(logoutMock).not.toHaveBeenCalled();
114+
});
115+
});

apps/builder/app/routes/builder-logout.ts

Lines changed: 80 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,44 +7,103 @@ import { isRedirectResponse } from "~/services/cookie.server";
77

88
const debug = createDebug(import.meta.url);
99

10+
const getCorsHeaders = (request: Request) => {
11+
const origin = request.headers.get("Origin");
12+
const allowedOrigin = getAuthorizationServerOrigin(request.url);
13+
14+
if (origin !== allowedOrigin) {
15+
return;
16+
}
17+
18+
return {
19+
"Access-Control-Allow-Credentials": "true",
20+
"Access-Control-Allow-Headers": "Content-Type",
21+
"Access-Control-Allow-Methods": "POST, OPTIONS",
22+
"Access-Control-Allow-Origin": origin,
23+
};
24+
};
25+
26+
const withCors = (request: Request, response: Response) => {
27+
const corsHeaders = getCorsHeaders(request);
28+
if (corsHeaders === undefined) {
29+
return response;
30+
}
31+
32+
const headers = new Headers(response.headers);
33+
for (const [name, value] of Object.entries(corsHeaders)) {
34+
headers.set(name, value);
35+
}
36+
headers.append("Vary", "Origin");
37+
38+
return new Response(response.body, {
39+
status: response.status,
40+
statusText: response.statusText,
41+
headers,
42+
});
43+
};
44+
1045
export const action = async ({ request }: ActionFunctionArgs) => {
1146
// IMPORTANT: This route allows cross-origin cookies to enable dashboard logout from builders.
1247
// Enforcing preflight by checking Content-Type to be application/json.
13-
// At the SaaS proxy side, only the allowed "access-control-allow-origin" header is set for OPTIONS requests.
48+
// Both the preflight and actual logout response must include CORS headers
49+
// or browser fetch reports the project logout as failed.
1450

1551
if (false === isBuilder(request)) {
1652
throw new Response("Not found", {
1753
status: 404,
1854
});
1955
}
2056

57+
if (request.method === "OPTIONS") {
58+
const corsHeaders = getCorsHeaders(request);
59+
if (corsHeaders === undefined) {
60+
return new Response("Origin not allowed", {
61+
status: 403,
62+
});
63+
}
64+
65+
return new Response(null, {
66+
status: 204,
67+
headers: {
68+
...corsHeaders,
69+
Vary: "Origin",
70+
},
71+
});
72+
}
73+
2174
if (request.method !== "POST") {
22-
return json(
23-
{ message: "Method not allowed" },
24-
{
25-
status: 405,
26-
}
75+
return withCors(
76+
request,
77+
json(
78+
{ message: "Method not allowed" },
79+
{
80+
status: 405,
81+
}
82+
)
2783
);
2884
}
2985

3086
if (request.headers.get("sec-fetch-site") === "same-origin") {
3187
// To prevent logout initiated from the builder iframe
3288

33-
throw new Response("Only cross-origin requests are allowed", {
34-
status: 403,
35-
});
89+
return withCors(
90+
request,
91+
new Response("Only cross-origin requests are allowed", {
92+
status: 403,
93+
})
94+
);
3695
}
3796

3897
if (
3998
false === request.headers.get("Content-Type")?.includes("application/json")
4099
) {
41100
// Enforce preflight request, preflight is checked on allowed origin
42101

43-
throw new Response(
44-
"Invalid content type, only application/json is allowed",
45-
{
102+
return withCors(
103+
request,
104+
new Response("Invalid content type, only application/json is allowed", {
46105
status: 415,
47-
}
106+
})
48107
);
49108
}
50109

@@ -59,13 +118,16 @@ export const action = async ({ request }: ActionFunctionArgs) => {
59118
} catch (error) {
60119
if (error instanceof Response) {
61120
if (isRedirectResponse(error)) {
62-
return new Response(null, {
63-
status: 204,
64-
headers: error.headers,
65-
});
121+
return withCors(
122+
request,
123+
new Response(null, {
124+
status: 204,
125+
headers: error.headers,
126+
})
127+
);
66128
}
67129

68-
return error;
130+
return withCors(request, error);
69131
}
70132

71133
console.error(error);

0 commit comments

Comments
 (0)