Bug 209926 - Prevent non app-bound domain cookies from being read or set using API calls
Summary: Prevent non app-bound domain cookies from being read or set using API calls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-02 13:45 PDT by katherine_cheney
Modified: 2020-04-03 16:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (26.75 KB, patch)
2020-04-02 15:46 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (26.93 KB, patch)
2020-04-02 16:20 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch for landing (26.95 KB, patch)
2020-04-03 16:17 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description katherine_cheney 2020-04-02 13:45:56 PDT
We should adjust WebKit API calls to only be able to set and access app-bound domain cookies.
Comment 1 katherine_cheney 2020-04-02 13:46:29 PDT
<rdar://problem/61071428>
Comment 2 katherine_cheney 2020-04-02 15:46:22 PDT
Created attachment 395312 [details]
Patch
Comment 3 katherine_cheney 2020-04-02 16:20:15 PDT
Created attachment 395318 [details]
Patch
Comment 4 Brady Eidson 2020-04-03 14:25:09 PDT
Comment on attachment 395318 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:42
> +#define HTTP_COOKIE_STORE_ADDITIONS_1 false
> +#define HTTP_COOKIE_STORE_ADDITIONS_2

Working with Kate out-of-band on a better name for these

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:65
> +void HTTPCookieStore::filterAppBoundCookies(const Vector<WebCore::Cookie>& cookies, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&& completionHandler)

I'd change this to:

Vector<WebCore::Cookie> HTTPCookieStore::filterAppBoundCookies(const Vector<WebCore::Cookie>& cookies) {...}

Have it return the filtered vector.

And have the caller call the completion handler.

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:96
> +            filterAppBoundCookies(allCookies, WTFMove(completionHandler));

"Filter" is an operation completely distinct from calling a completion handler.

Make my above change, and this becomes:
completionHandler(filterAppBoundCookies(allCookies));

Reads nicer.

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:103
> +        filterAppBoundCookies(cookies, WTFMove(completionHandler));

Ditto.

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:115
> +        filterAppBoundCookies(cookies, WTFMove(completionHandler));

Ditto.

> Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:121
> +    filterAppBoundCookies(cookies, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (auto&& appBoundCookies) mutable {

Can get rid of the lambda here and just continue executing code on the resulting vector.
Comment 5 Brady Eidson 2020-04-03 16:05:52 PDT
(In reply to Brady Eidson from comment #4)
> Comment on attachment 395318 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395318&action=review
> 
> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:42
> > +#define HTTP_COOKIE_STORE_ADDITIONS_1 false
> > +#define HTTP_COOKIE_STORE_ADDITIONS_2
> 
> Working with Kate out-of-band on a better name for these
> 
> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:65
> > +void HTTPCookieStore::filterAppBoundCookies(const Vector<WebCore::Cookie>& cookies, CompletionHandler<void(Vector<WebCore::Cookie>&&)>&& completionHandler)
> 
> I'd change this to:
> 
> Vector<WebCore::Cookie> HTTPCookieStore::filterAppBoundCookies(const
> Vector<WebCore::Cookie>& cookies) {...}
> 
> Have it return the filtered vector.
> 
> And have the caller call the completion handler.

I did not note this at the time - THere's an inner async lambda in here, necessitating passing in the outer lambda.  NM!


> 
> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:96
> > +            filterAppBoundCookies(allCookies, WTFMove(completionHandler));
> 
> "Filter" is an operation completely distinct from calling a completion
> handler.
> 
> Make my above change, and this becomes:
> completionHandler(filterAppBoundCookies(allCookies));
> 
> Reads nicer.
> 
> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:103
> > +        filterAppBoundCookies(cookies, WTFMove(completionHandler));
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:115
> > +        filterAppBoundCookies(cookies, WTFMove(completionHandler));
> 
> Ditto.
> 
> > Source/WebKit/UIProcess/API/APIHTTPCookieStore.cpp:121
> > +    filterAppBoundCookies(cookies, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (auto&& appBoundCookies) mutable {
> 
> Can get rid of the lambda here and just continue executing code on the
> resulting vector.
Comment 6 Brady Eidson 2020-04-03 16:06:20 PDT
Comment on attachment 395318 [details]
Patch

Change the names as we discussed then this is good
Comment 7 katherine_cheney 2020-04-03 16:17:02 PDT
Created attachment 395415 [details]
Patch for landing
Comment 8 katherine_cheney 2020-04-03 16:17:12 PDT
(In reply to Brady Eidson from comment #6)
> Comment on attachment 395318 [details]
> Patch
> 
> Change the names as we discussed then this is good

Thank you!
Comment 9 EWS 2020-04-03 16:39:11 PDT
Committed r259520: <https://trac.webkit.org/changeset/259520>

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