Closed Bug 619092 Opened 14 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox38 - ---
firefox60 --- verified

People

(Reporter: shotokanzh, Assigned: yaroslav.taben, Mentored)

References

(
URL
)

Details

(Keywords: sec-low, Whiteboard: [sg:low][lang=c++][adv-main60-])

Attachments

(6 files)

619092.htm - TC attempt, unsuccessful I think
433 bytes, text/html
Details
exploit.html
561 bytes, text/html
Details
Bug 619092 - Add testcase to ensure wyciwyg URL remains hidden
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 619092 - Refactor code for removing wyciwyg scheme from URL into util method
59 bytes, text/x-review-board-request
Details
Bug 619092 - Truncate wyciwyg URLs on Reload to prevent exposing them;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
nsDocument.cpp patch with more spaces
1.07 KB, patch
Details | Diff | Splinter Review 
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)
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.
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?
Status: UNCONFIRMED → NEW
Component: Security → Document Navigation
Ever confirmed: true
Product: Firefox → Core
QA Contact: firefox → docshell
Summary: wyciwig appears when writing text with document.write(), refreshing the page and writing text again. → wyciwyg URL is visible (document.write + stop + reload)
Version: unspecified → Trunk
(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 :)
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.
(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
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?
Keywords: qawanted
The exploit works for me on trunk. The page displays:

Done. Url: "wyciwyg://107/http://92.245.187.245/exploit.html" contains "wyciwyg://"
Interesting.  It does for me with a clean profile, but not with my usual (no-extensions) profile.  OK.
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?
And in particular, should we be using createExposableURI to set mDocumentURI?  The latter is exposed directly in the DOM, right?
Blocks: 528925
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?
Whiteboard: [sg:low]
(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.
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.
Group: core-security
Whiteboard: [sg:low] → [sg:low][mentor=bz][lang=c++]
(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?
Flags: needinfo?
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?
Flags: needinfo?
> 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.
Keywords: qawanted
Please re-add the qawanted keyword if you decide to keep this and still need help from qa.
I can't reproduce this (and the original URL is gone). Jesse - you could reproduce, can you recall what the test actually looked like?
Flags: needinfo?(jruderman)
I don't remember. Maybe Shotokan still has it somewhere, or maybe bz remembers?
Flags: needinfo?(jruderman) → needinfo?(shotokan_91)
(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.
Flags: needinfo?(shotokan_91)
(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.
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?
(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.
(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?
I've updated the exploit page. Enjoy.
http://vps.aitch.me/explffx.html
Yep, that demo shows a problem for me.
Attached file exploit.html — 
Working exploit
I've uploaded an attachment so if the vps goes down we've the code there.
I can reproduce with the bookmarklet if I click it twice.  I can also reproduce using exploit.html. Thanks!
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.
Oh, sorry. 
*Could anyone assign this bug to me?
The place to start looking is probably by looking into what mDocumentURI ends up being after nsDocument::Open() returns.
Assignee: nobody → hotsmileband
Mentor: bzbarsky
Whiteboard: [sg:low][mentor=bz][lang=c++] → [sg:low][lang=c++]
Releasing bug. hotsmileband if you are still working on this please comment and I'll add you back as the assignee.
Assignee: hotsmileband → nobody
Blocks: 1131310
any news about this?
[Tracking Requested - why for this release]:  since bug 1131310 was duped to this one.
status-firefox38: --- → ?
tracking-firefox38: --- → ?
(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?
Flags: needinfo?(jduell.mcbugs)
sec low, not tracking. Please resubmit if you disagree.
tracking-firefox38: ? → -
Flags: needinfo?(jduell.mcbugs)
this problem persist.
Bug confirmed also in Firefox 57 :-/
Please stop repeatedly commenting in the bug saying it's still here.  It's still open; we know it's still here.
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.
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.
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).
> 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.
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?).
> 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/
So I just tested, and navigating to "http://foo:bar@example.com/" 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...
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.
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.
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 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 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 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.
Attachment #8950104 - Flags: review?(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.
Attachment #8950105 - Flags: review?(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.
Attachment #8950106 - Flags: review?(bzbarsky) → review+
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 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]
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 on attachment 8950104 [details]
Bug 619092 - Add testcase to ensure wyciwyg URL remains hidden

https://reviewboard.mozilla.org/r/219382/#review226932
Attachment #8950104 - Flags: review+
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.
Attachment #8950105 - Flags: review+
> 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 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 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.
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.
Assignee: nobody → yaroslav.taben
Pushed by bzbarsky@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/64f840ae00a9
https://hg.mozilla.org/mozilla-central/rev/961f1ab67632
https://hg.mozilla.org/mozilla-central/rev/50a26bd1c5af
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Flags: needinfo?(yaroslav.taben)
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.
Flags: needinfo?(yaroslav.taben)
Ugh.  I wish mozreview didn't hide spacing quite as much.  :(

I'll just fix the indentation.
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Flags: qe-verify+
status-firefox38: ? → ---
Whiteboard: [sg:low][lang=c++] → [sg:low][lang=c++][adv-main60-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: