Closed Bug 1149250 Opened 9 years ago Closed 6 years ago

Ensure Web Extensions surface allowing manual http->https (secure update) redirects not blocked by CORS checks

Categories

(WebExtensions :: Request Handling, defect, P2)

Product:
WebExtensions
Component:
Request Handling
Type:
defect

Tracking

(firefox57 wontfix, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: mayhemer, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete, Whiteboard: triaged)

Attachments

(2 files, 2 obsolete files)

Bug 1149250 add support for https upgrades from webextensions,
59 bytes, text/x-review-board-request
rpl
: review+
mayhemer
: review+
bzbarsky
: review+
Details
cross origin.gif
617.93 KB, image/gif
Details 
+++ This bug was initially created as a clone of Bug #881830 +++

Not surprisingly, this (the fix for bug 881830) has been causing lots of breakage with HTTPS Everywhere. Hopefully the CORS-with-preflight-case being addressed?

cf https://github.com/EFForg/https-everywhere/issues/49

see "development" XPIs at https://eff.org/https-everywhere
Ping: This is still causing a lot of bugs for HTTPS Everywhere. Any suggestions on how to proceed?

We may wind up having to work around by telling HTTPS Everywhere never to upgrade preflight requests. Is there any way to distinguish a preflight request using the http-on-modify-request observer event (https://developer.mozilla.org/en-US/docs/Observer_Notifications)?
Is there any progress on this? The related issue in HTTPS Everywhere is quite frustrating.
Whiteboard: [necko-active]
Possible solutions:

In Gecko:
- add nsIHttpChannel.upgradeToHttps public method, that will be used instead of redirectTo

In HTTPS Everywhere:
- persuade loadinfo of the channel (if there is one!) to behave as there were "upgrade-insecure-requests" CSP directive set on the loading document
- set setAllowSTS to true on the channel and then persuade (or potentially reimplement) nsISiteSecurityService to return true for calls from isSecureURI for the channel

yan, what do you think?  I'm not sure I want to add upgradeToHttps, but that sounds like the simplest solution that preserves full functionality.
Flags: needinfo?(yan)
Whiteboard: [necko-active] → [necko-backlog]
Disassigning myself for lack of information progress and interest here.
Assignee: honzab.moz → nobody
yan, ping on ni? here.
Closing for no feedback.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(yan)
Resolution: --- → INCOMPLETE
The summary of this bug isn't https-everywhere specific... is the summary just wrong?
Flags: needinfo?(honzab.moz)
Resolved? Closed? Please. This is a significant issue. I am the developer of Jmol (JSmol) and have had huge problems with this issue in the context of jQuery. Sites are automatically switching to https now, so IT professionals at the host site have no options open to them. All is fine outside of AJAX as far as I can see. But we really need AJAX to do this. 

I'm hoping that Mozilla can at least lead the way for other browsers. 

Or am I missing something?

Bob hanson
Bob, I think whatever you're missing is what I'm missing too.  And specifically, what the difference is between this bug and bug 881830 (which is marked fixed).

But if you have a specific site that is showing a problem in a current release, please file it with steps to reproduce.  Because if my guess about what _this_ bug is about is correct, whatever you are seeing is not this bug.
This is still occurring for me on http://js2.coffee which has assets from code.ionicframework.com that are upgraded by HTTPS Everywhere, as reported a long time ago here: https://github.com/EFForg/https-everywhere/issues/49#issuecomment-167002867

Firefox 49 on macOS 10.12, HTTPS Everywhere 5.2.5

The same issue does not occur in Chrome with HTTPS Everywhere
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> Bob, I think whatever you're missing is what I'm missing too.  And
> specifically, what the difference is between this bug and bug 881830 (which
> is marked fixed).
> 
> But if you have a specific site that is showing a problem in a current
> release, please file it with steps to reproduce.  Because if my guess about
> what _this_ bug is about is correct, whatever you are seeing is not this bug.

Thanks Boris.  Yes, this has been specifically filed as a response to HTTPS Everywhere add-on issue 49, see comment 0.  This bug is not about content at all.  The title had to be updated, I admit.

I didn't get any feedback here, I'll try to ping them on github directly.

(In reply to Bob Hanson from comment #8)
> Resolved? Closed? Please. This is a significant issue. I am the developer of
> Jmol (JSmol) and have had huge problems with this issue in the context of
> jQuery. Sites are automatically switching to https now, so IT professionals
> at the host site have no options open to them. All is fine outside of AJAX
> as far as I can see. But we really need AJAX to do this. 
> 
> I'm hoping that Mozilla can at least lead the way for other browsers. 
> 
> Or am I missing something?
> 
> Bob hanson

As Boris mentions, I am not sure how your issue relates to this bug.  Sorry for confusion if the bad title misled you.
Flags: needinfo?(honzab.moz)
Summary: CORS requests that STS upgrades to https fail → No XPCOM API to tell an HTTP channel to internally redirect to a secure scheme, redirect otherwise blocked by CORS checks.
(In reply to Stefan Wrobel from comment #10)
> This is still occurring for me on http://js2.coffee which has assets from
> code.ionicframework.com that are upgraded by HTTPS Everywhere, as reported a
> long time ago here:
> https://github.com/EFForg/https-everywhere/issues/49#issuecomment-167002867
> 
> Firefox 49 on macOS 10.12, HTTPS Everywhere 5.2.5
> 
> The same issue does not occur in Chrome with HTTPS Everywhere

Commented there with reference to this bug.  Hopefully I will get a feedback now.
(And apparently closing a bug with no feedback is a good thing - I got some feedback instantly ;))
Hi! Sorry for the delay in responding. I think my Bugzilla mail was getting eaten. A little more detail:

Chrome has a similar bug at https://bugs.chromium.org/p/chromium/issues/detail?id=387198. It looks like they were able to solve it internally, without new APIs, by setting headers on the redirect. Do you think that is possible for Firefox?

I think HTTPS Everywhere would be happy to have an upgradeToHttps API, but we are planning to move the whole extension to WebExtensions as recommended by Mozilla. So any API we would want to use would have to exist in WebExtensions. Additionally, it's not clear to me that how adding the new API would fix the problem. Could you go into more detail on that?

Thanks,
Jacob
(In reply to Jacob from comment #14)
> Hi! Sorry for the delay in responding. I think my Bugzilla mail was getting
> eaten. A little more detail:
> 
> Chrome has a similar bug at
> https://bugs.chromium.org/p/chromium/issues/detail?id=387198. It looks like
> they were able to solve it internally, without new APIs, by setting headers
> on the redirect. Do you think that is possible for Firefox?

I will go thought that bug a but later, maybe we can do something similar/same.

> 
> I think HTTPS Everywhere would be happy to have an upgradeToHttps API, but
> we are planning to move the whole extension to WebExtensions as recommended
> by Mozilla. 

Good to hear!

> So any API we would want to use would have to exist in
> WebExtensions. Additionally, it's not clear to me that how adding the new
> API would fix the problem. Could you go into more detail on that?

So, the problem is that redirect from http://foo.com to httpS://foo.com is a cross-origin redirect and must be submitted to CORS checks, that is clear.  When it's triggered internally (from inside the platform) as part of HSTS upgrade (for instance) via an API that is available only internally, we set a flag marking the redirect as internal and as 'intentional secure update from http->https' only.  We can then safely bypass some CORS checks.  Problem is that we cannot simply set this "do bypass CORS checks" flag whenever a channel does only http->https redirect (secure update) because we can't determine the true cause of the redirect and we may actually go to a totally different server.

Hence, a new API you may then call (regardless being XPCOM or WebExtensions) from a privileged code marks this redirect intention as "secure update" explicitly and set that "do bypass CORS checks" flag.

I will have to discuss this also with some people responsible for designing WebExtensions APIs, whether there is something we could offer right now or we have to invent something.  

Cameron, can you elaborate or forward to the right people?

Renaming the bug, ensure Web Extensions surface allowing this kind "secure update" redirect not being blocked by CORS checks.

> 
> Thanks,
> Jacob
Status: RESOLVED → REOPENED
Component: Networking → WebExtensions: Request Handling
Flags: needinfo?(spectre)
Product: Core → Toolkit
Resolution: INCOMPLETE → ---
Summary: No XPCOM API to tell an HTTP channel to internally redirect to a secure scheme, redirect otherwise blocked by CORS checks. → Ensure Web Extensions surface allowing manual http->https (secure update) redirects not blocked by CORS checks
Status: REOPENED → NEW
Whiteboard: [necko-backlog]
I don't think this is me directly, though we were talking about a protocol handler API. bz might know the right person.
Flags: needinfo?(spectre)
Priority: -- → P2
Whiteboard: triaged
What I have suggested in the past is to expose two notifications to web extensions.

The first notification happens after gecko creates a channel and calls AsyncOpen2(), but before AsyncOpen2() does any security checks.

The second notification happens after AsyncOpen2 has done security checks, but before we hit the network/httpcache.


If the webextension does a redirect during the first notification, then we never do any CORS, CheckLoadURI, etc checks on the URI which was actually opened by gecko. Instead the AsyncOpen2 process restarts and we do security checks only on the new URI (unless the channel gets redirected again to yet another URI).


There are some difficulties implementing this. AsyncOpen2 currently does security checks synchronously and returns an error value if they fail. But we can't call into webextensions synchronously.

Generally speaking it should be possible to make AsyncOpen2 do security checks asynchronously and instead fire OnStartRequest/OnStopRequest with an error value if security checks fail.

But there are a few web APIs which rely on synchronous security checks. For example I *think* that we intentionally do some security checks synchronously when an <a href="..." target="_blank"> is clicked to avoid opening a new window if the load is blocked. There are likely more examples like that.

new Worker(...) used to do synchronous same-origin checks in the spec, and might still in gecko, but I believe the spec now calls for these checks to be async.
I just wrote a test to redirect a fetch of http://example.com to https://example.com and it passes.  Am I missing something here?
Ah.  I know what I missed.
Attached patch redirectUpgrade (obsolete) — — Splinter Review 
This test shows my understanding of the basic problem outlined in this bug.  If the functionality needed is different it would be great to clarify it.
Attachment #8823882 - Attachment is obsolete: true
Comment on attachment 8824195 [details] [diff] [review]
redirectUpgrade

moved test patch to reviewboard
Attachment #8824195 - Attachment is obsolete: true
I fixed the test while testing another http->https upgrade bug I saw, and am not able to reproduce the problem.  I'll need a more specific STR in order to validate whether this is actually a problem.
I'm also getting CORS error with a simple noop redirect on latest Nightly, so I can't create my ext..
See code: https://gist.github.com/klausenbusk/f368f01d0412aff02b6fe736ccb9cf70 and I'm getting:
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://0-edge-chat.facebook.com/pullxxxxx. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing).
I can still reproduce this in Nightly 08-25, STR:

1. Install the beta version of https everywhere at [0] since only WebExtension works in Fx 57
2. Open http://js2.coffee/
3. Open HTTPS Everywhere's panel and toggle 'Ionic Framework.com (partial)' (This ruleset has been re-enabled later at [1], but it is not included in the WebExtension version yet. Anyway code.ionicframework.com is the only one matters here)
4. Hard reload

Error message:
Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at http://code.ionicframework.com/ionicons/2.0.1/fonts/ionicons.ttf?v=2.0.1. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing).

Hope this can help to validate the problem

[0]: https://www.eff.org/files/https-everywhere-test/https-everywhere-2017.7.21.1338-eff.xpi
[1]: https://github.com/EFForg/https-everywhere/pull/11950
Flags: needinfo?(mixedpuppy)
status-firefox57: --- → wontfix
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

Honza, this seems to work just fine given the simple test (included).  I'm not sure if there are issues I am unaware of that would make this approach a bad idea.
Flags: needinfo?(mixedpuppy)
Attachment #8843518 - Flags: feedback?(honzab.moz)
Assignee: nobody → mixedpuppy
Attachment #8843518 - Flags: feedback?(honzab.moz)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review202544


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

for feedback on the webext side of things.
Attachment #8843518 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review202998

::: toolkit/components/extensions/schemas/web_request.json:153
(Diff revision 3)
>            "redirectUrl": {
>              "type": "string",
>              "optional": true,
>              "description": "Only used as a response to the onBeforeRequest and onHeadersReceived events. If set, the original request is prevented from being sent/completed and is instead redirected to the given URL. Redirections to non-HTTP schemes such as data: are allowed. Redirects initiated by a redirect action use the original request method for the redirect, with one exception: If the redirect is initiated at the onHeadersReceived stage, then the redirect will be issued using the GET method."
>            },
> +          "redirectUpgrade": {

Seems reasonable, but maybe a bit arcane. Maybe call it something like "maybeUpgradeToHttps"?
Attachment #8843518 - Flags: feedback?(kmaglione+bmo) → feedback+
Capturing some irc feedback/discussion with Kris

- maybe use "upgradeToHttps" or "maybeUpgradeToHttps" instead of "redirectUpgrade"
- me: we should no-op in webrequest.jsm if it is already https
- perhaps return a bool if upgrade is necessary
- me: not sure that is easy to do, it is determined at a later time in the channel, but easy to return !schemeIs(https).
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review203370

::: dom/webidl/ChannelWrapper.webidl:85
(Diff revision 3)
> +   * Upgrades the wrapped HTTP channel to a secure request. For other channel
> +   * types, this method will throw. The redirect is an internal redirect, and
> +   * the  behavior is the same as nsIHttpChannel.redirectUpgrade.
> +   */
> +  [Throws]
> +  void redirectUpgrade();

is the name of this method under some web-spec control?  if not, please make the name clearer, like ensureUpgradeToSecure or something.

internal methods and members definitely must have a clearer name.

the comment here must explain how this effects the channel when also redirectTo was called.

and, that bit (both redirectTo and redirectUpgrade being called on a single channel) has to be discussed before landing this.

::: netwerk/protocol/http/HttpBaseChannel.h:490
(Diff revision 3)
>    nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
>    nsCOMPtr<nsIProgressEventSink> mProgressSink;
>    nsCOMPtr<nsIURI> mReferrer;
>    nsCOMPtr<nsIApplicationCache> mApplicationCache;
>    nsCOMPtr<nsIURI> mAPIRedirectToURI;
> +  bool mAPIRedirectUpgrade;

I'd like to see this in the list of bit fields to pack.  and give it also a better name.

::: netwerk/protocol/http/nsIHttpChannel.idl:447
(Diff revision 3)
>       * @throws NS_ERROR_NOT_AVAILABLE if called after the channel has already
>       *         started to deliver the content to its listener.
>       */
>      [must_use] void redirectTo(in nsIURI aTargetURI);
>  
> +    [must_use] void redirectUpgrade();

add a good comment

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp:1081
(Diff revision 3)
>      return !mHttpChannel ? NS_ERROR_NULL_POINTER :
>          mHttpChannel->RedirectTo(uri);
>  }
>  
>  NS_IMETHODIMP
> +nsViewSourceChannel::RedirectUpgrade()

a blank line before this line

::: toolkit/modules/addons/WebRequest.jsm:814
(Diff revision 3)
>            channel.suspended = false;
>            channel.cancel(Cr.NS_ERROR_ABORT);
>            return;
>          }
>  
> -        if (result.redirectUrl) {
> +        if (result.redirectUpgrade) {

so, secure upgrade takes precedence over redirectTo, is this according the spec?  is this what we want?  and is this documented in our (web)idls properly?

::: toolkit/modules/addons/WebRequest.jsm:818
(Diff revision 3)
>  
> -        if (result.redirectUrl) {
> +        if (result.redirectUpgrade) {
> +          channel.suspended = false;
> +          channel.redirectUpgrade();
> +          return;
> +        } else if (result.redirectUrl) {

no else after return please
Attachment #8843518 - Flags: review-
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

stupid rb...
Attachment #8843518 - Flags: review-
Attachment #8843518 - Flags: feedback?(honzab.moz)
Attachment #8843518 - Flags: feedback-
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review203370

> a blank line before this line

I'm not clear on this request, I formated as everything else in this file is formatted.

> so, secure upgrade takes precedence over redirectTo, is this according the spec?  is this what we want?  and is this documented in our (web)idls properly?

They are mutually exclusive in httpchannel.  At the webrequest level we'll prefer the upgrade over the redirect.
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review203370

> They are mutually exclusive in httpchannel.  At the webrequest level we'll prefer the upgrade over the redirect.

I think, if it were a matter of simple precedence, I'd prefer the opposite, and have `redirectUrl` take precedence over upgrade, since redirecting to a different URL is the more dramatic change.

That said, the way this is currently implemented, an single listener should only be allowed to specify one of the two, and in the case of multiple listeners, the first one to redirect or cancel the request takes precedence, no matter which method it used to do so.

Which is perhaps not ideal, in this case, since `redirectUpgrade` won't necessarily actually redirect the channel.
(In reply to Honza Bambas (:mayhemer) from comment #34)

> and, that bit (both redirectTo and redirectUpgrade being called on a single
> channel) has to be discussed before landing this.

My intention is that either will throw if the other was called, making them mutually exclusive.  It does however, allow one to first upgrade, then on the new channel do a redirect (or visa-verse).  You'll see that in the test in the upcoming patch.
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

Still only requesting f? until we resolve any behavior issues between upgradeToSecure and redirectTo.  

In the channel I've made them mutually exclusive, first wins.  

In webrequest, if both are returned from the listener, upgrade wins.  I'd rather choose secure channel of not.

On item of note is that, you can first upgrade the channel, then on the new channel you can redirect.  That is illustrated in the test file.
Attachment #8843518 - Flags: feedback?(kmaglione+bmo)
Attachment #8843518 - Flags: feedback?(honzab.moz)
I'd rather choose secure channel *over* not.
(In reply to Shane Caraveo (:mixedpuppy) from comment #36)
> Comment on attachment 8843518 [details]
> Bug 1149250 upgrade to https
> 
> https://reviewboard.mozilla.org/r/117226/#review203370
> 
> > a blank line before this line
> 
> I'm not clear on this request, I formated as everything else in this file is
> formatted.

Sorry, overlook, it's because I didn't have my second coffee by that time :)

> 
> > so, secure upgrade takes precedence over redirectTo, is this according the spec?  is this what we want?  and is this documented in our (web)idls properly?
> 
> They are mutually exclusive in httpchannel.  At the webrequest level we'll
> prefer the upgrade over the redirect.

so, when I meant to 'discuss' this, I meant to carry this outside this bug.  if this is not clearly specified - is there even a spec for this bit or is it firefox/gecko specific? - then it has to be specified SOMEWHERE (SW spec/MDN at least) and before it's written down, it has to be brought to a wider audience, which means web developers and people involved in writing specs.

the outcome may be to invent a yet different api for this.

btw, when I call redirectTo multiple times, what happens?

I personally think redirect is stronger.  if it targets a non-secure url, the extension will likely capture the new channel again and instruct it to 'update to HTTPS' using the second method that time, if found desired.
(In reply to Honza Bambas (:mayhemer) from comment #42)
> it has to be brought to a
> wider audience, which means web developers and people involved in writing
> specs.

I mean to give them a chance to review or comment on this this api.
(In reply to Honza Bambas (:mayhemer) from comment #43)
> (In reply to Honza Bambas (:mayhemer) from comment #42)
> > it has to be brought to a
> > wider audience, which means web developers and people involved in writing
> > specs.
> 
> I mean to give them a chance to review or comment on this this api.

This is firefox specific so I'm not certain about why we'd need specs on it.  To my understanding ChannelWrapper is only available to privileged code, it was written specifically for webextensions performance reasons.  It is also only used internally, not exposed to extensions.  So the "api" exposed is just an ability to set the flag from WebRequest.onBeforeRequest as illustrated in the test.  Kris may have better insight to that.
I can change to having redirectTo have precedence in webrequest, but is that what we want in nsHttpChannel?
(In reply to Honza Bambas (:mayhemer) from comment #42)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #36)

> > 
> > > so, secure upgrade takes precedence over redirectTo, is this according the spec?  is this what we want?  and is this documented in our (web)idls properly?

This is an internal api, not part of any spec.

> btw, when I call redirectTo multiple times, what happens?

Up until the redirect actually happens, the last call wins.

> I personally think redirect is stronger.  if it targets a non-secure url,
> the extension will likely capture the new channel again and instruct it to
> 'update to HTTPS' using the second method that time, if found desired.

The last patch I pushed makes them mutually exlusive.  If someone calls redirectTo then someone calls upgradeToSecure, the latter call will fail (and visa-verse).  This is all internal use, so I feel like it's really about how we prefer these to work.  We could always make redirectTo unset the upgrade flag, and have upgradeToSecure always fail if redirectTo was previously called.
I thought the changes to ChannelWrapper.webidl were public.  If not, then we just want to come to a consensus from parties effected by each particular change.

ni? me for how httpchannel should behave in necko.
Flags: needinfo?(honzab.moz)
(In reply to Shane Caraveo (:mixedpuppy) from comment #40)
> Comment on attachment 8843518 [details]
> Bug 1149250 upgrade to https
> 
> Still only requesting f? until we resolve any behavior issues between
> upgradeToSecure and redirectTo.  
> 
> In the channel I've made them mutually exclusive, first wins.  
> 
> In webrequest, if both are returned from the listener, upgrade wins.  I'd
> rather choose secure channel of not.
> 
> On item of note is that, you can first upgrade the channel, then on the new
> channel you can redirect.  That is illustrated in the test file.

What if multiple listeners return results? If we have a request for "http://example.com/", and one listener returns:

  {upgradeToSecure: true}

and another returns:

  {redirectUrl: "https://captive-portal.to/"}

arguably we want to redirect to the captive portal, not upgrade to https, even if the listener may get another chance after the redirect. There's also the possibility that the upgrade won't actually take place. The fact that we return from the modification loop at this point means that any subsequent modification attempts will be ignored in the case of a failed upgrade.

(In reply to Honza Bambas (:mayhemer) from comment #47)
> I thought the changes to ChannelWrapper.webidl were public.  If not, then we
> just want to come to a consensus from parties effected by each particular
> change.

It's only exposed to chrome-privileged code. It's public in the sense that it should be available to any gecko code that needs fast JS access to channels, but it's currently only used by the WebExtensions framework.
Well, the other option is just let both be set and not worry about any error for that case.  If both are set during onBeforeRequest (on-modify) redirectUrl will take precedence and upgradeToSecure would have no effect.
(In reply to Shane Caraveo (:mixedpuppy) from comment #49)
> Well, the other option is just let both be set and not worry about any error
> for that case.  If both are set during onBeforeRequest (on-modify)
> redirectUrl will take precedence and upgradeToSecure would have no effect.

That sounds like the correct approach to me.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #50)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #49)
> > Well, the other option is just let both be set and not worry about any error
> > for that case.  If both are set during onBeforeRequest (on-modify)
> > redirectUrl will take precedence and upgradeToSecure would have no effect.
> 
> That sounds like the correct approach to me.

Yes!  I agree.  This is how I would prefer it in http channel too.  There is no need to throw errors from the setters, just let both set.  

Make sure everything is correctly LOG()'ed/  I believe mostly it already is, but please double check your new code paths!  That way when something goes wrong we known why.

Thanks.
Flags: needinfo?(honzab.moz)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

I think the current patch should be fine, so moving to review.  Honza for the netwerk channel changes, Kris for the web-ext changes.
Attachment #8843518 - Flags: review?(kmaglione+bmo)
Attachment #8843518 - Flags: review?(honzab.moz)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review207790

::: commit-message-d6ae6:1
(Diff revision 6)
> +Bug 1149250 upgrade to https

defnitely needs an update ;)
Attachment #8843518 - Flags: review?(honzab.moz) → review+
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

bz for webidl changes
rpl for webextension changes
Attachment #8843518 - Flags: review?(lgreco)
Attachment #8843518 - Flags: review?(kmaglione+bmo)
Attachment #8843518 - Flags: review?(bzbarsky)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review210688

r=me on the webidl bits with the nits below addressed.

::: dom/webidl/ChannelWrapper.webidl:82
(Diff revision 7)
>    void redirectTo(URI url);
>  
> +  /**
> +   * Upgrades the wrapped HTTP channel to a secure request. For other channel
> +   * types, this method will throw. The redirect is an internal redirect, and
> +   * the  behavior is the same as nsIHttpChannel.upgradeToSecure.  Setting this

Extra space before "behavior"?

::: dom/webidl/ChannelWrapper.webidl:83
(Diff revision 7)
>  
> +  /**
> +   * Upgrades the wrapped HTTP channel to a secure request. For other channel
> +   * types, this method will throw. The redirect is an internal redirect, and
> +   * the  behavior is the same as nsIHttpChannel.upgradeToSecure.  Setting this
> +   * flag is only effective during or before http-on-modify-request.  Setting

Is http-on-modify-request a thing that consumers of this API are expected to deal with?  If not, is there a webextension concept we should be using here?

::: netwerk/protocol/http/HttpBaseChannel.cpp:2219
(Diff revision 7)
> +{
> +  // We cannot upgrade after OnStartRequest of the listener
> +  // has been called, since to redirect we have to switch channels
> +  // and the dance with OnStartRequest et al has to start over.
> +  // This would break the nsIStreamListener contract.
> +  NS_ENSURE_FALSE(mOnStartRequestCalled, NS_ERROR_NOT_AVAILABLE);

This is a much weaker check than "during or before on-modify-request"...  In particular, this seems like it would not throw if called during on-examine-response, no?  That doesn't seem to be how the API described the behavior...
Attachment #8843518 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #58)
> Comment on attachment 8843518 [details]

> ::: dom/webidl/ChannelWrapper.webidl:83
> (Diff revision 7)
> >  
> > +  /**
> > +   * Upgrades the wrapped HTTP channel to a secure request. For other channel
> > +   * types, this method will throw. The redirect is an internal redirect, and
> > +   * the  behavior is the same as nsIHttpChannel.upgradeToSecure.  Setting this
> > +   * flag is only effective during or before http-on-modify-request.  Setting
> 
> Is http-on-modify-request a thing that consumers of this API are expected to
> deal with?  If not, is there a webextension concept we should be using here?

Removing that terminology from the webidl and placing it into base channel below.

> ::: netwerk/protocol/http/HttpBaseChannel.cpp:2219
> (Diff revision 7)
> > +{
> > +  // We cannot upgrade after OnStartRequest of the listener
> > +  // has been called, since to redirect we have to switch channels
> > +  // and the dance with OnStartRequest et al has to start over.
> > +  // This would break the nsIStreamListener contract.
> > +  NS_ENSURE_FALSE(mOnStartRequestCalled, NS_ERROR_NOT_AVAILABLE);
> 
> This is a much weaker check than "during or before on-modify-request"...  In
> particular, this seems like it would not throw if called during
> on-examine-response, no?  That doesn't seem to be how the API described the
> behavior...

That was a copy/pase comment, fixed it.  It's actually not important to throw here at all, as there is only one place where the flag will have any affect, so this has to be called before or during http-on-modify-request.  Setting it afterwards has no side affect like the redirect does.
Honza, I've removed the mOnStartRequestCalled check, see comment 59.  Because this flag will have no effect at a late stage of the request like redirects can, this is unnecessary.
Flags: needinfo?(honzab.moz)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review211412

r+ (with some small nits, and a note about improving the errors reported from WebRequest.jsm in followups).

::: dom/webidl/ChannelWrapper.webidl:82
(Diff revision 8)
>    void redirectTo(URI url);
>  
> +  /**
> +   * Requests an upgrade of the HTTP channel to a secure request. For other channel
> +   * types, this method will throw. The redirect is an internal redirect, and
> +   * the  behavior is the same as nsIHttpChannel.upgradeToSecure.  Setting this

Nit, there are 2 spaces between "the" and "behavior".

::: dom/webidl/ChannelWrapper.webidl:85
(Diff revision 8)
> +   * Requests an upgrade of the HTTP channel to a secure request. For other channel
> +   * types, this method will throw. The redirect is an internal redirect, and
> +   * the  behavior is the same as nsIHttpChannel.upgradeToSecure.  Setting this
> +   * flag is only effective during the WebRequest.onBeforeRequest in
> +   * Web Extensions, calling this at any other point during the request will
> +   * have no effect.  Setting this flag in addition to calling redirectTo

Nit, there are 2 spaces also here (between "." and "Setting").

::: netwerk/protocol/http/nsIHttpChannel.idl:451
(Diff revision 8)
>  
>      /**
> +     * Flags a channel to be upgraded to HTTPS.
> +     *
> +     * Upgrading to a secure channel must happen before or during
> +     * "http-on-modify-request".  If redirectTo is called early as well, it

Nit, There are 2 spaces between "." and "If".

::: toolkit/components/extensions/schemas/web_request.json:156
(Diff revision 8)
>              "description": "Only used as a response to the onBeforeRequest and onHeadersReceived events. If set, the original request is prevented from being sent/completed and is instead redirected to the given URL. Redirections to non-HTTP schemes such as data: are allowed. Redirects initiated by a redirect action use the original request method for the redirect, with one exception: If the redirect is initiated at the onHeadersReceived stage, then the redirect will be issued using the GET method."
>            },
> +          "upgradeToSecure": {
> +            "type": "boolean",
> +            "optional": true,
> +            "description": "Only used as a response to the onBeforeRequest event. If set, the original request is prevented from being sent/completed and is instead redirected to a secure request.  If this or another extension returns <code>redirectUrl</code> during onBeforeRequest, <code>upgradeToSecure</code> will have no affect."

Nit, how about "is instead upgraded to a secure request" instead of "is instead redirected..."?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_upgrade.html:22
(Diff revision 8)
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: [
> +        "webRequest",
> +        "webRequestBlocking",
> +        "<all_urls>",

Nit, it seems that this permission could be restricted to the "mochi.test" urls instead of being <all_urls> (same for line 63).

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_upgrade.html:27
(Diff revision 8)
> +        "<all_urls>",
> +      ],
> +    },
> +    background() {
> +      browser.webRequest.onSendHeaders.addListener((details) => {
> +        browser.test.log(`onSendHeaders ${details.requestId} ${details.url}`);

Nit, this logged message (and the ones at line 35) are not present in the other test from this file, I guess that they were helpful while working on this patch and we can remove them.

::: toolkit/modules/addons/WebRequest.jsm:823
(Diff revision 8)
>              return;
>            } catch (e) {
>              Cu.reportError(e);
>            }
>          }
> +        if (result.upgradeToSecure && kind === "opening") {

Nit, we could add one empty line between the previous block and the newly added if block.

::: toolkit/modules/addons/WebRequest.jsm:827
(Diff revision 8)
>          }
> +        if (result.upgradeToSecure && kind === "opening") {
> +          try {
> +            channel.upgradeToSecure();
> +          } catch (e) {
> +            Cu.reportError(e);

If I'm not reading it wrong, the errors that we are going to log here are not going to provide enough useful information to help an addon developer to understand which was the request that raised the exception and which was the extension that was trying to upgrade it to a secure channel.

(Nevertheless, it also looks that the errors reported at line 820 have probably the same issue, and so it sounds reasonable to me if we look into improving the errors reported from WebRequest.jsm as a separate follow-up issue).
Attachment #8843518 - Flags: review?(lgreco) → review+
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review211412

> If I'm not reading it wrong, the errors that we are going to log here are not going to provide enough useful information to help an addon developer to understand which was the request that raised the exception and which was the extension that was trying to upgrade it to a secure channel.
> 
> (Nevertheless, it also looks that the errors reported at line 820 have probably the same issue, and so it sounds reasonable to me if we look into improving the errors reported from WebRequest.jsm as a separate follow-up issue).

Actually, thowing an exception was removed lower level, so I've removed that from this.
(In reply to Shane Caraveo (:mixedpuppy) from comment #61)
> Honza, I've removed the mOnStartRequestCalled check, see comment 59. 
> Because this flag will have no effect at a late stage of the request like
> redirects can, this is unnecessary.

Hmm.. so when you call this too late (so that it won't take effect) the consumer has no chance to know that..

That doesn't seem right to me, we are trying to keep the consumer informed when such "modify the channel behavior" APIs is misused.  I think if you just update the idl comments this check could stay, but if you are ok with silently ignoring the action, it's up to you.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #66)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #61)
> > Honza, I've removed the mOnStartRequestCalled check, see comment 59. 
> > Because this flag will have no effect at a late stage of the request like
> > redirects can, this is unnecessary.
> 
> Hmm.. so when you call this too late (so that it won't take effect) the
> consumer has no chance to know that..
> 
> That doesn't seem right to me, we are trying to keep the consumer informed
> when such "modify the channel behavior" APIs is misused.  I think if you
> just update the idl comments this check could stay, but if you are ok with
> silently ignoring the action, it's up to you.

With that check, it would still be silently ignored if set between the on-modify notification and before the response is started.  I think that is less desirable.  The only way we can be accurate is to add another flag that is set after the upgrade block in nsHttpChannel::OnBeforeConnect.  I'm fine with either a silent ignore or adding the flag.
Flags: needinfo?(honzab.moz)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

Honza, asking r? again for the addition of the new flag.  I'm added an internal mUpgradeable flag that is set to false where we handle the upgrade.  UpgradeToSecure will throw if that is false.
Flags: needinfo?(honzab.moz)
Attachment #8843518 - Flags: review+ → review?(honzab.moz)
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review212302

r+ with the comments addressed, thanks!

::: netwerk/protocol/http/HttpBaseChannel.h:655
(Diff revisions 10 - 11)
>    bool mOnStartRequestCalled;
>    bool mOnStopRequestCalled;
>  
> +  // Defaults to true.  This is set to false when it is no longer possible
> +  // to upgrade the request.
> +  bool mUpgradeable;

make it a bit field please (unless this is accessed on multiple threads, which would need to be atomic or locked) and rename to mUpgradableToSecure
Attachment #8843518 - Flags: review?(honzab.moz) → review+
Comment on attachment 8843518 [details]
Bug 1149250 add support for https upgrades from webextensions,

https://reviewboard.mozilla.org/r/117226/#review212304

::: netwerk/protocol/http/HttpBaseChannel.h:654
(Diff revisions 10 - 11)
>    // OnStopRequest more than once.
>    bool mOnStartRequestCalled;
>    bool mOnStopRequestCalled;
>  
> +  // Defaults to true.  This is set to false when it is no longer possible
> +  // to upgrade the request.

in general 'upgrade' alone is too general, always use 'upgrade to secure'  (hence, please also update this comment)  thanks.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8475e73ada98
add support for https upgrades from webextensions, r=bz,mayhemer,rpl
https://hg.mozilla.org/mozilla-central/rev/8475e73ada98
Status: NEW → RESOLVED
Closed: 8 years ago → 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Keywords: dev-doc-needed
Attached image cross origin.gif — 
Retested and reproduced on Windows 10 64Bit, Firefox 59.0a1 (20180103100101) using the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1149250#c26.
Reopening issue.
Assignee: mixedpuppy → marius.santa
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: marius.santa → mixedpuppy
But does this add-on [1] use "upgradeToSecure"? No?
I have looked at the source of this add-on [1] but it seems that it is not using upgradeToSecure [2]?

[1] https://www.eff.org/files/https-everywhere-test/https-everywhere-2017.7.21.1338-eff.xpi
[2] https://reviewboard.mozilla.org/r/117226/diff/12#15
Flags: needinfo?(marius.santa)
Please verify that https://www.eff.org/files/https-everywhere-test/https-everywhere-2017.7.21.1338-eff.xpi uses "upgradeToSecure" and if not please provide a webext that does in order to perform manual testing on the issue.
Flags: needinfo?(marius.santa) → needinfo?(mixedpuppy)
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Flags: needinfo?(mixedpuppy) → qe-verify-
Resolution: --- → FIXED
I've added this to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/BlockingResponse, please let me know if this covers it.
Flags: needinfo?(mixedpuppy)
lgtm
Flags: needinfo?(mixedpuppy)
Keywords: dev-doc-needed → dev-doc-complete
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: