Impossible to set sub-domain cookies with Marionette

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: majorpetya, Assigned: majorpetya)

Tracking

({regression})

Trunk
mozilla58
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 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.
Component: Untriaged → Marionette
Product: Firefox → Testing
Attachment #8911991 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 1

2 years ago
The originally attached diff file had incomplete lines, this new diff should address this problem..
(Assignee)

Updated

2 years ago
Summary: Cookies created with Marionette are not working well with "regular" cookies → Impossible to set sub-domain cookies with Marionette
(Assignee)

Comment 2

2 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.
Version: 55 Branch → Trunk
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?
Blocks: 1371733
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox58: --- → affected
Flags: needinfo?(majorpetya)
Flags: needinfo?(ato)
Keywords: regression
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 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. :)
Flags: needinfo?(majorpetya)
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.
Flags: needinfo?(ato)
Attachment #8913693 - Flags: review?(ato)
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.
Attachment #8913693 - Flags: review?(hskupin) → review?(ato)
Assignee: nobody → majorpetya
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 8

2 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.
Attachment #8913693 - Flags: review?(ato) → review-

Comment 9

2 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!
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 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.
(Assignee)

Comment 12

2 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.
(Assignee)

Comment 13

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

Comment 14

2 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

Comment 15

2 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?

Comment 16

2 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.
Attachment #8913693 - Flags: review?(ato) → review-
(Assignee)

Comment 17

2 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
Comment hidden (mozreview-request)

Comment 19

2 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
Attachment #8913693 - Flags: review?(ato) → review+

Comment 20

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

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a9c620351c9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Do we want this on Beta too or can it ride the 58 train?
status-firefox-esr52: --- → unaffected
Flags: needinfo?(ato)
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.
Attachment #8913693 - Flags: review?(hskupin)
(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.
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/1e557c44b802
status-firefox57: affected → fixed
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.