Bug 31157

Summary: Web Inspector: support editing cookie key/values from inspector
Product: WebKit Reporter: Jonas Due Vesterheden <jonasduevesterheden>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Enhancement CC: bjonesbe, bweinstein, cdumez, ddkilzer, drousso, ews-watchlist, graouts, inspector-bugzilla-changes, japhet, joepeck, keishi, keith_miller, mark.lam, msaboff, pfeldman, pmuellr, rik, sbarati, timothy, tim.willison, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=156298
Bug Depends on: 156091    
Bug Blocks: 209389    
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied none

Description Jonas Due Vesterheden 2009-11-05 01:44:41 PST
It would be great if it was possible to edit and delete cookies in the Storage section of the web inspector.
Comment 1 Timothy Hatcher 2009-11-05 02:05:36 PST
You can delete. Edit will be harder.
Comment 2 Jonas Due Vesterheden 2009-11-05 02:11:06 PST
Sorry, I managed to overlook the 'X'-button!

Would it be possible to bind backspace to "Delete cookie"?
Comment 3 Brian Weinstein 2009-11-05 09:53:30 PST
(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.
Comment 4 Brian Weinstein 2009-12-09 13:23:18 PST
Delete is now bound to delete cookie as of http://trac.webkit.org/changeset/50613.
Comment 5 timmywil 2011-03-02 13:18:38 PST
It would be awesome to be able to delete more than one cookie at a time on a site.
Comment 6 Radar WebKit Bug Importer 2014-12-17 11:22:55 PST
<rdar://problem/19281523>
Comment 7 Brian Burg 2015-11-18 10:39:53 PST
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.
Comment 8 Bem Jones-Bey 2019-02-12 11:39:21 PST
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.
Comment 9 Devin Rousso 2020-03-21 19:54:05 PDT
Created attachment 394190 [details]
Patch
Comment 10 Devin Rousso 2020-03-21 19:54:26 PDT
Created attachment 394191 [details]
[Image] After Patch is applied
Comment 11 EWS Watchlist 2020-03-21 19:55:03 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 12 EWS 2020-03-28 20:07:58 PDT
Committed r259173: <https://trac.webkit.org/changeset/259173>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394190 [details].
Comment 13 Joseph Pecoraro 2020-03-28 21:16:20 PDT
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?
Comment 14 Devin Rousso 2020-03-28 21:49:29 PDT
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`.
Comment 15 David Kilzer (:ddkilzer) 2020-03-29 02:16:12 PDT
(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
Comment 16 David Kilzer (:ddkilzer) 2020-03-29 02:59:24 PDT
(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>