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

RESOLVED INVALID

Status

()

Core
History: Global
--
minor
RESOLVED INVALID
16 years ago
15 years ago

People

(Reporter: Jesse Ruderman, Assigned: Nisheeth Ranjan)

Tracking

Trunk
mozilla1.4alpha
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: security, URL)

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Created attachment 94360 [details]
testcase
(Reporter)

Comment 2

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

16 years ago
Created attachment 108153 [details] [diff] [review]
Patch to leave data and js urls out of history unless typed by user

Comment 4

16 years ago
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.

Updated

16 years ago
Attachment #108153 - Flags: superreview?(jst)
Attachment #108153 - Flags: review?(mstoltz)
Comment on attachment 108153 [details] [diff] [review]
Patch to leave data and js urls out of history unless typed by user

sr=jst
Attachment #108153 - Flags: superreview?(jst) → superreview+
Comment on attachment 108153 [details] [diff] [review]
Patch to leave data and js urls out of history unless typed by user

r=mstoltz.
Attachment #108153 - Flags: review?(mstoltz) → review+
(Assignee)

Comment 7

16 years ago
Re-assigning to myself as fixing this bug is related to fixing bug 161546...
Assignee: blaker → nisheeth
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.4alpha
(Assignee)

Comment 8

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

16 years ago
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".. 
(Assignee)

Comment 10

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

16 years ago
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.
(Assignee)

Comment 12

16 years ago
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.
(Assignee)

Comment 13

16 years ago
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.
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

16 years ago
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.
(Assignee)

Comment 16

16 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

16 years ago
I've opened bug 196297 to fix the js/data url with space issue and cc'd some
folks from this bug.
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

16 years ago
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.
(Assignee)

Comment 20

16 years ago
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.
(Reporter)

Comment 21

16 years ago
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.
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

16 years ago
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.
(Assignee)

Comment 24

16 years ago
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.
(Reporter)

Comment 25

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

Comment 26

16 years ago
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.
(Assignee)

Comment 27

16 years ago
The fix to bug 197127 just got checked into the trunk.  Reopening this bug and
marking it invalid.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

16 years ago
Marking invalid as per comment 24 and comment 26.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → INVALID
(Reporter)

Updated

15 years ago
Whiteboard: security
You need to log in before you can comment on or make changes to this bug.