Open view-source:https:// link in a new tab is not working on view-source:http:// pages (and vice versa)

RESOLVED FIXED in Firefox 51

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Swarnava, Assigned: Gijs)

Tracking

(Blocks 1 bug, {regression})

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 wontfix, firefox49 wontfix, firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

Attachments

(1 attachment)

STR:- 
Go to view-source:http://swarnava.in/
Click on any link, or right click and click Open link on new tab.

Expected behavior:-
It should open link on a new tab

Actual behavior:-
Nothing happening
Reproducible
Version 	47.0.1
Build ID 	20160623154057
User Agent 	Mozilla/5.0 (X11; Linux i686; rv:47.0) Gecko/20100101 Firefox/47.0
Setting Even Handling for further investigation.
Access link above for swarnava
Open in new tab https://www.kayako.com/ 
Open in new tab https://www.mozilla.org/ 
  - Nothing Happens - No New Tab opens when clicking these links.
Component: Untriaged → Event Handling
Product: Firefox → Core
Hi Olli, do you know if this is a regression?
Flags: needinfo?(bugs)
Hmm, some links do open in a new tab, http://twitter.com/sw4rn4v4 for example, but
https://www.mozilla.org/ doesn't, nor https://www.kayako.com/
Hmm, something somewhere throws
JavaScript error: , line 0: uncaught exception: Load of view-source:https://www.mozilla.org/ from view-source:http://swarnava.in/ denied.
This is a regression since FF46 works fine.

 NS_IMETHODIMP
 nsViewSourceHandler::GetProtocolFlags(uint32_t *result)
 {
-    *result = URI_NORELATIVE | URI_NOAUTH | URI_DANGEROUS_TO_LOAD |
+    *result = URI_NORELATIVE | URI_NOAUTH | URI_LOADABLE_BY_ANYONE |
         URI_NON_PERSISTABLE;
     return NS_OK;
 }
fixes this, so backing out part of bug 1172165.

Gijs is on vacation and not accepting needinfos.
Blocks: 1172165
Flags: needinfo?(bugs)
I performed MozRegression - here are the results
14:11.15 INFO: Last good revision: e8d1d92fa59f8648497ba43975e11dbe45e375ad
14:11.15 INFO: First bad revision: c714d452b06e0cdb916909de3682abea7cc987e5
14:11.15 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e8d1d92fa59f8648497ba43975e11dbe45e375ad&tochange=c714d452b06e0cdb916909de3682abea7cc987e5

14:11.41 INFO: Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1277583
I'm taking a stab at a better component here but CAPS still doesn't feel quite right ...
Component: Event Handling → Security: CAPS
Hi Gijs, could you please take per comment 5 and comment 6? Thanks.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 9

