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

[wanderview]

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]

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]

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]

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

[jungkees]

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]

• 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.

[annevk]

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.

[jungkees]

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]

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

[wanderview]

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]

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

[wanderview]

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]

@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]

@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]

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]

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

[wanderview]

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]

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]

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

[jungkees]

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]

See #710 (comment) for the pointer.

[wanderview]

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]

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]

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]

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

[wanderview]

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]

Here is where gecko ignores the body regardless of what the server sends:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#285
https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#1618

[annevk]

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

[wanderview]

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]

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

[jakearchibald]

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]

Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1290933

[inexorabletash]

Blink bug: https://bugs.chromium.org/p/chromium/issues/detail?id=633358

[wanderview]

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

[wanderview]

This fix will ship in FF53.