New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: Easier request/response rewrites #671
Comments
There has been #245 (comment) in the past. The main problem with mutations is keeping all the security boundaries non-broken, but it seems this circumvents that largely by always starting with a fresh Request/Response object, just filled in from another one. |
Maybe instead of making the constructors support two purposes, their usual one and the copying use case of the OP, it'd be better to add a dedicated static factory method. I don't like duplicating |
I'm fine with that, but I note that the Request constructor already appears to be designed to support this use case, by passing the request as the first parameter. It just seems to be missing a couple pieces. So since that's already there, I feel like filling it out is cleaner than creating another thing.
Since headers are mutable on newly-constructed objects, I can live with telling people to modify them there. But it's a lot more than just typing a It doesn't help that when people try to "fix" their code to be consistent, it leads to weird failure modes. For example, say you have:
You decide you don't like the style mixing, so you try to rewrite it to be all-declarative:
But it turns out this completely wipes out all the existing headers and replaces them. Whoops! But it seems to work because most request headers are not essential for GETs. You just silently lose things like caching (missing Or maybe you try to go the other way -- all-imperative:
This produces no error, but the redirect mode change is silently ignored. (Well, it throws an exception in strict mode, but few people use strict mode, unfortunately.) This can be really hard for inexperienced developers to wrap their heads around. And with CF Workers, we get a lot of very inexperienced developers trying to write code. (I suspect our developers are on average much less experienced then Service Worker develpoers.) So my hope is that we can develop one common syntax for rewrites, with minimal warts, and get everyone to use that. Alternative ProposalMaybe could introduce a new
This mostly has the obivous semantics, except when it comes to the I think this keeps Thoughts? |
Indeed, I think what I'm proposing is just syntax sugar and shouldn't have security implications. That said, these kinds of security issues don't really affect CF Workers so I could be missing something. (CORS is a complete non-issue for us since a CF Worker obviously cannot hijack other origins' cookies nor reach behind-the-firewall services.)
How does this work given that I am an implementer? :) It's not necessarily important to me that browsers prioritize implementing these proposals. I just want to avoid a situation where we've implemented interfaces that browsers outright refuse to support for some reason, or a situation where browsers implement something incompatible. |
We basically want at least two browsers to also support it to ensure long term success (or at least be more certain of it). https://whatwg.org/working-mode#changes goes into it to some extent. |
Here's a developer on Stack Overflow who ran into this problem: https://stackoverflow.com/questions/51485738/how-can-i-make-a-cloudflare-worker-which-overwrites-a-response-status-code-but-p/51488911 (Probably many others have written similar code but not recognized that there's a problem, or not bothered to ask about it.) |
Speaking as a WebKit person: Request rewriting seems like a common use case for anyone using fetch with Service Workers, not just in the special CloudFlare environment. Given that, it seems worthwhile to make it easier and more convenient, if that can be done without making the API overly complex. I expect WebKit would be happy to implement a reasonable API along these lines. I don't have much of an opinion on what specific syntax to use for it. It's worth noting that sometimes APIs that have little active browser support, but no hard opposition either, sometimes still fail. Media sync API is one past example. So it would be good to get other implementers on the record. |
FWIW here's several examples of me answering StackOverflow questions where people didn't understand the current API: https://stackoverflow.com/a/55958061/2686899 https://stackoverflow.com/a/54334947/2686899 https://stackoverflow.com/a/54281243/2686899 At some point when we have time (haha), we'll probably go ahead and introduce a better API for this, likely along the lines of my "alternative proposal" in my second comment in this thread. I'd love to work with the browser vendors on this if there's interest, but we're also happy to do it as a non-standard extension if you don't feel it matters for the browser. |
thinking this would be useful also, but in my current case it was not about rewriting but instead construct and mock a response object... var res = new Response('', { url: 'https://example.com' })
console.log(res.url) // ""
res.url = 'https://example.com'
console.log(res.url) // "" Why don't this work? I basically have to redefine or use a Proxy: Object.defineProperty(res, 'url', { value: 'https://example.com' })
console.log(res.url) // "https://example.com" |
Hi all,
For those that don't know, I'm the tech lead for Cloudflare Workers, which implements the Service Workers API (but runs code on Cloudflare's servers rather than in the browser).
In general the Service Workers and Fetch APIs have been very good for us (certainly better than what we would have ended up with had we invented our own). But, based on user feedback we are finding some pain points. This is the first of a few proposals I'll be making to solve these issues.
/cc @harrishancock who does most of the API implementation work on our team.
Problem statement
Today, making minor "rewrites" to a Request or Response (e.g. starting from an existing object and modifying just one property or header) is not very ergonomic. I'm not sure if this is common in browser Service Workers, but it is probably the dominant use case for Cloudflare Workers, so we're very interested in making it better.
For example, consider the case where I want to change the hostname, change the redirect mode to "follow" (to resolve redirects server-side), and add a header. This might look like:
Notice how the best way to change each property is wildly different!
request = new Request(newUrl, request)
. The fact that this works is somewhat of a fluke: we're actually duck-typing the old request object as aRequestInit
, which happens to work because all the member names line up. It's unclear to me if the spec actually intended for this to work, but without this trick, there's no way to construct the new request without enumerating every request property individually, which is error-prone and not future-proof.redirect
in the intended way, by passing the old request as the first parameter to the new request's constructor, and passing aRequestInit
containing onlyredirect
. This is fine.headers
viaRequestInit
is inconvenient because it replaces all of the headers. Since we don't want to remove the existing headers, we'd need to make a copyHeaders
object first, modify the copy, then pass that inRequestInit
. It turns out, though, that once we've made our ownRequest
object, we can just modify itsheaders
directly. This is convenient, but weirdly inconsistent: properties likeurl
andredirect
cannot be modified post-construction.When it comes to the
Response
type, we have a bigger problem: you cannot pass an existing response object as the first parameter toResponse
's constructor, the way you can do with requests. (If you try to do so, the response object will be stringified as[object Response]
and that will become the new response's body.) So, if you want to modify the status code of a Response:This is bad, because if
Response
andResponseInit
are ever extended with a new field, that field will be inadvertently dropped during the rewrite. (We commonly see people doing Request rewrites this way, too, where it's an even bigger issue asRequestInit
has quite a few fields that tend to be forgotten.)Proposal
Let's make
Request
's constructor be the One True Way to rewrite requests. To that end:RequestInit.url
as an alternative way to specify the URL. This field would only be used when the constructor's first parameter is an existing request object, in which caseRequestInit.url
overwrites the URL. (It's important thatRequestInit.url
is ignored when the first parameter is itself a string URL. Otherwise, existing code which rewrites URLs using therequest = new Request(url, request)
idiom would break.)RequestInit.setHeaders
andRequestInit.appendHeaders
as typerecord<ByteString, ByteString>
. If specified, this is equivalent to callingrequest.headers.set()
orrequest.headers.append()
with each key/value pair after the request object is constructed.Similarly, let's fix
Response
to use the same rewrite idiom:Response
's constructor to take anotherResponse
object as the first parameter, in the same wayRequest
's constructor does today.ResponseInit.body
as an alternative way to override the body, in the specific case where the constructor's first parameter is an existingResponse
object.ResponseInit.setHeaders
andResponseInit.appendHeaders
to work the same as with requests.The text was updated successfully, but these errors were encountered: