Closed
Bug 804446
Opened 12 years ago
Closed 12 years ago
Disable the javascript: protocol when it is entered in the location bar of browser in Firefox OS
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-basecamp:+, firefox18+ fixed, firefox19+ fixed)
VERIFIED
FIXED
blocking-basecamp | + |
People
(Reporter: gkw, Assigned: baku)
Details
(Keywords: b2g-testdriver, sec-high, unagi)
Attachments
(1 file, 6 obsolete files)
7.38 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
I enter:
javascript:alert("hi");
in the Firefox browser in Firefox OS and the alert shows. It does not show on desktop because bug 656433 disabled it.
We should disable it for the browser in Firefox OS too.
![]() |
Reporter | |
Updated•12 years ago
|
blocking-basecamp: --- → ?
Eep.
blocking-basecamp: ? → +
![]() |
Reporter | |
Comment 2•12 years ago
|
||
My Git commit info currently shows:
2012-10-17 12:16:24
e3efbd0411218762cf9a62278bf58ee513ff331f
Also, seeing my comments in https://b2gtestdrivers.allizom.org/comments_table - my build ID currently is:
20121018092134
Assignee | ||
Comment 3•12 years ago
|
||
Is this good enough? A simple black list of protocols.
Attachment #674136 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 4•12 years ago
|
||
A check was missing.
Attachment #674136 -
Attachment is obsolete: true
Attachment #674136 -
Flags: review?(jones.chris.g)
Attachment #674137 -
Flags: review?(jones.chris.g)
![]() |
Reporter | |
Comment 5•12 years ago
|
||
Can a test also be added for this?
![]() |
Reporter | |
Updated•12 years ago
|
Flags: in-testsuite?
![]() |
Reporter | |
Comment 6•12 years ago
|
||
I met w/ Tony f2f and he mention we should probably also block the protocols, as per mobile Fennec (I'm not sure if I spelt them correctly): file, wysiwyg, chrome and resource
Comment on attachment 674137 [details] [diff] [review]
patch 1b to Gaia
We should do this in gecko, ideally using the same code that other products do. I don't know how to do that though.
Attachment #674137 -
Flags: review?(jones.chris.g)
Attachment #674137 -
Flags: feedback?(justin.lebar+bug)
Attachment #674137 -
Flags: feedback?(bzbarsky)
![]() |
Reporter | |
Comment 8•12 years ago
|
||
CC'ing Gavin - he originally wrote the patch in bug 656433.
Assignee | ||
Comment 9•12 years ago
|
||
> We should do this in gecko, ideally using the same code that other products
> do. I don't know how to do that though.
Can we block these protocols for any <iframe mozbrowser> ?
If not, how to detect which iframe should not allow these protocols and which should?
![]() |
||
Comment 10•12 years ago
|
||
Comment on attachment 674137 [details] [diff] [review]
patch 1b to Gaia
The only way to do it in Gecko is to communicate to Gecko that you're doing a "url bar" load....
Attachment #674137 -
Flags: feedback?(bzbarsky)
Comment 11•12 years ago
|
||
> Can we block these protocols for any <iframe mozbrowser> ?
We should be able to, yes.
Boris, can we just force LOAD_FLAGS_DISALLOW_INHERIT_OWNER for all loads inside mozbrowsers? Or will that break something?
> Can a test also be added for this?
If we fix it in Gecko, we can add an automated test, yes.
Updated•12 years ago
|
Attachment #674137 -
Attachment description: patch 1b → patch 1b to Gaia
Comment 12•12 years ago
|
||
Comment on attachment 674137 [details] [diff] [review]
patch 1b to Gaia
I agree we should fix this in Gecko if we can.
Attachment #674137 -
Flags: feedback?(justin.lebar+bug) → feedback-
Assignee | ||
Comment 13•12 years ago
|
||
Maybe this patch is "extreme". It enabled these 2 flags for any TabChild Browser:
nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP
and
nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_OWNER
An other approach can be to change LoadURL() so that we share the flags that we want to use. I haven't written this code because right now LoadURL seems to be used just for browsers. I'm not confident with this code so I don't know if it's breaking something but I'm using b2g desktop and it seems working.
Attachment #674137 -
Attachment is obsolete: true
Attachment #674613 -
Flags: review?(justin.lebar+bug)
![]() |
||
Comment 14•12 years ago
|
||
> Or will that break something?
That really depends on the use cases, no? Doing that for Firefox tabs, for example, would break bookmarklets. Do you plan to have people writing browsers that offer bookmarklets using mozbrowser?
Comment 15•12 years ago
|
||
> Doing that for Firefox tabs, for example, would break bookmarklets. Do you plan to have people
> writing browsers that offer bookmarklets using mozbrowser?
I don't think so, but Ben would know for sure.
Comment 16•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #14)
> That really depends on the use cases, no? Doing that for Firefox tabs, for
> example, would break bookmarklets. Do you plan to have people writing
> browsers that offer bookmarklets using mozbrowser?
We have no plans for bookmarklets in v1 of the Gaia browser and any security issues the javascript: protocol creates would be higher priority to fix than allowing third party browsers to support them.
Please feel free to go ahead and take the nuclear option.
Comment 17•12 years ago
|
||
Comment on attachment 674613 [details] [diff] [review]
patch 2
This works, but only for out-of-process <iframe mozbrowser>'s. I haven't kept up with the status of everything.me, but IIRC we were at some point using an in-process mozbrowser there. So I think we should fix this for in-process mozbrowser's as well.
The fix would involve setting load flags somewhere within nsDocShell, if IsBrowserElement(). Probably in nsDocShell::InternalLoad would be sufficient.
Updated•12 years ago
|
Attachment #674613 -
Flags: review?(justin.lebar+bug)
![]() |
Reporter | |
Updated•12 years ago
|
OS: Mac OS X → Gonk (Firefox OS)
Assignee | ||
Comment 18•12 years ago
|
||
Can you give me a feedback about this patch? I sent it to try. Thanks.
Attachment #674613 -
Attachment is obsolete: true
Attachment #674790 -
Flags: review?(justin.lebar+bug)
Comment 19•12 years ago
|
||
Comment on attachment 674790 [details] [diff] [review]
patch 3
I'd prefer to do this in just one place, rather than duplicating the code. Is
there some reason you can't put this in nsDocShell::InternalLoad?
>diff --git a/content/base/src/nsFrameLoader.cpp b/content/base/src/nsFrameLoader.cpp
>--- a/content/base/src/nsFrameLoader.cpp
>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -476,21 +476,34 @@ nsFrameLoader::ReallyStartLoadingInterna
> loadInfo->SetOwner(mOwnerContent->NodePrincipal());
>
> nsCOMPtr<nsIURI> referrer;
> rv = mOwnerContent->NodePrincipal()->GetURI(getter_AddRefs(referrer));
> NS_ENSURE_SUCCESS(rv, rv);
>
> loadInfo->SetReferrer(referrer);
>
>+ // Default flags:
>+ int32_t flags = nsIWebNavigation::LOAD_FLAGS_NONE;
>+
>+ // Flags for mozbrowser:
>+ nsCOMPtr<nsIDOMMozBrowserFrame> mozbrowser = do_QueryInterface(mOwnerContent);
>+ if (mozbrowser) {
>+ bool isMozbrowser = false;
>+ mozbrowser->GetMozbrowser(&isMozbrowser);
You need to use nsIMozBrowserFrame::ReallyIsBrowser; this would apply the flags to any iframe which has the mozbrowser attribute, but we want to check permissions etc.
>+ if (isMozbrowser) {
>+ flags = nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP
>+ nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_OWNER;
>+ }
>+ }
>+
> // Kick off the load...
> bool tmpState = mNeedsAsyncDestroy;
> mNeedsAsyncDestroy = true;
>- rv = mDocShell->LoadURI(mURIToLoad, loadInfo,
>- nsIWebNavigation::LOAD_FLAGS_NONE, false);
>+ rv = mDocShell->LoadURI(mURIToLoad, loadInfo, flags, false);
> mNeedsAsyncDestroy = tmpState;
> mURIToLoad = nullptr;
> NS_ENSURE_SUCCESS(rv, rv);
>
> return NS_OK;
> }
>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp
> bool
> TabChild::RecvLoadURL(const nsCString& uri)
> {
> printf("loading %s, %d\n", uri.get(), NS_IsMainThread());
> SetProcessNameToAppName();
>
> nsresult rv = mWebNav->LoadURI(NS_ConvertUTF8toUTF16(uri).get(),
>- nsIWebNavigation::LOAD_FLAGS_NONE,
>+ nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP |
>+ nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_OWNER,
> NULL, NULL, NULL);
TabChild knows whether it's a browser element, so please check that.
Attachment #674790 -
Flags: review?(justin.lebar+bug)
Comment on attachment 674790 [details] [diff] [review]
patch 3
Review of attachment 674790 [details] [diff] [review]:
-----------------------------------------------------------------
bz should review this.
It's important that we don't disable webpages initiating navigation to javascript URLs since that would likely break the web.
Attachment #674790 -
Flags: review?(bzbarsky)
![]() |
||
Comment 21•12 years ago
|
||
Comment on attachment 674790 [details] [diff] [review]
patch 3
This looks fine assuming that all loads coming in over IPC there are directly from the UI and not via loads in named windows or whatnot.
Attachment #674790 -
Flags: review?(bzbarsky) → review+
Comment 22•12 years ago
|
||
> It's important that we don't disable webpages initiating navigation to javascript URLs since that
> would likely break the web.
Pages navigate themselves to javascript: URLs and expect that to work, really?
Comment 23•12 years ago
|
||
Comment on attachment 674790 [details] [diff] [review]
patch 3
At the very least, I would like to see a new patch with comment 19 addressed.
But I'd still like to put this somewhere closer to docshell if we can, so that we don't have to introduce this same code at every place which calls LoadURI.
Attachment #674790 -
Flags: review-
![]() |
||
Comment 24•12 years ago
|
||
> Pages navigate themselves to javascript: URLs and expect that to work, really?
All the time, yes. They use it to run some script. Typically like this:
<a href="javascript:foo()">Click me</a>
![]() |
||
Comment 25•12 years ago
|
||
I don't see how you can put it "closer to docshell" without breaking the use case from comment 24. We really need to do this somewhere where we know the exact source of the URI we're loading.
Comment 26•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #25)
> I don't see how you can put it "closer to docshell" without breaking the use
> case from comment 24. We really need to do this somewhere where we know the
> exact source of the URI we're loading.
Sounds good, thanks.
Will this work as expected for something like:
window.open("javascript:...", "blank_");
I believe such a call should evaluate the javascript URL in the context of the opening page and then open a new window which displays the content of whatever the javascript code returned.
Assignee | ||
Comment 28•12 years ago
|
||
I'm a bit lost... is this patch good enough or should I do something else?
Assignee | ||
Comment 29•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=897c048afff4 green on try
![]() |
||
Comment 30•12 years ago
|
||
> such a call should evaluate the javascript URL in the context of the opening page
It should evaluate it with the principal of the opening page, in the context of the new about:blank window.
Comment 31•12 years ago
|
||
> Will this work as expected for something like:
>
> window.open("javascript:...", "blank_");
This /may/ work properly. nsWindowWatcher::ProvideWindowInternal has its own call to LoadURI.
Andrea, can you write a test for this patch (and ideally, for the window.open case as well)? See the tests in dom/browser-element/mochitest. There's a python script in there which may alleviate some of the pain of creating these tests.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 32•12 years ago
|
||
This new patch contains a mochitest.
Attachment #674790 -
Attachment is obsolete: true
Attachment #675333 -
Flags: review?(bzbarsky)
Comment 33•12 years ago
|
||
Comment on attachment 675333 [details] [diff] [review]
patch + test
This is incorrect per comment 19. You can't use nsIDOMMozBrowserFrame for this.
In nsFrameLoader, you can call OwnerIsBrowserFrame(), which should be what you want.
Attachment #675333 -
Flags: review-
Assignee | ||
Comment 34•12 years ago
|
||
Right, OwnerIsBrowserFrame() does exactly what I need.
Attachment #675333 -
Attachment is obsolete: true
Attachment #675333 -
Flags: review?(bzbarsky)
Attachment #675360 -
Flags: review?(justin.lebar+bug)
Comment 35•12 years ago
|
||
Comment on attachment 675360 [details] [diff] [review]
patch + test 2
This looks good to me. The only place I see us calling PTab::LoadURL is from nsFrameLoader::ReallyStartLoadingInternal(), so this is effectively putting the flags in the same place for in-process and OOP, which is good.
I wish there was some way for us to test that iframeJS isn't doing anything, but I don't see how. I guess we can rely on the fact that the test on the other iframe should take long enough to shake out any failures on iframeJS.
Attachment #675360 -
Flags: review?(justin.lebar+bug)
Attachment #675360 -
Flags: review?(bzbarsky)
Attachment #675360 -
Flags: review+
![]() |
||
Comment 36•12 years ago
|
||
Comment on attachment 675360 [details] [diff] [review]
patch + test 2
r=me
Attachment #675360 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 37•12 years ago
|
||
Thanks guys!
Attachment #675360 -
Attachment is obsolete: true
Attachment #675477 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 38•12 years ago
|
||
Keywords: checkin-needed
Updated•12 years ago
|
status-firefox18:
--- → affected
Comment 39•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
![]() |
Reporter | |
Comment 40•12 years ago
|
||
This will likely need backporting to mozilla-aurora too.
status-firefox19:
--- → fixed
![]() |
Reporter | |
Comment 41•12 years ago
|
||
Setting sec-high instead of sec-want because this requires no more than normal phone-using actions and is not a new feature for security. Feel free to adjust as necessary.
![]() |
Reporter | |
Updated•12 years ago
|
Hardware: x86 → ARM
![]() |
Reporter | |
Updated•12 years ago
|
tracking-firefox18:
--- → ?
tracking-firefox19:
--- → ?
![]() |
Reporter | |
Updated•12 years ago
|
Flags: in-testsuite? → in-testsuite+
Updated•12 years ago
|
![]() |
Reporter | |
Comment 42•12 years ago
|
||
Setting checkin-needed for landing on aurora, since (I think) that's what B2G is building off now. Feel free to change this if this is incorrect.
Keywords: checkin-needed
Comment 43•12 years ago
|
||
Well, it would have landed on aurora if filed in the correct component...
Component: Gaia → General
Keywords: checkin-needed
Comment 44•12 years ago
|
||
![]() |
Reporter | |
Comment 45•12 years ago
|
||
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•