Skip to content

Commit

Permalink
fix(wrangler): Validate routes for Workers with assets (cloudflare#…
Browse files Browse the repository at this point in the history 
…6621)

We want wrangler to error if users are trying to deploy
a Worker with assets, and routes with a path component.

All Workers with assets must have either:

- custom domain routes
- pattern routes which have no path component (except
for the wildcard splat) "some.domain.com/\*"
  • Loading branch information
@emily-shen
emily-shen authored Sep 11, 2024
1 parent 3f5b934 commit 6523db2
Show file tree
Hide file tree
Showing 8 changed files with 272 additions and 39 deletions.
12 changes: 12 additions & 0 deletions .changeset/lazy-poems-mate.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"wrangler": patch
---

fix: Validate `routes` in `wrangler dev` and `wrangler deploy` for Workers with assets

We want wrangler to error if users are trying to deploy a Worker with assets, and routes with a path component.

All Workers with assets must have either:

- custom domain routes
- pattern routes which have no path component (except for the wildcard splat) "some.domain.com/\*"
111 changes: 101 additions & 10 deletions packages/wrangler/src/__tests__/deploy.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1455,11 +1455,12 @@ Update them to point to this script instead?`,
routes: [{ pattern: "*.example.com", custom_domain: true }],
});
writeWorkerSource();
await expect(
runWrangler("deploy ./index")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use "*.example.com" as a Custom Domain; wildcard operators (*) are not allowed]`
);
await expect(runWrangler("deploy ./index")).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: Invalid Routes:
*.example.com:
Wildcard operators (*) are not allowed in Custom Domains]
`);

writeWranglerToml({
routes: [
Expand All @@ -1469,11 +1470,12 @@ Update them to point to this script instead?`,
writeWorkerSource();
mockServiceScriptData({});

await expect(
runWrangler("deploy ./index")
).rejects.toThrowErrorMatchingInlineSnapshot(
`[Error: Cannot use "api.example.com/at/a/path" as a Custom Domain; paths are not allowed]`
);
await expect(runWrangler("deploy ./index")).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: Invalid Routes:
api.example.com/at/a/path:
Paths are not allowed in Custom Domains]
`);
});

it("should not continue with publishing an override if user does not confirm", async () => {
Expand Down Expand Up @@ -1516,6 +1518,95 @@ Update them to point to this script instead?`,
});
});

