1403998 - view-source gets wrong result principal URI on redirects

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: can lead to people accidentally loading dangerous code (e.g. it just happened to me on a crash testcase in bugzilla).  Probability is low, though.

STEPS TO REPRODUCE:

1) Open a new tab.
2) Paste view-source:https://bugzilla.mozilla.org/attachment.cgi?id=8444969 in the URL bar.
3) Hit enter.
4) Close tab.
5) Undo close tab.

EXPECTED RESULTS:
* After step 3, URL in URL bar is view-source:https://bug1027978.bmoattachments.org/attachment.cgi?id=8444969
* After step 5, URL in URL bar is same as after step 3 and source is shown.

ACTUAL RESULTS:
* After step 3, URL in URL bar is "https://bug1027978.bmoattachments.org/attachment.cgi?id=8444969"
* After step 5, the web page is rendered instead of the source being shown.

I expect this is fallout from bug 1319111.

Honza, can you take a look, please?

Comment 1

[Honza Bambas (:mayhemer)]

This is bizarre!  Just today after a very long time I remembered the bug 1319111 and that it's been sticking with no problems! :))

Will look at this.

Comment 2

[Honza Bambas (:mayhemer)]

This IMO used to work (before 1319111) a bit "by accident".  


Before 1319111:

If there was no redirect of the http channel, there was no LOAD_REPLACE flag set and we used originalURI of the view source channel as the result principal URI (final channel URI), which was (and still is) the view-source:http:// url.

When there was a redirect, GetLoadFlags of the view source channel, forwarded to the target http channel (updated in v-s channel's OnStartRequest), returned LOAD_REPLACE.  That led to using GetURI on the v-s channel which rebuilds the view-source URI from the http channel's *URI*.


After 1319111:

If there is no redirect of the http channel, there also is no rpURI on the load info set (http doesn't set it for non-redirected channels).  Hence, we use v-s channel's originalURI as result URI.

If there is a redirect, the loadinfo of the redirected channel is set to the original URL of the redirected channel.  v-s channel's GetLoadInfo simply redirects to the http channel, now being the redirected one.


Hence, what we have to do is to update the rpURI (break some rules, actually*) on the redirected channel's load info (or clone its loadinfo to the v-s channel) to be view-source:http:// again.  I believe we can do this in nsViewSourceChannel::OnStartRequest, since all the redirect veto logic will see the original http channel and the target http channel.  Updating the loadinfo in AsyncOnChannelRedirect would break it.

So, I'm personally for cloning the load info into the v-s channel and modify it accordingly.  I think we may need to do it even when the redirect is not happening to be consistent in case when the handler sets rpURI even for the very first channel (some different schema, maybe).

Comment 3

[Boris Zbarsky [:bzbarsky]]

That sounds plausible, yes.  Thank you for looking into this!

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

- the patch finally does NOT clone the loadinfo
- we replace the underlying channel's loadinfo with the one given to NS_NewChannel*, so we can modify that directly in the view-source protocol handler during v-s channel creation
- the redirected channel uses clone of that loadinfo, hence an update from inside v-s channel's OnStartRequest is IMO OK to do (it's a clone of "our" loadinfo)
- code comments should explain the rest

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

Comment 5

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

Tracking for 56, sounds worrisome but maybe a bit complicated to test and fix on the release channel. 
We can try landing this, verifying the fix, and hopefully, uplift at least as far as 57.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8913771 [details] [diff] [review]
v1

>+                     /* XXX Gross hack -- NS_NewURI goes into an infinite loop on
>+                     non-flat specs.  See bug 136980 */

That bug is long-since fixed.  Maybe a followup to remove this comment and the hack?

>+++ b/netwerk/protocol/viewsource/nsViewSourceHandler.cpp

Why are the changes here needed?  That is, why is the OnStartRequest change enough?

r=me with that last bit explained or removed.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #6)
> Comment on attachment 8913771 [details] [diff] [review]
> v1
> 
> >+                     /* XXX Gross hack -- NS_NewURI goes into an infinite loop on
> >+                     non-flat specs.  See bug 136980 */
> 
> That bug is long-since fixed.  Maybe a followup to remove this comment and
> the hack?

bug 1405022

> 
> >+++ b/netwerk/protocol/viewsource/nsViewSourceHandler.cpp
> 
> Why are the changes here needed?  That is, why is the OnStartRequest change
> enough?

Hmm... you are right.  We actually replace the loadinfo, so anything that the underlying protocol may have set will be erased.  No need to update (ensure) to view-source URI then.

Will remove.


> 
> r=me with that last bit explained or removed.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Update according comment 6.  I don't believe we need another try push.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f693bf1345f9
Make view-source channels return correct resultPrincipalURI via LoadInfo after redirect. r=bz

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f693bf1345f9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8914396 [details] [diff] [review]
v1.1

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1319111

[User impact if declined]:
when view-source inner URI redirects: partially broken view-source functionality (on tab restore), potential security considerations since the URL we do security checks against has different (probably higher) privileges than the inner (http) url

[Is this code covered by automated tests?]:
no

[Has the fix been verified in Nightly?]:
yes, manually by me

[Needs manual test from QE? If yes, steps to reproduce]: 
probably, but not necessarily, just in case - comment 0 has a very good steps to reproduce

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
I tend to say no

[Why is the change risky/not risky?]:
we reuse the same functionality that has been in the code for nsViewSourceChannel::GetURI for updating the result principal URI ; the change is well understood and straightforward

[String changes made/needed]:
none

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914396 [details] [diff] [review]
v1.1

Recent regression, Beta57+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f22fa49006ca

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Too late for 56.

Comment 16

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 58.0a1 (2017-09-28) on WIndows 8.1, 64 Bit!

This Bug's fix is verified with latest Beta and latest Nightly!

Build ID   : 20171016185129
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0



Build ID   : 20171023220222
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Comment 17

[Mohammed Adam]

I have reproduced this bug with Nightly 59.0a1 (2017-12-26) Windows 7(32-bit)

This Bug's fix is verified with latest Nightly!

Build ID   : 20171226100306
User Agent : Mozilla/5.0 (Windows NT 6.1; rv:59.0) Gecko/20100101 Firefox/59.0