Bug 182328 – WebDriver: addCookie command should prepend a dot to domain if missing

• Component: WebDriver
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 332753 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r227964: <https://trac.webkit.org/changeset/227964>

[Radar WebKit Bug Importer]

<rdar://problem/37116398>

[Carlos Garcia Campos]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 182423

[Carlos Garcia Campos]

Moving to WebDriver

[Carlos Garcia Campos]

Created attachment 332951 [details]
Patch

[Michael Catanzaro]

Comment on attachment 332951 [details]
Patch

This looks safer, at least ;)

[Carlos Garcia Campos]

Committed r228087: <https://trac.webkit.org/changeset/228087>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 182508

[Carlos Garcia Campos]

Committed r228262: <https://trac.webkit.org/changeset/228262>

[Matt Lewis]

Reverted r228262 for reason:

This broke an internal build alongside r228261.

Committed r228283: <https://trac.webkit.org/changeset/228283>

[Carlos Garcia Campos]

Committed r228371: <https://trac.webkit.org/changeset/228371>