1293476 - Mixed content blocker blocks Safe Browsing interstitials in iframes

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[François Marier [:francois]]

The test page link to in the URL field of this bug includes a blacklisted phishing URL inside an iframe and should display the Safe Browsing interstitial inside the iframe (at least until bug 1195242 is fixed) but it's erroneously considered mixed content by the MCB:

Blocked loading mixed active content “about:blocked?e=deceptiveBlocked&u=https%3A//testsafebrowsing.appspot.com/s/phishing.html&o=1&s=blacklist&c=UTF-8&f=regular&d=This%20web%20page%20at%20testsafebrowsing.appspot.com%20has%20been%20reported%20as%20a%20deceptive%20site%20and%20has%20been%20blocked%20based%20on%20your%20security%20preferences.”[Learn More]

Comment 1

[Daniel Veditz [:dveditz]]

Just tried ESR-45.3 and it worked correctly. When did this regress?

Comment 2

[Daniel Veditz [:dveditz]]

Ethan still sees the warnings in Nightly, but I don't and arroway does not in 48.

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

Edit: I _can_ see the warnings in 48.

Comment 4

[Daniel Veditz [:dveditz]]

It could also be a difference between the clean-ish testing profile I used in ESR-45 and my old default profile.

Comment 5

[Daniel Veditz [:dveditz]]

Using the same profile that saw the SafeBrowsing interstitials in the frames in ESR-45.x:
 - 47.0 gave me the insterstitials
 - 48.0 did not

That gives a regression range between 47 and 48, except given comment 3 and Ethan seeing them in Nightly we can't rely on that.

Comment 6

[Alice0775 White]

[Tracking Requested - why for this release]: 
I can reproduce the problem on Nightly51.0a1, Aurora50.0a2, Beta49.0b2 and Firefix48.0 on Windows10.

Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=9a8ff2f4978493c3fd9982652a09ffcdf2333a75&tochange=d4b74afcc60b937f25d54f365be46a8133980209

Regressed by: Bug 1253673

Comment 7

[Tanvi Vyas[:tanvi]]

moz-safe-about doesn't get blocked by mixed content blocker.  But there are other about: protocols that are not marked as "safe" that get treated as mixed content:
http://searchfox.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#529

Does about:blocked get the URI flag URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT?

Looking at the regression range, perhaps bug https://bugzilla.mozilla.org/show_bug.cgi?id=1253673 changed how we treat about:blocked?

Really, we should change the Mixed Content Blocker implementation.  I'm not sure if we should be using URI flags, or if we should just use isOriginPotentiallyTrustworthy.
http://searchfox.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#599

isOriginPotentiallyTrustworthy does use protocol flags, and instead whitelists specific schemes.  Maybe what we need is a combination of the logic in MixedContentBlocker and the logic in isOriginPotentiallyTrustworthy.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Does about:blocked get the URI flag URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT?

No, because it's not nsIAboutModule::MAKE_LINKABLE.

> perhaps bug 1253673 changed how we treat about:blocked?

That bug changed the default for about: URIs from "linkable" (but overridable with MAKE_UNLINKABLE) to "unlinkable" and added MAKE_LINKABLE to some of them, but not all.  So yes, about:blocked used to be URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and now is not.

It sounds like we need to decouple the URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT bit from MAKE_LINKABLE, perhaps?  I'm not sure why it was conditioned on linkability before bug 1253673.  Do you know?  The main point of MAKE_LINKABLE is to control whether URI_LOADABLE_BY_ANYONE is set....

> isOriginPotentiallyTrustworthy does use protocol flags, and instead whitelists specific schemes.

That means it's fundamentally broken for extension-implemented protocols, right?  The whole point of using protocol flags is that it lets us handle protocols implemented by extensions correctly...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Looks like that bit was added in bug 1172165.  Before that the safe-about bit controlled all this stuff, and this particular URI is safe-about.

It really looks to me like we should set URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT just based on URI_SAFE_FOR_UNTRUSTED_CONTENT without reference to MAKE_LINKABLE.  Or possibly always set it for about: URIs, even....

I would needinfo Gijs, but he is not accepting needinfos.  And by the time he gets back I may be on vacation.

I guess needinfo on me to remember to look into this whenever I get back.

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> > Does about:blocked get the URI flag URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT?
> 
> No, because it's not nsIAboutModule::MAKE_LINKABLE.
> 
> > perhaps bug 1253673 changed how we treat about:blocked?
> 
> That bug changed the default for about: URIs from "linkable" (but
> overridable with MAKE_UNLINKABLE) to "unlinkable" and added MAKE_LINKABLE to
> some of them, but not all.  So yes, about:blocked used to be
> URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and now is not.
> 
> It sounds like we need to decouple the URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT
> bit from MAKE_LINKABLE, perhaps?  I'm not sure why it was conditioned on
> linkability before bug 1253673.  Do you know?  The main point of
> MAKE_LINKABLE is to control whether URI_LOADABLE_BY_ANYONE is set....
> 
This is the first I'm hearing about the linkable/unlinkable flags.  From what I knew, we had two categories of about: pages.  Some are safe to embed as subresources (moz-safe-about) and some are not.  When you say the default used to be that about: uris were "linkable", does that translate into about: uris were marked as safe (moz-safe-about) by default?

Needinfo'ing dveditz since he has a better understand of why some about: urls are "safe" and others are not.

> > isOriginPotentiallyTrustworthy does use protocol flags, and instead whitelists specific schemes.
> 
That should say "doesn't use protocol flags".

> That means it's fundamentally broken for extension-implemented protocols,
> right?  The whole point of using protocol flags is that it lets us handle
> protocols implemented by extensions correctly...

Yes that means it is broken.  We need someone to do the following:
* Fix isOriginPotentiallyTrustworthy to use protocol flags + potentially some whitelisted schemes (ex: localhost)
* Fix nsMixedContentBlocker to call isOriginPotentiallyTrustworthy
* Fix the protocol flags for about: pages, either by decoupling linkable and safe_to_load_in_secure_context, or by adding the linkable flag to the missing about: pages.  The later means that we will continue to run into this bug many times as new about: pages are added without setting the linkable flag.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> From what I knew, we had two categories of about: pages.

Looks like that hasn't been the case since bug 1150703, a year ago.  That patch decoupled the concept of "safe" into two dimensions, kinda:

1)  Does the about: page get forced into a non-system principal or not?  This is the "about" vs "moz-safe-about" distinction ever since that patch bug. 

2)  Can the about: page be linked to by untrusted content?  This is only allowed for forced-non-system pages which are also linkable.

So now we have three states: may-be-system, non-system but not linkable, non-system and linkable.  Only the last one of these gets URI_LOADABLE_BY_ANYONE for purposes of CheckLoadURI checks.

> does that translate into about: uris were marked as safe (moz-safe-about) by default?

No, the whole linkable vs non-linkable distinction is all about two subsets of the "safe" about: URIs.  Some are linkable from content, some are not.  All are "safe" in that they all guarantee a non-system principal.

I think it should be fine to set URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT for all "safe" about: URIs.  And possibly for all about: URIs in general.  I can't think offhand of cases in which an about: URI is OK for CheckLoadURI purposes (or had CheckLoadURI bypassed as in this safe browsing case) but is OK to load inside only non-https content pages...  But the safe thing would be to do it for all the "safe" ones, since that gets us back to the state before bug 1253673, more or less.

Comment 12

