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

spec should be more explicit about accessing internal body on opaque Responses #710

Closed
wanderview opened this Issue Jun 11, 2015 · 34 comments

Comments

Projects
None yet
6 participants
@wanderview
Copy link
Member

wanderview commented Jun 11, 2015

From reading the non-normative text it appears that respondWith() and Cache.put() should process the internal body of an opaque response. This is confusing from the normative steps, however, since they only refer to the "body" which fetch defines as null for opaque responses.

It would be nice to explicitly reference the internal body in these cases in the normative text.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jun 11, 2015

It's a bit more complicated. Since if respondWith() is passed an opaque response, the fact that it is opaque needs to propagate to the original caller (e.g. <img>).

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Jun 11, 2015

I guess the part about handling the respondWith() body really needs to go in the html spec when it integrates with fetch.

And I guess SW spec implicitly handles this for Cache by just kind of keeping them in a conceptual list.

Maybe this can be closed?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 15, 2015

I think you're correct that some aspects of respondWith() need to be clearer about response vs response's internal response.

@jakearchibald jakearchibald added this to the Version 1 milestone Oct 28, 2015

@jungkees jungkees added the decided label Jan 7, 2016

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Mar 9, 2016

Replaced "used flag" text with "disturbed" and made the input response's body stream of put(request, response) and respondWith(r) consumed: ccff1f1 (The body streams stored in Cache and returned to Fetch will not be int disturbed state of course.)

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Mar 10, 2016

  • Used "internal response" to set the body to null for a HEAD request in matchAll(request, options)
  • Set the Response's Headers' guard to "immutable" in addAll(requests) and to the input Response's guard value in put(request, response)

I believe more filtered response bits for foreign fetch are being followed up in #841 and we'll continue to discuss there.

Closing.

@jungkees jungkees closed this Mar 10, 2016

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 10, 2016

Wait wait wait. You allow modification of opaque responses? I don't think that's a good idea. HEAD to an opaque response should be a network error.

@annevk annevk reopened this Mar 10, 2016

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Mar 10, 2016

Sorry to have not been specific with the description above. Setting the body to null is for the case where matchAll() steps run with request's method HEAD and options.ignoreMethod set to false. I didn't change the original behavior but just introduced "internal response" to the text.

We're not mutating the state of the stored response, but just returning a copy of the response after removing the body as it's a HEAD request.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 10, 2016

Yes and I'm saying that should not be possible for opaque responses. You don't "just" change opaque responses.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 10, 2016

Pretty sure this behavior has been in the spec for a long time, @annevk. We definitely honor it in gecko:

https://dxr.mozilla.org/mozilla-central/source/dom/cache/Manager.cpp#501

The rational is if you do a HEAD request to a cross-origin server without CORS, you still get a response without a body. The Cache API should behave the same.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 10, 2016

Yes, but you cannot do a "no-cors" HEAD. Allowing that, even just for the cache, seems like a same-origin policy violation.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 10, 2016

I think this is probably more an issue with Cache API spec not reflecting that Responses are serialized and deserialized. Its really creating a new Response on each .match(). If its a HEAD, its creating a new Response without a body. That seems reasonable and safe for opaque responses to me. (And its what we currently do in the impl.)

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 10, 2016

@wanderview see tc39/proposal-cancelable-promises#4 (comment) for some of the concerns with altering opaque responses. They don't necessarily apply here, but I'm still deeply skeptical of allowing such things. E.g., for one, you shouldn't be able to tell if an opaque response has a body or not.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 10, 2016

@annevk, so you think these should have different behavior?

Where a fetch event is generated from an element:

  evt.respondWith(fetch('http://otherorigin.com/cats.jpg', { mode: 'no-cors', method: 'HEAD' }));

Vs:

  // cache populated with a GET providing the body
  evt.respondWith(cache.match(new Request('http://otherorigin.com/cats.jpg',
                                                  { mode: 'no-cors', method: 'HEAD' })));

You are saying that code snippet 1 should NOT show the image in the element, but snippet 2 should show the image?

Per the current spec language both of these snippets result in the same behavior. The image is not shown because the Response was created via a HEAD Request.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 10, 2016

I'm worried about changing opaque responses. I'd appreciate input from security folks since this changes the same-origin policy. I would probably throw for the latter case as I suggested earlier.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 10, 2016

We would need @jakearchibald's input to change this as well, I think.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 10, 2016

In any case, this should probably be a separate issue. @jungkees did not add this behavior to the spec in this commit. It has been there for a long time.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 10, 2016

I'm worried about changing opaque responses. I'd appreciate input from security folks since this changes the same-origin policy. I would probably throw for the latter case as I suggested earlier.

Also, I really don't understand the security concerns. We're already hiding opaque response bodies from script. All removing the body on HEAD does it further hide the body from the browser. Removing information seems generally safe.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 10, 2016

I just pointed to a thread that explained how it'd be unsafe if you remove half of the body.

@jungkees

This comment has been minimized.

Copy link
Collaborator

jungkees commented Mar 11, 2016

To me, @wanderview's snippet 1 and snippet 2 having the same behavior makes sense. Setting the response's body to null for a HEAD request in matchAll() here seems nothing but the same as what main fetch step 14 does for a corresponding HEAD request to the network.

it'd be unsafe if you remove half of the body.

I'm not sure I understand this from the context of our discussion here. Can you share a pointer?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 11, 2016

See #710 (comment) for the pointer.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Mar 11, 2016

I just pointed to a thread that explained how it'd be unsafe if you remove half of the body.

This is not removing half the body. Its removing the full body just like the network stack has always done for HEAD requests.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Mar 11, 2016

Yes, but you stated "Removing information seems generally safe." which I had just shown to be untrue. And there's a significant difference between the server not including the body based on a HEAD request and the client modifying a response it's not supposed to be able to alter.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Apr 11, 2016

F2F: we should do whatever fetch does here. If fetch disallows no-cors HEAD requests, cache.match shouldn't generate them from opaque responses.

At a quick glance, it appears that fetch does allow it, though. @wanderview is writing an issue.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Apr 14, 2016

I don't understand why the F2F considered network behavior identical to the user agent modifying an opaque response.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Apr 14, 2016

Its not arbitrary modification. Its treating the response has having no body. That is exactly what the browser already does if it gets a HEAD response with a body.

@wanderview

This comment has been minimized.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Apr 14, 2016

Sure, but there might be different HTTP headers in play.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Apr 14, 2016

Not sure what other browsers do, but in gecko our http cache will only match a HEAD request if the entry in the cache is also a HEAD. Is that what other browsers do?

@aliams

This comment has been minimized.

Copy link
Contributor

aliams commented Jul 26, 2016

I do not believe that HTTP caching is supported at all for a HEAD request in Edge.

@jakearchibald

This comment has been minimized.

Copy link
Collaborator

jakearchibald commented Jul 28, 2016

F2F:

  • Method is a key in HTTP cache matching, so we should do the same for the cache API
  • So with cache.match(request) where request is a HEAD, return nothing
  • This is a breaking change, but we don't expect any issues here
@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Aug 1, 2016

@inexorabletash

This comment has been minimized.

@jungkees jungkees closed this in a3f9db8 Aug 3, 2016

dstockwell pushed a commit to dstockwell/chromium that referenced this issue Aug 10, 2016

Cache API should not match() HEAD requests
Per spec issue, the match()/matchAll() methods should not match HEAD
requests, yielding undefined (or an empty result set).

[1] w3c/ServiceWorker#710 (comment)

BUG=633358

Review-Url: https://codereview.chromium.org/2204683002
Cr-Commit-Position: refs/heads/master@{#411172}
@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 14, 2016

Looks like blink fixed this in August. We are working on our fix now.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented Dec 21, 2016

This fix will ship in FF53.

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