soup_cookie_parse() adds the initial '.' to the domain if missing before creating the SoupCookie, but soup_cookie_new() allows for domain to be a hostname that needs to match exactly. When converting a WebCore Cookie into a SoupCookie we always want the domain to be considered as such and not as a hostname, so we need to prepend the '.' if missing. This is causing a WebDriver test to fail: ___________________________________________________________________________________ test_add_domain_cookie ___________________________________________________________________________________ session = <webdriver.client.Session object at 0x7fbf6c169dd0>, url = <function url at 0x7fbf6c183140> server_config = {'domains': {'': 'localhost'}, 'host': 'localhost', 'ports': {'http': ['8802']}} def test_add_domain_cookie(session, url, server_config): session.url = url("/common/blank.html") clear_all_cookies(session) create_cookie_request = { "cookie": { "name": "hello", "value": "world", "domain": server_config["domains"][""], "path": "/", "httpOnly": False, "secure": False } } result = session.transport.send("POST", "session/%s/cookie" % session.session_id, create_cookie_request) assert result.status == 200 assert "value" in result.body assert result.body["value"] is None result = session.transport.send("GET", "session/%s/cookie" % session.session_id) assert result.status == 200 assert "value" in result.body assert isinstance(result.body["value"], list) assert len(result.body["value"]) == 1 assert isinstance(result.body["value"][0], dict) cookie = result.body["value"][0] assert "name" in cookie assert isinstance(cookie["name"], basestring) assert "value" in cookie assert isinstance(cookie["value"], basestring) assert "domain" in cookie assert isinstance(cookie["domain"], basestring) assert cookie["name"] == "hello" assert cookie["value"] == "world" > assert cookie["domain"] == ".%s" % server_config["domains"][""] E AssertionError: assert 'localhost' == '.localhost' E - localhost E + .localhost E ? + cookie = {'domain': 'localhost', 'httpOnly': False, 'name': 'hello', 'path': '/', ...} create_cookie_request = {'cookie': {'domain': 'localhost', 'httpOnly': False, 'name': 'hello', 'path': '/', ...}} result = <Responsetatus=200 body={"value": [{"domain": "localhost", "name": "hello", "value": "world", "path": "/", "httpOnly": false, "secure": false}]}> server_config = {'domains': {'': 'localhost'}, 'host': 'localhost', 'ports': {'http': ['8802']}} session = <webdriver.client.Session object at 0x7fbf6c169dd0> url = <function url at 0x7fbf6c183140> WebDriverTests/imported/w3c/webdriver/tests/cookies/add_cookie.py:39: AssertionError
Created attachment 332753 [details] Patch
Comment on attachment 332753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332753&action=review > Source/WebCore/platform/network/soup/CookieSoup.cpp:66 > + // soup_cookie_new() will handle the given domain as a hostname if it doesn't start with '.'. > + auto cookieDomain = domain.utf8(); > + if (cookieDomain.length() && !g_hostname_is_ip_address(cookieDomain.data()) && cookieDomain.data()[0] != '.') > + cookieDomain = makeString('.', domain).utf8(); I don't know. RFC 2965 says: Domain=value OPTIONAL. The value of the Domain attribute specifies the domain for which the cookie is valid. If an explicitly specified value does not start with a dot, the user agent supplies a leading dot The dot is actually significant and probably shouldn't change when converting from a WebCore::Cookie to SoupCookie. I'm worried that the dot should have already been prepended somewhere else.
(In reply to Michael Catanzaro from comment #2) > Comment on attachment 332753 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332753&action=review > > > Source/WebCore/platform/network/soup/CookieSoup.cpp:66 > > + // soup_cookie_new() will handle the given domain as a hostname if it doesn't start with '.'. > > + auto cookieDomain = domain.utf8(); > > + if (cookieDomain.length() && !g_hostname_is_ip_address(cookieDomain.data()) && cookieDomain.data()[0] != '.') > > + cookieDomain = makeString('.', domain).utf8(); > > I don't know. RFC 2965 says: > > Domain=value > OPTIONAL. The value of the Domain attribute specifies the domain > for which the cookie is valid. If an explicitly specified value > does not start with a dot, the user agent supplies a leading dot > > The dot is actually significant and probably shouldn't change when > converting from a WebCore::Cookie to SoupCookie. I'm worried that the dot > should have already been prepended somewhere else. The dot is indeed prepended by libsoup when parsing a cookie, that is, when a cookie is set by DOM. When we convert a WebCore Cookie to Soup is usually because it's not a cookie added by DOM but by some API (either the user API or WebDriver ins this case). What we are doing here is the same soup does when a cookie is added by DOM, prepending the dot if it's not there already.
Committed r227964: <https://trac.webkit.org/changeset/227964>
<rdar://problem/37116398>
(In reply to Michael Catanzaro from comment #2) > Comment on attachment 332753 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332753&action=review > > > Source/WebCore/platform/network/soup/CookieSoup.cpp:66 > > + // soup_cookie_new() will handle the given domain as a hostname if it doesn't start with '.'. > > + auto cookieDomain = domain.utf8(); > > + if (cookieDomain.length() && !g_hostname_is_ip_address(cookieDomain.data()) && cookieDomain.data()[0] != '.') > > + cookieDomain = makeString('.', domain).utf8(); > > I don't know. RFC 2965 says: > > Domain=value > OPTIONAL. The value of the Domain attribute specifies the domain > for which the cookie is valid. If an explicitly specified value > does not start with a dot, the user agent supplies a leading dot > > The dot is actually significant and probably shouldn't change when > converting from a WebCore::Cookie to SoupCookie. I'm worried that the dot > should have already been prepended somewhere else. You were indeed right, I missed an important point: "If an *explicitly* specified value". The spec also says: "Defaults to the effective request-host. (Note that because there is no dot at the beginning of effective request-host, the default Domain can only domain-match itself.)" So, it can indeed be a hostname, when omitted in Set-Cookie header. In the case of user provided cookie, we should probably keep what the user is providing. So, yes this is not the right place to do this, because we don't know if that cookie came from the API or not. In the case of WebDriver, I'll file an issue in the spec to clarify if we should add the dot or not. I'll revert this.
Re-opened since this is blocked by bug 182423
Moving to WebDriver
Created attachment 332951 [details] Patch
Comment on attachment 332951 [details] Patch This looks safer, at least ;)
Committed r228087: <https://trac.webkit.org/changeset/228087>
Re-opened since this is blocked by bug 182508
Committed r228262: <https://trac.webkit.org/changeset/228262>
Reverted r228262 for reason: This broke an internal build alongside r228261. Committed r228283: <https://trac.webkit.org/changeset/228283>
Committed r228371: <https://trac.webkit.org/changeset/228371>