93027 - form element contents are stored in session history with Cache-Control: no-store

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Bradley Baetz (:bbaetz)]

Darin's fix for bug 92598 means that pages with cache-control: no-store are not
placed in the http cache.

Pollmann's fix for bug 65947 means that password fields are never stored in
session history.

However, there is still the case where a page sending cache-control: no-store
will have name/address/etc data stored in session history. We should either not
store this data if http got a no-store header, or possibly not store data in
session history when autocomplete="off" is present on the form.

For comparison, IE doesn't store the data in session history when it gets a
"Cache-Control: no-cache" header.

Also see my 2001-07-19 17:38 comment in bug 65947.

NOTE: The test page is just a form with a text element and a submit button,
which sends the cache-control header for that page. Theres no point in attaching
it to bugzilla because bugzilla won't send the http header, which is the
important part.

Comment 1

[Eric Pollmann]

Passing to Radha, who owns the logic that controls higher level operation of
session history, such as this.  Let me know if there is anything I can help
with, though.  :)

Comment 2

[Darin Fisher]

radha: you can determine that HTTP has decided NOT to cache its data by checking
the cacheToken attribute on nsICachingChannel (which nsHttpChannel implements).
if the cacheToken is NULL, then that means that HTTP has decided not to cache its
data.

Comment 3

[Arun Ranganathan]

I suppose we have 2 choices:
1. Extend the implementation of autocomplete="off" so that it also thwarts Form
Manager from picking up the values from session history (despite the fact that a
fresh form has been received).
2. Blow away the form values from session history upon receipt of Cache-control:
no-store header.
I'm in favor of the latter (#2 above).  We ought to have at least one mechanism
(albeit HTTP/1.1 header based) to reproduce the caching behavior that used to be
present in Comm. 4.x (and IE).  Obliging programmers to first send us the
header, followed by a markup based solution (autocomplete='off') to clean up
session history isn't as robust as having this ride with the header alone. 
Nominating topembed and nsbranch respectively.  Giving sites some control of
cache is important!

Comment 4

[Bradley Baetz (:bbaetz)]

I'm in favour of doing both. Since we've decided to follow these specs for
no-store (ie never even keep it for session history), then forms should follow.

By the same logic, if people don't want it in the password manager, then we
shouldn't autocomplete. OTOH, there could be a large form which people will make
a typo in, then be told press back to change the value, and it wouldn't work. So
I'm not as convinced about this second case - what do other browsers do?

Comment 5

[Radha on family leave (not reading bugmail)]

darin: would there be a cachekey when the page has "no-cache" control key?  Can
CacheKey(instead of cacheToken)  be used to determine this? 

The reason I ask is, SH already stores cacheKey for all pages and passes it back
to the channel for all history operations. If I can use cacheKey instead of
cacheToken, I don't have to add additional attributes to nsISHEntry.

Another question is, can a page have a cacheKey but not a cacheToken? If so,
Should  SH pass the cacheKey to the channel when a user goes back to a page with
"no-cache"?

Comment 6

[Darin Fisher]

i'm assuming you mean 'no-store' and not 'no-cache' ... believe it or not
'no-cache' doesn't actually mean that the data shouldn't be put in the cache ;-)

to answer your question, a channel can have a cacheKey without a cacheToken. 
the presence of a cacheToken means that the cache is being used.  the presence
of a cacheKey just indicates where the data would be stored if it were to be
stored in the cache.

Comment 7

[Bradley Baetz (:bbaetz)]

However, IE does this on the autocomplete="off" tag - it disables their password
manager and session history, while we just disable wallet.

Can we consider IE compatability for this, as well as the no-store stuff?

Comment 8

[Darin Fisher]

well then, sounds good to me.

Comment 9

[Arun Ranganathan]

