161531 - global history should not store javascript: urls except those typed by the user

Status: RESOLVED INVALID

(Core Graveyard :: History: Global, defect)

Description

[Jesse Ruderman]

Global history should not keep javascript: urls execpt javascript: urls typed by
the user.  In general, javascript: URLs in web pages don't work or make sense
out of the context of the page.  Also, it's somewhat dangerous to run a
javascript: url you don't trust on a site that trusts you.  (It's only somewhat
dangerous; see bug 28387.)

Steps to reproduce:
1. Load the testcase.
2. Click the javascript: link.
3. Go to www.google.com.
4. Start typing "javas" into the address bar.
5. Press enter.

Result: the javascript: url is completed and runs at www.google.com, showing
your www.google.com cookie.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

data: urls not entered by the user should also be left out of global history. 
Not only do they have the same (minor) security issues as javascript: urls, but
they tend to be very long urls.

Comment 3

[Stephen P. Morse]

Attachment: Patch to leave data and js urls out of history unless typed by user

Comment 4

[Stephen P. Morse]

Reason above patch works is because urls typed by user don't go through
nsDocShell but rather comes in from js code.  Links clicked on come in through
nsDocShell.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 108153 [details] [diff] [review]
Patch to leave data and js urls out of history unless typed by user

sr=jst

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 108153 [details] [diff] [review]
Patch to leave data and js urls out of history unless typed by user

r=mstoltz.

Comment 7

[Nisheeth Ranjan]

Re-assigning to myself as fixing this bug is related to fixing bug 161546...

Comment 8

[Nisheeth Ranjan]

So, Steve's check for data and js urls in ShouldAddToGlobalHistory() might
prevent js urls typed in by the user from getting into the history database. 
I'm seeing something pretty weird.  I'll look into it more but if anyone can
explain this off the top of their head, it'll save me a lot of time.

With the patch:

a) when I click on js urls, they don't enter global history, which is good.

b) when I type a short js url like "javascript:alert('foo');" into the url bar
and hit enter, the url does get entered into history, which is also good.

BUT, 

c) when I type a longer js url like "javascript:alert('This is to test if length
has anything to do with this.');" into the url bar and hit enter, the url hits
the check in ShouldAddToGlobalHistory() and does NOT get entered into history,
which is bad.

Why does the length of the JS url make a difference?  ccing some people who
might know the answer...

Comment 9

[Alec Flett]

be careful here - in our current implementation, the url that the user types in
does always get added to global history, but is marked as "hidden" (and also
marked as "typed") - when the url actually loads, the url gets un-marked "hidden".. 

Comment 10

[Nisheeth Ranjan]

hey alec, thanks for the comment.  yes, i knew about the MarkPageAsTyped() call
that marks typed url as typed and hidden.  The weird thing is that it gets
called when I type a) and doesn't get called when I type b).  So, b) doesn't get
added to the history.  Go, figure.  See comment 8 for definitions of a) and b).  :-)

Comment 11

[Alec Flett]

oh! So this probably has nothing to do with global history, and more to do with
the JS that calls markPageAsTyped() - that stuff should be:
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/sessionHistoryUI.js#265

it looks like that's coming out of the fixupURI stuff - i.e. nsIURIFixup

see
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDefaultURIFixup.cpp#102

for the implementation. maybe we need to special-case javascript: urls.

Comment 12

[Nisheeth Ranjan]

ok, this is what I'm getting bitten by in sessionhistoryUI.js:

+
+       var url = entryToAdd.Value;
+       if (url.indexOf(" ") == -1) {
+         var fixedUpURI = gURIFixup.createFixupURI(url, 0);
+         gGlobalHistory.markPageAsTyped(fixedUpURI.spec);
+       }
+
        // Put the value as it was typed by the user in to RDF
        // Insert it to the beginning of the list.
-       var fixedUpURI = gURIFixup.createFixupURI(entryToAdd.Value, 0);
-       gGlobalHistory.markPageAsTyped(fixedUpURI.spec);

This patch is a part of attachment 70207 [details] [diff] [review], the patch that fixed bug 126320
(Invalid typed urls should be hidden and expire quickly).  So, Alec, why do we
call MarkPageAsTyped only if the url doesn't have a space in it?

The js url in c) (see comment 8) has a space in it, so MarkPageAsTyped() is
never called for it.

Comment 13

[Nisheeth Ranjan]

OK, so the "js url not making it to the history" problem is completely
independent of the patch attached to this bug.  Since it has a review and a
super review, I'm gonna go ahead and check it in.

Comment 14

[Daniel Veditz [:dveditz]]