3 years ago
(In reply to Olli Pettay [:smaug] from comment #3)
> Hmm, some links do open in a new tab, http://twitter.com/sw4rn4v4 for
> example, but
> https://www.mozilla.org/ doesn't, nor https://www.kayako.com/

So the issue is that not all protocols are equal when an http:// thing links to an https:// thing. It's possible we should allow this for the "are all the protocols the same" check... Would that make sense?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugs)
Assignee

Updated

3 years ago
Summary: Open link in a new tab is not working on view-source:http://swarnava.in/ → Open view-source:https:// link in a new tab is not working on view-source:http:// pages (and vice versa)
I'm really not at all familiar with recent changes to view-source handling and don't understand why anything was restricted for top level view-source...

But looking at code... shouldn't AllSchemesMatch do some flag checks, not scheme checks.
(and then its name should be changed). Or perhaps AllSchemesMatch should be backed out and
CheckLoadURIFlags should handle nested uris.
Does that make sense?
Flags: needinfo?(bugs)
Assignee

Comment 11

3 years ago
(In reply to Olli Pettay [:smaug] from comment #10)
> I'm really not at all familiar with recent changes to view-source handling
> and don't understand why anything was restricted for top level view-source...

We restricted the top-level view-source: protocol because it's been an ongoing part of security issues, e.g. bug 1248209. It's basically not safe-for-web. IE removed access to it years ago, we did it more recently, to avoid shooting ourselves in the foot with it any longer.

> But looking at code... shouldn't AllSchemesMatch do some flag checks, not
> scheme checks.

Almost all flag checks already happen across the entire nested URI chain (I just noticed we missed out URI_IS_UI_RESOURCE, which doesn't impact this bug, or any others, AIUI). This was the change that bug 1172165 made, in fact. Previously, we did flag checks *on the inner URI*. So if you had:

view-source:jar:file:///

the flag checks happened on file:///

and if you had:

view-source:data:

then it happened on data: .

This meant that restricting view-source was impossible, because flag checks happened on the inner URI, ignoring flags on the outer URI.

Simultaneously, we used to have a check for the inner schemes to be equal that ignored flags. So if you had a foo:// URI it could always link to another foo:// URI. But also, view-source:jar:foo:// could link to foo:// and http:// could link to view-source:http:// .

So we changed the flag checks to happen on the entire URI (outer-to-inner) and made view-source "dangerous to load". If *any* of the protocols in the URI chain have that flag, we deny access.

Unfortunately, that wouldn't be enough to stop http:// linking to view-source:http://, because the inner schemes are still the same, and we do the scheme equality check before the flag checks. So we also changed the scheme matching to check *all* the schemes. So we only ignore protocol flags if the entire scheme chain is identical, so jar:file:// can link to jar:file://, but file:// can only link to jar:file:// if the protocol flags allow it to do so.

> (and then its name should be changed). Or perhaps AllSchemesMatch should be
> backed out and
> CheckLoadURIFlags should handle nested uris.
> Does that make sense?

Based on the above, I don't think so? CheckLoadURIFlags already uses aTargetURI for the check for DANGEROUS_TO_LOAD, which is a flag that's set for view-source (and for feed:, by now, in fact...). Maybe you mean something else when you say "handle" nested URIs?

In a way, the issue is that one DANGEROUS_TO_LOAD URI/principal can't load another one unless all their schemes are identical. In fact, even if we relaxed that last check to make it check only the inner schemes, that would still break view-source:http to view-source:https links.

Thinking about this a bit more, I'm not even convinced my earlier suggestion to treat http and https as identical for the purposes of AllSchemesMatch is a sane thing to do. Right now, the plain linking of http to https passes through the flag checking path where both have URI_LOADABLE_BY_ANYONE, but linking from http to http passes through the "these schemes are equal" path (and did so before the changes in bug 1172165 as well).
Priority: -- → P2
Hi Gijs, I am inferring this is something to fix in the next release/in a few months, so set P2. Please change it as you see fit.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 13

3 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> Hi Gijs, I am inferring this is something to fix in the next release/in a
> few months, so set P2. Please change it as you see fit.

P2 sounds OK. I just wish we had an idea about *how* to fix, exactly, short of very ugly view-source specific hacks...
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > Hi Gijs, I am inferring this is something to fix in the next release/in a
> > few months, so set P2. Please change it as you see fit.
> 
> P2 sounds OK. I just wish we had an idea about *how* to fix, exactly, short
> of very ugly view-source specific hacks...

Have you had any ideas recently? :)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 15

3 years ago
The best thing I can think of is a new protocol flag called something like "URI_LOADABLE_IF_INNER_LOADABLE" or whatever, and iff

- both URI A and URI B have the same scheme/protocol
- that protocol has that flag
- both have inner URIs

we return the result of CheckLoadURIWithPrincipal for the two inner URIs.

Of course, we could consider doing this without a special flag if both URIs have the same outer protocol and both have an inner URI (though we might need to change some of the reporting so it's clearer, ie so we complain about e.g. foo:bar can't link to foo:baz rather than just bar can't link to baz). I just don't know if that would break anything else. Olli/Boris, thoughts / sanity checks?
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
Still trying to page this stuff back in, but it seems like a reasonable general policy might be to pass CheckLoadURI checks if, going through the entire protocol chain, at each step the first protocol can link to the second.  That is, either they're identical or the second one allows linking from the first one in general.
Flags: needinfo?(bzbarsky)
Most likely too late for 49 (building RC2 tomorrow), but if someone works on this it could still land for 50.
Is this something we should try to find cycles to fix this cycle, Gijs?
Flags: needinfo?(gijskruitbosch+bugs)
(And is this something we could perhaps mentor another engineer to work on if you don't have the cycles?)
Assignee

Comment 20

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> (And is this something we could perhaps mentor another engineer to work on
> if you don't have the cycles?)

It's not that I'm unwilling to mentor someone, but I'm worried that while comment 16 is a general direction, I'm not sure how many problems you'd run into when implementing it. That is, when Boris says "still trying to page this stuff back in", and from my experience with this (having to fix reftest frameworks, edgecases, getting it wrong initially and having to crashland something on RC not to ship a vulnerable browser, ...) it's totally possible that when we try to do what comment 16 suggests, we run into issues on the way. It'd also be a significant refactoring of that bit of caps/ code, which not a lot of people are awfully familiar with. So all in all, doesn't seem like an amazing fit for mentoring new-ish contributors. If there are new platform engineers that we pay to get stuck into this kind of thing, sure... but I don't know where to find those people, if they even exist.

I think most things in my queue today will be finished in relatively short order (famous last words), so I might just see if I can at least get a WIP / some more ideas about how hard this is before either taking myself or mentoring someone else. I'll leave needinfo.
Flags: needinfo?(bugs)
Comment hidden (mozreview-request)
Assignee

Comment 22

3 years ago
I ripped out the b2g code because AIUI it's now officially dead ( https://groups.google.com/forum/#!msg/mozilla.dev.fxos/FoAwifahNPY/Lppm0VHVBAAJ ) and it was in the way. I could potentially put it back because I ended up with (ignoring the view-source case) only one NS_OK, so I guess I could reinsert it there, maybe with a helper or something to keep that loop readable? But if we're killing it anyway...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

I prefer smaug to review this code.
Attachment #8795275 - Flags: review?(amarchesini) → review?(bugs)

Comment 25

3 years ago
mozreview-review
Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

https://reviewboard.mozilla.org/r/81394/#review80334

::: caps/nsScriptSecurityManager.cpp
(Diff revision 1)
> -                return NS_ERROR_DOM_BAD_URI;
> -            }
> +    }
> +
> +    // If we get here, check all the schemes can link to each other, from the top down:
> +    auto stringComparator = nsCaseInsensitiveCStringComparator();

Simpler to write:
nsCaseInsensitiveCStringComparator stringComparator;

::: caps/tests/mochitest/browser_checkloaduri.js:47
(Diff revision 1)
>      ["feed:https://www.example2.com", false, false, true],
>      ["chrome://foo/content/bar.xul", false, false, true],
>      ["feed:chrome://foo/content/bar.xul", false, false, false],
>      ["view-source:http://www.example2.com", true, true, true],
> +    ["view-source:https://www.example2.com", true, true, true],
>      ["view-source:feed:http://www.example2.com", false, false, true],

Could you add also tests for view-source:http* linking to view-source:data://*
Attachment #8795275 - Flags: review?(bugs) → review+

Comment 26

3 years ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2554774482
unbreak view-source links between http and https pages, r=smaug

Comment 27

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed2554774482
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee

Updated

3 years ago
Blocks: nukeb2g
Hey Gijs, should we uplift this to Aurora51/Beta50?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 29

3 years ago
I'm not convinced about how safe this is to uplift. Thoughts, Olli?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugs)
aurora maybe, but beta feels a bit risky. This needs some baking time.
Flags: needinfo?(bugs)
Assignee

Comment 31

3 years ago
Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

Approval Request Comment
[Feature/regressing bug #]: bug 1172165
[User impact if declined]: inter-view-source links that traverse from http to https or vice versa are broken
[Describe test coverage new/current, TreeHerder]: there are automated tests that cover this
[Risks and why]: low to medium. Changes to this bit of CAPS don't have an awesome history risk-wise (hi, bug 1277583). OTOH, there are now tests for obvious breakage, so we should have noticed if I made another stupid mistake on that scale. As Olli says, this needs baking. It's still another 3-4 weeks until aurora goes to beta, so there is indeed some actual baking potential. :-)
[String/UUID change made/needed]: no.
Attachment #8795275 - Flags: approval-mozilla-aurora?
Assignee

Comment 32

3 years ago
TBH, if sheriffs are keen to take this, you can... but I personally don't think the regression is bad enough to warrant the extra risk of uplifting. Very few users use view-source:, fewer users have links affected by this bug, and we already shipped this (twice!).
To be clear, s/sheriffs/RelMan
Assignee

Comment 34

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)
> To be clear, s/sheriffs/RelMan

Ugh, yes. Sorry. It's late, I should leave bugzilla until tomorrow.
Comment on attachment 8795275 [details]
Bug 1290668 - unbreak view-source links between http and https pages,

Fix a regression. Take it in 51 aurora.
Attachment #8795275 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Based on Smaug and Gij's assessment and the fact that this is a regression since 48 (at least), we'll let this fix ride the 51 train.
No longer blocks: 1369194
You need to log in before you can comment on or make changes to this bug.