[Daniel Veditz [:dveditz]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> It really looks to me like we should set URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT
> just based on URI_SAFE_FOR_UNTRUSTED_CONTENT without reference to
> MAKE_LINKABLE.  Or possibly always set it for about: URIs, even....

I agree: should be fine to make all about: URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT. That flag is about whether the transport is insecure or not and for the most part we can pretend all about: are built-in (the reality is sometimes different). The other flags (linkable and loadable-by-anyone) control whether it's even possible for an https page to load the thing, but that's a different issue and, as we see here, sometimes things "get loaded" that can't be loaded directly.

(In reply to Tanvi Vyas [:tanvi] from comment #10)
> > That means it's fundamentally broken for extension-implemented protocols,
> > right?  The whole point of using protocol flags is that it lets us handle
> > protocols implemented by extensions correctly...

Yup, that was the whole point of URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT!

> * Fix isOriginPotentiallyTrustworthy to use protocol flags + potentially
> some whitelisted schemes (ex: localhost)

Not "localhost". We've added explicit 127.0.0.1 and ::1 as a "potentially trustworthy origin" in the spec but we've held off on "localhost" because we and the Chrome team have encountered weird set-ups where it resolves to something remote. Then again maybe Mike is coming around to including that one: https://github.com/w3c/webappsec-secure-contexts/issues/43

> * Fix the protocol flags for about: pages, either by decoupling linkable and
> safe_to_load_in_secure_context, or by adding the linkable flag to the
> missing about: pages.

The former, please. There's no connection between "linkable" and "secure transport", and we don't want to add the linkable flag back to the about: urls we explicitly removed it from in that other bug.

Comment 13

[Ethan Tseng [:ethan]]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> Ethan still sees the warnings in Nightly, but I don't and arroway does not
> in 48.
Sorry I was wrong at the first place. I cannot see the warnings in Nightly actually.

Comment 14

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #12)
> (In reply to Boris Zbarsky [:bz] from comment #9)
> > It really looks to me like we should set URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT
> > just based on URI_SAFE_FOR_UNTRUSTED_CONTENT without reference to
> > MAKE_LINKABLE.  Or possibly always set it for about: URIs, even....
> 
> I agree: should be fine to make all about:
> URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT. That flag is about whether the transport
> is insecure or not and for the most part we can pretend all about: are
> built-in (the reality is sometimes different). The other flags (linkable and
> loadable-by-anyone) control whether it's even possible for an https page to
> load the thing, but that's a different issue and, as we see here, sometimes
> things "get loaded" that can't be loaded directly.

I concur. I think my implementation in bug 1253673 was wrong in that it didn't leave that flag on all content-privileged ("safe for untrusted content") URIs - in hindsight, I must have misunderstood the purpose of the flag. It's somewhat unfortunate that both are URI_SAFE_* flags but that their 'standards' for security are somewhat different, but fixing that can be a separate bug.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Doesn't seem to be critical enough for a 48 dot release.

Comment 16

[Henry Chang [:hchang]]

Attachment: Bug 1293476 - Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.

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

Comment 17

[Henry Chang [:hchang]]

Comment on attachment 8781038 [details]
Bug 1293476 - Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.

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

Comment 18

[Henry Chang [:hchang]]

Hi Gijs,

Per comment 14 and the previous comments, I submitted a patch to 

1) Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.
2) Mark all safe about: pages URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT.

Could you have it a review :)

Hi Tanvi,

You mentioned we have to do something around |isOriginPotentiallyTrustworthy|
according to your comment 10. Do you think we have to do it even if we
decoupled URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE?

Thanks you two!

Comment 19

[:Gijs (he/him)]

Comment on attachment 8781038 [details]
Bug 1293476 - Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.