Best to ask Blake about the space thing but maybe it's because typing
independent words with spaces triggers a keyword lookup, and we don't want to
save those. But if so it's awfully simplistic; for one thing it should probably
verify there's no scheme first, for another (if that's really the reason) it
should check whether keyword lookup is even turned on or not.

Comment 15

[timeless]

if you're like me and you have lots of things like 'bug 161531' in your url
history then you don't want to use venkman's break on exception while using
mozilla, the number of exceptions we get because it loops through url history
asking if everything is a url is really painful.  I could probably write a patch
if someone would help me get it approved. personally i'd love to be able to see
which bookmark keyword strings i've used.

Comment 16

[Nisheeth Ranjan]

Fix checked into trunk.

Comment 17

[Nisheeth Ranjan]

I've opened bug 196297 to fix the js/data url with space issue and cc'd some
folks from this bug.

Comment 18

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

I'm not so sure this entire thing is a good idea. I know there are e-commerce
sites out there that wants links like <a href="javascript:popup('foo')"> colored
as visited once the user clicks them.

I would think that what we really want is for javascript/data urls to get into
global history but not be shown in the url-autocomplete.

Comment 19

[Jeremy M. Dolan]

Will this prevent JS links to visited pages from being colored as a vlink?

Some major sites (eg allmusic.com) have every link on the site as a javascript:
href. This breaks "open in new tab", which is annoying, but it shouldn't break
history coloring as well.

Comment 20

[Nisheeth Ranjan]

I agree that we shouldn't break link coloring.  Thanks for pointing that out,
Jonas and Jeremy.

On thinking through this more carefully, it seems that the real security issue
only happens if the "Autocomplete best match as you type" preference in the
autocomplete pref panel is checked because, in this case, the js url is expanded
automatically in the url bar when the user presses enter.

When this pref isn't checked, the js url shows up in the autocomplete dropdown
and the user has to explicitly select it.  Some power users might want clicked
js urls to show up in the autocomplete dropdown.

So, here's a proposed fix that solves the security issue without causing undue
pain to power users:

1) Populate the autocomplete dropdown with JS urls all the time, BUT
2) NEVER automatically expand JS urls in the url bar

What do you think about this approach?  If you approve, I'll file a new bug for
doing the above and mark this one invalid.

Comment 21

[Jesse Ruderman]

Were URLs like javascript:void(0) getting shown as visited when you clicked them
before this bug was fixed?  Or only javascript: URLs that evaluated to
non-undefined values, like javascript:5 ?

If the first type of javascript: URL was getting marked as visited before, I
think that can be fixed by hiding the URL instead of omitting it from global
history.

I don't agree with Nisheeth's distinction between autocomplete and
autocomplete-best-match-as-you-type.  Most "clicked-on" javascript: URLs don't
make sense outside of the context of the page they're in, so allowing them to be
autocompleted at all would be more annoying than useful.  On the other hand,
most "typed-in" javascript: URLs do make sense in multiple contexts, so I want
them to autocomplete according to my prefs.

Comment 22

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

I think i agree with Jesse. Typed js-urls should behave just like any other url
in the autocomplete (weather in the dropdown or inline) but clicked js-urls
should not appear in the autocomplete at all.

Comment 23

[Alec Flett]

interesting. I would go so far as to say that javascript: urls should simply
remain "hidden" in global history, such that they don't show up in the
autocomplete dropdown at all.. i.e. special case javascript: urls in global
history, rather than in the autocomplete UI.

You'll notice that you can set a "hidden" attribute on the entries in global
history, and they won't appear in the autocomplete dropdown - we use this
elsewhere to hide ugly urls like redirects.

Comment 24

[Nisheeth Ranjan]

ok, i think the consensus opinion is to store clicked js urls in the global
history but mark them as hidden so that they don't show up in the autocomplete
ui.  typed js urls should be stored in global history and also show up in the
autocomplete ui.

I've filed bug 197127 to do this.  Will mark this bug invalid and backout this
patch when I check that fix in.

Comment 25

[Jesse Ruderman]

Or you could change the summary of this bug to "javascript: URLs not typed by
the user shouldn't appear in history/autocomplete".

Comment 26

[Nisheeth Ranjan]

No, I'd rather mark the bug invalid because there are posts on the bug that talk
about keeping js urls out of history.  We've decided against doing that and a
simple RESOLVED FIXED on this bug doesn't reflect that.

Comment 27

[Nisheeth Ranjan]

The fix to bug 197127 just got checked into the trunk.  Reopening this bug and
marking it invalid.

Comment 28

[Nisheeth Ranjan]

Marking invalid as per comment 24 and comment 26.