Bug 169356 - [SOUP] Implement strict secure cookies specification
Summary: [SOUP] Implement strict secure cookies specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Patrick Griffis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-08 07:13 PST by Michael Catanzaro
Modified: 2019-12-12 09:51 PST (History)
14 users (show)

See Also:


Attachments
Patch (2.56 KB, patch)
2019-06-01 16:07 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.85 MB, application/zip)
2019-06-01 17:53 PDT, Build Bot
no flags Details
Patch (7.87 KB, patch)
2019-12-11 19:24 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-03-08 07:13:08 PST
Insecure webpages should be prevented from setting secure cookies. See:

https://bugzilla.mozilla.org/show_bug.cgi?id=976073
Comment 1 Patrick Griffis 2018-11-08 11:48:57 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.
Comment 2 Patrick Griffis 2018-11-08 11:51:59 PST
Hmm, I guess that snippet is only accessing cookies not setting them.
Comment 3 Michael Catanzaro 2018-11-08 12:57:43 PST
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....
Comment 4 Patrick Griffis 2019-02-28 13:36:56 PST
Initial work in libsoup: https://gitlab.gnome.org/GNOME/libsoup/merge_requests/47
Comment 5 Patrick Griffis 2019-06-01 16:07:09 PDT
Created attachment 371127 [details]
Patch
Comment 6 Michael Catanzaro 2019-06-01 16:22:14 PDT
Tests?
Comment 7 Build Bot 2019-06-01 17:53:26 PDT Comment hidden (spam)
Comment 8 Build Bot 2019-06-01 17:53:28 PDT Comment hidden (spam)
Comment 9 Patrick Griffis 2019-11-13 03:13:16 PST
This is covered by tests from WPT, considering how simple this is would that be acceptable or should I import them over?
Comment 10 Michael Catanzaro 2019-11-13 06:55:41 PST
They should be imported under LayoutTests/imported/w3c/web-platform-tests.
Comment 11 Carlos Alberto Lopez Perez 2019-11-13 07:11:45 PST
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
Comment 12 Carlos Alberto Lopez Perez 2019-11-13 07:17:03 PST
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
Comment 13 Patrick Griffis 2019-12-11 19:24:37 PST
Created attachment 385472 [details]
Patch
Comment 14 Michael Catanzaro 2019-12-12 09:07:12 PST
Comment on attachment 385472 [details]
Patch

Nice.
Comment 15 WebKit Commit Bot 2019-12-12 09:51:36 PST
Comment on attachment 385472 [details]
Patch

Clearing flags on attachment: 385472

Committed r253431: <https://trac.webkit.org/changeset/253431>
Comment 16 WebKit Commit Bot 2019-12-12 09:51:38 PST
All reviewed patches have been landed.  Closing bug.