Bug 209926 – Prevent non app-bound domain cookies from being read or set using API calls

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

We should adjust WebKit API calls to only be able to set and access app-bound domain cookies.

[Kate Cheney]

<rdar://problem/61071428>

[Kate Cheney]

Created attachment 395312 [details]
Patch

[Kate Cheney]

Created attachment 395318 [details]
Patch

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Comment on attachment 395318 [details]
Patch

Change the names as we discussed then this is good

[Kate Cheney]

Created attachment 395415 [details]
Patch for landing

[Kate Cheney]

(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!

[EWS]

Committed r259520: <https://trac.webkit.org/changeset/259520>

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