Web Inspector: support editing cookie key/values from inspector

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jonas Due Vesterheden]

It would be great if it was possible to edit and delete cookies in the Storage section of the web inspector.

[Timothy Hatcher]

You can delete. Edit will be harder.

[Jonas Due Vesterheden]

Sorry, I managed to overlook the 'X'-button!

Would it be possible to bind backspace to "Delete cookie"?

[Brian Weinstein]

(In reply to comment #2)
> Sorry, I managed to overlook the 'X'-button!
> 
> Would it be possible to bind backspace to "Delete cookie"?

That would be useful, if you want to file a new bug and assign it to me, I can take a look at it today or tomorrow.

[Brian Weinstein]

Delete is now bound to delete cookie as of http://trac.webkit.org/changeset/50613.

[timmywil]

It would be awesome to be able to delete more than one cookie at a time on a site.

[Radar WebKit Bug Importer]

<rdar://problem/19281523>

[BJ Burg]

The DataGrid editing should just be a flag, and the backend will need a new "setCookie" command in Page.json. That's the easy part.

If we only support editing name and value, then that can be implemented as a delete + adding a new cookie via Document.cookie= under the hood.

If editing other fields is to be supported, or loading/saving cookies to file, then a new CookieJar method is needed that takes in the various header fields and makes an already-parsed cookie. This could be nontrivial depending on the port.

[Bem Jones-Bey]

I recently ran into this issue, and it forced me to use Firefox/Chrome to test some code I was working on.

Editing just the name and value would be sufficient for me, and seems like it might be sufficient for this issue given that it specifically says key/value in the title. If there is agreement that that simple path is good enough, I may try my hand at putting together a patch for this.

[Devin Rousso]

Created attachment 394190 [details]
Patch

[Devin Rousso]

Created attachment 394191 [details]
[Image] After Patch is applied

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS]

Committed r259173: <https://trac.webkit.org/changeset/259173>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394190 [details].

[Joseph Pecoraro]

Comment on attachment 394190 [details]
Patch

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

Nice!!

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:627
> +    switch (sameSite.value()) {
> +    case Inspector::Protocol::Page::CookieSameSitePolicy::None:
> +        cookie.sameSite = Cookie::SameSitePolicy::None;
> +
> +        break;
> +    case Inspector::Protocol::Page::CookieSameSitePolicy::Lax:
> +        cookie.sameSite = Cookie::SameSitePolicy::Lax;
> +
> +        break;
> +    case Inspector::Protocol::Page::CookieSameSitePolicy::Strict:
> +        cookie.sameSite = Cookie::SameSitePolicy::Strict;
> +        break;
> +    }

Style: Weird blank lines.

> Source/WebCore/loader/CookieJar.cpp:177
> +void CookieJar::setRawCookie(const Document&, const Cookie& cookie)
> +{
> +    if (auto* session = m_storageSessionProvider->storageSession())
> +        session->setCookie(cookie);

Is this safe to do? If someone sets an HttpOnly cookie via the inspector can they get it back out with `document.cookie`, or is that checked at access time?

[Devin Rousso]

Comment on attachment 394190 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:627
>> +    }
> 
> Style: Weird blank lines.

o.0 how did that happen?  Damn it's times like these I wish I could amend a commit so that I could fix obvious mistakes like this :(

>> Source/WebCore/loader/CookieJar.cpp:177
>> +        session->setCookie(cookie);
> 
> Is this safe to do? If someone sets an HttpOnly cookie via the inspector can they get it back out with `document.cookie`, or is that checked at access time?

I don't think so.  I tried testing this scenario and I didn't see it in `document.cookie`.  Also, reading the code, it looks like `cookiesForDOM` passes `DoNotIncludeHTTPOnly`.

[David Kilzer (:ddkilzer)]

(In reply to EWS from comment #12)
> Committed r259173: <https://trac.webkit.org/changeset/259173>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 394190 [details].

This broke the Windows build:  https://build.webkit.org/builders/Apple%20Win%2010%20Release%20(Build)?numbuilds=50

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #15)
> (In reply to EWS from comment #12)
> > Committed r259173: <https://trac.webkit.org/changeset/259173>
> > 
> > All reviewed patches have been landed. Closing bug and clearing flags on
> > attachment 394190 [details].
> 
> This broke the Windows build: 
> https://build.webkit.org/builders/
> Apple%20Win%2010%20Release%20(Build)?numbuilds=50

Atempt to fix Windows build:

Committed r259179: <https://trac.webkit.org/changeset/259179>