Bug 182328 - WebDriver: addCookie command should prepend a dot to domain if missing
Summary: WebDriver: addCookie command should prepend a dot to domain if missing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar, Soup
Depends on: 182423 182508
Blocks: 184334
  Show dependency treegraph
 
Reported: 2018-01-31 03:13 PST by Carlos Garcia Campos
Modified: 2019-04-24 18:58 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.17 KB, patch)
2018-01-31 03:15 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch (2.04 KB, patch)
2018-02-02 02:59 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-01-31 03:13:09 PST
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
Comment 1 Carlos Garcia Campos 2018-01-31 03:15:21 PST
Created attachment 332753 [details]
Patch
Comment 2 Michael Catanzaro 2018-01-31 14:00:39 PST
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.
Comment 3 Carlos Garcia Campos 2018-01-31 23:51:11 PST
(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.
Comment 4 Carlos Garcia Campos 2018-02-01 07:53:55 PST
Committed r227964: <https://trac.webkit.org/changeset/227964>
Comment 5 Radar WebKit Bug Importer 2018-02-01 07:54:25 PST
<rdar://problem/37116398>
Comment 6 Carlos Garcia Campos 2018-02-02 01:48:17 PST
(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.
Comment 7 WebKit Commit Bot 2018-02-02 01:50:19 PST
Re-opened since this is blocked by bug 182423
Comment 8 Carlos Garcia Campos 2018-02-02 02:55:40 PST
Moving to WebDriver
Comment 9 Carlos Garcia Campos 2018-02-02 02:59:37 PST
Created attachment 332951 [details]
Patch
Comment 10 Michael Catanzaro 2018-02-02 08:48:53 PST
Comment on attachment 332951 [details]
Patch

This looks safer, at least ;)
Comment 11 Carlos Garcia Campos 2018-02-05 00:46:55 PST
Committed r228087: <https://trac.webkit.org/changeset/228087>
Comment 12 WebKit Commit Bot 2018-02-05 14:00:09 PST
Re-opened since this is blocked by bug 182508
Comment 13 Carlos Garcia Campos 2018-02-07 23:49:32 PST
Committed r228262: <https://trac.webkit.org/changeset/228262>
Comment 14 Matt Lewis 2018-02-08 11:31:05 PST
Reverted r228262 for reason:

This broke an internal build alongside r228261.

Committed r228283: <https://trac.webkit.org/changeset/228283>
Comment 15 Carlos Garcia Campos 2018-02-11 23:33:58 PST
Committed r228371: <https://trac.webkit.org/changeset/228371>