Last Comment Bug 302618 - eval in bookmarklet fails when opening the bookmarklet in a new tab
: eval in bookmarklet fails when opening the bookmarklet in a new tab
Status: VERIFIED FIXED
: fixed1.8, regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla1.8beta5
Assigned To: Blake Kaplan (:mrbkap)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 33961
  Show dependency treegraph
 
Reported: 2005-07-29 05:06 PDT by Peter Ross
Modified: 2006-11-08 07:29 PST (History)
13 users (show)
mscott: blocking1.8b5+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use better principals (621 bytes, patch)
2005-09-30 16:30 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Use better principals (3.13 KB, patch)
2005-09-30 16:33 PDT, Blake Kaplan (:mrbkap)
jonas: review+
jst: superreview+
asa: approval1.8b5+
Details | Diff | Splinter Review

Description Peter Ross 2005-07-29 05:06:04 PDT
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 2005-07-30 02:20:02 PDT
Related to/duplicate of Core bug 55696?
Comment 2 Henrik Skupin (:whimboo) 2005-07-30 06:34:34 PDT
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] 2005-07-30 10:10:25 PDT
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] 2005-07-30 10:37:34 PDT
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) 2005-07-30 10:50:30 PDT
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] 2005-07-30 11:06:03 PDT
(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] 2005-09-02 12:46:49 PDT
peterv, can you look into this?
Comment 8 Peter Van der Beken [:peterv] - away till Aug 1st 2005-09-09 10:12:17 PDT
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] - away till Aug 1st 2005-09-09 10:28:37 PDT
When opening in the current tab, the principals are equal.
Comment 10 Scott MacGregor 2005-09-13 10:17:03 PDT
this seems like a serious regression from 1.0.x
Comment 11 Brendan Eich [:brendan] 2005-09-13 10:49:21 PDT
(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] - away till Aug 1st 2005-09-16 09:45:15 PDT
(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] 2005-09-23 12:28:47 PDT
Does this have any impact outside of bookmarklets?
Comment 14 Brendan Eich [:brendan] 2005-09-23 15:37:08 PDT
(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] 2005-09-29 12:47:12 PDT
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] 2005-09-29 18:45:54 PDT
Blake's gonna look into a spot fix for this one.
Comment 17 Blake Kaplan (:mrbkap) 2005-09-30 16:30:53 PDT
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.
Comment 18 Blake Kaplan (:mrbkap) 2005-09-30 16:32:34 PDT
Comment on attachment 198077 [details] [diff] [review]
Use better principals

This isn't quite the right patch.
Comment 19 Blake Kaplan (:mrbkap) 2005-09-30 16:33:13 PDT
Created attachment 198078 [details] [diff] [review]
Use better principals

This is actually the right patch...
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-30 17:42:23 PDT
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) PTO Until July 5th 2005-09-30 20:00:59 PDT
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) 2005-10-03 10:08:52 PDT
Fix checked into MOZILLA_1_8_BRANCH and trunk.

Note You need to log in before you can comment on or make changes to this bug.