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

VERIFIED FIXED in mozilla1.8beta5

Status

()

Core
DOM: Core & HTML
--
minor
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Peter Ross, Assigned: mrbkap)

Tracking

({fixed1.8, regression})

Trunk
mozilla1.8beta5
fixed1.8, regression
Points:
---
Bug Flags:
blocking1.8b5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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

12 years ago
Related to/duplicate of Core bug 55696?
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?
Status: UNCONFIRMED → NEW
Component: Bookmarks → JavaScript Engine
Ever confirmed: true
Keywords: regression
OS: Windows XP → All
Product: Firefox → Core
Hardware: PC → All
Version: unspecified → Trunk
Summary: javascript bookmarks don't execute when opened in a new tab → eval in bookmark location causes a js error when trying to open bookmark in a new tab
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
Component: JavaScript Engine → DOM: Level 0
Flags: blocking-aviary1.5+
QA Contact: bookmarks → ian
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
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
(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

Updated

12 years ago
Summary: eval in bookmark location causes a js error when trying to open bookmark in a new tab → eval in bookmarklet fails when opening the bookmarklet in a new tab

Comment 7

12 years ago
peterv, can you look into this?

Updated

12 years ago
Flags: blocking-aviary1.5+ → blocking1.8b5?
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
Assignee: nobody → peterv
When opening in the current tab, the principals are equal.

Comment 10

12 years ago
this seems like a serious regression from 1.0.x
Flags: blocking1.8b5? → blocking1.8b5+
(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
(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

12 years ago
Does this have any impact outside of bookmarklets?
(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

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

Comment 16

12 years ago
Blake's gonna look into a spot fix for this one.
Assignee: peterv → mrbkap
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 17

12 years ago
Created attachment 198077 [details] [diff] [review]
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.
Attachment #198077 - Flags: review?(bugmail)
(Assignee)

Comment 18

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

This isn't quite the right patch.
Attachment #198077 - Attachment is obsolete: true
Attachment #198077 - Flags: review?(bugmail)
(Assignee)

Comment 19

12 years ago
Created attachment 198078 [details] [diff] [review]
Use better principals

This is actually the right patch...
Attachment #198078 - Flags: review?(bugmail)
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.
Attachment #198078 - Flags: superreview+
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.
Attachment #198078 - Flags: review?(bugmail) → review+
Attachment #198078 - Flags: approval1.8b5?

Updated

12 years ago
Attachment #198078 - Flags: approval1.8b5? → approval1.8b5+
Whiteboard: [checkin needed]
(Assignee)

Comment 22

12 years ago
Fix checked into MOZILLA_1_8_BRANCH and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9alpha
Target Milestone: mozilla1.9alpha → mozilla1.8beta5
Blocks: 33961
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.