fix: try to serve assets from unencoded and encoded paths #6728
Conversation
|
emily-shen
force-pushed
the
emily/redirect-to-encoded
branch
from
18fecaf
to
f0266c3
Compare
|
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/10936309370/npm-package-wrangler-6728You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6728/npm-package-wrangler-6728Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-wrangler-6728 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-create-cloudflare-6728 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-cloudflare-kv-asset-handler-6728npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-miniflare-6728npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-cloudflare-pages-shared-6728npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-cloudflare-vitest-pool-workers-6728npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-cloudflare-workers-editor-shared-6728npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10936309370/npm-package-cloudflare-workers-shared-6728Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
emily-shen
force-pushed
the
emily/redirect-to-encoded
branch
from
f0266c3
to
7781371
Compare
There was a problem hiding this comment.
Love these changes. Will make it much easier for WfP/API users.
There was a problem hiding this comment.
cool. nicely broken down :)
| // exact matches should win | ||
| { | ||
| title: "/[boop] -> 200 (with /[boop].html)", | ||
| files: ["/%5Bboop%5D.html", "/[boop].html"], |
There was a problem hiding this comment.
Since we're now asking clients to always just upload unencoded, and we always serve at the encoded path, why do we end up having ambiguity in which to serve? If they upload something like /[boop].html and also /%5Bboop%5D.html, can we not just understand that the first should be served at /%5Bboop%5D and the later should be served at /%255Bboop%255D (double-encoded)?
There was a problem hiding this comment.
If only /%5Bboop%5D.html existed, would that still be served at /%255Bboop%255D only and /%5Bboop%5D should redirect? And similarly /bin%2F only at /bin%252F?
(these are niche enough that I don't have any strong opinions either way, just want to clarify expected behaviour!)
There was a problem hiding this comment.
Sorry, needed to think about this for a while. I think that's what I'd (intellectually) lean towards, yeah — if they want to serve it at some other location, they need to encode it however they want to at upload — but you make a pretty good point with that example. I'd be surprised, but being that "strict" may break the expectation of some other people/frameworks, I simply don't know for sure.
We've validated that this PR in it's current state makes Michael happy, so I'd say we should just proceed with this for now. If you have time, maybe cook up another version which does "/%5Bboop%5D.html at /%255Bboop%255D only" and we can evaluate if we want to make that change separately? I think it might allow you to simplify the asset serving logic a fair bit — it makes us more consistent and deterministic.
Interestingly, having now written a bunch of additional test cases for this PR, I've discovered this seems to only be an issue for auto-trailing-slash somehow. Can't quite wrap my brain around why that is, but yeah, that seems to be the behavior right now.
There was a problem hiding this comment.
If you have time, maybe cook up another version which does "/%5Bboop%5D.html at /%255Bboop%255D only" and we can evaluate if we want to make that change separately? I think it might allow you to simplify the asset serving logic a fair bit — it makes us more consistent and deterministic.
I actually tried this out the other day, and indeed it does make the logic a lot simpler. I've chucked that on a branch here: https://github.com/cloudflare/workers-sdk/tree/double-encode
| // exact matches should win | ||
| { | ||
| title: "/[boop] -> 200 (with /[boop].html)", | ||
| files: ["/%5Bboop%5D.html", "/[boop].html"], |
There was a problem hiding this comment.
Sorry, needed to think about this for a while. I think that's what I'd (intellectually) lean towards, yeah — if they want to serve it at some other location, they need to encode it however they want to at upload — but you make a pretty good point with that example. I'd be surprised, but being that "strict" may break the expectation of some other people/frameworks, I simply don't know for sure.
We've validated that this PR in it's current state makes Michael happy, so I'd say we should just proceed with this for now. If you have time, maybe cook up another version which does "/%5Bboop%5D.html at /%255Bboop%255D only" and we can evaluate if we want to make that change separately? I think it might allow you to simplify the asset serving logic a fair bit — it makes us more consistent and deterministic.
Interestingly, having now written a bunch of additional test cases for this PR, I've discovered this seems to only be an issue for auto-trailing-slash somehow. Can't quite wrap my brain around why that is, but yeah, that seems to be the behavior right now.
| matchedFile: "/%5Bboop%5D.html", | ||
| finalPath: "/%5Bboop%5D", | ||
| }, | ||
| // mix of encoded and unencoded paths |
There was a problem hiding this comment.
Great that we can handle this!
emily-shen
force-pushed
the
emily/redirect-to-encoded
branch
2 times, most recently
from
9ff1521
to
b31019a
Compare
Co-authored-by: Greg Brimble <[email protected]>
emily-shen
force-pushed
the
emily/redirect-to-encoded
branch
from
b31019a
to
6416648
Compare
What this PR solves / how to test
Fixes WC-2693
Author has addressed the following