Closed
Bug 1402978
Opened 7 years ago
Closed 7 years ago
Impossible to set sub-domain cookies with Marionette
Categories
(Remote Protocol :: Marionette, defect)
Remote Protocol
Marionette
Tracking
(firefox-esr52 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
firefox58 | --- | fixed |
People
(Reporter: majorpetya, Assigned: majorpetya)
References
Details
(Keywords: regression)
Attachments
(3 files)
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.
Updated•7 years ago
|
Component: Untriaged → Marionette
Product: Firefox → Testing
Updated•7 years ago
|
Attachment #8911991 -
Attachment mime type: text/x-patch → text/plain
Assignee | ||
Comment 1•7 years ago
|
||
The originally attached diff file had incomplete lines, this new diff should address this problem..
Assignee | ||
Updated•7 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•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.
Version: 55 Branch → Trunk
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?
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•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. :)
Flags: needinfo?(majorpetya)
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.
Flags: needinfo?(ato)
Attachment #8913693 -
Flags: review?(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.
Attachment #8913693 -
Flags: review?(hskupin) → review?(ato)
Updated•7 years ago
|
Assignee: nobody → majorpetya
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #8913693 -
Flags: review?(ato) → review-
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!
Comment hidden (mozreview-request) |
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.
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.
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.
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
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?
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.
Attachment #8913693 -
Flags: review?(ato) → review-
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
Comment hidden (mozreview-request) |
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
Attachment #8913693 -
Flags: review?(ato) → review+
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8a9c620351c9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 22•7 years ago
|
||
Do we want this on Beta too or can it ride the 58 train?
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(ato)
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.
Updated•7 years ago
|
Attachment #8913693 -
Flags: review?(hskupin)
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.
Flags: needinfo?(ato)
Whiteboard: [checkin-needed-beta]
Comment 25•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1e557c44b802
Whiteboard: [checkin-needed-beta]
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•