1. I'm going to agitate for an nsenterprise nomination for this bug for
Enterprise Client purposes.
2. I'm going to farm out a new bug for the autocomplete="off" issue.  I've come
around to bbaetz's way of thinking about the problem and am completely in favor
of IE compatibility for the autocomplete="off" feature that IE supports (see
http://msdn.microsoft.com/library/default.asp?url=/workshop/author/forms/autocomplete_ovr.asp
 -- scroll down to about 2/3 of the way for a discussion of this).  Having
no-store header prevent Session History from storing form elements, *and also*
having autocomplete="off" do this, strike me as a good idea, but it's a separate
bug and shouldn't confuse the fixer of this bug :-)

Comment 10

[Arun Ranganathan]

OK, as promised, the new bug is bug 93972 which addresses the autocomplete="off"
issue.  Any discussion of a markup based solution to removing form values from
session history should be posted there.  However, I'm not touching the issue of
META tag with a barge pole.  Perhaps some new bugs ought to be farmed out to for
this and they ought to be labeled nsenterprise or topembed.  Darin, if they
aren't logged already, could you log them?  I'm not totally sure about what
works and what doesn't.

Comment 11

[Radha on family leave (not reading bugmail)]

Does anyone have a testcase with form values, so I can test this?vd

Comment 12

[Radha on family leave (not reading bugmail)]

Attachment: patch to docshell and Session History

Comment 13

[Arun Ranganathan]

Radha,
Here's some easy test cases.  Here's a CGI script that I use written in shell:

#!/bin/sh
echo "HTTP/1.1 200 OK"
echo "Content-type: text/html"
echo "Cache-control: no-store, no-cache"
echo "Connection: close"
echo "Pragma: no-cache"
echo ""
cat myFormFile.html

Then merely append any test file which posts forms to it.  I've got tons of
these types of tests on my machine in my cube, so if you wish, you can stop by.

Comment 14

[Arun Ranganathan]

whoops, not post forms *to it* but posts forms from it.  for example, the file
myFormFile.html in above cgi can look like this:

<body><form method="post"
action="http://mombo:8043/cgi-bin/post-query.cgi"><input type="text"><input
type="submit"></form></body>

Click 'Back'.  You should be back to seeing the form values filled in, despite
the fresh 'GET' request.

Comment 15

[Darin Fisher]

a couple nits:

1) replace C style comments with C++ comments
2) indentation is inconsistent (surrounding text uses 4 spaces, so should you)
3) consider PRPackedBool instead of PRBool when declaring member variables.  it 
   saves space.
fix these and r/sr=darin.

Comment 16

[Eric Pollmann]

r=[e-mail] - with the aforementioned nits fixed, this looks good!

Comment 17

[Radha on family leave (not reading bugmail)]

*** Bug 92608 has been marked as a duplicate of this bug. ***

Comment 18

[Marek Z. Jeziorek]

[e-mail], could you verify it on the trunk? When you are done please
let radha know, so she can check this into the0.9.2 branch (permission granted)

Comment 19

[Madhur Bhatia]

I am not the right QA contact for the session history bugs. The right person to 
verify this bug would be [e-mail]

Comment 20

[Radha on family leave (not reading bugmail)]

Claudius, could you get back to me on this ASAP so that Ic an check in the patch
to the branch

Comment 21

[Claudius Gayle]

...talked to radha, did some testing, and uh - we're working on it. The results
are currently inconclusive.

Comment 22

[Radha on family leave (not reading bugmail)]

I tried to verify this bug and this is what i'm seeing:

Looking thro' the debugger, the url provided above, does not seem to have any
no-store cache controls, becasue I do see a cache token for the page, but there
is no cache token for the cgi, no-store.cgi.   To verify this bug, I think the
form page no-store.html should have the "no-store" cache controls. bbaetz, if
you provided the test case above, can you confirm that the page no-store.html
does have "cache-control: nostore" set in it.  Thanks,

Comment 23

[Bradley Baetz (:bbaetz)]

That test page doesn't - you need to use the one arun mentioned in his comments.

Comment 24

[Radha on family leave (not reading bugmail)]

I tried the example given by arun  few weeks ago in his presence and got it
verified then. Fix checked in to 0.9.2 branch. markign fixed.