Peter Major Assignee
	Description • 7 years ago

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

Steps to reproduce:

When Marionette is used to create a cookie and later on the cookie is rewritten by a web application (i.e. by something else than Marionette), then Marionette will get really confused about the cookies and start to think that there are two cookies, one with a leading dot ('.') character in the cookie domain, the other without.

I've tried to report this issue first for geckodriver: https://github.com/mozilla/geckodriver/issues/823
But further investigation revealed that it is Marionette that returns silly data.

I have attached a very simple test that demonstrates this issue. To run the test:
./mach test testing/web-platform/tests/webdriver/tests/cookies/cookies.py


Actual results:

The Javascript code for some reason creates a different cookie than the Marionette cookie, which means that both of them are returned erroneously when asking for all the cookies.


Expected results:

Cookies created by application should overwrite cookies created by Marionette, if the cookie name/domain/path is the same.

	Peter Major Assignee
	Comment 1 • 7 years ago

The originally attached diff file had incomplete lines, this new diff should address this problem..

	Peter Major Assignee
	Comment 2 • 7 years ago

I think there are two different issues involved with this bug. There is one issue with Firefox 55.0.3 and Marionette where cookies that were meant to be created as domain cookies are created as host cookies instead.

The other issue is with latest on mozilla/central whereby the cookies created using Marionette ignore the domain parameter altogether and you always end up with a host cookie. This latter issue I've just fixed locally (although I reckon further development will be needed) and was caused by the fix made to Bug 1371733 (this commit specifically: https://hg.mozilla.org/mozilla-central/rev/24650e8a712b ).

The former issue still needs to be tracked down...

Hopefully I got the module owner right via CC field, I think this is a serious enough regression in Marionette that should be fixed ahead of release.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 7 years ago

Thank you majorpetya! Sadly it's too late to be fixed for the 56 release. It's available since yesterday. But lets see what we can do to get this quickly fixed for 57 beta. Given that you say that you got it fixed locally, would you mind to provide a fix? Having a test included would be fantastic!

In any case, Andreas can you work with majorpetya so that we can get this fixed?

	Peter Major Assignee
	Comment 5 • 7 years ago

As this is my first ever contribution to Firefox (and first time use of Mercurial) hopefully the above review request is appropriate. Looking forward to your comments. :)

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

I’m actually not sure this is the correct behaviour because I
don’t understand what our current cookie implementation is doing.
In any case, I don’t have capacity to look into this right now.

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 7 years ago

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

Disregard my above comment.  I will have a look at this in the morning.

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190556

::: commit-message-cd9c8:1
(Diff revision 1)
> +Bug 1402978 - Fix missing cookie domain handling in Marionette implementation r?ato, whimboo

I would rephrase this as:

> Add cookie domain field to WebDriver:AddCookie

::: commit-message-cd9c8:3
(Diff revision 1)
> +There were two issues with the previous implementation:
> +* Domain cookies were created as host only cookies (due to lack of leading '.' characters)
> +* The cookie domain included in the Marionette request was completely ignored, which always resulted in host-only cookies

Please run this through fmt(1) or limit the columns to ~72 characters.

::: testing/marionette/cookie.js:64
(Diff revision 1)
>    newCookie.name = assert.string(json.name, "Cookie name must be string");
>    newCookie.value = assert.string(json.value, "Cookie value must be string");
>  
> +  if (typeof json.domain != "undefined") {
> +    let domain = assert.string(json.domain, "Cookie domain must be string");
> +    if (domain.charAt(0) !== ".") {

Use domain.substring(0, 1) because charAt does not work for Unicode
surrogate pairs.  For example, if the first code point consists of
two surrogate halves, this would only get the first half.

::: testing/web-platform/tests/webdriver/tests/cookies/cookie.html:1
(Diff revision 1)
> +<html>
> + <head>
> +  <title>Title</title>
> + </head>
> + <body>
> +  <script>
> +   document.cookie = "hello=newworld; domain=web-platform.test; path=/";
> +  </script>
> + </body>
> +</head>

Make this an inline page instead, using the inline() fixture
available from the tests.support.inline package:

> inline("<script>document.cookie = '…'</script>")

::: testing/web-platform/tests/webdriver/tests/cookies/cookies.py:1
(Diff revision 1)
> +import time;
> +

Unused.

::: testing/web-platform/tests/webdriver/tests/cookies/cookies.py:34
(Diff revision 1)
>      assert cookie["name"] == "foo"
>      assert cookie["value"] == "bar"
> +
> +def test_duplicated_cookie(session, url):
> +    session.url = url("/common/blank.html")
> +    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, """{"cookie":{"path":"/","domain":"web-platform.test","name":"hello","httpOnly":false,"secure":false,"value":"world"}}""")

Construct the JSON Object from a Python dictionary which you
serialise using json.dumps for better readability.

::: testing/web-platform/tests/webdriver/tests/cookies/cookies.py:38
(Diff revision 1)
> +    session.url = url("/common/blank.html")
> +    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, """{"cookie":{"path":"/","domain":"web-platform.test","name":"hello","httpOnly":false,"secure":false,"value":"world"}}""")
> +    assert result.status == 200
> +    assert isinstance(result.body["value"], dict)
> +
> +    session.url = url("/webdriver/tests/cookies/cookie.html")

This is where you use inline() I talked about earlier.

::: testing/web-platform/tests/webdriver/tests/cookies/cookies.py:41
(Diff revision 1)
> +    assert isinstance(result.body["value"], dict)
> +
> +    session.url = url("/webdriver/tests/cookies/cookie.html")
> +    result = session.transport.send("GET", "session/%s/cookie" % session.session_id)
> +    assert result.status == 200
> +    assert isinstance(result.body["value"], list)

Missing check for whether value is on result.body and a check for whether it’s a dict.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190790

Nothing further to add right now, except that it would be great if you could also check `testing/marionette/test_cookie.js` if a new testcase would make sense. I most likely think so (without having a look at this file). Thanks a lot for working on that patch!

	Peter Major Assignee
	Comment 11 • 7 years ago
mozreview-review-reply

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190556

> I would rephrase this as:
> 
> > Add cookie domain field to WebDriver:AddCookie

I've updated the commit message as suggested. As a sidenote, this is not really WebDriver:AddCookie, but more like Cookie:Add, I assume the latter is considered as an unimportant implementation detail.

> Use domain.substring(0, 1) because charAt does not work for Unicode
> surrogate pairs.  For example, if the first code point consists of
> two surrogate halves, this would only get the first half.

JavaScript isn't my main language so I really appreciate the detail about the difference between the two approaches. I presume the concern here was to do with unicode domain names specifically.

> Make this an inline page instead, using the inline() fixture
> available from the tests.support.inline package:
> 
> > inline("<script>document.cookie = '…'</script>")

Thanks for the tip, works like a charm.

> Construct the JSON Object from a Python dictionary which you
> serialise using json.dumps for better readability.

Just had to realize that in Python the capitalization of False matters, d'oh :)

> Missing check for whether value is on result.body and a check for whether it’s a dict.

I've done these, and ended up creating a new test for the domain cookies specifically as well. The problem was then that the new test seen the cookies created in the first test, so I ended up writing an additional fixture to clean up all cookies.. Let me know if there is a better approach to tackle this.

	Peter Major Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190790

Thanks for the pointer, I've added a small test case in there and made sure that the tests actually succeed.

	Peter Major Assignee
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190750

::: testing/marionette/cookie.js:122
(Diff revision 1)
>      date.setYear(now.getFullYear() + 20);
>      newCookie.expiry = date.getTime() / 1000;
>    }
>  
>    if (restrictToHost) {
> -    if (newCookie.domain !== restrictToHost) {
> +    // TODO: handle gTLDs

Reading the [webdriver spec](https://www.w3.org/TR/webdriver/#add-cookie) I'm not really sure this would be actually necessary. The spec only seems to require that the active document's address matches the cookie domain.

::: testing/marionette/test_cookie.js:184
(Diff revision 2)
>      Assert.throws(
>          () => cookie.add({name: "name", value: invalidType}),
>          "Cookie must have string value");
>      Assert.throws(
>          () => cookie.add({name: "name", value: "value", domain: invalidType}),
> -        "Cookie must have string value");
> +        "Cookie must have string domain");

It looks like that this last parameter can be pretty much anything, the test framework doesn't actually seem to exact match these, not sure if this is a feature or a bug.

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190556

> I've updated the commit message as suggested. As a sidenote, this is not really WebDriver:AddCookie, but more like Cookie:Add, I assume the latter is considered as an unimportant implementation detail.

Here I’m referring to the name of the Marionette command:

	https://searchfox.org/mozilla-central/source/testing/marionette/driver.js#3556

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190750

> Reading the [webdriver spec](https://www.w3.org/TR/webdriver/#add-cookie) I'm not really sure this would be actually necessary. The spec only seems to require that the active document's address matches the cookie domain.

Then let’s remove the comment.

> It looks like that this last parameter can be pretty much anything, the test framework doesn't actually seem to exact match these, not sure if this is a feature or a bug.

Keenly spotted!  This is a bug with the test case.  Do you mind
filing a bug in the Testing :: Marionette component?

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190970

Thanks for the changes, it is close to landable now.  Can you please
fix up the minor nits whilst I trigger a try run of this?

::: testing/web-platform/tests/webdriver/tests/cookies/cookies.py:36
(Diff revision 2)
>      assert cookie["value"] == "bar"
> +
> +def test_duplicated_cookie(session, url):
> +    session.url = url("/common/blank.html")
> +    clear_all_cookies(session)
> +    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, {"cookie":{"path":"/","domain":"web-platform.test","name":"hello","httpOnly":False,"secure":False,"value":"world"}})

For readbility, can you construct the cookie on a variable with
better indentation and spacing between properties and values?

::: testing/web-platform/tests/webdriver/tests/cookies/cookies.py:61
(Diff revision 2)
> +    assert cookie["value"] == "newworld"
> +
> +def test_add_domain_cookie(session, url):
> +    session.url = url("/common/blank.html")
> +    clear_all_cookies(session)
> +    result = session.transport.send("POST", "session/%s/cookie" % session.session_id, {"cookie":{"path":"/","domain":"web-platform.test","name":"hello","httpOnly":False,"secure":False,"value":"world"}})

Same here.

	Peter Major Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review190750

> Then let’s remove the comment.

Done

> Keenly spotted!  This is a bug with the test case.  Do you mind
> filing a bug in the Testing :: Marionette component?

Done, https://bugzilla.mozilla.org/show_bug.cgi?id=1405240

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8913693 [details]
Bug 1402978 - Add cookie domain field to WebDriver:AddCookie

https://reviewboard.mozilla.org/r/185100/#review191016

	Pulsebot
	Comment 20 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a9c620351c9
Add cookie domain field to WebDriver:AddCookie r=ato

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8a9c620351c9

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 7 years ago

Do we want this on Beta too or can it ride the 58 train?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 23 • 7 years ago

It's a regression we would ship in Marionette with 57, so I would prefer to see it uplifted to beta. But lets wait for Andreas.

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)

> Do we want this on Beta too or can it ride the 58 train?

This could ride the 58 train, but if whimboo wants to uplift the
regression I’m happy with that.  I don’t consider this at great
risk.

Let’s uplift to 58.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/1e557c44b802