182328 2018-01-31 03:13:09 -0800 WebDriver: addCookie command should prepend a dot to domain if missing 2019-04-24 18:58:01 -0700 1 1 1 Unclassified WebKit WebDriver WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=182427 InRadar, Soup P2 Normal --- 182423 182508 184334 1 cgarcia webkit-unassigned bburg berto bugs-noreply clopez commit-queue ews-watchlist gustavo jlewis3 mcatanzaro webkit-bug-importer oldest_to_newest 1394959 0 cgarcia 2018-01-31 03:13:09 -0800 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 1394961 1 332753 cgarcia 2018-01-31 03:15:21 -0800 Created attachment 332753 Patch 1395146 2 332753 mcatanzaro 2018-01-31 14:00:39 -0800 Comment on attachment 332753 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. 1395369 3 cgarcia 2018-01-31 23:51:11 -0800 (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. 1395460 4 cgarcia 2018-02-01 07:53:55 -0800 Committed r227964: <https://trac.webkit.org/changeset/227964> 1395461 5 webkit-bug-importer 2018-02-01 07:54:25 -0800 <rdar://problem/37116398> 1395778 6 cgarcia 2018-02-02 01:48:17 -0800 (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. 1395780 7 commit-queue 2018-02-02 01:50:19 -0800 Re-opened since this is blocked by bug 182423 1395794 8 cgarcia 2018-02-02 02:55:40 -0800 Moving to WebDriver 1395795 9 332951 cgarcia 2018-02-02 02:59:37 -0800 Created attachment 332951 Patch 1395873 10 332951 mcatanzaro 2018-02-02 08:48:53 -0800 Comment on attachment 332951 Patch This looks safer, at least ;) 1396239 11 cgarcia 2018-02-05 00:46:55 -0800 Committed r228087: <https://trac.webkit.org/changeset/228087> 1396523 12 commit-queue 2018-02-05 14:00:09 -0800 Re-opened since this is blocked by bug 182508 1397468 13 cgarcia 2018-02-07 23:49:32 -0800 Committed r228262: <https://trac.webkit.org/changeset/228262> 1397606 14 jlewis3 2018-02-08 11:31:05 -0800 Reverted r228262 for reason: This broke an internal build alongside r228261. Committed r228283: <https://trac.webkit.org/changeset/228283> 1398323 15 cgarcia 2018-02-11 23:33:58 -0800 Committed r228371: <https://trac.webkit.org/changeset/228371> 332753 2018-01-31 03:15:21 -0800 2018-02-02 02:59:37 -0800 Patch wd-soup-cookie.diff text/plain 2220 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAzZjNkNmVmZDQ3YS4uZGUxMDI1YTJhNzMgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMCBAQAorMjAxOC0wMS0zMSAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGln YWxpYS5jb20+CisKKyAgICAgICAgW1NPVVBdIEVuc3VyZSBkb21haW4gaXMgdmFsaWQgd2hlbiBj b252ZXJ0aW5nIGEgV2ViQ29yZSBDb29raWUgdG8gU291cAorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgyMzI4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgc291cF9jb29raWVfcGFyc2UoKSBhZGRzIHRoZSBp bml0aWFsICcuJyB0byB0aGUgZG9tYWluIGlmIG1pc3NpbmcgYmVmb3JlIGNyZWF0aW5nIHRoZSBT b3VwQ29va2llLCBidXQKKyAgICAgICAgc291cF9jb29raWVfbmV3KCkgYWxsb3dzIGZvciBkb21h aW4gdG8gYmUgYSBob3N0bmFtZSB0aGF0IG5lZWRzIHRvIG1hdGNoIGV4YWN0bHkuIFdoZW4gY29u dmVydGluZyBhIFdlYkNvcmUKKyAgICAgICAgQ29va2llIGludG8gYSBTb3VwQ29va2llIHdlIGFs d2F5cyB3YW50IHRoZSBkb21haW4gdG8gYmUgY29uc2lkZXJlZCBhcyBzdWNoIGFuZCBub3QgYXMg YSBob3N0bmFtZSwgc28gd2UgbmVlZCB0bworICAgICAgICBwcmVwZW5kIHRoZSAnLicgaWYgbWlz c2luZy4KKworICAgICAgICBGaXhlczogaW1wb3J0ZWQvdzNjL3dlYmRyaXZlci90ZXN0cy9jb29r aWVzL2FkZF9jb29raWUucHk6OnRlc3RfYWRkX2RvbWFpbl9jb29raWUKKworICAgICAgICAqIHBs YXRmb3JtL25ldHdvcmsvc291cC9Db29raWVTb3VwLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkNv b2tpZTo6dG9Tb3VwQ29va2llIGNvbnN0KToKKwogMjAxOC0wMS0zMCAgRnVqaWkgSGlyb25vcmkg IDxIaXJvbm9yaS5GdWppaUBzb255LmNvbT4KIAogICAgICAgICBbV2luQ2Fpcm9dIHN5bnRoZXRp Y0JvbGRPZmZzZXQgbWFrZXMgYSBmb250IHdpdGggZW1iZWRkZWQgYml0bWFwIGZvbnRzIHNob3du IGFzIGRvdWJsZSBzdHJpa2UgaW4gSGlEUEkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL25ldHdvcmsvc291cC9Db29raWVTb3VwLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL25ldHdvcmsvc291cC9Db29raWVTb3VwLmNwcAppbmRleCBlZWMxNjliYWMxZS4uNGNjNTU2 NTBjMTAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9D b29raWVTb3VwLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAv Q29va2llU291cC5jcHAKQEAgLTYwLDggKzYwLDEzIEBAIFNvdXBDb29raWUqIENvb2tpZTo6dG9T b3VwQ29va2llKCkgY29uc3QKICAgICBpZiAobmFtZS5pc051bGwoKSB8fCB2YWx1ZS5pc051bGwo KSB8fCBkb21haW4uaXNOdWxsKCkgfHwgcGF0aC5pc051bGwoKSkKICAgICAgICAgcmV0dXJuIG51 bGxwdHI7CiAKKyAgICAvLyBzb3VwX2Nvb2tpZV9uZXcoKSB3aWxsIGhhbmRsZSB0aGUgZ2l2ZW4g ZG9tYWluIGFzIGEgaG9zdG5hbWUgaWYgaXQgZG9lc24ndCBzdGFydCB3aXRoICcuJy4KKyAgICBh dXRvIGNvb2tpZURvbWFpbiA9IGRvbWFpbi51dGY4KCk7CisgICAgaWYgKGNvb2tpZURvbWFpbi5s ZW5ndGgoKSAmJiAhZ19ob3N0bmFtZV9pc19pcF9hZGRyZXNzKGNvb2tpZURvbWFpbi5kYXRhKCkp ICYmIGNvb2tpZURvbWFpbi5kYXRhKClbMF0gIT0gJy4nKQorICAgICAgICBjb29raWVEb21haW4g PSBtYWtlU3RyaW5nKCcuJywgZG9tYWluKS51dGY4KCk7CisKICAgICBTb3VwQ29va2llKiBzb3Vw Q29va2llID0gc291cF9jb29raWVfbmV3KG5hbWUudXRmOCgpLmRhdGEoKSwgdmFsdWUudXRmOCgp LmRhdGEoKSwKLSAgICAgICAgZG9tYWluLnV0ZjgoKS5kYXRhKCksIHBhdGgudXRmOCgpLmRhdGEo KSwgLTEpOworICAgICAgICBjb29raWVEb21haW4uZGF0YSgpLCBwYXRoLnV0ZjgoKS5kYXRhKCks IC0xKTsKIAogICAgIHNvdXBfY29va2llX3NldF9odHRwX29ubHkoc291cENvb2tpZSwgaHR0cE9u bHkpOwogICAgIHNvdXBfY29va2llX3NldF9zZWN1cmUoc291cENvb2tpZSwgc2VjdXJlKTsK 332951 2018-02-02 02:59:37 -0800 2018-02-02 08:48:53 -0800 Patch wd-add-cookie-domain.diff text/plain 2091 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggMjEyZWEyZDBhNDAuLjUyYjljYjU2NDA3IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZwpAQCAtMSwzICsx LDE4IEBACisyMDE4LTAyLTAyICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlh LmNvbT4KKworICAgICAgICBXZWJEcml2ZXI6IGFkZENvb2tpZSBjb21tYW5kIHNob3VsZCBwcmVw ZW5kIGEgZG90IHRvIGRvbWFpbiBpZiBtaXNzaW5nCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODIzMjgKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzM3 MTE2Mzk4PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IFJGQyAyOTY1OiBJZiBhbiBleHBsaWNpdGx5IHNwZWNpZmllZCB2YWx1ZSBkb2VzIG5vdCBzdGFy dCB3aXRoIGEgZG90LCB0aGUgdXNlciBhZ2VudCBzdXBwbGllcyBhIGxlYWRpbmcgZG90LgorCisg ICAgICAgIEZpeGVzOiBpbXBvcnRlZC93M2Mvd2ViZHJpdmVyL3Rlc3RzL2Nvb2tpZXMvYWRkX2Nv b2tpZS5weTo6dGVzdF9hZGRfZG9tYWluX2Nvb2tpZQorCisgICAgICAgICogVUlQcm9jZXNzL0F1 dG9tYXRpb24vV2ViQXV0b21hdGlvblNlc3Npb24uY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJB dXRvbWF0aW9uU2Vzc2lvbjo6YWRkU2luZ2xlQ29va2llKToKKwogMjAxOC0wMi0wMSAgVGltIEhv cnRvbiAgPHRpbW90aHlfaG9ydG9uQGFwcGxlLmNvbT4KIAogICAgICAgICBXZWJLaXQgZmFpbHMg dG8gYnVpbGQgKF9zdGFydEFzc2lzdGluZ05vZGUgaGFzIGNvbmZsaWN0aW5nIHBhcmFtZXRlcnMp CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1VJUHJvY2Vzcy9BdXRvbWF0aW9uL1dlYkF1dG9t YXRpb25TZXNzaW9uLmNwcCBiL1NvdXJjZS9XZWJLaXQvVUlQcm9jZXNzL0F1dG9tYXRpb24vV2Vi QXV0b21hdGlvblNlc3Npb24uY3BwCmluZGV4IGVmMDFkMzAxNjk5Li40ODE1MGIxOWIzYSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQXV0b21hdGlvbi9XZWJBdXRvbWF0aW9u U2Vzc2lvbi5jcHAKKysrIGIvU291cmNlL1dlYktpdC9VSVByb2Nlc3MvQXV0b21hdGlvbi9XZWJB dXRvbWF0aW9uU2Vzc2lvbi5jcHAKQEAgLTEyMDcsNyArMTIwNywxNCBAQCB2b2lkIFdlYkF1dG9t YXRpb25TZXNzaW9uOjphZGRTaW5nbGVDb29raWUoRXJyb3JTdHJpbmcmIGVycm9yU3RyaW5nLCBj b25zdCBTdHJpbgogICAgIC8vIEluaGVyaXQgdGhlIGRvbWFpbi9ob3N0IGZyb20gdGhlIG1haW4g ZnJhbWUncyBVUkwgaWYgaXQgaXMgbm90IGV4cGxpY2l0bHkgc2V0LgogICAgIGlmIChkb21haW4u aXNFbXB0eSgpKQogICAgICAgICBkb21haW4gPSBhY3RpdmVVUkwuaG9zdCgpOwotCisgICAgZWxz ZSBpZiAoZG9tYWluWzBdICE9ICcuJykgeworICAgICAgICAvLyBSRkMgMjk2NTogSWYgYW4gZXhw bGljaXRseSBzcGVjaWZpZWQgdmFsdWUgZG9lcyBub3Qgc3RhcnQgd2l0aCBhIGRvdCwgdGhlIHVz ZXIgYWdlbnQgc3VwcGxpZXMgYSBsZWFkaW5nIGRvdC4KKyAgICAgICAgLy8gQXNzdW1lIHRoYXQg YW55IGhvc3QgdGhhdCBlbmRzIHdpdGggYSBkaWdpdCBpcyB0cnlpbmcgdG8gYmUgYW4gSVAgYWRk cmVzcy4KKyAgICAgICAgLy8gRklYTUU6IHdlIGFyZSB1c2luZyB0aGlzIHRvIGRldGVjdCBJUCBh ZGRyZXNzZXMgaW4gc2V2ZXJhbCBwbGFjZXMsIHdlIHNob3VsZCBhZGQgYSB3YXkgdG8gcHJvcGVy bHkgZG8gdGhpcworICAgICAgICAvLyB0byBXZWJDb3JlIHBsYXRmb3JtIG9yIFBBTC4KKyAgICAg ICAgaWYgKCFpc0FTQ0lJRGlnaXQoZG9tYWluW2RvbWFpbi5sZW5ndGgoKSAtIDFdKSkKKyAgICAg ICAgICAgIGRvbWFpbiA9IG1ha2VTdHJpbmcoJy4nLCBkb21haW4pOworICAgIH0KICAgICBjb29r aWUuZG9tYWluID0gZG9tYWluOwogCiAgICAgaWYgKCFjb29raWVPYmplY3QuZ2V0U3RyaW5nKFdU Rjo6QVNDSUlMaXRlcmFsKCJwYXRoIiksIGNvb2tpZS5wYXRoKSkK