Bug 209926

Summary:

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

Product:

WebKit

Reporter:

Kate Cheney <katherine_cheney>

Component:

WebKit Misc.

Assignee:

Kate Cheney <katherine_cheney>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

beidson, bfulgham, cdumez, thorton, wilander

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none

Kate Cheney

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

Attachments
Patch (26.75 KB, patch) 2020-04-02 15:46 PDT, Kate Cheney	no flags	Details Formatted Diff Diff
Patch (26.93 KB, patch) 2020-04-02 16:20 PDT, Kate Cheney	no flags	Details Formatted Diff Diff
Patch for landing (26.95 KB, patch) 2020-04-03 16:17 PDT, Kate Cheney	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Kate Cheney

Comment 1 2020-04-02 13:46:29 PDT

<rdar://problem/61071428>

Kate Cheney

Comment 2 2020-04-02 15:46:22 PDT

Created attachment 395312 [details] Patch

Kate Cheney

Comment 3 2020-04-02 16:20:15 PDT

Created attachment 395318 [details] Patch

Brady Eidson

Comment 4 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.

Brady Eidson

Comment 5 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.

Brady Eidson

Comment 6 2020-04-03 16:06:20 PDT

Comment on attachment 395318 [details] Patch Change the names as we discussed then this is good

Kate Cheney

Comment 7 2020-04-03 16:17:02 PDT

Created attachment 395415 [details] Patch for landing

Kate Cheney

Comment 8 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!

EWS

Comment 9 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].

Note You need to log in before you can comment on or make changes to this bug.