302618 - eval in bookmarklet fails when opening the bookmarklet in a new tab

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Ross]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

When a javascript bookmark is opened in a new tab, the javascript doesn't execute.

If the javascript bookmark is clicked on directly, then it evaluates the javascript.

This is a regression from 1.0.5.

Reproducible: Always

Steps to Reproduce:
1. Create a folder "JS"
2. Create a bookmark with the following location in the folder "JS"
   javascript:eval("location.href='http://slashdot.org/';")
3. Middle click the folder "JS" so that the folder opens in a new tab.
Actual Results:  
The new tab opens, with the URL set to be "javacript:eval(...)", but it doesn't
execute the javascript.

Expected Results:  
It should open the new tab and execute the javascript and thus
http://slashdot.org/ should load.

The workaround for this bug is simple to press return in the url box, which
contains the javascript, at which point it's executed.

Comment 1

[zug_treno]

Related to/duplicate of Core bug 55696?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That's not a dupe of bug 55696. Using eval in this way causes a javascript error:

Error: function eval must be called directly, and not by way of a function of
another name.
Source File: javascript:eval("location.href='http://slashdot.org/';")
Line: 1

So further processing is stopped and the requested page is not opened.

Brendan, your fix on security bugs 300008 (300867) causes this issue. How should
we handle it?

Comment 3

[Brendan Eich [:brendan]]

This is probably a bug in dom/src/jsurl code.  It should be fixed for 1.8
(Firefox 1.5).  Sicking, can you take a look?

/be

Comment 4

[Brendan Eich [:brendan]]

The example in comment 0 is probably a reduced testcase, but I wanted to point
out that eval is unnecessary there, and perhaps in real-world javascript: URLs
known to be broken by this bug.  Avoiding unnecessary eval may be a workaround.

/be

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

FYI: Reverting the changes in jsdbgapi.c brings back the functionality.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/js/src&command=DIFF_FRAMESET&file=jsdbgapi.c&rev1=3.54&rev2=3.55&root=/cvsroot

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> FYI: Reverting the changes in jsdbgapi.c brings back the functionality.

Yes, that's what you said in comment 2's last paragraph.

The correct fix lies elsewhere.

/be

Comment 7

[Asa Dotzler [:asa]]

peterv, can you look into this?

Comment 8

[Peter Van der Beken [:peterv]]

We fail at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsobj.c&rev=3.209&root=/cvsroot&mark=1163#1160
principals is from the JS URI, scopePrincipals is from about:blank

Comment 9

[Peter Van der Beken [:peterv]]

When opening in the current tab, the principals are equal.

Comment 10

[Scott MacGregor]

this seems like a serious regression from 1.0.x

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #8)
> We fail at
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsobj.c&rev=3.209&root=/cvsroot&mark=1163#1160
> principals is from the JS URI, scopePrincipals is from about:blank

Argh!  Does "principals is from the JS URL" mean it's a codebase principal that
has javascript:... as its URL?

That's so totally broken.  Paging Dr. Sicking!  Bug 293363 needed, or some piece
of its patch.

/be

Comment 12

[Peter Van der Beken [:peterv]]

(In reply to comment #11)
> Argh!  Does "principals is from the JS URL" mean it's a codebase principal that
> has javascript:... as its URL?

Yup. So this is coming in from bookmarks code through tabs code trough browser
code to end up in nsDocShell::LoadURI. Eventually we try to get the current
document owner (GetCurrentDocumentOwner) in nsDocShell but that returns null. So
eventually we then end up in nsJSThunk::EvaluateScript which gets the principal
using the URL (see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp&rev=1.115&root=/cvsroot&mark=250-252#213).

> That's so totally broken.  Paging Dr. Sicking!  Bug 293363 needed, or some piece
> of its patch.

It doesn't have currently have a patch ;-).

Comment 13

[Asa Dotzler [:asa]]

Does this have any impact outside of bookmarklets?

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #12)
> Yup. So this is coming in from bookmarks code through tabs code trough browser
> code to end up in nsDocShell::LoadURI. Eventually we try to get the current
> document owner (GetCurrentDocumentOwner) in nsDocShell but that returns null.

LazyWeb query: why does it fail?

> So eventually we then end up in nsJSThunk::EvaluateScript which gets the
> principal using the URL ...

So what should this javascript: URL's origin be?  If you bookmarked it by typing
it in yourself, it could be given a powerful origin, but we are not ready for
such a general trusted data labeling architecture.  In general, a bookmarked
javascript: URL might be part of an attack, relying on social engineering to get
you to bookmark a link that seems useful.

So how about we spot-fix this for 1.5 by making the javascript: URL handler use
about:blank instead of the (useless) javascript: URL itself as the origin?

/be

Comment 15

[Asa Dotzler [:asa]]

PeterV, or brendan, can one of you come up with a patch here? Time is running
out for this release.

Comment 16

[Asa Dotzler [:asa]]

Blake's gonna look into a spot fix for this one.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Use better principals

This patch makes us use the principal of the newly-opened window. jst and I
weren't very confident about whether we could get into a situation with a
non-newly created document, so I'm checking to make sure that we don't give the
javascript URL the principals of some random page.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198077 [details] [diff] [review]
Use better principals

This isn't quite the right patch.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Use better principals

This is actually the right patch...

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 198078 [details] [diff] [review]
Use better principals

sr=jst, but maybe comment that we need to do this rather than creating a new
princiapls object for about:blank due to caps' treatment of about:blank URIs.

Comment 21

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

Comment on attachment 198078 [details] [diff] [review]
Use better principals

Looks great! This should be very safe as the only change is to sometimes
execute script using an 'about:blank' principal.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into MOZILLA_1_8_BRANCH and trunk.