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

referrer same-origin constraint is a footgun for people trying to "copy" a Request #245

Closed
wanderview opened this Issue Mar 11, 2016 · 78 comments

Comments

10 participants
@wanderview
Copy link
Member

wanderview commented Mar 11, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Mar 11, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented Mar 12, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Mar 14, 2016

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:

  1. Otherwise, if guard is "request" and name is a forbidden header name, return.
  2. Otherwise, if guard is "request-no-cors" and name/value is not a simple header, return.
  3. 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

This comment has been minimized.

Copy link
Member

annevk commented Mar 14, 2016

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

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented Mar 15, 2016

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

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented Mar 15, 2016

What pattern? Workarounds for ignoreSearch?

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

domenic commented Mar 15, 2016

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 15, 2016

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Mar 15, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Mar 18, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented May 4, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Jul 9, 2016

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

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 19, 2016

Right, that is bullet point 3 above.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 19, 2016

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 20, 2016

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

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 29, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Aug 1, 2016

@annevk What do you think?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 4, 2016

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

This comment has been minimized.

Copy link

TimvdLippe commented Aug 4, 2016

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
  });
}
@RReverser

This comment has been minimized.

Copy link
Member

RReverser commented Aug 26, 2016

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 26, 2016

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

This comment has been minimized.

Copy link
Member

RReverser commented Aug 26, 2016

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

@jdalton

This comment has been minimized.

Copy link

jdalton commented Aug 28, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented Aug 28, 2016

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

This comment has been minimized.

Copy link

jdalton commented Aug 28, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Aug 29, 2016

@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

This comment has been minimized.

Copy link
Member

annevk commented Aug 29, 2016

#245 (comment) 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

This comment has been minimized.

Copy link
Member

annevk commented Aug 29, 2016

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

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Aug 29, 2016

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 30, 2016

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

This comment has been minimized.

Copy link
Member

wanderview commented Sep 15, 2016

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

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Sep 15, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented Sep 15, 2016

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

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Sep 20, 2016

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

This comment has been minimized.

Copy link
Member

annevk commented Sep 26, 2016

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

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Sep 26, 2016

I certainly am. No one else objected.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Sep 26, 2016

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Sep 27, 2016

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.

annevk added a commit that referenced this issue Sep 27, 2016

Make Request constructor more forgiving
Folks are using the Request constructor in unexpected ways and
therefore it is throwing in unexpected ways (in particular when mode is
“navigate” or when setting referrer to a cross-origin URL). This will
make it throw less, while not really being less useful.

Fixes #245.

@annevk annevk closed this in #377 Sep 27, 2016

annevk added a commit that referenced this issue Sep 27, 2016

Make Request constructor more forgiving
Folks are using the Request constructor in unexpected ways and
therefore it is throwing in unexpected ways (in particular when mode is
“navigate” or when setting referrer to a cross-origin URL). This will
make it throw less, while not really being less useful.

Fixes #245.
@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Feb 28, 2017

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

@pronebird

This comment has been minimized.

Copy link

pronebird commented Mar 4, 2018

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

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 6, 2018

new Request(newURL, oldRequest).

@RReverser

This comment has been minimized.

Copy link
Member

RReverser commented Mar 16, 2018

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

This comment has been minimized.

Copy link
Member

annevk commented Mar 16, 2018

Right, see pointers to browser bugs in #377 (comment).

@GammaGames

This comment has been minimized.

Copy link

GammaGames commented Jun 20, 2018

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

This comment has been minimized.

Copy link

GammaGames commented Jul 12, 2018

(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 ¯\_(ツ)_/¯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment