619092 - wyciwyg URL is visible (document.write + stop + reload)

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Shotokan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; it; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; it; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13

If I dinamically write text in the document using the document.write() function, and then I reload the page (using F5, clicking the icon or using window.reload()) if i try to write text again using the same document.write() function, the page will change its url to wyciwyg://#/ plus the previous url.

Reproducible: Always

Steps to Reproduce:
1.Go on http://92.245.187.245/exploit.html with your JavaScript turned ON.
2.When the page shows "Done." instead of "Exploit in progress.." look at the URL.
Actual Results:  
The page changes its url to wyciwyg://#/ plus the previous url.

Expected Results:  
I should only be able to see the url without wyciwyg://#/.

the wyciwyg pseudo-URI is also accessible typing view-source:wyciwyg://#/ plus the page address.
all of that is not secure.

(the # is an auto-increment integer)

Comment 1

[Lucas Adamski [:ladamski]]

Does this help: https://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/ab351c9e99698064/34136b1c4cdba80b

Comment 2

[Shotokan]

hello lucas. Well, the post you showed me shows a firebug "bug"! But (in THIS case) firefox without firebug gives me the same bug. So, (I guess), it's a firefox bug, not a firebug "bug". I hope I've posted something useful.

Comment 3

[Jesse Ruderman]

I can reproduce the bug.

Not sure how severe this is. Can an attacker do anything other than obfuscate the URL in the address bar?

Comment 4

[Shotokan]

(In reply to comment #3)
> I can reproduce the bug.
> 
> Not sure how severe this is. Can an attacker do anything other than obfuscate
> the URL in the address bar?

well, that's all to see. in the far 2007 a man made an exploit that made use of that pseudo-URI to fake SSL certificates. Dunno if that's can be done again.

but the sure, unsecure thing, is that an attacker can see the source of a cached page (may contains sensitive data!) by typing view-source:wyciwyg://#/pageaddr , he've  just to "find" the #.

Hope that helps :)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One possible attack is that you can see how many other wyciwyg channels another page creates by running this attack twice and window.open-ing another page in between.

Comment 6

[Shotokan]

(In reply to comment #5)
> One possible attack is that you can see how many other wyciwyg channels another
> page creates by running this attack twice and window.open-ing another page in
> between.

Can you write an exploit or do you want me to do it?
Just to prove it

Comment 7

[Boris Zbarsky [:bzbarsky]]

This worksforme on trunk (though I can reproduce on 1.9.2).  Is that what others are seeing too?  If so, what's the trunk fix range?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The exploit works for me on trunk. The page displays:

Done. Url: "wyciwyg://107/http://92.245.187.245/exploit.html" contains "wyciwyg://"

Comment 9

[Boris Zbarsky [:bzbarsky]]

Interesting.  It does for me with a clean profile, but not with my usual (no-extensions) profile.  OK.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So what happens is that the docshell's current URI is set to something like this:

  wyciwyg://7/wyciwyg://6/file:///Users/bzbarsky/test.html

(in particular, it's a simple URI, with the part starting "//7" as the path).

In other words, somewhere in there we used a wyciwyg URI as the base for constructing another wyciwyg URI.  If that's supposed to be allowed, then createExposableURI needs to be smarter.  But I doubt it's supposed to be allowed...  Then again, I see nothing in nsHTMLDocument::CreateAndAddWyciwygChannel preventing it, if mDocumentURI is a wyciwyg URI.  Is it not, usually, for document.write cases?

Comment 11

[Boris Zbarsky [:bzbarsky]]

And in particular, should we be using createExposableURI to set mDocumentURI?  The latter is exposed directly in the DOM, right?

Comment 12

[Daniel Veditz [:dveditz]]

Seems that at best you could figure out how many document.writes() happened on a page, which might give you hints about what site/context you're in? Or am I missing something far more sinister?

Comment 13

[Shotokan]

(In reply to comment #12)
> Seems that at best you could figure out how many document.writes() happened on
> a page, which might give you hints about what site/context you're in? Or am I
> missing something far more sinister?

well, typing on the url view-source:wyciwyg://#/site you'll see the previously cached page, that's not secure |:
after that, the exploit i've posted it's only to show that the browser doesn't work properly.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Removing security flag; I don't think this is worth keeping hidden, and we should instead try to find someone to fix it, which is easier if this is not hidden.

Comment 15

[Mihai Morar, (:MihaiMorar)]

(In reply to Shotokan from comment #0)
> Reproducible: Always
> 
> Steps to Reproduce:
> 1.Go on http://92.245.187.245/exploit.html with your JavaScript turned ON.

Your URL mentioned in Comment 0 is no longer available on Latest Nightly 27 (nor Chrome or Opera). Can anyone provide any URL's that reproduces this isssue?

Comment 16

[Henri Sivonen (:hsivonen)]

Do we really still want to keep the wyciwyg feature? We could simplify things if we did what WebKit/Blink do with document.open() (use the URL of the old doc and break reloading document.open()ed docs from the network cache). Besides, it's already impossible to get pages that have both external and internal document.writes reload to their expected state from wyciwyg.

Does the wyciwyg stuff really bring competitive value over WebKit/Blink that justifies the trouble?

Comment 17

[Boris Zbarsky [:bzbarsky]]

> and break reloading document.open()ed docs from the network cache)

They also break back/forward across document.open(): they don't treat it as a navigation at all.  That's not what the spec says, what we or IE do, or what's user-friedly...

> Does the wyciwyg stuff really bring competitive value over WebKit/Blink that justifies
> the trouble?

An interesting question, indeed.

Comment 18

[Ioana (away)]

Please re-add the qawanted keyword if you decide to keep this and still need help from qa.

Comment 19

[Hallvord R. M. Steen [:hallvors]]

Attachment: 619092.htm - TC attempt, unsuccessful I think

Comment 20

[Hallvord R. M. Steen [:hallvors]]

I can't reproduce this (and the original URL is gone). Jesse - you could reproduce, can you recall what the test actually looked like?

Comment 21

[Jesse Ruderman]

I don't remember. Maybe Shotokan still has it somewhere, or maybe bz remembers?

Comment 22

[Shotokan]

(In reply to Jesse Ruderman from comment #21)
> I don't remember. Maybe Shotokan still has it somewhere, or maybe bz
> remembers?

I have it in the old server somewhere, let me check if I can find it.

Comment 23

[Shotokan]

(In reply to Jesse Ruderman from comment #21)
> I don't remember. Maybe Shotokan still has it somewhere, or maybe bz
> remembers?

I can't find it but I'll re-create the exploit page soon.
In the meanwhile, you can test it by adding this code to a bookmark and double clicking it.
javascript:document.write("test");window.stop();location.reload();
Confirmed working on firefox 26, wow.

Comment 24

[Jesse Ruderman]

I tried the bookmarklet in comment 23.  Nothing weird in the URL bar for me.  (Tried Firefox Nightly and Firefox 26 on Mac OS X 10.9.1.)

Shotokan, what OS are you using nowadays?  Can you reproduce the bug even in a new profile?

Comment 25

[Shotokan]

(In reply to Jesse Ruderman from comment #24)
> I tried the bookmarklet in comment 23.  Nothing weird in the URL bar for me.
> (Tried Firefox Nightly and Firefox 26 on Mac OS X 10.9.1.)
> 
> Shotokan, what OS are you using nowadays?  Can you reproduce the bug even in
> a new profile?

I tried it in Firefox 26 (fresh install) on windows 8.1, and Firefox 26 on Kubuntu 13.10.
Here you've a screenshot with Firefox 26 on Kubuntu: http://i.imgur.com/E4ZvrQV.png
Later today I'll release the exploit page, just in case.

Comment 26

[Shotokan]

(In reply to Jesse Ruderman from comment #24)
> I tried the bookmarklet in comment 23.  Nothing weird in the URL bar for me.
> (Tried Firefox Nightly and Firefox 26 on Mac OS X 10.9.1.)

By the way: you DOUBLE clicked it?

Comment 27

[Shotokan]

I've updated the exploit page. Enjoy.
http://vps.aitch.me/explffx.html

Comment 28

[Hallvord R. M. Steen [:hallvors]]

Yep, that demo shows a problem for me.

Comment 29

[Shotokan]

Attachment: exploit.html

Working exploit

Comment 30

[Shotokan]

I've uploaded an attachment so if the vps goes down we've the code there.

Comment 31

[Jesse Ruderman]

I can reproduce with the bookmarklet if I click it twice.  I can also reproduce using exploit.html. Thanks!

Comment 32

[hotsmileband]

Hi, could anyone assign this bug to me. And I would appreciate any information on where to start looking as I am completely new to code base.

Comment 33

[hotsmileband]

Oh, sorry. 
*Could anyone assign this bug to me?

Comment 34

[Boris Zbarsky [:bzbarsky]]

The place to start looking is probably by looking into what mDocumentURI ends up being after nsDocument::Open() returns.

Comment 35

[Kevin Brosnan [Ex-Mozilla]]

Releasing bug. hotsmileband if you are still working on this please comment and I'll add you back as the assignee.

Comment 37

[Daniele "Mte90" Scasciafratte]

any news about this?

Comment 38

[Jason Duell]

[Tracking Requested - why for this release]:  since bug 1131310 was duped to this one.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #38)
> [Tracking Requested - why for this release]:  since bug 1131310 was duped to
> this one.
It is a sec-low, why do you think we should track it?

Comment 40

[Sylvestre Ledru [:Sylvestre]]

sec low, not tracking. Please resubmit if you disagree.

Comment 41

[Daniele "Mte90" Scasciafratte]

this problem persist.

Comment 42

[Daniele "Mte90" Scasciafratte]

Bug confirmed also in Firefox 57 :-/

Comment 43

[Boris Zbarsky [:bzbarsky]]

Please stop repeatedly commenting in the bug saying it's still here.  It's still open; we know it's still here.

Comment 44

[Yaroslav Taben]

Hi, I'm looking to work on my first firefox bug, and this looks like something I can fix.

I've reproduced the problem by making a simple HTML page with a button that runs document.write(...), clicking that button, stopping page load, refreshing and clicking the button again.

I didn't explore the solution yet, but it seems that "nsHTMLDocument::Open" is relevant to this (from a suggestion above to check how the URI field is changed. I see that this function gets ran when document.write() is ran in JS).

If nobody has any objections, I will begin investigating and find a fix for this.

Any useful info would be appreciated.

Comment 45

[Boris Zbarsky [:bzbarsky]]

document.open() is what creates wyciwyg URLs, yes...  A good place to start might be breakpointing in nsHTMLDocument::CreateAndAddWyciwygChannel and seeing under what circumstances "originalSpec" is already a wyciwyg URL.

Comment 46

[Yaroslav Taben]

I'm continuing the investigation, but maybe there are a few things you can quickly clarify (and advise on how to approach the fix).

mDocumentURI field remains fine until the page is actual refreshed with the refresh button.
At that point, breaking at nsHTMLDocument::Reset shows that *this has mDocumentURI as NULL, and Reset calls ResetToURI with URI containing wyciwyg part (URL is obtained using channel->GetOriginalURI).

The displayed URL is not changed until another call to call to CreateAndAddWyciwygChannel, which gets wygiwyc URL as part of original spec at that point.

Another interesting observation is that new tabs are created with (don't remember exact names) *File* channel, and after entering an http url, with *HTTP* channel. After Document.Write, mChannel seems to be of *Redirect* type, and mWyciwygChannel field is of WyciwygChannelChild type. When I refresh the page, however, the document's mChannel and mWygiwycChannel are both NULL, but the aChannel passed to Reset is of mozilla::net::WyciwygChannelChild type. Is this expected behaviour?
Is WyciwygChannelChild type the expected type upon refresh here?
It seems that mChannel field gets set to that, rather than mWygiwycChannel.

So in order to avoid this problem, we need to not set mDocumentURI to the one containing wyciwyg part. 
My question is, how do I approach the fix without breaking wyciwyg? Should I look at what happens during document writes, or rather, how the actual refresh is performed (i.e. maybe we shouldn't end up with WyciwygChannelChild?).

I might not be fully familiar with the concept of channels in this context as of yet (I will try to look up info about it), and if nobody can answer these questions on top of their head - it's fine, I can do some more digging to get to more specific questions and fix proposals (in particular, where ::Reset gets aChannel from).

Comment 47

[Boris Zbarsky [:bzbarsky]]

> and Reset calls ResetToURI with URI containing wyciwyg part (URL is obtained using channel->GetOriginalURI).

This is the part that looks buggy to me.  What is document.URL in the resulting document?

For that matter, what is the document.URL of the document we call open() on right after the open() call?  Fundamentally, on reload we should match that document.URL value.

> Is this expected behaviour?

Yes, in terms of the types of channels you are seeing.

> we need to not set mDocumentURI to the one containing wyciwyg part.

I suspect you're right.  What you probably want is to CreateExposableURI like Location::GetURI does at the end.    Need to test what happens right now with document.URL when navigating to a url with a userpass.

Comment 48

[Yaroslav Taben]

If we don't refresh the page, breaking on every call document.write shows that mDocumentURI is still correct.
Only mWyciwygChannel.mURI contains wyciwyg part in the URI.

When we refresh after using document.write, nsHTMLDocument::Reset gets WyciwygChannelChild which already contains URI with wyciwyg 
 in it.
That URL is used to set Document's mDocumentURI field.

document.URL in javascript AFTER refreshing (but NOT before ::Open) also changes to wyciwyg url (I presume, it just gets mDocumentURI which is the source of the problem. Not sure if this is correct behaviour, but it doesn't seem to be so far).

> This is the part that looks buggy ...
Does it mean that we don't want "wyciwyg" in the document URL? Is simply truncating it a viable solution? If not, I don't see a lot of choice in that regard, since document gets its URL from the channel.

> ... CreateExposableURI ...
I was not yet able to explore the ExposableURI solution (It sounds like you keep one URL, but externally display another?). I will look into that asap on Friday (I'm still curious what actual mDocumentURI we are supposed to end up after refresh).

> ... navigating to a url with a userpass
Not entirely sure what that means (does refreshing the page count?).

Comment 49

[Boris Zbarsky [:bzbarsky]]

> breaking on every call document.write shows that mDocumentURI is still correct

OK.  Is that because Open() updates mDocumentURI with something that is not the wyciwyg URI?

> Not sure if this is correct behaviour

It's not.  wyciwyg should never appear in document.URL.

> Is simply truncating it a viable solution?

Yes.

> It sounds like you keep one URL, but externally display another?

Well, you would set mDocumentURI to the return value of CreateExposableURI.  So we would be keeping the thing that should be displayed, in this case.

> I'm still curious what actual mDocumentURI we are supposed to end up after refresh

In an ideal world, exactly what we had before the refresh.

> Not entirely sure what that means

A URL of the form http://user:password@hostname/

Comment 50

[Boris Zbarsky [:bzbarsky]]

So I just tested, and navigating to "http://foo:[e-mail]/" gives that exact string as document.URL.  So we can't just use CreateExposableURI here as-is, since it would strip out the "foo:bar" and change behavior in that case.

We could factor out the wyciwyg parts of CreateExposableURI into a separate utility method that we call both from here and from CreateExposableURI...

Comment 51

[Yaroslav Taben]

I have developed a preliminary solution that seems to work (I'm looking up on how to do proper testing in the mantime).

I need a bit of advice on how to structure the code though.

I moved the wyciwyg part of CreateExposableURI into a "static nsresult nsDefaultURIFixup::RemoveWyciwygScheme(nsIURI* aURI, nsIURI** aReturn)" method.
I'm not sure if that method can be static (I don't see why not, as it's just code that doesn't actually access member variables), but I wanted to clarify, since other methods that seem to serve similar purpose aren't static, and getting an instance of nsIURIFixup didn't seem trivial.

I've also added a check in nsDocument::Reset whether uri->SchemeIs("wyciwyg",...). If it is - I invoke use the new utility method to clean it. This seems to have solved the URL problem.

I'm still in the process of figuring some things out (such as proper usage of nsCOMPtr<T>), but I just wanted to know if this is the right approach in the first place.

If it would make things easier, I could get what I have so far on reviewboard.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Making this static is reasonable.  nsContentUtils is probably a better home for it.

What you describe sounds like the right approach.

If you want to post on reviewboard, I'd be happy to take a look.  Whatever works better for you.

Comment 53

[Yaroslav Taben]

Attachment: Bug 619092 - Add testcase to ensure wyciwyg URL remains hidden

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

Comment 54

[Yaroslav Taben]

Attachment: Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method

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

Comment 55

[Yaroslav Taben]

Attachment: Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

This change prevents URLs with wyciwyg schemas to be set as mDocumentURI.
Otherwise, in JS, document.URL returns a wyciwyg:\\* URL and a subsequent
call to document.write makes wycywyg URL visible in the address bar.

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

Comment 56

[Yaroslav Taben]

I have created a review request here: https://reviewboard.mozilla.org/r/219380

Let me know if I've done it correctly and if you can see the changes.

I've noticed a few things I need to add in retrospect (mainly, check of the return value of the RemoveWyciwygScheme. I wasn't sure if I should use NS_ENSURE_SUCCESS, but I now think I should).
I'm also not sure if 'uri = cleanURI;' is the correct way to swap uri for the clean one.


I've also added a testcase (I'm not 100% if the type of the testcase is correct, but it seemed to have caught the issue).

Let me know what other changes are required.

Comment 57

[Code Review Bot [:reviewbot]]

Comment on attachment 8950105 [details]
Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method

https://reviewboard.mozilla.org/r/219384/#review225080


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: docshell/base/nsDefaultURIFixup.cpp:73
(Diff revision 1)
>    }
>  
>    // Rats, we have to massage the URI
>    nsCOMPtr<nsIURI> uri;
>    if (isWyciwyg) {
> -    nsAutoCString path;
> +    nsresult rv = nsContentUtils::RemoveWyciwygScheme(aURI, getter_AddRefs(uri));

Warning: Value stored to 'rv' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment 58

[Code Review Bot [:reviewbot]]

Comment on attachment 8950106 [details]
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

https://reviewboard.mozilla.org/r/219386/#review225084


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/base/nsDocument.cpp:2251
(Diff revision 1)
>  
> +	bool isWyciwyg = false;
> +	uri->SchemeIs("wyciwyg", &isWyciwyg);
> +	if (isWyciwyg) {
> +		nsCOMPtr<nsIURI> cleanURI;
> +		nsresult rv = nsContentUtils::RemoveWyciwygScheme(uri, getter_AddRefs(cleanURI));

Warning: Value stored to 'rv' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950104 [details]
Bug 619092 - Add testcase to ensure wyciwyg URL remains hidden

https://reviewboard.mozilla.org/r/219382/#review225908

::: dom/html/test/browser_refresh_wyciwyg_url.js:5
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> + /*
> + Test that after using document.write(...), refreshing the document and calling write again, 

Trailing whitespace.

::: dom/html/test/browser_refresh_wyciwyg_url.js:8
(Diff revision 1)
> +
> + /*
> + Test that after using document.write(...), refreshing the document and calling write again, 
> + resulting document.URL does not contain 'wyciwyg' schema
> + and instead is identical to the original URL.
> + 

Trailing whitespace.

::: dom/html/test/browser_refresh_wyciwyg_url.js:24
(Diff revision 1)
> +    is(aBrowser.contentDocument.URL, testURL, "Make sure we start at the correct URL");
> +
> +    // The button should call document.write and location.reload
> +    let test_btn = aBrowser.contentDocument.getElementById("test_btn");
> +    test_btn.click();
> +    await sleep(500); // Ensure that changes take effect

This is racy.  Nothing guarantees the changes take effect during that time period.  It would be better to just watch for the load event or something.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950105 [details]
Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method

https://reviewboard.mozilla.org/r/219384/#review225910

::: docshell/base/nsDefaultURIFixup.cpp:73
(Diff revision 1)
>    }
>  
>    // Rats, we have to massage the URI
>    nsCOMPtr<nsIURI> uri;
>    if (isWyciwyg) {
> -    nsAutoCString path;
> +    nsresult rv = nsContentUtils::RemoveWyciwygScheme(aURI, getter_AddRefs(uri));

We probably want NS_ENSURE_SUCCESS(rv, rv); after this line.

::: dom/base/nsContentUtils.h:212
(Diff revision 1)
>    typedef mozilla::TimeDuration TimeDuration;
>  
>  public:
>    static nsresult Init();
>  
> +  // Truncate "wyciwyg://n/" part of a URL. aURI must have "wyciwyg" scheme.

s/Truncate/strip off/, since we're taking it off the beginning, not the end.

::: dom/base/nsContentUtils.cpp:765
(Diff revision 1)
>  
>    return NS_OK;
>  }
>  
> +nsresult nsContentUtils::RemoveWyciwygScheme(nsIURI* aURI, nsIURI** aReturn)
> +{

Might be a good idea to assert the "scheme is wyciwyg" invariant on entry to the function.

::: dom/base/nsContentUtils.cpp:766
(Diff revision 1)
>    return NS_OK;
>  }
>  
> +nsresult nsContentUtils::RemoveWyciwygScheme(nsIURI* aURI, nsIURI** aReturn)
> +{
> +	nsAutoCString path;

This file uses two-space indent.  It looks like this diff ended up with tabs, not spaces.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950106 [details]
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

https://reviewboard.mozilla.org/r/219386/#review225912

r=me with the nits fixed.  Thank you for doing this!

::: commit-message-b6a08:3
(Diff revision 1)
> +Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them; r?bz
> +
> +This change prevents URLs with wyciwyg schemas to be set as mDocumentURI.

s/schemas/schemes/

::: commit-message-b6a08:4
(Diff revision 1)
> +Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them; r?bz
> +
> +This change prevents URLs with wyciwyg schemas to be set as mDocumentURI.
> +Otherwise, in JS, document.URL returns a wyciwyg:\\* URL and a subsequent

//, not \\

::: dom/base/nsDocument.cpp:2247
(Diff revision 1)
>      // Note: this code is duplicated in XULDocument::StartDocumentLoad and
>      // nsScriptSecurityManager::GetChannelResultPrincipal.
>      // Note: this should match nsDocShell::OnLoadingSite
>      NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
>  
> +	bool isWyciwyg = false;

Again, tabs vs spaces for indent.

Comment 62

[Yaroslav Taben]

Comment on attachment 8950106 [details]
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

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

Comment 63

[Yaroslav Taben]

Thank you for your feedback.

I have addressed the issues, and I have 2 quick questions about them:

In nsDocument.cpp, do we need to NS_ENSURE_SUCCESS_VOID for return of nsContentUtils::RemoveWyciwygScheme(uri, getter_AddRefs(cleanURI));?

In nsContentUtils, I have added the precondition for checking wyciwyg, but only the check is wrapped in assert. I didn't find a method that tells me what the scheme is in one line. Should I wrap the whole check into #ifdef DEBUG, find a way to perform the check in one expression or leave it as it?

Finally, I'm not sure if it was correct to amend all changes to the most recent commit, rather than rewriting individual commits to which the changes should've applied.

Comment 64

[Code Review Bot [:reviewbot]]

Comment on attachment 8950105 [details]
Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method

https://reviewboard.mozilla.org/r/219384/#review226548


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: docshell/base/nsDefaultURIFixup.cpp:73
(Diff revision 1)
>    }
>  
>    // Rats, we have to massage the URI
>    nsCOMPtr<nsIURI> uri;
>    if (isWyciwyg) {
> -    nsAutoCString path;
> +    nsresult rv = nsContentUtils::RemoveWyciwygScheme(aURI, getter_AddRefs(uri));

Warning: Value stored to 'rv' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment 65

[Yaroslav Taben]

Comment on attachment 8950104 [details]
Bug 619092 - Add testcase to ensure wyciwyg URL remains hidden

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

Comment 66

[Yaroslav Taben]

Comment on attachment 8950105 [details]
Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method

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

Comment 67

[Yaroslav Taben]

Comment on attachment 8950106 [details]
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

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

Comment 68

[Yaroslav Taben]

I found a way to rewrite the history properly, but it might have messed up the review flags (sorry if it caused inconvenience).

In any case, let me know if the changes are good to be pushed, or if the two issues I pointed out need to be addressed.

Thank you

Comment 69

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950104 [details]
Bug 619092 - Add testcase to ensure wyciwyg URL remains hidden

https://reviewboard.mozilla.org/r/219382/#review226932

Comment 70

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950105 [details]
Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method

https://reviewboard.mozilla.org/r/219384/#review226934

::: dom/base/nsContentUtils.cpp:766
(Diff revisions 1 - 2)
> +  bool isWyciwyg = false;
> +  aURI->SchemeIs("wyciwyg", &isWyciwyg);

This bit should go inside #ifdef DEBUG, since that boolean is unused except for the assertion.

Comment 71

[Boris Zbarsky [:bzbarsky]]

> In nsDocument.cpp, do we need to NS_ENSURE_SUCCESS_VOID 

No.  Bailing out of Reset without doing its work would be a really bad idea.

What we should do is not assign to "uri" on failure.

> Should I wrap the whole check into #ifdef DEBUG

Yes.

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950106 [details]
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

https://reviewboard.mozilla.org/r/219386/#review226936

::: commit-message-b6a08:1
(Diff revision 3)
> +Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them; r=me

"r=bz", right?

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8950106 [details]
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

https://reviewboard.mozilla.org/r/219386/#review226938

::: dom/base/nsDocument.cpp:2252
(Diff revision 3)
> +  bool isWyciwyg = false;
> +  uri->SchemeIs("wyciwyg", &isWyciwyg);
> +  if (isWyciwyg) {
> +    nsCOMPtr<nsIURI> cleanURI;
> +    nsContentUtils::RemoveWyciwygScheme(uri, getter_AddRefs(cleanURI));
> +    uri = cleanURI;

Shouldn't assign "uri" if RemoveWyciwygScheme fails.

Comment 74

[Boris Zbarsky [:bzbarsky]]

The static analysis bot issues (which I believe are fixed) I think you need to mark fixed yourself in the mozreview UI....  I can't seem to do it.

Comment 75

[Yaroslav Taben]

Comment on attachment 8950105 [details]
Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method

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

Comment 76

[Yaroslav Taben]

Comment on attachment 8950106 [details]
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;

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

Comment 77

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64f840ae00a9
Add testcase to ensure wyciwyg URL remains hidden r=bz
https://hg.mozilla.org/integration/autoland/rev/961f1ab67632
Refactor code for removing wyciwyg scheme from URL into util method r=bz
https://hg.mozilla.org/integration/autoland/rev/50a26bd1c5af
Truncate wyciwyg URLs on Reload to prevent exposing them; r=bz

Comment 78

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/64f840ae00a9
https://hg.mozilla.org/mozilla-central/rev/961f1ab67632
https://hg.mozilla.org/mozilla-central/rev/50a26bd1c5af

Comment 79

[Tom S [:evilpie]]

Hey, nice work! It's seems like you didn't put enough spaces before your code: https://hg.mozilla.org/integration/mozilla-inbound/rev/50a26bd1c5af#l1.12. Can you attach a new patch to fix this? Thanks.

Comment 80

[Yaroslav Taben]

Attachment: nsDocument.cpp patch with more spaces

Thanks, I have double checked, and you're right.
I've modified the patch to add two extra spaces before my additions.

Let me know if this is correct format.

Comment 81

[Boris Zbarsky [:bzbarsky]]

Ugh.  I wish mozreview didn't hide spacing quite as much.  :(

I'll just fix the indentation.

Comment 82

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48f24f31126e
indentation fix followup.

Comment 83

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/48f24f31126e

Comment 84

[roxleitan]

Managed to reproduce the issue on Nightly 60.0a1(Build ID:20180131220303) using the exploit.html attachment.

Verified fixed using the latest Nightly 61.0a1(2018-03-13) and Beta 60.0b3 on Windows 10 x64, Mac OS X 10.12, Ubuntu 16.04 x64.