Drop Set-Cookie from XHR response headers

Add a third flavor to response header persistence.
XMLHttpRequest response headers should not contain Set-Cookie or Set-Cookie2 headers.

[Dean McNamee, 2008-11-19 17:04:59]

Just skimmed it, looks really good over all.  Removing the true / false
parameter is nice even without the additional mode, because it makes the callers
much more clear.

http://codereview.chromium.org/11264/diff/1/4
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/11264/diff/1/4#newcode69
Line 69: // over IPC.
Maybe say why, that we want to filter http only cookies, but instead we're just
filtering all cookies, because we don't want to leak this knowledge into the
renderer.

http://codereview.chromium.org/11264/diff/1/4#newcode108
Line 108: switch (options) {
I think a switch for 2 cases is overkill.

http://codereview.chromium.org/11264/diff/1/4#newcode120
Line 120: // this just copies the status line w/ terminator
I saw this code before and thought it was a little bit sketchy.  Might just want
to improve the comment, noting that raw_headers_ has embedded nulls, and we want
the substring before the first null.  I realize this isn't your code.

http://codereview.chromium.org/11264/diff/1/4#newcode135
Line 135: // includes terminator
// We write comments like this.

Realize it isn't your code, but might as well clean it up.

http://codereview.chromium.org/11264/diff/1/4#newcode630
Line 630: // statically cached hash_set to avoid duplicated work?
Should this be a TODO?

http://codereview.chromium.org/11264/diff/1/5
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/11264/diff/1/5#newcode53
Line 53: // Appends a representation of this object to the given pickle.  If
options equals
80 cols

http://codereview.chromium.org/11264/diff/1/5#newcode256
Line 256: // Returns the headers that should not be passed to IPC requests.
Could you improve this comment a bit.  Which headers?  Why are they special?

[darin (slow to review), 2008-11-19 17:11:37]

Do we need this change given:
http://trac.webkit.org/changeset/38566
https://bugs.webkit.org/show_bug.cgi?id=10957

http://codereview.chromium.org/11264/diff/1/5
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/11264/diff/1/5#newcode50
Line 50: PERSIST_FOR_IPC = 2,
it is a little strange to mention things like IPC here in the HttpCache code. 
that implies knowledge of chrome internals, which this layer knows nothing of
otherwise.  is there a better way to describe what this alternative mode is
doing?

[Dean McNamee, 2008-11-19 17:34:50]

http://codereview.chromium.org/11264/diff/1/4
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/11264/diff/1/4#newcode110
Line 110: GetNonIPCHeaders(&filtered_headers);
I guess now this should be GetCookieHeaders, and this should be
PERSIST_WITHOUT_COOKIES or something.

http://codereview.chromium.org/11264/diff/1/5
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/11264/diff/1/5#newcode50
Line 50: PERSIST_FOR_IPC = 2,
As Darin pointed out, it's definitely important to keep these components
conceptually isolated, so FOR_IPC doesn't make sense here.

Maybe a better name would just be to make it explicit as to what it's doing. 
PERSIST_WITHOUT_COOKIE_HEADERS ?  I'm not familiar with the code that much, I'm
guessing _RAW is everything, _CACHE has some things filtered, is your _IPC the
same filtering as _CACHE plus cookies?  Maybe that should be denoted some how,
like FOR_CACHE_WITHOUT_COOKIES.  Dunno.

http://codereview.chromium.org/11264/diff/1/5#newcode257
Line 257: static void GetNonIPCHeaders(HeaderSet* header_names);
On second reading, this name is really strange.  Do you mean
GetHeadersThatAreUnsafeForIPC?

[Dean McNamee, 2008-11-19 22:13:38]

Looks better, still some nits.  It's up to Darin on the real code.

http://codereview.chromium.org/11264/diff/408/18
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/11264/diff/408/18#newcode1392
Line 1392: net::HttpResponseHeaders::PERSIST_WITHOUT_SENSITIVE_HEADERS);
p->Persist(
    m, net:: ... );

http://codereview.chromium.org/11264/diff/408/16
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/11264/diff/408/16#newcode112
Line 112: }
I find the switch here strange.  I liked it better the way you had it I think. 
Can you just early terminate on RAW, and then handle the case for _CACHE /
SENESITVE.  filter_headers will be empty in the else case.

[mschilder, 2008-11-19 22:29:38]

On 2008/11/19 22:13:38, Dean McNamee wrote:
> Looks better, still some nits.  It's up to Darin on the real code.
> 
> http://codereview.chromium.org/11264/diff/408/18
> File chrome/common/render_messages.h (right):
> 
> http://codereview.chromium.org/11264/diff/408/18#newcode1392
> Line 1392: net::HttpResponseHeaders::PERSIST_WITHOUT_SENSITIVE_HEADERS);
> p->Persist(
>     m, net:: ... );

done

> 
> http://codereview.chromium.org/11264/diff/408/16
> File net/http/http_response_headers.cc (right):
> 
> http://codereview.chromium.org/11264/diff/408/16#newcode112
> Line 112: }
> I find the switch here strange.  I liked it better the way you had it I think.

> Can you just early terminate on RAW, and then handle the case for _CACHE /
> SENESITVE.  filter_headers will be empty in the else case.

done. Added a comment and NOREACHED for else as well. Overkill?

[mschilder, 2008-11-19 22:30:27]

On 2008/11/19 17:11:37, darin wrote:
> Do we need this change given:
> http://trac.webkit.org/changeset/38566
> https://bugs.webkit.org/show_bug.cgi?id=10957

I think it still has merit to filter on the trusted end of the IPC, whether
webkit also filters or not.

> 
> http://codereview.chromium.org/11264/diff/1/5
> File net/http/http_response_headers.h (right):
> 
> http://codereview.chromium.org/11264/diff/1/5#newcode50
> Line 50: PERSIST_FOR_IPC = 2,
> it is a little strange to mention things like IPC here in the HttpCache code. 
> that implies knowledge of chrome internals, which this layer knows nothing of
> otherwise.  is there a better way to describe what this alternative mode is
> doing?

fixed

[Dean McNamee, 2008-11-19 22:32:42]

Style wise looks ok by me.  Darin on the rest.

http://codereview.chromium.org/11264/diff/412/416
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/11264/diff/412/416#newcode257
Line 257: // e.g. Set-Cookie, Set-Cookie2.
it actually returns them lower cased, right?  So maybe your example is
misleading.

[darin (slow to review), 2008-11-19 22:35:42]

http://codereview.chromium.org/11264/diff/411/23
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/11264/diff/411/23#newcode50
Line 50: PERSIST_WITHOUT_SENSITIVE_HEADERS = 2,
hmm... vaguely naming this seems not so great.  i think it is better to be
specific here so that the caller, who needs to make the choice about renderer
security, can be the one making the policy decision about what "sensitive
headers" means.

so, i would name these like so:

  PERSIST_ALL,
  PERSIST_SANS_COOKIES,
  PERSIST_SANS_TRANSIENTS

you could comment that 'cookies' are a subset of 'transients', where transients
also include the hop-by-hop response headers (as defined by RFC 2616), response
headers deemed non-cacheable by the server, and auth challenge response headers.

we could also break this down further into an exclusive set:

  PERSIST_ALL,
  PERSIST_SANS_COOKIES,
  PERSIST_SANS_CHALLENGES,
  PERSIST_SANS_HOP_BY_HOP,
  PERSIST_SANS_NON_CACHEABLE,

then the HttpCache code could combine all of these to get its desired behavior.

i'm fine with either breakdown of flags.

[mschilder, 2008-11-19 22:40:25]

On 2008/11/19 22:32:42, Dean McNamee wrote:
> Style wise looks ok by me.  Darin on the rest.
> 
> http://codereview.chromium.org/11264/diff/412/416
> File net/http/http_response_headers.h (right):
> 
> http://codereview.chromium.org/11264/diff/412/416#newcode257
> Line 257: // e.g. Set-Cookie, Set-Cookie2.
> it actually returns them lower cased, right?  So maybe your example is
> misleading.

yeah, habit. The actual matching is done case-insensitive due to lower-casing
the other input. I'll update the comment to lower-case.

[mschilder, 2008-11-20 02:37:50]

On 2008/11/19 22:35:42, darin wrote:
> http://codereview.chromium.org/11264/diff/411/23
> File net/http/http_response_headers.h (right):
> 
> http://codereview.chromium.org/11264/diff/411/23#newcode50
> Line 50: PERSIST_WITHOUT_SENSITIVE_HEADERS = 2,
> hmm... vaguely naming this seems not so great.  i think it is better to be
> specific here so that the caller, who needs to make the choice about renderer
> security, can be the one making the policy decision about what "sensitive
> headers" means.
> 
> so, i would name these like so:
> 
>   PERSIST_ALL,
>   PERSIST_SANS_COOKIES,
>   PERSIST_SANS_TRANSIENTS
> 
> you could comment that 'cookies' are a subset of 'transients', where
transients
> also include the hop-by-hop response headers (as defined by RFC 2616),
response
> headers deemed non-cacheable by the server, and auth challenge response
headers.
> 
> we could also break this down further into an exclusive set:
> 
>   PERSIST_ALL,
>   PERSIST_SANS_COOKIES,
>   PERSIST_SANS_CHALLENGES,
>   PERSIST_SANS_HOP_BY_HOP,
>   PERSIST_SANS_NON_CACHEABLE,
> 
> then the HttpCache code could combine all of these to get its desired
behavior.

Ok, i took the latter suggestion and broke the options down to bits. Not sure
whether the bitmask/const int approach is clearer than say a PersistOptions
object. I see either approach in the codebase..

> 
> i'm fine with either breakdown of flags.

[darin (slow to review), 2008-11-20 03:50:17]

LGTM !