1462006 - add webrequest redirect flags for onBeforeRedirect

Status: NEW

(WebExtensions :: Request Handling, defect, P3)

Description

[Mikhail Gusaurov]

Attachment: Reproducer browser extension

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180503143129

Steps to reproduce:

In a browser extension I have attached two listeners to browser.webRequest.onBeforeRedirect:

- onBeforeRedirect with ["responseHeaders"] option.
- onBeforeRedirect2 without ["responseHeaders"] option.

Then I navigated to "yandex.ru" from a FF with this extension loaded.


Actual results:

For the main page redirect an exception was fired and logged in extension console:

[Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://gre/modules/WebRequest.jsm :: readHeaders :: line 174"  data: no]  (unknown)
onBeforeRedirect Object { requestId: "1337", url: "https://kiks.yandex.ru/fu/", originUrl: "https://yandex.ru/", documentUrl: "https://yandex.ru/", method: "GET", type: "image", timeStamp: 1526483241609, frameId: 0, parentFrameId: -1, fromCache: false, 8 more… }  bg.js:72:5
onBeforeRedirect2 Object { requestId: "1337", url: "https://kiks.yandex.ru/fu/", originUrl: "https://yandex.ru/", documentUrl: "https://yandex.ru/", method: "GET", type: "image", timeStamp: 1526483241611, frameId: 0, parentFrameId: -1, fromCache: false, 7 more… }  bg.js:81:5

Note that

- subresource redirect was handled properly
- onBeforeRedirect2 was not triggered after the exception in the first one.



Expected results:

The listener should have been called. Below is the log for the same extension with ["responseHeaders"] option removed:

onBeforeRedirect Object { requestId: "1381", url: "http://yandex.ru/", originUrl: undefined, documentUrl: undefined, method: "GET", type: "main_frame", timeStamp: 1526483412514, frameId: 0, parentFrameId: -1, fromCache: false, 7 more… }  bg.js:72:5
onBeforeRedirect2 Object { requestId: "1381", url: "http://yandex.ru/", originUrl: undefined, documentUrl: undefined, method: "GET", type: "main_frame", timeStamp: 1526483412515, frameId: 0, parentFrameId: -1, fromCache: false, 7 more… }  bg.js:81:5
onBeforeRedirect Object { requestId: "1407", url: "https://kiks.yandex.ru/fu/", originUrl: "https://yandex.ru/", documentUrl: "https://yandex.ru/", method: "GET", type: "image", timeStamp: 1526483414381, frameId: 0, parentFrameId: -1, fromCache: false, 7 more… }  bg.js:72:5
onBeforeRedirect2 Object { requestId: "1407", url: "https://kiks.yandex.ru/fu/", originUrl: "https://yandex.ru/", documentUrl: "https://yandex.ru/", method: "GET", type: "image", timeStamp: 1526483414382, frameId: 0, parentFrameId: -1, fromCache: false, 7 more… }  bg.js:81:5

Comment 1

[Mikhail Gusaurov]

Attachment: Reproducer browser extension (no "responseHeaders")

Comment 2

[Mikhail Gusaurov]

In FF61 exception is not thrown, but the redirect handlers are not called either.

Comment 3

[Shane Caraveo (:mixedpuppy)]

I have verified this behavior.  Specifically the fetching of the response headers is resulting in NS_ERROR_NOT_AVAILABLE.  onBeforeRedirect is happening prior to a response from the server, which means that STS is doing an upgrade first.

If I create a new profile and load a tracing extension (slightly modified from attached) I see the expected results, which is 

onheadersreceived
onbeforeredirect
onheadersreceived

If I then enter "yandex.ru" in the url bar to reload, I see:

onbeforeredirect
onheadersreceived

The above illustrates that a request to "yandex.ru" with a clean profile results in an initial request/response before redirect.  STS then is primed.  The second request results in onbeforeredirect happening due to an STS upgrade before a request is made.  At that point there are no headers, and we throw an exception.

The fix here is to make use of the flags in the redirect call[1], specifically checking for nsIChannelEventSink::REDIRECT_STS_UPGRADE.  We should add an stsUpgrade boolean to the listener details, and catch the error thrown when trying to get response headers.  This way the extension can know that the redirect is an stsUpgrade.

We're also going to need to document the alternate flow on mdn[2].

[1] https://searchfox.org/mozilla-central/rev/83a923ef7a3b95a516f240a6810c20664b1e0ac9/toolkit/modules/addons/WebRequest.jsm#342
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1462006 - add redirect flags to webRequest listener details,

onBeforeRedirect happens prior to a request/response if the request is internally
upgraded. In this case response headers are not avialable and we throw an exception.
Extensions should have a way to understand the redirect event is an internal sts
upgrade and we should ignore responseHeaders during one.

Review commit: https://reviewboard.mozilla.org/r/248232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248232/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8982288 [details]
Bug 1462006 - add redirect flags to webRequest listener details,

https://reviewboard.mozilla.org/r/248232/#review254482

The change on its own looks good to me, I've added some comments about very small stuff (a typo, an additional test case if we are going to expose a new property in the details exposed to the extension code, and a proposed follow up related to check if we need to also take care of a similar scenario for other internal redirects).

I have only two doubts about this:

- the first one is about exposing the new stsUpgrade property to the extensions code, ideally I don't see anything immediately wrong on exposing it to the extensions (especially given that we are firing an event when it happens), but we may want to double-check if anyone (e.g. in our team or from security) have different opinions (and reasons) before exposing it to the extensions for the first time

- the second one is about the behavior of chrome, e.g. I'm wondering if chrome if also firing an event when the redirect related to the sts upgrade is happening (and what happens if the "responseHeader" options is specified by the listener), we may still want to apply this kind of change and keep our behavior consistent even if it is in some cases incompatible with Chrome, but given that we are looking into this it may be good to know what are the incompatibilities around this edge case (e.g. so that we can add a note about it in our docs)

::: commit-message-763f3:4
(Diff revision 1)
> +Bug 1462006 - ignore response headers during stsUpgrade, r?rpl
> +
> +onBeforeRedirect happens prior to a request/response if the request is internally
> +upgraded. In this case response headers are not avialable and we throw an exception.

Nit, avialable -> available

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_hsts.html:73
(Diff revision 1)
>        return {responseHeaders: headers};
>      }, {urls}, ["blocking", "responseHeaders"]);
>      browser.webRequest.onBeforeRedirect.addListener(details => {
> -      browser.test.assertEq(expect.shift(), "onBeforeRedirect");
> -    }, {urls});
> +      browser.test.assertEq(expect.events.shift(), "onBeforeRedirect");
> +      if (expect.stsUpgrade) {
> +        browser.test.assertEq(details.responseHeaders, undefined, "responseHeaders no present during stsUpgrade");

I see that stsUpgrade is being added to the API schema, and so if we opt for exposing the new stsUpgrade property in the details we should also add an assertion which checks it is expected presence/value based on expected.stsUpgrade.

::: toolkit/modules/addons/WebRequest.jsm:919
(Diff revision 1)
>      let channel = this.getWrapper(oldChannel);
>  
>      // We want originalURI, this will provide a moz-ext rather than jar or file
>      // uri on redirects.
>      if (this.hasRedirects) {
> -      this.runChannelListener(channel, "onRedirect", {redirectUrl: newChannel.originalURI.spec});
> +      let stsUpgrade = !!(flags & Ci.nsIChannelEventSink.REDIRECT_STS_UPGRADE);

I'm wondering if there may be other "internally generated redirects" that can also be caught by a webRequest listener and trigger an exception if responseHeaders is being used by an extension listener. 

It could be something for a follow up: double check if/when that could happen and if we need to handle it, and if we need we could add a check for nsIChannelEventSink::REDIRECT_INTERNAL here and then do something in response of that scenario (e.g. skip the responseHeaders and/or the redirect completely if the extension is not supposed to receive it)

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8982288 [details]
Bug 1462006 - add redirect flags to webRequest listener details,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248232/diff/1-2/

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Luca Greco [:rpl] from comment #5)
> Comment on attachment 8982288 [details]
> Bug 1462006 - ignore response headers during stsUpgrade,
> 
> https://reviewboard.mozilla.org/r/248232/#review254482
> 
> The change on its own looks good to me, I've added some comments about very
> small stuff (a typo, an additional test case if we are going to expose a new
> property in the details exposed to the extension code, and a proposed follow
> up related to check if we need to also take care of a similar scenario for
> other internal redirects).
> 
> I have only two doubts about this:
> 
> - the first one is about exposing the new stsUpgrade property to the
> extensions code,

This isn't an issue.  We've already done work to make it possible for extensions to force an upgrade, and we're exposing securityInfo.

> - the second one is about the behavior of chrome

This is an area where it's highly likely we're simply different.  We've long been incompatible with how, and more specifically when, the event flows happen, whenever the request steps outside the basic case.  While that gap has been reduced, we just behave different in many cases.

> I'm wondering if there may be other "internally generated redirects" that
> can also be caught by a webRequest listener and trigger an exception if

I've changed that to just catching the exception.  Also changed the stsUpgrade flag to a redirectFlags object with all available flags on it.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8982288 [details]
Bug 1462006 - add redirect flags to webRequest listener details,

Honza, can you look at what I'm doing from an api longevity perspective?

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8982288 [details]
Bug 1462006 - add redirect flags to webRequest listener details,

https://reviewboard.mozilla.org/r/248232/#review256222

please read the comments before landing.

::: toolkit/components/extensions/schemas/web_request.json:66
(Diff revision 2)
> +        "description": "Flags indicating attributes of the redirected requeset.",
> +        "properties": {
> +          "internal": {
> +            "type": "boolean",
> +            "optional": true,
> +            "description": "The redirect is an internal redirect that may happen for various reasons, such as redirecting the request to a proxy."

the description is inaccurate.  internal redirects are various fail over retries to the same URL (I don't know about an internal redir to a different URL..) like next proxy in the list retry, late HTTP cache failures, and redirects to intercepting channels.

generally there are two cases: retry or intercept, both with the same flag (somewhat unfortunately for the purpose here).

note that internal redirs may happen both before or after the server response.

for internal redirects, as we don't expose the channel itself (not sure if details.requestId is per channel), that response headers are optional and that we redirect only to the same URI, I'm not sure we even want to fire the onBeforeRedirect callbacks.  OTOH, in some cases (when the internal redirect happens after server response) various onBefore will fire again, potentially unexpectedly.  so, we probably need to fire to keep the graph at [1] consistent and logical.  OT3H :) I don't see how the old channel (requestId?) and new channel are joined in the onBeforeRedirect.

but I don't write the specs for this.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest

::: toolkit/components/extensions/schemas/web_request.json:71
(Diff revision 2)
> +            "description": "The redirect is an internal redirect that may happen for various reasons, such as redirecting the request to a proxy."
> +          },
> +          "permanent": {
> +            "type": "boolean",
> +            "optional": true,
> +            "description": "The redirect is an considered a permanent redirect."

R+ CONDITION:

merge permanent and temporary to one flag "server" or alike

Comment 10

[Mikhail Gusaurov]

Is there any workaround for this issue besides not requesting responseHeaders in onBeforeRedirect at all?

Once it lands it's not a problem, but there are LTS releases.

Comment 11

[Shane Caraveo (:mixedpuppy)]

I'm going to try and land a separate patch (just the the try/catch) to uplift a fix into beta.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: BETA patch: ignore exception when response headers are not available

Comment 13

[Shane Caraveo (:mixedpuppy)]

Ryan: The beta patch is the base fix here, simple enough it IMO can be applied at any point.  I've just carried forward the r+ from rpl on the larger patch (which includes the same try/catch).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on the patch you want to uplift.

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> Comment on attachment 8982288 [details]

> for internal redirects, as we don't expose the channel itself (not sure if
> details.requestId is per channel), that response headers are optional and
> that we redirect only to the same URI, I'm not sure we even want to fire the
> onBeforeRedirect callbacks.  OTOH, in some cases (when the internal redirect
> happens after server response) various onBefore will fire again, potentially
> unexpectedly.  so, we probably need to fire to keep the graph at [1]
> consistent and logical.  

That flow graph is just the typical request, it is not always the case and I'm not certain it could be.  That's the reason I'm interested in providing the extra details (flags).  I am questioning that somewhat, but it does seem useful for an extension to have some insight into the reason when the flow is atypical.

> OT3H :) I don't see how the old channel
> (requestId?) and new channel are joined in the onBeforeRedirect.

The ChannelWrapper is updated with the new channel here:

https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/toolkit/modules/addons/WebRequest.jsm#912

ChannelWrapper maintains any necessary consistency between the two, such as the request id.

> ::: toolkit/components/extensions/schemas/web_request.json:71
> (Diff revision 2)
> > +            "description": "The redirect is an internal redirect that may happen for various reasons, such as redirecting the request to a proxy."
> > +          },
> > +          "permanent": {
> > +            "type": "boolean",
> > +            "optional": true,
> > +            "description": "The redirect is an considered a permanent redirect."
> 
> R+ CONDITION:
> 
> merge permanent and temporary to one flag "server" or alike

I'm not clear what "server" would represent.  For example, permanent is combined with sts_upgrade.  That can happen as a result of an extension calling upgradeToSecure, so not a server response.

Could you clarify?

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> > (requestId?) and new channel are joined in the onBeforeRedirect.
> 
> The ChannelWrapper is updated with the new channel here:
> 
> https://searchfox.org/mozilla-central/rev/
> d544b118e26422877108c42723b26e9bb4539721/toolkit/modules/addons/WebRequest.
> jsm#912
> 
> ChannelWrapper maintains any necessary consistency between the two, such as
> the request id.

OK, so it would be weird if the channel would suddenly change (I mean: get onBeforeRequest(channel-1), onBeforeSendHeaders(channel-1), onSendHeaders(channel-1), onHeadersReceived(channel-1) and then suddenly onBeforeRequest(channel-2) etc...  That may be pretty unexpected for devs and that's why we should probably fire onBeforeRedirect also for internal redirects.

But if the channel is something opaque and devs attach any properties or do any bindings on the wrapper and not the channel inside it (also when the requestId remains the same), we could go that way - don't trigger onBeforeRedirect for internals.  But this is not up to me to decide.


One unrelated side note, in onChannelReplaced you probably need newChannel.loadInfo.resultPrincipalURI instead of newChannel.originalURI.  Maybe file a new bug?


> 
> > ::: toolkit/components/extensions/schemas/web_request.json:71
> > (Diff revision 2)
> > > +            "description": "The redirect is an internal redirect that may happen for various reasons, such as redirecting the request to a proxy."
> > > +          },
> > > +          "permanent": {
> > > +            "type": "boolean",
> > > +            "optional": true,
> > > +            "description": "The redirect is an considered a permanent redirect."
> > 
> > R+ CONDITION:
> > 
> > merge permanent and temporary to one flag "server" or alike
> 
> I'm not clear what "server" would represent.  For example, permanent is
> combined with sts_upgrade.  That can happen as a result of an extension
> calling upgradeToSecure, so not a server response.
> 
> Could you clarify?

I would more tend to do have an enum instead of flags exposed to extensions.  We set REDIRECT_PERMANENT along with STS_UPGRADE (and few other places) only for legacy reasons, we may even want to remove it from the STS_UPGRAGE combination because I don't know about any consumer of it right now, see [1].  This is the reason why you should not expose the flags as they are.  

I would more tend to have an enum here, let's say: "regular", "internal", "secure-upgrade".  

regular is: any of the redirect 30X server responses or channel.redirectTo().  note that you can expose the http response code, if useful.  I don't know if we want "regular" be rather split to "permanent"/"temporary".  because redirectTo() is internally marked as permanent (just because there is no other reasonable flag to set) but the extension that called redirectTo() may next time decide to not redirect the request from some reason.

Does it make sense?

[1] https://searchfox.org/mozilla-central/diff/7413eecfda8528f7ac8f7d7e575238f75e6534a0/netwerk/protocol/http/nsHttpChannel.cpp#1759

Comment 17

[Shane Caraveo (:mixedpuppy)]

I'm moving the base fix to another bug to land today.  This bug will be about adding the flags for 63

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8982288 [details]
Bug 1462006 - add redirect flags to webRequest listener details,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248232/diff/2-3/

Comment 19

[Richard Bloor]

Hi Shane, I am about to look at the documentation for this but have a note "needs a change to the webRequest page; ask Shane for details of what exactly". Can you provide some more details of the "what"? Thank you!

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Richard Bloor from comment #19)

Hi Shane, I am about to look at the documentation for this but have a note "needs a change to the webRequest page; ask Shane for details of what exactly". Can you provide some more details of the "what"? Thank you!

This is not a closed bug, so you shouldn't be looking at it.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.