Bug 47894 – Web Inspector: expose request/response cookies in HAR

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Kosyakov]

Since we now support raw request/response headers that include Cookie/Set-Cookie on some platforms, we can expose cookies associated with a resource in HAR.

[Andrey Kosyakov]

Created attachment 71151 [details]
patch

[WebKit Review Bot]

Attachment 71151 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/inspector/front-end/CookieParser.js:1:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 182 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 71151 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71151&action=review

> LayoutTests/http/tests/inspector/resource-har-conversion.html:18
> +        var requestHeaders = JSON.parse(JSON.stringify(resource.requestHeaders));

You should instead make a separate test for cookies only, so that there is no need to preserve rest of the headers.

> LayoutTests/inspector/cookie-parser-expected.txt:136
> +    httponly : undefined

Is this correct?

> WebCore/inspector/front-end/CookieParser.js:33
> +WebInspector.CookieParser = function()

Two things I don't like there:
1) Cookie parsing is tough and is handled by the WebKit already
2) You make all of our remote debugging clients come up with the code like this.

Can we introduce an api for cookie parsing instead so that WebKit was doing it for us? That way we are going to be way more consistent. Ideally, we should get parsed cookies with raw resource data.

> WebCore/inspector/front-end/Resource.js:539
> +        return (new WebInspector.CookieParser()).parseCookie(this.requestHeaderValue("Cookie"));

You could encapsulate this into WebInspector.CookieParser.parseCookie() and .parseSetCookie() respectively.

[Andrey Kosyakov]

Created attachment 71178 [details]
patch

- Changed logic to be closer to real life (Mozilla, IE, Chrome) rather than RFCs
- Dropped support for Set-Cookie2, quoted values and Port field
- Changed semantics of session property
- Removed any special handling for "," (expect multiple cookies to come in multiple headers)
- Now use "\n" when pasting values of headers with same name
- Simplified tests

[Andrey Kosyakov]

(In reply to comment #3)
> (From update of attachment 71151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71151&action=review
> 
> > LayoutTests/http/tests/inspector/resource-har-conversion.html:18
> > +        var requestHeaders = JSON.parse(JSON.stringify(resource.requestHeaders));
> 
> You should instead make a separate test for cookies only, so that there is no need to preserve rest of the headers.

Done (we have multiple resources within this test, so I just sacrificed headers for one of the resources)

> > LayoutTests/inspector/cookie-parser-expected.txt:136
> > +    httponly : undefined
> 
> Is this correct?

This is intended. This is in raw attribute/value pairs, and the attribute in question appears bare (i.e. without any value) in the source, hence I think it's appropriate for it to appear with the value of undefined in cookie.attributes. I realize this could be a bit confusing, but clients are expected to use a higher-level cookie.httpOnly property getter instead, which properly check for ("httponly" in this._attributes).

> > WebCore/inspector/front-end/CookieParser.js:33
> > +WebInspector.CookieParser = function()
> 
> Two things I don't like there:
> 1) Cookie parsing is tough and is handled by the WebKit already

Not quite so -- it is not exposed to WebKit presently, but rather is performed by the platform. Also, at least some platforms only support parsing Set-Cookie header (Cookie: has a slightly different syntax and parsing it is generally of no use on the client side).

> 2) You make all of our remote debugging clients come up with the code like this.
> 
> Can we introduce an api for cookie parsing instead so that WebKit was doing it for us? That way we are going to be way more consistent. Ideally, we should get parsed cookies with raw resource data.
> 

As discussed offline -- this would be nice to save us from any possible inconsistencies WRT platform logic, but comes as a price of doing plumbing for cookie parsing call for all platforms and does not solve parsing for reqeust cookie. So propose to leave this as is for the time being.

> > WebCore/inspector/front-end/Resource.js:539
> > +        return (new WebInspector.CookieParser()).parseCookie(this.requestHeaderValue("Cookie"));
> 
> You could encapsulate this into WebInspector.CookieParser.parseCookie() and .parseSetCookie() respectively.

Done.

[Andrey Kosyakov]

Manually committed r70137: http://trac.webkit.org/changeset/70137