Summary: | [SOUP] Implement strict secure cookies specification | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||
Component: | WebKitGTK | Assignee: | Patrick Griffis <pgriffis> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | agomez, aperez, berto, bugs-noreply, cgarcia, clopez, commit-queue, ews-watchlist, gustavo, ltilve, mcatanzaro, pgriffis, psaavedra, rniwa | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=184955 | ||||||||||
Attachments: |
|
Description
Michael Catanzaro
2017-03-08 07:13:08 PST
This appears to already be implemented if I'm reading it right: https://github.com/WebKit/webkit/commit/ca55797f9037f4655f6f673b3ebb91b904899a5c#diff-0260b688ce1742d2664124764a517056R503 I don't know if there are any tests we have for this to ensure they run with Soup. Hmm, I guess that snippet is only accessing cookies not setting them. Yeah. Basically this eature boils down to an if statement somewhere to make sure it's impossible for an http:// connection to set secure cookies. Just block the cookie if so. Taking a quick look at the code, I guess that needs handled inside NetworkStorageSession::setCookies and NetworkStorageSession::setCookiesFromDOM, then audit all calls to NetworkStorageSession::setCookie.... Then there's a change to ensure existing secure cookies can't be replaced with insecure cookies, and a bit about expiration policy: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone-01. All that needs to be tested. Writing tests will probably be harder than implementing the change itself. I think they need to go under LayoutTests/http/tests/ssl as those are the only tests that run under an HTTPS server. You could add a directory LayoutTests/http/tests/ssl/cookies. Alternatively, instead of writing your own, it might be better to try importing the relevant WPT tests like https://github.com/web-platform-tests/wpt/blob/master/cookies/secure/set-from-http.sub.html and its friends, although I've never done this before and I'm not certain whether all these tests are actually compatible with our infrastructure.... Initial work in libsoup: https://gitlab.gnome.org/GNOME/libsoup/merge_requests/47 Created attachment 371127 [details]
Patch
Tests? Comment on attachment 371127 [details] Patch Attachment 371127 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12352027 New failing tests: http/wpt/service-workers/service-worker-networkprocess-crash.html Created attachment 371135 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
This is covered by tests from WPT, considering how simple this is would that be acceptable or should I import them over? They should be imported under LayoutTests/imported/w3c/web-platform-tests. BTW, don't import the test by manually copying them. Instead use the importer script that does the required rewrites of resources. Something like this should work: $ Tools/Scripts/import-w3c-tests -t web-platform-tests/cookies/samesite See also: https://trac.webkit.org/wiki/WebKitW3CTesting I also find this easier for everybody doing the process of importing tests and implementing a feature in two steps (two different bugs): 1. First import the WPT tests in another bug and mark the new imported tests as failing on the TestExpectation. 2. Then land the bug with the implementation and remove the failures from the TestExpectation file that now pass and/or adjusting test expectation results. See for example r251163 and r251182 Created attachment 385472 [details]
Patch
Comment on attachment 385472 [details]
Patch
Nice.
Comment on attachment 385472 [details] Patch Clearing flags on attachment: 385472 Committed r253431: <https://trac.webkit.org/changeset/253431> All reviewed patches have been landed. Closing bug. |