1143155 - Filtered response should allow access to underlying unfiltered headers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Since SW interception happens lower in our network stack than CORS handling it is important to allow access to unfiltered headers internally. For example, if a SW intercepts CORS requests and replies transparently

onfetch = function(e) { e.respondWith(fetch(e.request)); }

fetch() will return a Response which is CORS filtered and does not have access-control-allow-origin and other headers. But the intercepted channel needs these headers since there are other cors listeners 'upstream' which will fail the request if they can't observe the headers.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: bz://1143155/nsm

/r/5405 - Bug 1143155 - Filtered response stores internal response and allows access to headers

Pull down this commit:

hg pull review -r 26caee261cc759d54634915cd8e1b36592ad4045

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

FYI relevant spec section https://fetch.spec.whatwg.org/#concept-filtered-response

Comment 3

[Josh Matthews [:jdm]]

I'm not sure my review here is very meaningful, as I've never looked at most of the code being touched.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8577481 [details]
MozReview Request: bz://1143155/nsm

https://reviewboard.mozilla.org/r/5403/#review4371

I don't think this handles `Clone()` correctly.  Right now you will clone the wrapper filtered response, but the underlying mWrappedResponse is not cloned.  Does this pass our currentl clone() tests?  I would expect it to fail if we are actually generating filtered responses from fetch().

::: dom/fetch/InternalResponse.h
(Diff revision 1)
> +      return mWrappedResponse->GetBody(aStream);

`MOZ_ASSERT(!mBody);`

::: dom/fetch/InternalResponse.cpp
(Diff revision 1)
>    if (!mBody) {

You need to check for mWrappedResponse here and clone the wrapped response as well.

::: dom/fetch/InternalResponse.h
(Diff revision 1)
> +  UnfilteredHeaders()

Should we expose this as a [ChromeOnly] webidl method so we can verify the unfiltered headers in a mochitest?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Also, I think we need test coverage for wrapped and non-wrapped cases.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8577481 [details]
MozReview Request: bz://1143155/nsm

/r/5405 - Bug 1143155 - Filtered response stores internal response and allows access to headers

Pull down this commit:

hg pull review -r 2757349e508b89e4af2a9a1837a10d669c8de6c9

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Ben, it is not possible to test for the wrapped headers  (via [ChromeOnly]) through JS when the worker intercepts it because the Necko intermediate code will swallow the InternalResponse and put the properties on the channel, where stuff like nsCORSListenerProxy will remove the headers. The closest we can get to is https://bugzilla.mozilla.org/show_bug.cgi?id=1143981 since our fetch() tests already check response.type and ensure the body is hidden and headers match and so on.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8577481 [details]
MozReview Request: bz://1143155/nsm

https://reviewboard.mozilla.org/r/5403/#review4535

Sorry, before r+'ing, can you clarify all the extract changes regarding mSecurityInfo. etc?

::: dom/fetch/InternalResponse.h
(Diff revisions 1 - 2)
> +    response->mSecurityInfo = mSecurityInfo;

Why did all this stuff change in the patch?  Is it a rebase?  The mozreview interface does not make it obvious...

::: dom/fetch/InternalResponse.cpp
(Diff revisions 1 - 2)
> -InternalResponse::InternalResponse(const InternalResponse& aOther)

If you want to disallow copying, I think you should add this to your header:

```
InternalResponse(const InternalResponse&) = delete;
InternalResponse&
operator=(const InternalResponse&) = delete;
```

::: dom/fetch/InternalResponse.h
(Diff revisions 1 - 2)
> -    if (mWrappedResponse) {
> +    if (mWrappedResponse && (Type() != ResponseType::Opaque)) {

I think this would be easier to understand as:

```
if (Type() == ResponseType::Opaque) {
  return nullptr;
}
```

::: dom/fetch/InternalResponse.cpp
(Diff revisions 1 - 2)
> +    clone->mWrappedResponse = mWrappedResponse->Clone();

I think it would be clearer to do this:
```
MOZ_ASSERT(!mBody);
return clone.forget();
```

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8577481 [details]
MozReview Request: bz://1143155/nsm

/r/5405 - Bug 1143155 - Filtered response stores internal response and allows access to headers

Pull down this commit:

hg pull review -r 3581bdf02cd5f51be7beef5fedfb7a6dfd4ab92d

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8577481 [details]
MozReview Request: bz://1143155/nsm

https://reviewboard.mozilla.org/r/5403/#review4547

r=me with the !mBody check fixed in the Clone method.

::: dom/fetch/InternalResponse.cpp
(Diff revisions 2 - 3)
> -  if (!mBody) {

You still need the !mBody check for the case where its a non-wrapped Response, but it just doesn't have a body.

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8577481 [details]
MozReview Request: bz://1143155/nsm

/r/5405 - Bug 1143155 - Filtered response stores internal response and allows access to headers

Pull down this commit:

hg pull review -r 8168e08463535783a46ceb931d9033481f6a580b

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8577481 [details]
MozReview Request: bz://1143155/nsm

/r/5405 - Bug 1143155 - Filtered response stores internal response and allows access to headers

Pull down this commit:

hg pull review -r 8168e08463535783a46ceb931d9033481f6a580b

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fb84103d54c

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fb84103d54c

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: MozReview Request: Bug 1143155 - Filtered response stores internal response and allows access to headers