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.
<rdar://problem/34071927>
Created attachment 322974 [details] [IMAGE] Cookies View
Created attachment 322975 [details] [IMAGE] Cookies - some empty
Created attachment 322976 [details] [IMAGE] Headers - Set-Cookie (before)
Created attachment 322977 [details] [IMAGE] Headers - Set-Cookie (after)
Created attachment 322978 [details] [PATCH] Proposed Fix
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?!
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 :)
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.
> 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.
Created attachment 323143 [details] [PATCH] Proposed Fix
Do you have a screenshot with a redirect?
(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.
(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>
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?
<https://trac.webkit.org/r223058>