https://reviewboard.mozilla.org/r/71552/#review69046

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:94
(Diff revision 2)
> -        (aboutModuleFlags & nsIAboutModule::MAKE_LINKABLE)) {
> -        *aFlags = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE |
> -            URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT;
> +        // See Bug 1293476 -
> +        //
> +        //   1) Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and
> +        //      MAKE_LINKABLE.
> +        //   2) Mark all safe about: pages URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT.
> +        //

I don't think this comment helps the reader understand, also because you left the comment before the if block the same. Please:

- change the comment before the if block to say something like "Secure (https) pages can load safe about pages without becoming mixed content"
- add a comment before the inner if statement that says something like "about: pages can only be loaded by unprivileged principals if they are marked as LINKABLE".

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:100
(Diff revision 2)
> +        *aFlags = URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT;
> +
> +        if (aboutModuleFlags & nsIAboutModule::MAKE_LINKABLE) {
> +            *aFlags |= URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE;
> +        }

This changes behaviour for the 'safe for untrusted content' URIs that do not have the 'linkable' flag, which is most of them.

Before your patch, their value of `aFlags` would have been the protocol default:

```
*result = URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD;
```

(from `GetProtocolFlags`, which is called a few lines earlier and will have altered the value of `aFlags`, and which you're overwriting here).

Now you're setting it to *only* `URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT`, which misses out (a) a security flag saying whether you need the system principal to load them, and (b) the noauth/norelative specifiers. So, if I'm reading this right, this patch is actually dangerous the way it's written, because it makes all the "safe" about: pages loadable from content principals again (!), which shouldn't be the case. (Part of the problem here is that the 'default' for the flags that control who can load a particular URL is "everybody" (ie not specifying a flag == specifying URI_LOADABLE_BY_ANYONE), which is an unhappy default that is not your fault, but one we have to live with anyway...)



TL;DR: please OR in the `URI_SAFE_TO_LOAD_IN_SECURE_CONTEXTS` into `aFlags` for the `URI_SAFE_FOR_UNTRUSTED_CONTENT` case, and if `MAKE_LINKABLE` is also specified, remove the `URI_DANGEROUS_TO_LOAD` flag from `aFlags` and replace it with `URI_LOADABLE_BY_ANYONE` .

Comment 20

[:Gijs (he/him)]

Note that I am technically not a peer, and therefore shouldn't review caps/ changes unless asked by a peer/owner. bz is away - maybe you can ask dveditz to do your next review?

Also... did you trypush this change? I really kind of hope it would have failed some tests... if it didn't (which is possible and not your fault!) we should add some in a different bug, so that we don't inadvertently end up regressing who can open these about: links...

Comment 21

[Henry Chang [:hchang]]

Comment on attachment 8781038 [details]
Bug 1293476 - Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.

https://reviewboard.mozilla.org/r/71552/#review69046

> This changes behaviour for the 'safe for untrusted content' URIs that do not have the 'linkable' flag, which is most of them.
> 
> Before your patch, their value of `aFlags` would have been the protocol default:
> 
> ```
> *result = URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD;
> ```
> 
> (from `GetProtocolFlags`, which is called a few lines earlier and will have altered the value of `aFlags`, and which you're overwriting here).
> 
> Now you're setting it to *only* `URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT`, which misses out (a) a security flag saying whether you need the system principal to load them, and (b) the noauth/norelative specifiers. So, if I'm reading this right, this patch is actually dangerous the way it's written, because it makes all the "safe" about: pages loadable from content principals again (!), which shouldn't be the case. (Part of the problem here is that the 'default' for the flags that control who can load a particular URL is "everybody" (ie not specifying a flag == specifying URI_LOADABLE_BY_ANYONE), which is an unhappy default that is not your fault, but one we have to live with anyway...)
> 
> 
> 
> TL;DR: please OR in the `URI_SAFE_TO_LOAD_IN_SECURE_CONTEXTS` into `aFlags` for the `URI_SAFE_FOR_UNTRUSTED_CONTENT` case, and if `MAKE_LINKABLE` is also specified, remove the `URI_DANGEROUS_TO_LOAD` flag from `aFlags` and replace it with `URI_LOADABLE_BY_ANYONE` .

The reason I used '=' but not '|=' is because the original is also '='. 
But you are totally right: I missed the case where (URI_SAFE_FOR_UNTRUSTED_CONTENT && !MAKE_LINKABLE).

Thanks!

Comment 22

[Henry Chang [:hchang]]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #20)
> Note that I am technically not a peer, and therefore shouldn't review caps/
> changes unless asked by a peer/owner. bz is away - maybe you can ask dveditz
> to do your next review?
> 
> Also... did you trypush this change? I really kind of hope it would have
> failed some tests... if it didn't (which is possible and not your fault!) we
> should add some in a different bug, so that we don't inadvertently end up
> regressing who can open these about: links...

I just triggered a trypush and waiting for the failures :p Thanks for your review
anyway!

Comment 23

[Henry Chang [:hchang]]

Comment on attachment 8781038 [details]
Bug 1293476 - Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.

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

Comment 24

[Henry Chang [:hchang]]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> It could also be a difference between the clean-ish testing profile I used
> in ESR-45 and my old default profile.

If you run from scratch, the safebrowsing database may not be downloaded and updated to the latest. If the safebrowsing database is not ready, the contents would always be considered non-mixed since the iframe URL is https.

Comment 25

[Tanvi Vyas[:tanvi]]

(In reply to Henry Chang [:henry][:hchang] from comment #18)
> 
> Hi Tanvi,
> 
> You mentioned we have to do something around |isOriginPotentiallyTrustworthy|
> according to your comment 10. Do you think we have to do it even if we
> decoupled URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE?
> 
> Thanks you two!

We do have to do something to isOriginPotentiallyTrustworthy... it currently doesn't use any URI flags and only whitelists certain schemes.  We have to also check a few URI flags.  That can be done in a separate bug.  If you take that bug, you can start by looking at the URI flags that nsMixedContentBlocker sets and then during the review Dan and I can take a closer look to make sure those are still the ones we want to use (or if there are any we want to add or remove).

Comment 26

[Henry Chang [:hchang]]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #20)
> Note that I am technically not a peer, and therefore shouldn't review caps/
> changes unless asked by a peer/owner. bz is away - maybe you can ask dveditz
> to do your next review?
> 
> Also... did you trypush this change? I really kind of hope it would have
> failed some tests... if it didn't (which is possible and not your fault!) we
> should add some in a different bug, so that we don't inadvertently end up
> regressing who can open these about: links...

Here comes the result:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c23003344fb7

It seems no relevant failures. Did I not run enough tests?

Comment 27

[:Gijs (he/him)]

(In reply to Henry Chang [:henry][:hchang] from comment #26)
> (In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #20)
> > Note that I am technically not a peer, and therefore shouldn't review caps/
> > changes unless asked by a peer/owner. bz is away - maybe you can ask dveditz
> > to do your next review?
> > 
> > Also... did you trypush this change? I really kind of hope it would have
> > failed some tests... if it didn't (which is possible and not your fault!) we
> > should add some in a different bug, so that we don't inadvertently end up
> > regressing who can open these about: links...
> 
> Here comes the result:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c23003344fb7
> 
> It seems no relevant failures. Did I not run enough tests?

No, I think the problem is there don't seem to be tests. I filed bug 1295543.

Comment 28

[:Gijs (he/him)]

(Also, I think I realized too late that technically this is a networking rather than a caps patch, which means that potentially :valentin, :jduell or :mcmanus can help with reviews if dveditz is busy.)

Comment 29

[Marcia Knous [:marcia]]

Tracking 51+ for this regression.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 8781038 [details]
Bug 1293476 - Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.

https://reviewboard.mozilla.org/r/71552/#review70564

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:95
(Diff revision 3)
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    // If marked as safe, and marked linkable, pass 'safe' flags.
> -    if ((aboutModuleFlags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) &&
> -        (aboutModuleFlags & nsIAboutModule::MAKE_LINKABLE)) {
> -        *aFlags = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE |
> +    // Secure (https) pages can load safe about pages without becoming
> +    // mixed content.
> +    if (aboutModuleFlags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) {
> +        *aFlags |= URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT;

I don't see a problem making all about: urls URI_SAFE_FOR_UNTRUSTED_CONTENT, that is, adding this flag into the default GetProtocolFlags() value. In practice it doesn't matter which way you go, but doing that makes this change potentially cleaner.

Comment 31

[:Gijs (he/him)]

(In reply to Daniel Veditz [:dveditz] from comment #30)
> Comment on attachment 8781038 [details]
> Bug 1293476 - Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE.
> 
> https://reviewboard.mozilla.org/r/71552/#review70564
> 
> ::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:95
> (Diff revision 3)
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > -    // If marked as safe, and marked linkable, pass 'safe' flags.
> > -    if ((aboutModuleFlags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) &&
> > -        (aboutModuleFlags & nsIAboutModule::MAKE_LINKABLE)) {
> > -        *aFlags = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE |
> > +    // Secure (https) pages can load safe about pages without becoming
> > +    // mixed content.
> > +    if (aboutModuleFlags & nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT) {
> > +        *aFlags |= URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT;
> 
> I don't see a problem making all about: urls URI_SAFE_FOR_UNTRUSTED_CONTENT,
> that is, adding this flag into the default GetProtocolFlags() value. In
> practice it doesn't matter which way you go, but doing that makes this
> change potentially cleaner.

Errr, you mean URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT, right? URI_SAFE_FOR_UNTRUSTED_CONTENT controls whether the page uses chrome/content privileges, URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT is about whether the channel through which we fetch the context is private (either local or encrypted).

More fodder for my "these flag names are confusing" argument!

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

We'd consider uplift all the way to beta if this can land and bake... Are there still concerns with the current patch?

Comment 34

[Daniel Veditz [:dveditz]]

This should be completely safe.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91eee4bb4b80
Decouple URI_SAFE_TO_LOAD_IN_SECURE_CONTEXT and MAKE_LINKABLE. r=dveditz

Comment 36

[Henry Chang [:hchang]]

(In reply to David Bolter [:davidb] from comment #33)
> We'd consider uplift all the way to beta if this can land and bake... Are
> there still concerns with the current patch?

(In reply to Daniel Veditz [:dveditz] from comment #34)
> This should be completely safe.

Same here :)

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/91eee4bb4b80

Comment 38

[Gerry Chang [:gchang]]

Track 49+/50+ as this is a regression and is related to safe browsing.

Hi Henry,
Since this bug is a regression and also affects 49/50, are you also considering to uplift this patch to 49/50 if it's not too risky?

Comment 39

[Henry Chang [:hchang]]

(In reply to Gerry Chang [:gchang] from comment #38)
> Track 49+/50+ as this is a regression and is related to safe browsing.
> 
> Hi Henry,
> Since this bug is a regression and also affects 49/50, are you also
> considering to uplift this patch to 49/50 if it's not too risky?

Yes it's fine to uplift but I'd like to leave the fix lives
in m-c for a while (like a week) then do the uplift just to
be on the safe side.

Keep the ni as a reminder :)

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

We don't really have a week to wait, so I think it may be better to uplift this now to beta, and test it on beta quickly. 

Beta 9 build is Thursday, then we have the release candidate build on Monday.

Comment 41

[Henry Chang [:hchang]]

Attachment: Patch to uplift

Comment 42

[Henry Chang [:hchang]]

Comment on attachment 8786560 [details] [diff] [review]
Patch to uplift

Approval Request Comment
[Feature/regressing bug #]: Bug 1293476
[User impact if declined]: Blocked/malicious pages would be shown in the iframe.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: No
[String/UUID change made/needed]: No

Comment 43

[Gerry Chang [:gchang]]

Comment on attachment 8786560 [details] [diff] [review]
Patch to uplift

Review of attachment 8786560 [details] [diff] [review]:
-----------------------------------------------------------------

This patch fixes a regression and is related to safe browsing. Take it in 49 beta. Should be in 49 beta 9.

Comment 44

[Gerry Chang [:gchang]]

Hi Henry,
Please also create an uplift request for 50. We need this in 50 aurora, too.

Comment 45

[Henry Chang [:hchang]]

Comment on attachment 8786560 [details] [diff] [review]
Patch to uplift

Approval Request Comment
[Feature/regressing bug #]: Bug 1293476
[User impact if declined]: Blocked/malicious pages would be shown in the iframe.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: No
[String/UUID change made/needed]: No

Comment 46

[Gerry Chang [:gchang]]

Comment on attachment 8786560 [details] [diff] [review]
Patch to uplift

Review of attachment 8786560 [details] [diff] [review]:
-----------------------------------------------------------------

Take it in 50 aurora.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2031351aaad9

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3ca29b6e9f8c

Comment 49

[Ryan VanderMeulen [:RyanVM]]

It's worrying to me that this landed without a regression test. Can we please add one?

Comment 50

[Henry Chang [:hchang]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #49)
> It's worrying to me that this landed without a regression test. Can we
> please add one?

Hi,Ryan, what did you mean a regression test specifically here?
Did you mean what :Gijs mentioned in comment 27 or just a test case
to verify if the fix really resolves the issue? Thanks :)

Comment 51

[Ryan VanderMeulen [:RyanVM]]

I would assume that the latter covers the former here. This just seems like a pretty scary thing to break and it would be nice to have assurances that we won't accidentally do so again in the future. Especially since in this situation, we shipped a release with it too.

Comment 52

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #51)
> I would assume that the latter covers the former here. This just seems like
> a pretty scary thing to break and it would be nice to have assurances that
> we won't accidentally do so again in the future. Especially since in this
> situation, we shipped a release with it too.

Concur, we should try to get bug 1295543 prioritized.

Comment 53

[:Gijs (he/him)]

(In reply to :Gijs from comment #52)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #51)
> > I would assume that the latter covers the former here. This just seems like
> > a pretty scary thing to break and it would be nice to have assurances that
> > we won't accidentally do so again in the future. Especially since in this
> > situation, we shipped a release with it too.
> 
> Concur, we should try to get bug 1295543 prioritized.

Actually, that bug isn't about the failure in this bug... I'll file a separate bug.

Comment 54

[:Gijs (he/him)]

Apologies for the bugspam. :-\