102043 - After HTTP 302 redirect original URL should not exist in history

Status: VERIFIED FIXED

(Core Graveyard :: History: Global, defect, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

After HTTP 302 redirect original URL should not exist in history

From an email (from a developer of a major site that shall remain nameless
unless the reporter wants to make it public):

The scenario is:

(1) the browser hits a URL like
"http://www.foo.com/gosomewhereelse.asp?t=xxxx&y===yyyy"

(2) the server responds with 302 http redirect to
"http://www.foo.com/gosomewhereelse.asp"

(3) the browser does a get on "http://www.foo.com/gosomewhereelse.asp"

(4) the browser renders the final content.

 
The URL at step (2) is the same with step (1) except there is no query parameter.

 
Behavior difference:

NAV6.1 will cache the URL at step (1) in the history, but earlier versions of
the NAV does not.

 

Desired behavior:
any time there is 302 redirect, the originating url should not exist in the
browser history.

 
The reason that I bring this up is that there is the possibility for the query
parameters to be considered private information and we are discussing whether we
have to block the user of Nav 6.1 until we can work around this issue.

Comment 1

[Niels Aufbau]

Many, *many* porn sites listed in thumbnail gallery posts redirect to ad sites.
 I rely on global history to keep those links marked as visited so I don't click
them again while trying to visit each gallery listed in a given TGP.  If you
don't want your data stored in the user's history, don't put the data in a URL
(use the POST method for form submission).

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Radha, could you give this bug a priority and perhaps a preliminary ETA? The
reporter was wondering if there is any progress/when is this going to be
addressed...

Comment 3

[Radha on family leave (not reading bugmail)]

This is scheduled for 0.9.6 which is the next milestone. I will be working on
this in a few days. 

Comment 4

[Radha on family leave (not reading bugmail)]

Heikki,

Looking deeper, this is what I found.  HTTP 302 redirections are not added to
session history (accessible through back and forward buttons).  Sites like
http://www.mozilla.org/start and http://webmail.netscape.com do a redirection
and the original uris don't show up in back/forward/Go menus. 

However, the original uri is added to global history(accessible thro' the
History window and history sidebar panel). This is done so that the original url
will be marked as  'visited' and show up in the visited link color when the page
is visited again. This happens in most portals. Links to channels and news in
major portals are primarily redirections to partner sites etc.... When the
original url is not stored in global history, these links don't show up as
visited when the  site is visited again.  See bug 41973. 

I presume the reporter means the  history window, when he says, "NAV6.1 will
cache the URL at step (1) in the history". 

Specific urls and steps to reproduce  will help me debug this better. 

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think you have dissected the issue correctly as far as I can tell, but I'll
get a confirmation from the reporter.

It does feel like a legitimate concern, especially if there is only one profile
and several users are sharing the profile (like in kiosk-type applications).

I see several ways to solve this, none of which feel too good. The "parameters
stay in global history" is probably true for any URL, not just redirected ones,
and any parameters are potential privacy issues as far as I can tell.

Just about the safest way I can think of would be to store URLs with parameters
as hidden in global session history. To be really safe, they would need to be
encrypted on the disc as well, and only accessible to the browser internals for
determining :visited etc. This seems like an excessive amount of work for this
issue in my opinion. Besides, it would be the user's loss as well since they
would effectively loose access to part of their history.

I don't know if we currently do it, but this could be fixed partially by using
PSM to protect all profile specific data (encryption & password protection at
user's option). Do we do that for global history right now?
Are there any HTTP headers or something that the webserver could set to instruct
the browser not to store a URL in global history?

Comment 6

[Mitchell Stoltz (not reading bugmail)]

We have not really addressed the security concerns of "kiosk mode" yet. If we
decide we need to, then we would need to fix more than just this global history
issue, there are other concerns as well. I'm not sure we should spend the effort
to create a safe kiosk mode right now. Anyone sitting down at your computer can
find out a lot of information from your browsing sessions by looking through
your cache, among other things. The only way to prevent that sort of thing is to
encrypt the whole profile directory, and I really don't think we want to do
that, because a lot of people depend on being able to manually modify the
contents of their profile, and encryption would preclude that. I don't even want
to think about what effect encrypted profiles might have on performance - if you
include the disk cache and mail storage, we're potentially talking about a LOT
of data to be encrypted and decrypted on a regular basis.

I think we should tell the company that reported this problem that their
security concerns are overblown, or are better handled at the OS level. On any
multi-user OS, you should be able to save a profile in your user space so as to
be inaccessible from other users, right? Do we have bugs that prevent that from
working?

Comment 7

[Radha on family leave (not reading bugmail)]

I agree with mstoltz. We are definitely not in a mode to address kiosk mode
problems right now. Plus we have several options like, dynamic profile
switching, disabling global and session history, which can be used in kiosk
modes. If something is secure information, it probably shouldn't be part of a
non-secure url. Global history can probably be modified, so that it does not
save query parameters.  I do not know how feasible it will be to do that in
links in most portals. 

I think this is a candidate for RFE in global history than any serious security
issue. Giving to Global history owner.

Comment 8

[Alec Flett]

I could do this as a part of my title-setting patch.. intercept the load, and
when we get a 302, we remove the url.

However, that's pretty low on my priority list right now..

Comment 9

[Radha on family leave (not reading bugmail)]

Alec, Not all 302 redirections are like the problem described. If you do what
you have described, you will be regressing bug 41973.  Original urls of all
redirections are now purposely added to global history, so that visited link
color will work properly on those sites.

Comment 10

[Arun Ranganathan]

alecf, is there a way that HTTP/1.1 headers can affect how global history works?
 For example, can you wire a fix in the same way session history did w.r.t
headers like:

Cache-control: no-store, no-cache

Maybe providing developers (such as those responsible for the site that logged
this bug) with an HTTP header mechanism might help to determine which 302s get
stored or not.

Comment 11

[Alec Flett]

well, history doesn't have access to the channel or anything, its just notified
to add a url..

Now, I've done some work to add a "history load listener" that lists to all url
loads and tweaks history depending on how it gets loaded.

Comment 12

[Darin Fisher]

the redirected channel will report a failure code via OnStopRequest.  the
failure code will be NS_BINDING_REDIRECTED.  so it seems to me that whoever is
receiving the OnStopRequest needs to be sure to NOT make a session history entry
for the request.  (also, when OnStartRequest fires, the channel's
nsIRequest::status attribute will contain the NS_BINDING_REDIRECTED failure code.)

rick: do you think this could be related to bug 106558?

Comment 13

[Alec Flett]

Ok, I added the files that I've had in my tree for a while... they will show up
in LXR in the next few hours:

http://lxr.mozilla.org/seamonkey/source/xpfe/components/history/src/nsHistoryLoadListener.cpp
http://lxr.mozilla.org/seamonkey/source/xpfe/components/history/src/nsHistoryLoadListener.h

In any case, the basic idea is that on every uri that is loaded, we check the
status, and look for NS_BINDING_REDIRECTED. 
I haven't done the homework to figure out if the page has already been added to
global history or not. I'm hoping that it's not already in history.

The other way to solve this is simply to fix up the place where we call addPage()
I need this class anyway for other bugs, even if it isn't the right way to fix
THIS bug...

Comment 14

[rpotts (gone)]

hey darin,

you could be right...  assuming that this is a regression (that started ~
10/18/01) then this could be a dup...

at the global history level, i don't think that 302 redirects should be visible...

-- rick

Comment 15

[rpotts (gone)]

well, after reading the description a bit closer, i don't think that it is
related to bug #106558 :-(

as Radha pointed out, it sounds like this bug is in direct conflict with bug
#41973.  If the "desired behavior" for this bug is implemented, then we
re-introduce bug #41973.

Comment 16

[Darin Fisher]

yeah, i agree... i didn't consider that when i commented in this bug 2 days back.

Comment 17

[Alec Flett]

I'm mostly done with a fix for the trunk.. moving to 0.9.7

Comment 18

[Alec Flett]

Attachment: add capability to hide rows in global history

This patch ads a new method, hidePage(), to nsIBrowserHistory, which marks a
page as "Hidden" - and then fixes up the base enumerator class to ignore all
hidden pages.
I had to slightly tweak the enumerator's init method, because otherwise I had
no way of obtaining the hidden column's Token ID.. (it's retrieved from an
nsIMdbStore, which the enumerator knows nothing about)

This patch needs to go on the trunk and any branches that need this feature.

On any branches, if we want a quick-fix, we just have to tweak docshell to call
nsIBrowserHistory::HidePage() on this URL once we determine that its a 302
redirect.

On the trunk, we'll use the nsHistoryLoadListener to mark a url as hidden.

Note that this does not affect link coloring, it only affects the enumeration
of urls within history, which currently only happens in RDF.

Comment 19

[Alec Flett]

ignore the makefile.win changes, that's for something else.

Comment 20

[Alec Flett]

oh, and just to give readers an idea of the branch work, I think that in
addition to this patch, it will be either a 1 or 2 line change to nsDocShell.cpp
on the branch to make this work.

Comment 21

[lchiang]

What are some testing suggestions for this?  Claudius, pls work with alec on
this for testing.

Comment 22

[Claudius Gayle]

uhh, actually I think I've got a handle on this one just fine:

Visit some URL's that use 302 redirects (some are listed)
Make sure they don't appear in global history
Make sure we didn't regress bug 41973 (visited links)
Make sure we didn't break history in general
Try some fancy stuff like activating a 302 redirect URL from different
sources(bookmark, history window, ptoolbar, another redirect, launching with the
url)

...but I'll check with alecf just in case

Comment 23

[Alec Flett]

sounds about right...you got everything covered..!

Comment 24

[Chris Waterson]

Yeah, I guess this looks fine. The only thing that's ugly is the fact that
(hertofore general) nsMdbTableEnumerator is now bound up with global history
s3kr1ts. I'd have handled the column skipping in the callers (maybe we'll want
to enumerate those values some day).

Comment 25

[Arun Ranganathan]

Claudius,
A good cheap test is:

http://mombo.mcom.com:8043/redirect-tests/redirect-page.html
You want to see the following:
1. That clicking on the link (which is in fact a link to here:
http://mombo.mcom.com:8043/cgi-bin/nph-redirectTest.cgi but takes you here:
http://home.netscape.com).
2. http://mombo.mcom.com:8043/cgi-bin/nph-redirectTest.cgi is NOT put into
global history unlike pre-patch builds (N6.2).
3. Yet, visiting redirect-page.html shows that the visited link is "purpled-out"
as visited.
Note that 2 and 3 form the crux of a safe test, and match behavior in Comm. 4.x
and IE 6.  If you want to conduct additional test over SSL, I can activate this
for you.

Comment 26

[Alec Flett]

I don't really have a preference one way or the other (base class vs. derived
classes) so I'll move it there & attach a new patch

Comment 27

[Jaime Rodriguez, Jr.]

Nominating for review by EDT.

Comment 28

[Alec Flett]

Attachment: add hidePage to nsIBrowserHistory - updated

I moved the whole "Hidden" concept into the 3 derived classes (URLEnumerator,
SearchEnumerator, AutocompleteEnumerator) and made a static HasCell() to
determine if there is any value in the hidden cell. 

While I was there, I noticed that some of the derived classes were using
"mCurrent" which instead of the row that was passed to them in IsResult() - so
I made some of nsMdbTableEnumerator's member variables private, and used "aRow"
instead of "mCurrent"

waterson, blake, care to r=/sr=?

Comment 29

[Blake Ross]

Comment on attachment 57244 [details] [diff] [review]
add hidePage to nsIBrowserHistory - updated

Looks good to me, r=blake

Comment 30

[(not reading, please use [e-mail] instead)]

Comment on attachment 57244 [details] [diff] [review]
add hidePage to nsIBrowserHistory - updated

sr=sspitzer

Comment 31

[Chris Waterson]

sr=waterson, too.

Comment 32

[Alec Flett]

Attachment: quick fix for branch & trunk

And this is the fix that connects us with docshell... these lines are stuck
smack dab in the middle of the patches to bug 41973 - so it hides only those
pages which were affected by that bug. 

can I get an r=radha, sr=rpotts on this?

Comment 33

[Radha on family leave (not reading bugmail)]

r=radha
Alec, the patch atatched here to docshell looks good. However can you explain
how the patch to bug 71482 will eventually eliminate this patch to docshell?

Comment 34

[Alec Flett]

I'll explain here (it's kind of long-winded for the patch)
I'm hooking up a genric load listener which will listen on all uri loads.. that
load listener, in addition to fixing bug 71482, will also hide URLs that have a
status of NS_BINDING_REDIRECTED.

Comment 35

[Jaime Rodriguez, Jr.]

Adding PDT for tracking and 6.2 branch review purposes.

Comment 36

[rpotts (gone)]

Comment on attachment 58041 [details] [diff] [review]
quick fix for branch & trunk

sr=[e-mail]

Comment 37

[Alec Flett]

ok, fix is in the trunk. I think I mark this fixed now, though both these
patches may go on the embedding branch.

Comment 38

[Jaime Rodriguez, Jr.]

PDT+, pls try and get this one into the 6.2 branch tonight (or at least before 9
am PST tomorrow).

Comment 39

[Alec Flett]

along with this bug, bug 110290 must also be fixed. Its a slight patch on top of
this one.

Comment 40

[Jaime Rodriguez, Jr.]

Help Wanted: Radha or Rick, do either of you feel comfortable checking this in
to the 6.2 branch for alec?

Comment 41

[Jaime Rodriguez, Jr.]

Radha - Do you think you could check this one in today?

Comment 42

[Radha on family leave (not reading bugmail)]

I think I can. I'm building the branch right now. 

Comment 43

[Alec Flett]

Ok, the fix has been checked into the NETSCAPE_6_2_1_BRANCH branch

Comment 44

[Radha on family leave (not reading bugmail)]

 I got the commercial branch to build with the patch. What I see is, the bug is 
fixed in the mozilla build. But not in the commercial build. I used the 
mombo.mcom.com site given above as a sample site. Even if you delete the page 
from history, it will show up again in the history window, next time you open 
it. This does not happen in the mozilla build.  

Another way to get the page to appear in the history window is, keep the history 
window open while you are accessing the page.  It will show up there. Am I the 
only person seeing this?  

Comment 45

[Claudius Gayle]

my results:
Using Arun's test (in the url field of this bug) on a 2001112008
commercial trunk linux build things appear to be okay - as long as the history
window is not open.
However, if the global history window is open while traversing this link, it
gets added to the global  history list right before your eyes.

playing around with this s'more I've seen a littany of bad behaviors that could
be old bugs and/or a result of this checkin. I'm currently in a state where
neither link is being added to GH.

the best way to see all of this is to:

1. Edit-->preferences-->navigator-->history-->Clear History(not strictly necessary)
2. Tasks-->tools-->History
3. View-->Group By-->None (again not strictly necessary but more dramatic error)
4. Load http://mombo.mcom.com:8043/redirect-tests/redirect-page.html
5. Click the link.
6. Notice the http://mombo.mcom.com:8043/cgi-bin/nph-redirectTest.cgi link in
your GH window.
7. Note that closing and reopening the window doesn't cause it to go away.

reopening (based on my and radha's observations) so this bug doesn't slide under
the radar

Comment 46

[Jaime Rodriguez, Jr.]

ME thinks alec is out for the holiday, or at least will not be able to respond
until friday. is there someone else who can pcik this one up, and have a new
patch to address the concerns expressed in the bug (radha, rpotts, waterson ...
bueller)?

Comment 47

[Alec Flett]

Ugh.. I forgot about the fact that history might be visible.
what happens if you collapse and re-expand the folder which contains the url? 
I'm curious if the rdfliner is caching some of the state - I find it interesting
that closing and reopening the window doesn't help.

What we need to do is notify the observers that the item has gone away, and
acknowledge this in HasAssertion()

I can work on now, but won't be able to check in until probably saturday.. 

Comment 48

[Susie Wyshak]

The good news is in my testing I found the redirect cgi:
- can not filled in to the address bar
and
- does not appear in the Go menu as an item

Alec to answer your question, in History the cgi link does disappear when:
- I collapse and reexpand the folder
or
- I close and then reopen history (even without collapsing the folder)

But the bullet proof state is for it never to appear in history..as you know :)

Comment 49

[Peter Trudelle]

cc blake for visual history involvement.  Is there any way someone can get
traction on this today or Friday?  I'm sure generous comp-time or other
arrangement can be reached.

Comment 50

[Alec Flett]

Attachment: notify rdf that a page is now hidden

Ok, I wrote this while offline, so I've got to go test this now, but basically
the above patch tells RDF that the page that was just added is hidden in
hidePage(), and tweaks HasAssertion to return PR_FALSE if the page is hidden.

Comment 51

[Jaime Rodriguez, Jr.]

what are the chances we will have a patch today?

Comment 52

[Alec Flett]

I'd say 100% since I just attached a patch :)
once my build is complete, I'll be testing

Comment 53

[Alec Flett]

Attachment: fix order of notifications (ignore makefile changes)

that other patch was very very close, but I accidentally caused notifications
to happen BEFORE the row was actually hidden. This one does it the right way.
With this patch the redirect url never appears in the UI, tested with the
mambo.mcom.com links above! Yay!

Looking for r=/sr= now..

Comment 54

[Alec Flett]

Comment on attachment 59184 [details] [diff] [review]
fix order of notifications (ignore makefile changes)

oops, just realized there were random makefile changes in there. Ignore them.

Comment 55

[Blake Ross]

Comment on attachment 59184 [details] [diff] [review]
fix order of notifications (ignore makefile changes)

sr=blake

Comment 56

[Chris Waterson]

Comment on attachment 59184 [details] [diff] [review]
fix order of notifications (ignore makefile changes)

r=waterson

Comment 57

[Alec Flett]

edt- per valeski: since embeddors aren't using our history impl, this stuff
doesn't really matter.

That said, this has been checked into the 6_2_1 branch and the trunk.

Comment 58

[Niels Aufbau]

Wait a minute... you're saying that Mozilla embedders that have their own
history mechanism will automatically be vulnerable to this "security hole"? 
That's screwed up.  You should talk to the owner of the site that complained
about this problem, and convince them to use a POST form, instead of adding a
bloaty workaround for a specific use of Gecko.  Please undo this change so that
other users of Gecko don't have to be blocked by sites that rely on the hack.

By the way, how am I supposed to clear all traces of my porn surfing if the
history window doesn't list many of the URLs stored in the history database?  In
my opinion, that's a more important privacy issue than displaying URLs of sites
that negligently place sensitive information in a URL.

Comment 59

[Alec Flett]

no, I'm saying that our history implementation was flawed. Other history
implementations - who knows? depends on the implementation.

Comment 60

[Alec Flett]

by the way, it makes no sense to undo "this change" - this change is the right
fix.. you're right that the site in question is doing the wrong thing, but we
should still be fixing this problem.

Comment 61

[Darin Fisher]

Niels:

this patch makes mozilla behave identically to Netscape 4x and IE w.r.t. storing
redirect urls in global history.  and it is important that we also implement
this behavior because (unfortunately) many sites have grown up depending on it
for some level of percieved security.  this fact is something that can't be
changed overnight, nor will it help the reputation of mozilla to not compromise
on this issue.

also, if you are concerned with erasing your footsteps, there is always the
"Preferences->Navigator->History->Clear History" button.

Comment 62

[Alec Flett]

Also, the "porn feature" (Clear all urls from <porn>.com) still works even on
this hidden urls, so that redirected links won't be hidden/covered.

Comment 63

[Claudius Gayle]

VERIFIED Fixed with 2001112618 branch build -yeah! i shook this fix out every
which way I could think of including but not limited to those listed in my
comment #22. I  should note that these actions don't seem in any way 'secure'
because we show the url all over the place(urlbar,statusbar,bookmarks,etc.) but
never in any persistent way that I could find(like history).

VERIFIED Fixed with 2001112618 branch build