Skip to content

Commit 3f2348a

Browse files
WillTaylorDevWillTaylorDev
committed
Remove multiple slashes from asset pathing
1 parent 6f09f23 commit 3f2348a

File tree

5 files changed

+174
-40
lines changed

5 files changed

+174
-40
lines changed

‎.changeset/cyan-laws-mate.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@cloudflare/workers-shared": minor
3+
---
4+
5+
Prevent same-schema attacks

‎fixtures/asset-config/redirects.test.ts

+89
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
import { SELF } from "cloudflare:test";
2+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
3+
import { applyConfigurationDefaults } from "../../packages/workers-shared/asset-worker/src/configuration";
4+
import Worker from "../../packages/workers-shared/asset-worker/src/index";
5+
import { getAssetWithMetadataFromKV } from "../../packages/workers-shared/asset-worker/src/utils/kv";
6+
import type { AssetMetadata } from "../../packages/workers-shared/asset-worker/src/utils/kv";
7+
8+
const IncomingRequest = Request<unknown, IncomingRequestCfProperties>;
9+
10+
vi.mock("../../packages/workers-shared/asset-worker/src/utils/kv.ts");
11+
vi.mock("../../packages/workers-shared/asset-worker/src/configuration");
12+
const existsMock = (fileList: Set<string>) => {
13+
vi.spyOn(Worker.prototype, "unstable_exists").mockImplementation(
14+
async (pathname: string) => {
15+
if (fileList.has(pathname)) {
16+
return pathname;
17+
}
18+
}
19+
);
20+
};
21+
const BASE_URL = "http://example.com";
22+
23+
describe("[Asset Worker] `test redirects`", () => {
24+
afterEach(() => {
25+
vi.mocked(getAssetWithMetadataFromKV).mockRestore();
26+
});
27+
beforeEach(() => {
28+
vi.mocked(getAssetWithMetadataFromKV).mockImplementation(
29+
() =>
30+
Promise.resolve({
31+
value: "no-op",
32+
metadata: {
33+
contentType: "no-op",
34+
},
35+
}) as unknown as Promise<
36+
KVNamespaceGetWithMetadataResult<ReadableStream, AssetMetadata>
37+
>
38+
);
39+
40+
vi.mocked(applyConfigurationDefaults).mockImplementation(() => {
41+
return {
42+
html_handling: "none",
43+
not_found_handling: "none",
44+
};
45+
});
46+
});
47+
48+
it("returns 404 for non matched encoded url", async () => {
49+
const files = ["/christmas/starts/november/first.html"];
50+
const requestPath = "/%2f%2fbad.example.com%2f";
51+
52+
existsMock(new Set(files));
53+
const request = new IncomingRequest(BASE_URL + requestPath);
54+
let response = await SELF.fetch(request, { redirect: "manual" });
55+
expect(response.status).toBe(404);
56+
});
57+
58+
it("returns 200 for matched non encoded url", async () => {
59+
const files = ["/you/lost/the/game.bin"];
60+
const requestPath = "/you/lost/the/game.bin";
61+
62+
existsMock(new Set(files));
63+
const request = new IncomingRequest(BASE_URL + requestPath);
64+
let response = await SELF.fetch(request, { redirect: "manual" });
65+
expect(response.status).toBe(200);
66+
});
67+
68+
it("returns redirect for matched encoded url", async () => {
69+
const files = ["/awesome/file.bin"];
70+
const requestPath = "/awesome/file%2ebin";
71+
const finalPath = "/awesome/file.bin";
72+
73+
existsMock(new Set(files));
74+
const request = new IncomingRequest(BASE_URL + requestPath);
75+
let response = await SELF.fetch(request, { redirect: "manual" });
76+
expect(response.status).toBe(307);
77+
expect(response.headers.get("location")).toBe(finalPath);
78+
});
79+
80+
it("returns 200 for matched non encoded url", async () => {
81+
const files = ["/mylittlepony.png"];
82+
const requestPath = "/mylittlepony.png";
83+
84+
existsMock(new Set(files));
85+
const request = new IncomingRequest(BASE_URL + requestPath);
86+
let response = await SELF.fetch(request, { redirect: "manual" });
87+
expect(response.status).toBe(200);
88+
});
89+
});

‎fixtures/asset-config/url-normalization.test.ts

+77
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
import { SELF } from "cloudflare:test";
2+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
3+
import { applyConfigurationDefaults } from "../../packages/workers-shared/asset-worker/src/configuration";
4+
import Worker from "../../packages/workers-shared/asset-worker/src/index";
5+
import { getAssetWithMetadataFromKV } from "../../packages/workers-shared/asset-worker/src/utils/kv";
6+
import type { AssetMetadata } from "../../packages/workers-shared/asset-worker/src/utils/kv";
7+
8+
const IncomingRequest = Request<unknown, IncomingRequestCfProperties>;
9+
10+
vi.mock("../../packages/workers-shared/asset-worker/src/utils/kv.ts");
11+
vi.mock("../../packages/workers-shared/asset-worker/src/configuration");
12+
const existsMock = (fileList: Set<string>) => {
13+
vi.spyOn(Worker.prototype, "unstable_exists").mockImplementation(
14+
async (pathname: string) => {
15+
if (fileList.has(pathname)) {
16+
return pathname;
17+
}
18+
}
19+
);
20+
};
21+
const BASE_URL = "http://example.com";
22+
23+
describe("[Asset Worker] `test redirects`", () => {
24+
afterEach(() => {
25+
vi.mocked(getAssetWithMetadataFromKV).mockRestore();
26+
});
27+
beforeEach(() => {
28+
vi.mocked(getAssetWithMetadataFromKV).mockImplementation(
29+
() =>
30+
Promise.resolve({
31+
value: "no-op",
32+
metadata: {
33+
contentType: "no-op",
34+
},
35+
}) as unknown as Promise<
36+
KVNamespaceGetWithMetadataResult<ReadableStream, AssetMetadata>
37+
>
38+
);
39+
40+
vi.mocked(applyConfigurationDefaults).mockImplementation(() => {
41+
return {
42+
html_handling: "none",
43+
not_found_handling: "none",
44+
};
45+
});
46+
});
47+
48+
it("returns 200 leading encoded double slash", async () => {
49+
const files = ["/blog/index.html"];
50+
const requestPath = "/%2fblog/index.html";
51+
52+
existsMock(new Set(files));
53+
const request = new IncomingRequest(BASE_URL + requestPath);
54+
let response = await SELF.fetch(request);
55+
expect(response.status).toBe(200);
56+
});
57+
58+
it("returns 200 leading non encoded double slash", async () => {
59+
const files = ["/blog/index.html"];
60+
const requestPath = "//blog/index.html";
61+
62+
existsMock(new Set(files));
63+
const request = new IncomingRequest(BASE_URL + requestPath);
64+
let response = await SELF.fetch(request);
65+
expect(response.status).toBe(200);
66+
});
67+
68+
it("returns 404 for non matched url", async () => {
69+
const files = ["/blog/index.html"];
70+
const requestPath = "/%2fexample.com/";
71+
72+
existsMock(new Set(files));
73+
const request = new IncomingRequest(BASE_URL + requestPath);
74+
let response = await SELF.fetch(request);
75+
expect(response.status).toBe(404);
76+
});
77+
});

‎packages/workers-shared/asset-worker/src/handler.ts

+3-7
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ export const handleRequest = async (
1919
const { pathname, search } = new URL(request.url);
2020

2121
let decodedPathname = decodePath(pathname);
22-
// normalize the path
23-
decodedPathname = decodedPathname.replaceAll('//', '/');
22+
// normalize the path; remove multiple slashes which could lead to same-schema redirects
23+
decodedPathname = decodedPathname.replace(/\/+/g, "/");
2424

2525
const intent = await getIntent(decodedPathname, configuration, exists);
2626

@@ -44,9 +44,7 @@ export const handleRequest = async (
4444
* Thus we need to redirect if that is different from the decoded version.
4545
* We combine this with other redirects (e.g. for html_handling) to avoid multiple redirects.
4646
*/
47-
if (encodedDestination !== pathname || intent.redirect) {
48-
console.log(encodedDestination, pathname);
49-
47+
if ((encodedDestination !== pathname && intent.asset) || intent.redirect) {
5048
return new TemporaryRedirectResponse(encodedDestination + search);
5149
}
5250

@@ -643,8 +641,6 @@ const safeRedirect = async (
643641
return null;
644642
}
645643

646-
console.log(file, destination, configuration);
647-
648644
if (!(await exists(destination))) {
649645
const intent = await getIntent(destination, configuration, exists, true);
650646
// return only if the eTag matches - i.e. not the 404 case

‎packages/workers-shared/asset-worker/tests/handler.test.ts

-33
Original file line numberDiff line numberDiff line change
@@ -96,37 +96,4 @@ describe("[Asset Worker] `handleRequest`", () => {
9696

9797
expect(response.status).toBe(200);
9898
});
99-
100-
it("normalizes double slashes", async () => {
101-
const configuration: Required<AssetConfig> = {
102-
html_handling: "none",
103-
not_found_handling: "none",
104-
};
105-
const eTag = "some-etag";
106-
const exists = vi.fn().mockReturnValue(eTag);
107-
const getByETag = vi.fn().mockReturnValue({
108-
readableStream: new ReadableStream(),
109-
contentType: "text/html",
110-
});
111-
112-
let response = await handleRequest(
113-
new Request("https://example.com/abc/def//123/456"),
114-
configuration,
115-
exists,
116-
getByETag
117-
);
118-
119-
expect(response.status).toBe(307);
120-
expect(response.headers.get('location')).toBe('/abc/def/123/456');
121-
122-
response = await handleRequest(
123-
new Request("https://example.com/%2fexample.com/"),
124-
configuration,
125-
exists,
126-
getByETag
127-
);
128-
129-
expect(response.status).toBe(307);
130-
expect(response.headers.get('location')).toBe('/example.com/');
131-
});
13299
});

0 commit comments

Comments
 (0)