it("should error on routes with paths if experimental assets are present", async () => {
writeWranglerToml({
routes: [
"simple.co.uk/path",
"simple.co.uk/path/*",
"simple.co.uk/",
"simple.co.uk/*",
"simple.co.uk",
{ pattern: "route.co.uk/path", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/path/*", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/*", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/", zone_id: "asdfadsf" },
{ pattern: "route.co.uk", zone_id: "asdfadsf" },
{ pattern: "custom.co.uk/path", custom_domain: true },
{ pattern: "custom.co.uk/*", custom_domain: true },
{ pattern: "custom.co.uk", custom_domain: true },
],
});
writeWorkerSource();
writeAssets([{ filePath: "asset.txt", content: "Content of file-1" }]);

await expect(runWrangler(`deploy --experimental-assets="assets"`)).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: Invalid Routes:
simple.co.uk/path:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path with /*
simple.co.uk/path/*:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path/* with /*
simple.co.uk/:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
simple.co.uk:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
route.co.uk/path:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path with /*
route.co.uk/path/*:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path/* with /*
route.co.uk/:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
route.co.uk:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
custom.co.uk/path:
Paths are not allowed in Custom Domains
custom.co.uk/*:
Wildcard operators (*) are not allowed in Custom Domains
Paths are not allowed in Custom Domains]
`);
});

it("shouldn't error on routes with paths if there are no experimental assets", async () => {
writeWranglerToml({
routes: [
"simple.co.uk/path",
"simple.co.uk/path/*",
"simple.co.uk/",
"simple.co.uk/*",
"simple.co.uk",
{ pattern: "route.co.uk/path", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/path/*", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/*", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/", zone_id: "asdfadsf" },
{ pattern: "route.co.uk", zone_id: "asdfadsf" },
{ pattern: "custom.co.uk/path", custom_domain: true },
{ pattern: "custom.co.uk/*", custom_domain: true },
{ pattern: "custom.co.uk", custom_domain: true },
],
});
writeWorkerSource();

await expect(runWrangler(`deploy ./index`)).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: Invalid Routes:
custom.co.uk/path:
Paths are not allowed in Custom Domains
custom.co.uk/*:
Wildcard operators (*) are not allowed in Custom Domains
Paths are not allowed in Custom Domains]
`);
});

it.todo("should error if it's a workers.dev route");
});

Expand Down
84 changes: 84 additions & 0 deletions packages/wrangler/src/__tests__/dev.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,90 @@ describe("wrangler dev", () => {
})
);
});
it("should error if custom domains with paths are passed in but allow paths on normal routes", async () => {
fs.writeFileSync("index.js", `export default {};`);
writeWranglerToml({
main: "index.js",
routes: [
"simple.co.uk/path",
"simple.co.uk/*",
"simple.co.uk",
{ pattern: "route.co.uk/path", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/*", zone_id: "asdfadsf" },
{ pattern: "route.co.uk", zone_id: "asdfadsf" },
{ pattern: "custom.co.uk/path", custom_domain: true },
{ pattern: "custom.co.uk/*", custom_domain: true },
{ pattern: "custom.co.uk", custom_domain: true },
],
});
await expect(runWrangler(`dev`)).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: Invalid Routes:
custom.co.uk/path:
Paths are not allowed in Custom Domains
custom.co.uk/*:
Wildcard operators (*) are not allowed in Custom Domains
Paths are not allowed in Custom Domains]
`);
});
it("should error on routes with paths if experimental assets are present", async () => {
writeWranglerToml({
routes: [
"simple.co.uk/path",
"simple.co.uk/path/*",
"simple.co.uk/",
"simple.co.uk/*",
"simple.co.uk",
{ pattern: "route.co.uk/path", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/path/*", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/*", zone_id: "asdfadsf" },
{ pattern: "route.co.uk/", zone_id: "asdfadsf" },
{ pattern: "route.co.uk", zone_id: "asdfadsf" },
{ pattern: "custom.co.uk/path", custom_domain: true },
{ pattern: "custom.co.uk/*", custom_domain: true },
{ pattern: "custom.co.uk", custom_domain: true },
],
experimental_assets: {
directory: "assets",
},
});
fs.openSync("assets", "w");
await expect(runWrangler(`dev`)).rejects
.toThrowErrorMatchingInlineSnapshot(`
[Error: Invalid Routes:
simple.co.uk/path:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path with /*
simple.co.uk/path/*:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path/* with /*
simple.co.uk/:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
simple.co.uk:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
route.co.uk/path:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path with /*
route.co.uk/path/*:
Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /path/* with /*
route.co.uk/:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
route.co.uk:
Workers which have static assets must end with a wildcard path. Update the route to end with /*
custom.co.uk/path:
Paths are not allowed in Custom Domains
custom.co.uk/*:
Wildcard operators (*) are not allowed in Custom Domains
Paths are not allowed in Custom Domains]
`);
});
});

describe("host", () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/wrangler/src/api/startDevWorker/ConfigController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ async function resolveDevConfig(
routes: input.triggers?.filter(
(t): t is Extract<Trigger, { type: "route" }> => t.type === "route"
),
experimentalAssets: input.experimental?.assets?.directory,
},
config
);
Expand Down Expand Up @@ -163,6 +164,7 @@ async function resolveTriggers(
routes: input.triggers?.filter(
(t): t is Extract<Trigger, { type: "route" }> => t.type === "route"
),
experimentalAssets: input.experimental?.assets?.directory,
},
config
);
Expand Down
67 changes: 53 additions & 14 deletions packages/wrangler/src/deploy/deploy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,58 @@ function errIsStartupErr(err: unknown): err is ParseError & { code: 10021 } {
return false;
}

export const validateRoutes = (
routes: Route[],
hasExperimentalAssets: boolean
) => {
const invalidRoutes: Record<string, string[]> = {};
for (const route of routes) {
if (typeof route !== "string" && route.custom_domain) {
if (route.pattern.includes("*")) {
invalidRoutes[route.pattern] ??= [];
invalidRoutes[route.pattern].push(
`Wildcard operators (*) are not allowed in Custom Domains`
);
}
if (route.pattern.includes("/")) {
invalidRoutes[route.pattern] ??= [];
invalidRoutes[route.pattern].push(
`Paths are not allowed in Custom Domains`
);
}
} else if (hasExperimentalAssets) {
const pattern = typeof route === "string" ? route : route.pattern;
const components = pattern.split("/");

if (
// = ["route.com"] bare domains are invalid as it would only match exactly that
components.length === 1 ||
// = ["route.com",""] as above
(components.length === 2 && components[1] === "")
) {
invalidRoutes[pattern] ??= [];
invalidRoutes[pattern].push(
`Workers which have static assets must end with a wildcard path. Update the route to end with /*`
);
// ie it doesn't match exactly "route.com/*" = [route.com, *]
} else if (!(components.length === 2 && components[1] === "*")) {
invalidRoutes[pattern] ??= [];
invalidRoutes[pattern].push(
`Workers which have static assets cannot be routed on a URL which has a path component. Update the route to replace /${components.slice(1).join("/")} with /*`
);
}
}
}
if (Object.keys(invalidRoutes).length > 0) {
throw new UserError(
`Invalid Routes:\n` +
Object.entries(invalidRoutes)
.map(([route, errors]) => `${route}:\n` + errors.join("\n"))
.join(`\n\n`)
);
}
};

export function renderRoute(route: Route): string {
let result = "";
if (typeof route === "string") {
Expand Down Expand Up @@ -383,20 +435,7 @@ See https://developers.cloudflare.com/workers/platform/compatibility-dates for m

const routes =
props.routes ?? config.routes ?? (config.route ? [config.route] : []) ?? [];
for (const route of routes) {
if (typeof route !== "string" && route.custom_domain) {
if (route.pattern.includes("*")) {
throw new UserError(
`Cannot use "${route.pattern}" as a Custom Domain; wildcard operators (*) are not allowed`
);
}
if (route.pattern.includes("/")) {
throw new UserError(
`Cannot use "${route.pattern}" as a Custom Domain; paths are not allowed`
);
}
}
}
validateRoutes(routes, Boolean(props.experimentalAssetsOptions));

const jsxFactory = props.jsxFactory || config.jsx_factory;
const jsxFragment = props.jsxFragment || config.jsx_fragment;
Expand Down
15 changes: 10 additions & 5 deletions packages/wrangler/src/dev.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
extractBindingsOfType,
} from "./api/startDevWorker/utils";
import { findWranglerToml, printBindings, readConfig } from "./config";
import { validateRoutes } from "./deploy/deploy";
import { getEntry } from "./deployment-bundle/entry";
import { getNodeCompatMode } from "./deployment-bundle/node-compat";
import { getBoundRegisteredWorkers } from "./dev-registry";
Expand Down Expand Up @@ -1214,12 +1215,13 @@ export function maskVars(

export async function getHostAndRoutes(
args:
| Pick<StartDevOptions, "host" | "routes">
| Pick<StartDevOptions, "host" | "routes" | "experimentalAssets">
| {
host?: string;
routes?: Extract<Trigger, { type: "route" }>[];
experimentalAssets?: string;
},
config: Pick<Config, "route" | "routes"> & {
config: Pick<Config, "route" | "routes" | "experimental_assets"> & {
dev: Pick<Config["dev"], "host">;
}
) {
Expand All @@ -1241,7 +1243,12 @@ export async function getHostAndRoutes(
return r.pattern;
}
});

if (routes) {
validateRoutes(
routes,
Boolean(args.experimentalAssets || config.experimental_assets)
);
}
return { host, routes };
}

Expand Down Expand Up @@ -1304,9 +1311,7 @@ export async function validateDevServerSettings(
config,
"dev"
);

const { host, routes } = await getHostAndRoutes(args, config);

// TODO: Remove this hack
// This function throws if the zone ID can't be found given the provided host and routes
// However, it's called as part of initialising a preview session, which is nested deep within
Expand Down
Loading

0 comments on commit 6523db2

Please sign in to comment.