workers assets shared routing cleanup - #14878
Conversation
🦋 Changeset detectedLatest commit: 107ab47 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@cloudflare/autoconfig
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
2edf178 to
3f1b4c8
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
marked as ready to get AI feedback, will fix any feedback and finalize afterwards |
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| "@cloudflare/workers-shared": minor | |||
There was a problem hiding this comment.
I think technically should be patch
There was a problem hiding this comment.
the robot told me a telemetry change should be minor
but I'll do whatever the wrangler team wants here
| }; | ||
| } | ||
|
|
||
| // Avoid decoding and matching work for paths that cannot change. |
There was a problem hiding this comment.
super nit: your comment actually is actually indicating the reverse of what this function does. The use of this does help us avoid work, but maybe this comment needs to move, or verbiage be flipped 😅
There was a problem hiding this comment.
haha good call, this function was removed due to the bug you called out in the other comment
| }; | ||
|
|
||
| // Returns both path representations so routing cannot accidentally reuse lookup input. | ||
| export function canonicalizePath(pathname: string): CanonicalPath { |
There was a problem hiding this comment.
This reencodes each path segment, but the normalization bits and fast check only recognize % and //
For instance /docs+draft would canonicalize to /docs%2Bdraft, but the fast check returns false and the normalization mask would remain None. A literal /docs%2Bdraft rule would change decision and we wouldn't see any telemetry telemetry
There was a problem hiding this comment.
ah good catch!
to avoid this kind of pitfall, I removed the "fast path" skip thing and decided to be more comprehensive and always run the comparison.
| None = 0, | ||
| Decoded = 1 << 0, | ||
| CollapsedSlashes = 1 << 1, | ||
| MalformedEncoding = 1 << 2, |
There was a problem hiding this comment.
We should also add a test for this case
3f1b4c8 to
107ab47
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| function encodePath(pathname: string): string { | ||
| return pathname | ||
| .split("/") | ||
| .map((segment) => encodeURIComponent(segment)) | ||
| .join("/"); | ||
| } |
There was a problem hiding this comment.
🟡 Unusual web addresses can make a request error out instead of being served
When re-encoding a request path, each segment is passed to encodeURIComponent (packages/workers-shared/asset-worker/src/utils/canonical-path.ts:63) with no error guard, so a path that decodes to an unpaired surrogate makes the shadow-mode path processing throw instead of just recording telemetry.
Impact: For such rare malformed paths the request fails with a server error rather than being handled normally, contradicting the "customer request handling is unchanged" intent.
Why the throw propagates to the request
encodeURIComponent throws a URIError on lone surrogates. The equivalent encodePath in packages/workers-shared/asset-worker/src/handler.ts:1018-1030 wraps encodeURIComponent in a try/catch for exactly this reason, but the new canonicalizePath helper does not. canonicalizePath is called from recordCanonicalPathRuleDifferences (packages/workers-shared/asset-worker/src/handler.ts:1144-1145) and from the router's static-routing block (packages/workers-shared/router-worker/src/worker.ts:286), neither of which is wrapped in try/catch, so a throw here surfaces as a request error even though this code is meant to be shadow-only.
Note: reachability is limited because URL parsing normally prevents lone surrogates from appearing in url.pathname, so this is a defensive-robustness regression relative to the code it mirrors.
| function encodePath(pathname: string): string { | |
| return pathname | |
| .split("/") | |
| .map((segment) => encodeURIComponent(segment)) | |
| .join("/"); | |
| } | |
| function encodePath(pathname: string): string { | |
| return pathname | |
| .split("/") | |
| .map((segment) => { | |
| try { | |
| return encodeURIComponent(segment); | |
| } catch { | |
| return segment; | |
| } | |
| }) | |
| .join("/"); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
107ab47 to
406bd40
Compare
3 participants
Fixes #[insert GH or internal issue link(s)].
Please see ticket above.
This change centralizes some of the URL handling for Workers Assets and Pages and begins recording telemetry to detect any impact that would be caused by updating all code to use the canonical URL handling.
This is telemetry-only for now.
A picture of a cute animal (not mandatory, but encouraged)