1415828 - Marionette fails to add new cookie if cookie domain has a preceding dot character

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Peter Major]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

With a selenium client try to add a new cookie and make sure that the cookie domain has a preceding dot character. The simplest way to test this is to change testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py to have a cookie domain of .web-platform.test


Actual results:

The cookie wasn't created and an InvalidCookieDomainError was returned


Expected results:

The cookie should have been created as a domain cookie.

Comment 1

[Peter Major]

FYI I'm happy to pick this up in the next few days.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Great to hear that, Peter. I'm already assigning the bug to you then. Thanks.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Whereby if that is not clear by the spec yet (https://github.com/w3c/webdriver/issues/1143), then we might want to wait with this implementation.

Comment 4

[Peter Major]

Andreas, is the assumption correct that the cookie domain with a preceding dot character should work with Firefox or do you think that further clarification from w3c is needed before we can progress this issue?

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Peter Major from comment #4)

> Andreas, is the assumption correct that the cookie domain with
> a preceding dot character should work with Firefox or do you
> think that further clarification from w3c is needed before we can
> progress this issue?

I’m not sure what you’re really asking here.  Should a domain
prefixed by a dot be possible to add as a cookie to Firefox’
cookie service?  Or should Marionette/WebDriver deem that acceptable
input?

Comment 6

[Peter Major]

Firefox's cookie service appears to work this way:
* if the domain value is prefixed with a dot, the cookie is created as a domain cookie
* if the domain value does not start with a dot, the cookie is created as a host only cookie

So I think the question really is whether marionette/webdriver should accept domain with leading dot and handle it as a domain cookie?

Comment 7

[Andreas Tolfsen ❲:ato❳]

Sorry for my late reply here.  I have been on holiday.

(In reply to Peter Major from comment #6)

> So I think the question really is whether marionette/webdriver
should accept > domain with leading dot and handle it as a domain cookie?

Judging my Simon’s reply to
https://github.com/w3c/webdriver/issues/1143, I think the answer
to this is yes: we should treat a domain string with preceding dots
as a domain cookie.

It does also seem like the specification could use some tidying up
make this clearer.

Comment 8

[Peter Major]

Attachment: Bug 1415828 - Accept cookie domains with leading dot character

Domain cookies retrieved from Marionette have leading dot characters in
them. This change will ensure that adding cookies with leading dot
characters works as expected.

Review commit: https://reviewboard.mozilla.org/r/202526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202526/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

https://reviewboard.mozilla.org/r/202526/#review207902

::: testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py:153
(Diff revision 1)
> +    clear_all_cookies(session)
> +    create_cookie_request = {
> +        "cookie": {
> +            "name": "hello",
> +            "value": "world",
> +            "domain": ".web-platform.test"

We shouldn’t really hard code this domain name into the test.  Can
you extract this from the configuration["host"] fixture, please?

I see I’ve accepted a few cases of this in the past, so it would
be excellent if you could fix those too (-:

Comment 10

[Peter Major]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

https://reviewboard.mozilla.org/r/202526/#review207902

> We shouldn’t really hard code this domain name into the test.  Can
> you extract this from the configuration["host"] fixture, please?
> 
> I see I’ve accepted a few cases of this in the past, so it would
> be excellent if you could fix those too (-:

configuration["host"] returns 127.0.0.1 instead of web-platform.test, should I use something else instead?

Comment 11

[Peter Major]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202526/diff/1-2/

Comment 12

[Peter Major]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

https://reviewboard.mozilla.org/r/202526/#review207902

> configuration["host"] returns 127.0.0.1 instead of web-platform.test, should I use something else instead?

Used server_config["domains"][""] instead, wondering if I should have iterate through all the available domains instead

Comment 13

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

https://reviewboard.mozilla.org/r/202526/#review207902

> Used server_config["domains"][""] instead, wondering if I should have iterate through all the available domains instead

I think just picking one of them should be fine.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

https://reviewboard.mozilla.org/r/202526/#review209016

::: testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py:177
(Diff revision 2)
> +    assert "domain" in cookie
> +    assert isinstance(cookie["domain"], basestring)
> +
> +    assert cookie["name"] == "hello"
> +    assert cookie["value"] == "world"
> +    assert cookie["domain"] == ".web-platform.test"

You probably want to assert that it is the same as you set originally.

Comment 15

[Peter Major]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202526/diff/2-3/

Comment 16

[Peter Major]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

https://reviewboard.mozilla.org/r/202526/#review209016

> You probably want to assert that it is the same as you set originally.

d'oh, sorry about that.

Comment 17

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8931398 [details]
Bug 1415828 - Accept cookie domains with leading dot character

https://reviewboard.mozilla.org/r/202526/#review209162

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5e24faf36fc
Accept cookie domains with leading dot character r=ato

Comment 19

[Andreas Tolfsen ❲:ato❳]

Thanks for the patch and also thanks for spending the time digging
into this.  I appreciate it is not fun to drill down into what the
correct/expected behaviour is when all the sources are contradicting.
But I believe users will be happy that this is finally fixed!

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f5e24faf36fc