fix(wrangler): validate routes in dev and deploy #6621

emily-shen · 2024-09-03T10:52:37Z

What this PR solves / how to test

Fixes WC-2484.
Also now errors if invalid route config is present in wrangler dev.

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because: no behaviour change

changeset-bot · 2024-09-03T10:52:41Z

🦋 Changeset detected

Latest commit: a6591e6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
wrangler	Patch
@cloudflare/vitest-pool-workers	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-09-03T10:55:14Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-wrangler-6621

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6621/npm-package-wrangler-6621

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-wrangler-6621 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-create-cloudflare-6621 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-cloudflare-kv-asset-handler-6621

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-miniflare-6621

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-cloudflare-pages-shared-6621

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-cloudflare-vitest-pool-workers-6621

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-cloudflare-workers-editor-shared-6621

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10814015217/npm-package-cloudflare-workers-shared-6621

Note that these links will no longer work once the GitHub Actions artifact expires.

wrangler@3.76.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240821.2
`workerd`	1.20240909.0	1.20240909.0
`workerd --version`	1.20240909.0	2024-09-09

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

packages/wrangler/src/deploy/deploy.ts

RamIdeas · 2024-09-04T10:43:46Z

packages/wrangler/src/deploy/deploy.ts

+			if (
+				typeof route === "object" &&
+				hasExperimentalAssets &&
+				route.pattern.includes("/") &&
+				!route.pattern.endsWith("/*")
+			) {
+				invalidRoutes[route.pattern] = [
+					...(invalidRoutes[route.pattern] ?? []),
+					`Routes with paths (except for the wildcard /*) are not allowed in Workers + Assets projects`,
+				];
+			}


Is this handling routes with a path AND ending in /* correctly? eg /some/path/* – is that allowed or not?

nope that should be allowed whoops - fixed

RamIdeas · 2024-09-04T10:44:26Z

packages/wrangler/src/deploy/deploy.ts

+			if (
+				typeof route === "string" &&
+				hasExperimentalAssets &&
+				route.includes("/") &&
+				!route.endsWith("/*")
+			) {
+				invalidRoutes[route] = [
+					...(invalidRoutes[route] ?? []),
+					`Routes with paths (except for the wildcard /*) are not allowed in Workers + Assets projects`,
+				];
+			}


This if-block is duplicated

packages/wrangler/src/__tests__/deploy.test.ts

CarmenPopoviciu · 2024-09-10T10:28:59Z

packages/wrangler/src/deploy/deploy.ts

+				(components.length === 2 && components[1] === "")
+			) {
+				invalidRoutes[pattern] ??= [];
+				invalidRoutes[pattern].push(`Update the route to end with /*`);


Having just the action stated here might be a bit too cryptic for users. Can we also tell them what routing rule they're actually breaking here? like we do in all other places.

Ah I'm not sure what the phrasing should be - i stole this one from dash. tacking copy changes/discussion onto this https://jira.cfdata.org/browse/WC-2675

Could we preserve the Workers which have static assets cannot be routed on a URL which has a path component. prefix here? The dash has a pattern of hiding the extra informational text when displaying errors (which is why it's so terse), but at the point that error is shown in dash that user should have been able to read the informational text.

Ah yes, I just commented on this above. I agree it is not ideal copy.

packages/wrangler/src/deploy/deploy.ts

packages/wrangler/src/__tests__/deploy.test.ts

packages/wrangler/src/deploy/deploy.ts

penalosa · 2024-09-10T11:12:59Z

packages/wrangler/src/deploy/deploy.ts

+				(components.length === 2 && components[1] === "")
+			) {
+				invalidRoutes[pattern] ??= [];
+				invalidRoutes[pattern].push(`Update the route to end with /*`);


Could we preserve the Workers which have static assets cannot be routed on a URL which has a path component. prefix here? The dash has a pattern of hiding the extra informational text when displaying errors (which is why it's so terse), but at the point that error is shown in dash that user should have been able to read the informational text.

petebacondarwin · 2024-09-10T11:15:49Z

packages/wrangler/src/__tests__/deploy.test.ts

+				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/:
+				Update the route to end with /*


I'm slightly worried about this error message. In non-assets Workers this is a valid route, right?
So the message should be similar to those above, no?
Currently I would be a bit confused to see this message.

petebacondarwin · 2024-09-10T11:17:32Z

packages/wrangler/src/deploy/deploy.ts

+				(components.length === 2 && components[1] === "")
+			) {
+				invalidRoutes[pattern] ??= [];
+				invalidRoutes[pattern].push(`Update the route to end with /*`);


Ah yes, I just commented on this above. I agree it is not ideal copy.

RamIdeas · 2024-09-10T12:23:38Z

packages/wrangler/src/deploy/deploy.ts

+			if (
+				components.length === 1 ||
+				(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 /*`
+				);
+			} 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 /*`
+				);
+			}


I'm not entirely sure what this is meant to do. There is a magic number 2 used. Did you intend to only consider patterns with 0 or 1 /s (hence components.length === 1 or 2)? What if there are more segments? eg /multi/segment/path/*

sorry this is slightly convoluted to avoid my mortal enemy, regex.
its 2 because the only valid route is something like [route.com, *] - if its not that it hits that error.

We agreed on a call to keep the logic as-is but add comments to each condition

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/\*"

* main: Add pipeline binding to wrangler.toml (#6674) Fix Pages duplicating hash in redirects (#6680) Bradley/r2 event notification get (#6652) feat(wrangler): Add support for placement hints (#6625) fix(wrangler): Validate `routes` for Workers with assets (#6621) chore(deps): bump the workerd-and-workers-types group across 1 directory with 2 updates (#6673) chore(workers-shared): Configure GitHub Actions to deploy Asset Worker (#6542) feat: experimental workers assets can be ignored by adding a .assetsignore file (#6640)

emily-shen requested a review from a team as a code owner September 3, 2024 10:52

emily-shen added the e2e Run e2e tests on a PR label Sep 3, 2024

emily-shen force-pushed the emily/assets-route-error branch from abbe295 to cb7c7ab Compare September 3, 2024 11:01

RamIdeas reviewed Sep 4, 2024

View reviewed changes

packages/wrangler/src/deploy/deploy.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Sep 4, 2024

View reviewed changes

penalosa reviewed Sep 4, 2024

View reviewed changes

packages/wrangler/src/__tests__/deploy.test.ts Show resolved Hide resolved

packages/wrangler/src/__tests__/deploy.test.ts Show resolved Hide resolved

emily-shen requested review from RamIdeas and penalosa September 9, 2024 15:46

CarmenPopoviciu reviewed Sep 10, 2024

View reviewed changes

packages/wrangler/src/deploy/deploy.ts Show resolved Hide resolved

CarmenPopoviciu reviewed Sep 10, 2024

View reviewed changes

packages/wrangler/src/deploy/deploy.ts Show resolved Hide resolved

CarmenPopoviciu reviewed Sep 10, 2024

View reviewed changes

packages/wrangler/src/__tests__/deploy.test.ts Show resolved Hide resolved

penalosa approved these changes Sep 10, 2024

View reviewed changes

petebacondarwin approved these changes Sep 10, 2024

View reviewed changes

RamIdeas reviewed Sep 10, 2024

View reviewed changes

RamIdeas approved these changes Sep 10, 2024

View reviewed changes

CarmenPopoviciu force-pushed the emily/assets-route-error branch from 20ca75f to 4c05817 Compare September 11, 2024 10:29

petebacondarwin approved these changes Sep 11, 2024

View reviewed changes

RamIdeas approved these changes Sep 11, 2024

View reviewed changes

CarmenPopoviciu force-pushed the emily/assets-route-error branch 3 times, most recently from 8ee1fb1 to a6591e6 Compare September 11, 2024 14:43

CarmenPopoviciu merged commit 6523db2 into main Sep 11, 2024
37 of 42 checks passed

CarmenPopoviciu deleted the emily/assets-route-error branch September 11, 2024 15:39

workers-devprod mentioned this pull request Sep 11, 2024

Version Packages #6669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(wrangler): validate routes in dev and deploy #6621

fix(wrangler): validate routes in dev and deploy #6621

emily-shen commented Sep 3, 2024 •

edited by CarmenPopoviciu

Loading

changeset-bot bot commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading

RamIdeas Sep 4, 2024

emily-shen Sep 9, 2024

RamIdeas Sep 4, 2024

CarmenPopoviciu Sep 10, 2024

emily-shen Sep 10, 2024

penalosa Sep 10, 2024

petebacondarwin Sep 10, 2024

penalosa Sep 10, 2024

petebacondarwin Sep 10, 2024

petebacondarwin Sep 10, 2024

RamIdeas Sep 10, 2024

emily-shen Sep 10, 2024

RamIdeas Sep 10, 2024

fix(wrangler): validate routes in dev and deploy #6621

fix(wrangler): validate routes in dev and deploy #6621

Conversation

emily-shen commented Sep 3, 2024 • edited by CarmenPopoviciu Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Sep 3, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emily-shen commented Sep 3, 2024 •

edited by CarmenPopoviciu

Loading

changeset-bot bot commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading