Fetch: referrer same-origin constraint is a footgun for people trying to "copy" a Request

[wanderview]

Recently I saw a website in the wild attempting to do this:

var request(newURL, {
  referrer: oldRequest.referrer,
  // copy other attributes as well
});

This will work just fine during development on localhost, because .referrer will most likely always be same-origin. When the site is posted on twitter, for example, it will be visited through a t.co redirector. This results in a t.co referrer which is cross-origin.

So the site that worked fine in local development will blow up when its published to twitter. This seems like a bit of a footgun.

We could make new Request() silently ignore the value if its invalid instead of throwing. This is somewhat similar to using bad header values. They just get ignored.

[wanderview]

Note, this is really problem anywhere the attribute has stronger setter constraints compared to what the getter can return. The 'navigate' mode has a similar problem.

[annevk]

Bad header values in CORS result in a network error. Forbidden headers throw in the Headers classd (and invalid values are accepted without question, not ignored).

I wonder what @jakearchibald and @domenic think about this.

(Note that saying this is about setter/getter is confusing, since these attributes only have a getter. You can influence what they return through the constructor, but there's a number of reasons why you cannot create all the kinds of objects a user agent can.)

[wanderview]

Bad header values in CORS result in a network error. Forbidden headers throw in the Headers classd (and invalid values are accepted without question, not ignored).

This confuses me. Steps 4 to 6 of Headers append algorithm are:

4) Otherwise, if guard is "request" and name is a forbidden header name, return.
5) Otherwise, if guard is "request-no-cors" and name/value is not a simple header, return.
6) Otherwise, if guard is "response" and name is a forbidden response-header name, return.

None of those throw. We only throw for immutable headers or if the name/value contain illegal characters.

[annevk]

Okay, I guess I'm okay with silently ignoring certain values and maybe logging something to the console?

[domenic]

I wonder what @jakearchibald and @domenic think about this.

I am confused why people are trying to copy requests in this way instead of doing new Request(oldRequest, { propToOverride: newValue })?

[wanderview]

It doesn't let you change the URL of the request. In this case they wanted to sanitize the search string in the URL.

[annevk]

@wanderview for that scenario a synthetic redirect might be better?

[wanderview]

In this case they wanted to return the normal fetch'd response, but use a sanitized URL when saving in the cache. This particular problem would get easier if chrome implemented ignoreSearch.

Regardless, I have seen this pattern in the wild many times now.

[annevk]

What pattern? Workarounds for ignoreSearch?

[wanderview]

The pattern of wanting to copy all evt.request values, but use a different url.

Anyway, I'm tired of arguing this issue. Its a common mistake I see. We can try to put some seat belts on the API here or not. I'm fine either way.

[annevk]

I'm not trying to argue, I'm just trying to figure out what the problem is. I don't have the same context available as you, e.g., I have only seen the code you pasted in OP.

If it's common for folks to want to change the URL of a request, but nothing else, perhaps that's a utility we should introduce?

[wanderview]

If it's common for folks to want to change the URL of a request, but nothing else, perhaps that's a utility we should introduce?

I think something like that would help devs.

[domenic]

Naive question, why not new Request(oldRequest, { url: newURL })?

[annevk]

@domenic we could do that. Though new Request(url, { url: otherURL }) would be weird.

[annevk]

Note that if that's the feature this is a duplicate of #191 where I did point out that new Request(newURL, oldRequest) should largely work too (except that body is missing at the moment). So maybe we just need to wait a little longer.

[wanderview]

Note that if that's the feature this is a duplicate of #191 where I did point out that new Request(newURL, oldRequest) should largely work too (except that body is missing at the moment). So maybe we just need to wait a little longer.

Right, the issue is that new Request(newURL, oldRequest) will blow up given certain values contained in oldRequest. Since its treated like a RequestInit a cross-origin referrer will throw. A navigate mode will throw.

[wanderview]

Another case where someone is being caught out by this "copy a Request" footgun:

https://bugs.chromium.org/p/chromium/issues/detail?id=573937#c15

[annevk]

I tried to figure out what would have to silent fail:

• Overwriting a request whose mode is "navigate"
• Cross-origin referrer
• Using mode "navigate" as input (if you copy the mode)
• Overwriting method, integrity metadata, and headers for a request whose mode is "no-cors"
• ?

Not entirely sure which of those would be typically hit, but it's less than I expected so maybe this is worth doing. Any new thoughts @wanderview?

[wanderview]

Another variant of the "it's hard to tweak requests" issue:

https://mobile.twitter.com/RReverser/status/751837867350122497

[annevk]

Right, that is bullet point 3 above.

[jakearchibald]

I can't remember why creating a Request with mode navigate isn't allowed.

[annevk]

Because it's tricky to allow while not opening up new security holes.

[jakearchibald]

F2F:

• We could add url to requestInit, allowing new Request(oldRequest, {url})
• We still have the problem of throwing if oldRequest has mode navigate
• So... If RequestInfo's mode is navigate, change it to cors
• Then take requestInit's value if it has one
• _Then_ we can throw if the mode is navigate

[wanderview]

@annevk What do you think?

[annevk]

I don't like adding url to RequestInit (folks will then want to pass it as the first argument which will be really confusing and I'm not even sure if that's feasible). I'm also not a big fan of changing modes without there being a request to do so.

I think I still prefer silent failure (so certain things we just drop rather than throw for).

[TimvdLippe]

FWIW I am copying requests in the fetch event, because I need to insert an ETag header (retrieving the original response from the service-worker cache). The following function does this, but imo is really bloatcode. Would this fall into the same category as the issue described in OP? Would be nice if there is an easier and shorter solution.

function insertETagFromResponse(request, response) {
  var headers = new Headers();
  // Copy all original headers
  for (var header in request.headers.keys()) {
    var value = request.headers[header];
    if (value) {
      headers.append(header, value);
    }
  }
  // Add previous ETag value to receive 304 response.
  headers.append('If-None-Match', response.headers.get('ETag'));
  return new Request(request.url, {
    method: 'GET',
    headers: headers,
    body: request.body,
    mode: request.mode === 'navigate' ? 'cors' : request.mode,
    credentials: request.credentials,
    cache: request.cache,
    redirect: request.redirect,
    referrer: request.referrer,
    integrity: request.integrity
  });
}

[annevk]

Also, how would changing navigate to cors even work? Wouldn't that result in failure a lot for cross-origin cases?

[RReverser]

Also, how would changing navigate to cors even work? Wouldn't that result in failure a lot for cross-origin cases?

As I was told, it shouldn't as long as you're not trying to read the response? (e.g. just want to put into the cache or serve back to the browser)

[RReverser]

I'm also not a big fan of changing modes without there being a request to do so.

The issue is exactly that it's not easy to even request changing the mode, because error throws earlier than request is merged with RequestInit. So the only way is what @TimvdLippe showed above, and that doesn't even cover request bodies. What we want here is simply a way to modify headers etc. without cloning entire request field by field (so risk levels remain the same as they're now).

[ithinkihaveacat]

I've also written code very much like @TimvdLippe's also to transform the headers of a request, and it doesn't feel great. It would be wonderful if there were an easier way less fraught with worries about edge cases. (The edge cases are also difficult to test for.)

It's also unfortunate that modifying the headers of a _response_ needs to be done in a slightly different way.

[annevk]

As I was told, it shouldn't as long as you're not trying to read the response?

That is wrong.

I was kinda hoping for folks to write libraries to solve this kind of thing and we'd pick the best design later on to ship in browsers, but maybe this is a library that we should design. Anyone interested in working on this?

[RReverser]

@annevk Can we just move RequestInit merging before checking the mode? In that case, it would be possible to at least do new Request(oldRequest, { mode: 'cors' }).

[RReverser]

Alternatively, we could set mode conditionally to either no-cors or cors based on the origin (I'd really love browser to do this automatically so that we wouldn't need to even think about it in each SW).

[annevk]

@RReverser no-cors and cors have vastly different cross-origin semantics (same-origin is mostly similar). You need out-of-band-data to pick one.

Allowing mode to be overridden might be feasible if I stare at it long enough and don't forget anything...

[RReverser]

@annevk The only concern with overriding might be that it will override even modes that were initially no-cors, to cors, and that will break things too (or will leave us with at least ternary operator for comparison against navigate).

[RReverser]

Btw, what is the reason headers on the request are non-writable? Is it to prevent accidental modification of headers after the Request was sent or..?

[wanderview]

Also, how would changing navigate to cors even work? Wouldn't that result in failure a lot for cross-origin cases?

Navigate is by definition same-origin to the service worker. So cors mode should work. We could also change it to same-origin instead.

[wanderview]

FWIW I am copying requests in the fetch event, because I need to insert an ETag header (retrieving the original response from the service-worker cache).

With the changes proposed here you could use the more convenient Request constructor that takes the old request as the first argument. Something like:

  var headers = new Headers();
  // Copy all original headers
  for (var header in request.headers.keys()) {
    var value = request.headers[header];
    if (value) {
      headers.append(header, value);
    }
  }
  // Add previous ETag value to receive 304 response.
  headers.append('If-None-Match', response.headers.get('ETag'));

  return new Request(request, {    headers: headers,  });

This will copy all values that are safe to from the old request to the new request. If you have a navigate mode then it will be translated to cors (or maybe same-origin). The referrer will be changed to your current script.

[annevk]

Navigate is by definition same-origin to the service worker.

The reasons for which we added "navigate" in #126 seem mostly obsolete, perhaps that should be reconsidered too, although I guess it might be a useful value to branch on now that we have it. Hmm.

Still not really sure what to do here.

Btw, what is the reason headers on the request are non-writable?

The main reason was to keep things simple in v1 since there's quite a number of policy combinations. It would also be a little weird to make that the only mutable field.

[annevk]

Navigate is by definition same-origin to the service worker. So cors mode should work.

This is not true if you get a redirect that goes cross-origin, although I guess as long as the redirect mode stays manual and you only feed the response to navigate requests it should go okay.

[TimvdLippe]

Ah this is definetely an improvement. It would be even better if the
headers object is writable or even clonable with overrides, the same as
this proposal.

On Fri, 5 Aug 2016, 10:10 Ben Kelly, [email protected] wrote:

FWIW I am copying requests in the fetch event, because I need to insert an
ETag header (retrieving the original response from the service-worker
cache).

With the changes proposed here you could use the more convenient Request
constructor that takes the old request as the first argument. Something
like:

var headers = new Headers();
// Copy all original headers
for (var header in request.headers.keys()) {
var value = request.headers[header];
if (value) {
headers.append(header, value);
}
}
// Add previous ETag value to receive 304 response.
headers.append('If-None-Match', response.headers.get('ETag'));

return new Request(request, { headers: headers, });

This will copy all values that are safe to from the old request to the new
request. If you have a navigate mode then it will be translated to cors
(or maybe same-origin). The referrer will be changed to your current
script.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/whatwg/fetch/issues/245#issuecomment-237860530, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AFrDb3TvB9-hgOA8qFqlUwj-4X4NpI5vks5qc0RogaJpZM4HvAVb
.

[RReverser]

The more I try to write such request cloning with adding request headers, the more obvious it becomes how error-prone manual cloning with listing all the Request field is as it's evolving and might eventually become out of sync resulting in imperfectly cloned requests. I don't consider Object.keys + forEach as nice & safe alternative either as there are special handlings on some of them like mode and referrer mentioned above, and there might be more in future.

Given how error-prone this is, it would be really nice not to leave this up to SW or library implementors to figure out.

The reasons for which we added "navigate" in #126 seem mostly obsolete, perhaps that should be reconsidered too, although I guess it might be a useful value to branch on now that we have it. Hmm.

Do you think it would be feasible to change it to same-origin or cors then? This looks like it would avoid issue altogether.

[annevk]

Yeah, I'll try to create a PR for that as well as no longer throwing for a cross-origin referrer (we'll just silently make it about:client).

[RReverser]

@annevk Thank you!

we'll just silently make it about:client

As for this - wouldn't it be possible to preserve referrer but not allow JS to read it for such cases? Or the security concern is different?

[annevk]

The security concern is that you can modify a request while blaming someone else for it.

[RReverser]

@annevk Hm, true. On the other hand, I'm curious if this would break Google Analytics and other scripts that track referrers on the page with SW installed.

[annevk]

If you modify requests, they will definitely be affected in all kinds of ways. If you forward them, it should mostly be okay.

[annevk]

Note that I'm only talking about changing something that currently throws, so anything affected would already be affected.

[RReverser]

so anything affected would already be affected

Sure, just trying to think of the possible consequences. Generally, we do want to add some headers to all requests passing via SW, but this falls under "modification" (right?) and will break analytics for clients then...

[RReverser]

Although, if request cloning is fixed and we modify only headers by new Request(oldReq, { headers }) and don't touch referrer - does it still fall under such modification or referrer would be preserved in such case?

[annevk]

That's a modification.

[annevk]

That'll actually reset referrer outright, but that's also unrelated to this issue or OP.

[RReverser]

I see... Well, hopefully we can at least pass original referrer using a custom header as a workaround for analytics.

[annevk]

To be clear, you can set any same-origin referrer. And yes, you could use a custom header. Anyway, this is off-topic for this discussion, which is about making the Request constructor failing less.

[RReverser]

@annevk Well, it was slightly related to the original issue (referrer), but I agree. Thanks for the explanations!

[jdalton]

Just popping in to say I hit this issue to trying to add a cache-bust to requests because Chrome doesn't support cache modes yet. In this case though I only add the cache-bust to same-origin requests so I'm hitting the issue with something the boils down to

new Request(new Url(event.request.url), event.request)

[annevk]

Yeah, that would hit it. No need for new URL() there btw. If anyone can take a look at the PR to fix this that would be appreciated.

[jdalton]

No need for new URL() there btw.

Yep, yep. I wrote that it boiled down to something like that. In the real code the url passed through a helper to handle the cache-bust stuff.

[wanderview]

@annevk, the PR is pretty different from the f2f conclusion. I'm not sure I agree with it.

To summarize the difference, the f2f decision wanted to allow this:

var modified = new Request(originalRequest, { url: newURL });

But you seem to want to require this:

var modified = new Request(newURL, originalRequest);

Why must a new URL require the same semantics as a brand new Request? Why can't an existing Request have its URL modified?

For example, our proposed URL property on init would allow a cache busting param to be added to an existing Request without disturbing the Request window value. This would let things like basic auth, etc, continue to work. AFAICT from the spec if window is set in init, then a TypeError is thrown (step 10, although it makes little sense with step 11.)

Personally I would like to see us provide an "override the URL on an existing Request" operation since that is what we have seen developers need to do. I don't particularly think the current PR provides this.

[annevk]

https://github.com/whatwg/fetch/issues/245#issuecomment-236268491 mentions a number of things. This does not attempt to address all of them. Overriding the url can be considered separately. Not entirely sure what the implications of that would be.

[annevk]

Also, looking at my response to that comment you could have indicated sooner you were not happy with that 😞

[wanderview]

Sorry, I didn't see it with the other responses on the thread. I saw your responses about overriding the mode further down.

[annevk]

Personally I would like to see us provide an "override the URL on an existing Request" operation since that is what we have seen developers need to do.

I think that's somewhat more complicated as I indicated before. It also makes the API-shape very weird.

I don't particularly think the current PR provides this.

Nope, but it does solve the original problem. Unless you think that should no longer be solved?

[wanderview]

I still don't think this really addresses the use case that devs repeatedly try to perform (same request, but different URL). Since we disagree perhaps we could get @jakearchibald or others with a devs perspective to break the impasse. I'll go with whatever you collectively agree on.

[jakearchibald]

I think @annevk's PR is a step in the right direction, but agree that it's really confusing how you change URL. Some thoughts:

Allow mutations

So developers could just do request.url = newURL. Although this could means setters that throw if they're given an invalid value, meaning you'd have to apply changes in a particular order for some requests. Also, if you alter event.request but don't call respondWith, does the browser make the altered request?

An API like request.mutate(changes) would solve the ordering thing, but it's a bit weird.

Clone with changes

request.clone(changes) could clone and apply changes. We could limit the "forgiving" fallbacks to this methods, and keep new Request strict. The downside is it tees the body, meaning the developer has to cancel the original if they don't want to use it.

I'm not sure any of this is better than just adding url to RequestInit.

[annevk]

Allowing mutations is a rather big change. We could make request mutable, but then someone will have to figure out what invariants need to be preserved, etc. Also, I thought we decided at some point we liked it being immutable...

Cloning with changes has the teeing drawback.

"Just adding" url to RequestInit is not that easy as I said before.

It's still unclear to me why solving this is mutually exclusive with applying the PR.

[jakearchibald]

F2F Notes:

• Mutating is difficult, eg if you change url and mode is "navigate", we'd have to 'downgrade' mode to "same-origin" or something
• Allowing mutation on headers might be easier
• Could add a tag to particular headers saying they're "API blessed", so they shouldn't be dropped on passing to fetch

[annevk]

I think we also decided to land the PR, right?

[jakearchibald]

I certainly am. No one else objected.

[wanderview]

Just in case you're waiting for me, I defer to the TPAC decision.

[annevk]

Thanks @wanderview and @jakearchibald. I opened a new issue for the F2F notes: #391. I'll land the PR as a way of fixing the other issue raised in this bug. That still leaves URLs, for which we don't have a plan. If someone has a proposal for that a new issue would be good.

[wanderview]

The referrer changes to Request constructor should be in Firefox 54. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1298823

[pronebird]

Lengthy discussion but without any resolution. Was any of this implemented? How to clone the request changing the URL?

[annevk]

new Request(newURL, oldRequest).

[RReverser]

Actually, answering to the question above

Was any of this implemented?

Looks like this hasn't been implemented in Chrome yet.

Uncaught (in promise) TypeError: Failed to construct 'Request': Cannot construct a Request with a Request whose mode is 'navigate' and a non-empty RequestInit.

[annevk]

Right, see pointers to browser bugs in https://github.com/whatwg/fetch/pull/377#issuecomment-281287917.

[GammaGames]

It looks like they closed the issue on March 21st. I was able to do the following inside a service worker to act as a proxy for urls to forward requests to a domain

self.addEventListener('fetch', function(event) {
    var url = "https://baseserver.com";
    var req = new Request(url, event.request);
    fetch(req).then(response => {
        console.log(response);
    });
});

[GammaGames]

(except that body is missing at the moment).

Why is this? I can manually override the body and I can read the body of the original request with event.request.text().then..., but when I try to create a new request with the above method the body is an empty string.

Edit: I was able to keep the body of a post message while changing the URL with the following:

self.addEventListener('fetch', event => {
    let url = "https://baseserver.com/post";
    event.respondWith(new Promise(resolve => {
        event.request.text().then(text => {
            let newReq = new Request(url, event.request);
            resolve(fetch(new Request(newReq, {body: text})));
        });
    }));
});

It feels dirty but it works ¯\_(ツ)_/¯