Bug 177988 – Web Inspector: Network Tab - Cookies Detail View

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Web Inspector: Network Tab - Cookies Detail View

It can be useful to parse out the cookies from the difficult to read `Cookie` request and `Set-Cookie` response headers into tables. We do this already for cookies on a per-domain basis but this show more specifically what happened in an individual request.

[Joseph Pecoraro]

<rdar://problem/34071927>

[Joseph Pecoraro]

Created attachment 322974 [details]
[IMAGE] Cookies View

[Joseph Pecoraro]

Created attachment 322975 [details]
[IMAGE] Cookies - some empty

[Joseph Pecoraro]

Created attachment 322976 [details]
[IMAGE] Headers - Set-Cookie (before)

[Joseph Pecoraro]

Created attachment 322977 [details]
[IMAGE] Headers - Set-Cookie (after)

[Joseph Pecoraro]

Created attachment 322978 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Cookie I need to test:

> name=value==;Domain=.example.com;Expires=Wed, 04-Apr-2018 03:34:02 GMT

I'm looking for '; ' but it appears some servers send cookies without the space?!

[Devin Rousso]

Comment on attachment 322978 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:28
> +    constructor(type, name, value, raw, expires, maxAge, path, domain, secure, httpOnly)

I'd like to move more WebInspector code to having "optional" parameters be inside of an `options` argument, as it makes future changes to widespread usage much simpler.  That being said, I'm not sure how much cookies will change, but it might be worth it regardless.

    constructor(type, name, value, {raw, expires, maxAge, path, domain, secure, httpOnly} = {})

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:30
> +        console.assert(type === WI.Cookie.Type.Request || type === WI.Cookie.Type.Response);

You could "simplify" this by using Object.values.  It isn't necessarily simpler, but it is a bit more future-proof.

    console.assert(Object.values(WI.Cookie.Type).includes(type));

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:43
> +        this.name = name || "";
> +        this.value = value || "";

Since you assert above, it it necessary to have a default value?

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:73
> +            let match = pair.match(/^(?<name>[^\s=]+)[ \t]*=[ \t]*(?<value>.*)$/);

In this regex (and the others below) you check for `[^\s]`, but then use `[ \t]` later on.  Is there a reason for this difference?  I would think that they would match.

    let match = pair.match(/^(?<name>[^ \t=]+)[ \t]*=[ \t]*(?<value>.*)$/);

OR

    let match = pair.match(/^(?<name>[^\s=]+)\s*=\s*(?<value>.*)$/);

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:117
> +            let match = attribute.match(/^(?<name>[^\s=]+)(?:=(?<value>.*))?$/);

Are attributes not allowed to have spaces around the "=" (assuming it exists)?

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:30
> +        super(null);

NIT: use a const variable.

    const representedObject = null;
    super(representedObject);

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:99
> +    initialLayout()

Add `super.initialLayout()` call, or a comment as to why it isn't needed/wanted.

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:138
> +            return;

Style: since this function returns a value, this should be `return null;`.  I think ESLint should show a warning for this.

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:179
> +                return a > b ? 1 : -1;

What about if the dates are equal?  Does that case matter much for ordering?

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:185
> +            return;

Ditto (138).

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:272
> +        const rowsHeight = this._dataSourceForTable(table).length * table.rowHeight;

Style: I think this should be a `let` instead of a `const`.

> Source/WebInspectorUI/UserInterface/Views/TableColumn.js:28
> +    constructor(identifier, name, {initialWidth, minWidth, maxWidth, hidden, sortable, hideable, align, resizeType} = {})

This is why I like `options` objects in constructors :)

[Joseph Pecoraro]

Comment on attachment 322978 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:43
>> +        this.value = value || "";
> 
> Since you assert above, it it necessary to have a default value?

Asserts are what we expect. This protects us against something going wrong in the wild.

>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:73
>> +            let match = pair.match(/^(?<name>[^\s=]+)[ \t]*=[ \t]*(?<value>.*)$/);
> 
> In this regex (and the others below) you check for `[^\s]`, but then use `[ \t]` later on.  Is there a reason for this difference?  I would think that they would match.
> 
>     let match = pair.match(/^(?<name>[^ \t=]+)[ \t]*=[ \t]*(?<value>.*)$/);
> 
> OR
> 
>     let match = pair.match(/^(?<name>[^\s=]+)\s*=\s*(?<value>.*)$/);

So the syntax here from RFC 6265 is:
https://www.ietf.org/rfc/rfc6265.txt

   cookie-header = "Cookie:" OWS cookie-string OWS
   cookie-string = cookie-pair *( ";" SP cookie-pair )
   cookie-pair   = cookie-name "=" cookie-value

OWS is:

   OWS            = *( [ obs-fold ] WSP )
                    ; "optional" whitespace
   obs-fold       = CRLF

And RFC 5234 defines WSP as:
https://tools.ietf.org/html/rfc5234

    SP    =  %x20
    HTAB  =  %x09
          ; horizontal tab
    WSP   =  SP / HTAB
          ; white space

I was also expecting that newlines might terminate the header and might not be allowed.

In any case most of this doesn't matter so we can probably simplify.

For reference this is what Blink does, which is somewhat close to this, but less strict:
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/devtools/front_end/sdk/CookieParser.js#128

>> Source/WebInspectorUI/UserInterface/Models/Cookie.js:117
>> +            let match = attribute.match(/^(?<name>[^\s=]+)(?:=(?<value>.*))?$/);
> 
> Are attributes not allowed to have spaces around the "=" (assuming it exists)?

My reading of the spec is that spaces not allowed. In practice servers might send badly formatted data, and browsers might accept it, so we might want to loosen this.

>> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:138
>> +            return;
> 
> Style: since this function returns a value, this should be `return null;`.  I think ESLint should show a warning for this.

Oops, yes. Good catch.

>> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:179
>> +                return a > b ? 1 : -1;
> 
> What about if the dates are equal?  Does that case matter much for ordering?

I don't think 0 or 1 matters. I can probably just do a.getTime() - b.getTime() though.

[Joseph Pecoraro]

> So the syntax here from RFC 6265 is:
> https://www.ietf.org/rfc/rfc6265.txt
> 
>    cookie-header = "Cookie:" OWS cookie-string OWS
>    cookie-string = cookie-pair *( ";" SP cookie-pair )
>    cookie-pair   = cookie-name "=" cookie-value
> 

I forgot got copy my comments over from outside the comment field:

So the syntax here from RFC 6265 is:
https://www.ietf.org/rfc/rfc6265.txt

   cookie-header = "Cookie:" OWS cookie-string OWS
   cookie-string = cookie-pair *( ";" SP cookie-pair )
   cookie-pair   = cookie-name "=" cookie-value
   cookie-name   = token
   cookie-value  = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
   cookie-octet  = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                 ; US-ASCII characters excluding CTLs,
                 ; whitespace DQUOTE, comma, semicolon,
                 ; and backslash

The interesting part being whitespace isn't allowed in the octet.

[Joseph Pecoraro]

Created attachment 323143 [details]
[PATCH] Proposed Fix

[Sam Weinig]

Do you have a screenshot with a redirect?

[Joseph Pecoraro]

(In reply to Sam Weinig from comment #12)
> Do you have a screenshot with a redirect?

Since Web Inspector doesn't maintain redirect data yet, it'll just show the last redirect / request's cookies in the Request section.

[Blaze Burg]

(In reply to Joseph Pecoraro from comment #13)
> (In reply to Sam Weinig from comment #12)
> > Do you have a screenshot with a redirect?
> 
> Since Web Inspector doesn't maintain redirect data yet, it'll just show the
> last redirect / request's cookies in the Request section.

Tracked internally in <rdar://problem/5378164>

[Blaze Burg]

Comment on attachment 323143 [details]
[PATCH] Proposed Fix

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

r=me, this table code is super easy to follow now! (Well, at least it fits in just one file..)

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:560
> +localizedStrings["Max-Age"] = "Max-Age";

This is a header literal, yes? In that case it shouldn't be localized.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:29
> +    {

I tend to agree with Devin's comment about moving optional stuff into options={}.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:58
> +    // RFC 6265 Defines the HTTP Cookie and Set-Cookie header fields:

nit: defines

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:79
> +                console.error("Failed to parse Cookie pair", pair);

Please use WI.reportInternalError("Failed to parse Cookie", {header, pair}) so that engineers will report cookies that this code doesn't know how to parse.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:102
> +            console.error("Failed to parse Set-Cookie header", header);

Ditto.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:123
> +                console.error("Failed to parse Set-Cookie attribute:", attribute);

Ditto.

> Source/WebInspectorUI/UserInterface/Models/Cookie.js:142
> +                    console.warn("Invalid MaxAge value:", attributeValue);

Is this something we ever care about seeing in Inspector^2?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:439
> +            // FIXME: The backend sends multiple "Set-Cookie" headers in one "Set-Cookie" with multiple values

Is this something that we can fix without worrying about breakage, or is it performed outside of Inspector code?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:442
> +            // ", " in the the date format "Expires=Tue, 03-Oct-2017 04:39:21 GMT". To improve hueristics

Nit: heuristics

> Source/WebInspectorUI/UserInterface/Views/ResourceCookiesContentView.js:114
> +    _incompleteSectionWithMessage(section, message)

Nit: there's no verb here but it's doing stuff. Perhaps  _markSectionIncompleteWithMessage, _markSectionIncompleteWithLoadingIndicator?

[Joseph Pecoraro]

<https://trac.webkit.org/r223058>