Originally filed as https://github.com/mozilla/geckodriver/issues/1000 It's not that clear by the spec yet, but if it's a session cookie, the `expiry` key should not be returned. This can be done by only including it in the response if the `isSession` flag of the nsICookie2 interface (https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookie2) is false. The code to be updated can be found here: https://dxr.mozilla.org/mozilla-central/rev/a0488ecc201c04f2617e7b02f039344e8fbf0d9a/testing/marionette/cookie.js#185-200 It would also be good to get a testcase added for that fix in that file: https://dxr.mozilla.org/mozilla-central/source/testing/marionette/test_cookie.js

Would this be a good first bug to work on? If so, I would very much like to work on it.

(In reply to Øyvind Strømmen (:insulaventus) from comment #1) > Would this be a good first bug to work on? If so, I would very > much like to work on it. Sure, I think this is a good first bug. I’ve assigned it to you. You need to change [1] so that expiry is only included when cookie.isSession is false: > yield { > "name": cookie.name, > "value": cookie.value, > "path": cookie.path, > "domain": cookie.host, > "secure": cookie.isSecure, > "httpOnly": cookie.isHttpOnly, > "expiry": cookie.expiry, > }; [1] https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/testing/marionette/cookie.js#198

(In reply to Andreas Tolfsen ‹:ato› from comment #2) > I’ve assigned it to you. Sorry, I apparently broke policy by assigning this without a patch attached.

(In reply to Andreas Tolfsen ‹:ato› from comment #3) > (In reply to Andreas Tolfsen ‹:ato› from comment #2) > > > I’ve assigned it to you. Thank you. > Sorry, I apparently broke policy by assigning this without a patch > attached. But it is still ok for me to work on this?

Yes, totally! Once your first version of the patch has been uploaded, we will assign the bug to you. Sorry for the confusion. In case you have issues to get started, let us know here or join us on irc.mozilla.org/#ateam. Thanks

I think this issue may be partially dependent on Bug 1372582. The reasoning being that Marionette only implements "20 year default session expiry" per an earlier version of the spec, so actually it is not possible to create a session cookie with Marionette at the moment. This minor problem could be worked around locally by writing tests that create cookies via JS instead.

I had some issues setting up mozreview, so I hope it's ok that I uploaded the patch directly to the bug. If not, just let me know and I'll get it sorted out.

(In reply to Peter Major from comment #6) > I think this issue may be partially dependent on Bug 1372582. The reasoning > being that Marionette only implements "20 year default session expiry" per > an earlier version of the spec, so actually it is not possible to create a > session cookie with Marionette at the moment. This minor problem could be > worked around locally by writing tests that create cookies via JS instead. I apologize if this is a stupid question, but would it still make sense to apply this patch even though it's dependent on Bug 1372582? The way I interpret it, is that "cookie.iter" [1] won't find any session cookies until that bug is patched. Is that correct? [1] https://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/testing/marionette/cookie.js#198

Comment on attachment 8918688 [details] [diff] [review] Bug 1407695 - Do not return expiry key for session cookies Review of attachment 8918688 [details] [diff] [review]: ----------------------------------------------------------------- Generally it's always better to get a patch up via mozreview because it also makes it easier for us to trigger try pushes and finally land your patch. Maybe you can fix it for the next push? If you have issues please join us on irc.mozilla.org/#ateam. We are happy to help you on that. Beside the current coverage you will also add a test to the following file so we have coverage in the webplatform tests: https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py You can run those tests via `mach wpt testing/web-platform/tests/webdriver/tests/cookies` ::: mozilla/src/gecko/testing/marionette/cookie.js @@ +206,4 @@ > do { > if ((cookie.host == "." + hostname || cookie.host == hostname) && > isForCurrentPath(cookie.path)) { > + let newCookie = { I would call it `data` given that this is just the data we return but not a cookie object. ::: mozilla/src/gecko/testing/marionette/test_cookie.js @@ +24,4 @@ > cookie.manager.cookies.push(newCookie); > }, > > + remove: function (host, name, path, blocked, originAttributes) { Wow, good catch. That was a nasty one. @@ +267,5 @@ > equal(1, fooCookies.length); > equal(".foo.example.com", fooCookies[0].domain); > > + cookie.add({ > + session: true, I would suggest that you make a note here that this is just a workaround until bug 1408962 has been fixed. Basically should be accomplished by leaving out the `expiry` property. As such please make sure to not specify it for this cookie. @@ +283,5 @@ > + }); > + > + let sessionCookies = [...cookie.iter("session.com")]; > + equal(2, sessionCookies.length); > + sessionCookies = sessionCookies.filter(cookie => !cookie.hasOwnProperty("expiry")); Lets not filter but explicitly check. Also I don't see why we need another non-session cookie here given that we have tests for such a cookie type already above.

I'm still working on the webplatform test.

Comment on attachment 8919069 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189976/#review196030 Please make sure to apply the review comments which I have given for the patch as attached earlier to Bugzilla directly.

Comment on attachment 8919070 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189978/#review196032 Oh, so the changes actually got added as a separate commit. What you actually want here is to amend this commit with the first one. You most likely only want a separate commit for the changes of the webplatform-tests files. If you don't know how to combine the changes I can explain it tomorrow on IRC.

Comment on attachment 8918688 [details] [diff] [review] Bug 1407695 - Do not return expiry key for session cookies Marking patch as obsolete given that we have a mozreview version up now.

Comment on attachment 8919069 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189976/#review196716 The patch for Marionette looks great. Please have a look at the two requested changes for the test_cookie xpcshell test. Beside that I also triggered a try build which results should be visible here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4982ce48a63 ::: testing/marionette/test_cookie.js:266 (Diff revision 2) > + domain: "bar.example.com", > + }); > + > let fooCookies = [...cookie.iter("foo.example.com")]; > equal(1, fooCookies.length); > equal(".foo.example.com", fooCookies[0].domain); Please add a new check here which ensures that a non-session cookie has the `expiry` key included. ::: testing/marionette/test_cookie.js:283 (Diff revision 2) > + > + let sessionCookies = [...cookie.iter("session.com")]; > + equal(1, sessionCookies.length); > + equal(false, sessionCookies[0].hasOwnProperty("expiry")); > + equal("aSessionCookie", sessionCookies[0].name); > + I would just flip the checks for expiry and name here, so in case a wrong cookie is returned we fail on the domain. Otherwise it would not be clear why `expiry` has not been set.

Comment on attachment 8919069 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189976/#review196790 This looks fine now.

Comment on attachment 8919070 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189978/#review196792 ::: testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py:29 (Diff revision 3) > assert "expiry" in cookie > - assert isinstance(cookie["expiry"], (int, long)) > + assert cookie["expiry"] is None As I understand it, expiry should be left out when returning a session cookie? This necessitates a change to the webdriver crate in testing/webdriver and geckodriver in testing/geckodriver. ::: testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py:38 (Diff revision 3) > + a_year_from_now = int((datetime.now() + timedelta(days=365)).strftime('%s')) > + create_cookie_request = { > + "cookie": { > + "name": "biscuit", > + "value": "chocolate", > + "domain": "web-platform.test", > + "path": "/", > + "httpOnly": False, > + "secure": False, > + "expiry": a_year_from_now > + } > + } > + result = session.transport.send("POST", "session/%s/cookie" % session.session_id, create_cookie_request) > + assert result.status == 200 > + assert "value" in result.body > + assert isinstance(result.body["value"], dict) Ideally you should set the cookie using an injected document.cookie request because this test concerns the Get Named Cookie command, and not Add Cookie.

Comment on attachment 8919070 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189978/#review196948 I agree with :ato's suggestions. Happy to re-review once they're addressed.

Comment on attachment 8919070 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189978/#review196792 > As I understand it, expiry should be left out when returning a > session cookie? This necessitates a change to the webdriver crate > in testing/webdriver and geckodriver in testing/geckodriver. I talked to James yesterday and as he told it should be no difference if `expiry` is not present, or its value null. So we have two different opinions now for that. What's correct?

(In reply to Henrik Skupin (:whimboo) from comment #25) > > As I understand it, expiry should be left out when returning > > a session cookie? This necessitates a change to the > > webdriver crate in testing/webdriver and geckodriver in > > testing/geckodriver. > > I talked to James yesterday and as he told it should be no > difference if `expiry` is not present, or its value null. So we > have two different opinions now for that. What's correct? If the test assertion is "assert cookie["expiry"] is None", then there is a difference because it will fail if the "expiry" field is not included. It would have to change to "assert cookie.get("expiry") is None" to pass.

Comment on attachment 8919070 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189978/#review197242 I think we’re getting closer to land this change, thanks for your changes. As it was uncovered in the discussion on the bug, there’s no difference between a missing "expiry" field and an "expiry" field set to null. The existing test, which you rename to test_get_named_session_cookie, assumes the "expiry" field is always present. I would appreciate if you could do a separate commit that changes it to do "assert cookie.get("expiry") is None" because the change is a precursor to your other test. ::: testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py:29 (Diff revision 4) > assert "expiry" in cookie > + assert cookie["expiry"] is None Per the discussion in the bug, this needs to be cookie.get("expiry") because the field may be null or undefined (left out) from the response. Could you make a separate commit for your changes to test_get_named_session_cookie to not confuse them with the rest of this change? ::: testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py:55 (Diff revision 4) > + assert "expiry" in cookie > assert isinstance(cookie["expiry"], (int, long)) So for this case, the assertions are correct because we expect expiry to be included. ::: testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py:61 (Diff revision 4) > assert isinstance(cookie["expiry"], (int, long)) > > assert cookie["name"] == "foo" > assert cookie["value"] == "bar" > + # convert from seconds since epoch > + assert datetime.utcfromtimestamp(cookie["expiry"]).strftime("%a, %d %b %Y %H:%M:%S") == a_year_from_now Please put the string you pass to strftime in a constant because you reuse is further up.

Comment on attachment 8921213 [details] Bug 1407695 - Clear all cookies from session before running test; https://reviewboard.mozilla.org/r/192208/#review197422 Thanks for making this a separate commit, and also for ordering them correctly! I’m sorry to give this another r- for what may seem like nitpicks, but the remaining issues are minuscule. ::: commit-message-20389:1 (Diff revision 1) > +Bug 1407695 - When getting a session cookie we expect 'expiry' to be either null, or to be missing; r?ato You should try to limit the first line of the commit message to ~72 characters, then—if needed—follow that up with a blank line followed by longer description. Subsequently, the first line should explain exactly what the patch is changing, not an explanation. In this case a better first line would be “Allow cookie "expiry" to be optional” or similar, followed by the “when getting a session cookie […]” story. ::: testing/web-platform/tests/webdriver/tests/cookies/get_named_cookie.py:6 (Diff revision 1) > from tests.support.inline import inline > from tests.support.fixtures import clear_all_cookies > > -def test_get_named_cookie(session, url): > +def test_get_named_session_cookie(session, url): > session.url = url("/common/blank.html") > + clear_all_cookies(session) This line seems unrelated to the change described in the commit message. Perhaps it deserves a separate commit?

Comment on attachment 8919070 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189978/#review197424

On the upside, the try run looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=82a795172adf

So it seems the reviews https://reviewboard.mozilla.org/r/192384 and https://reviewboard.mozilla.org/r/189978/ got switched up. I was under the impression that as long as a commit has a new and unique mozreview-commit-id, a new review will be created. But in this case it overwrote an older one: Ref this diff: https://reviewboard.mozilla.org/r/189978/diff/5-6/ Not sure how to fix this. Should I leave it as is?

Comment on attachment 8921213 [details] Bug 1407695 - Clear all cookies from session before running test; https://reviewboard.mozilla.org/r/192208/#review197502

Comment on attachment 8921349 [details] Bug 1407695 - Allows cookie 'expiry' to be optional; https://reviewboard.mozilla.org/r/192384/#review197504

Comment on attachment 8919070 [details] Bug 1407695 - Do not return expiry key for session cookies; https://reviewboard.mozilla.org/r/189978/#review197506

Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cae1b578dacc Clear all cookies from session before running test; r=ato https://hg.mozilla.org/integration/autoland/rev/ff9010e2f402 Allows cookie 'expiry' to be optional; r=ato https://hg.mozilla.org/integration/autoland/rev/9bd0f859aa80 Cover non session cookies in web platform tests; r=ato

(In reply to Øyvind Strømmen (:insulaventus) from comment #37) > So it seems the reviews https://reviewboard.mozilla.org/r/192384 > and https://reviewboard.mozilla.org/r/189978/ got switched > up. I was under the impression that as long as a commit has > a new and unique mozreview-commit-id, a new review will be > created. But in this case it overwrote an older one: Ref this > diff: https://reviewboard.mozilla.org/r/189978/diff/5-6/ > > Not sure how to fix this. Should I leave it as is? A trick is to simply remove the MozReview-Commit-Id from the commit message, and it will generate a new SHA1, but this looks fine to me.

Thanks for fixing this bug! The try run seemed fine, so I’ve sent it off to autoland. That will run some more tests, and if they pass, it will land on mozilla-central within a day or two.

Backed out 3 changesets (bug 1407695) for failing in /webdriver/tests/cookies/get_named_cookie.py r=backout on a CLOSED TREE. https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9c968cad8e6ae47aec9ec71d5ace795c2468a82b&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception https://hg.mozilla.org/integration/autoland/rev/b97c815ae0ab9eee3d51cd33b6df83f0248c8105

(In reply to Andreas Tolfsen ‹:ato› from comment #43) > Thanks for fixing this bug! The try run seemed fine, so I’ve sent > it off to autoland. That will run some more tests, and if they > pass, it will land on mozilla-central within a day or two. Glad I could contribute! And thanks for the reviews. It looks like only the 3 changesets to get_named_cookie.py ran on autoland. Not the one to cookie.js (https://reviewboard.mozilla.org/r/189976/diff/3#index_header). Therefore the 'assert cookie.get("expiry") is None' check fails. Is there anything I can do to remedy this?

Øyvind, you should go into mozreview, click on review summary, and then reopen the review. Once done you can push other changes to mozreview again.

There. Hopefully that does it.

Comment on attachment 8921433 [details] Bug 1407695 - Cover non session cookies in web platform tests; https://reviewboard.mozilla.org/r/192458/#review197698

Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6960879cb1e3 Do not return expiry key for session cookies; r=ato https://hg.mozilla.org/integration/autoland/rev/f5b8efb37541 Clear all cookies from session before running test; r=ato https://hg.mozilla.org/integration/autoland/rev/d5d1935726bb Allows cookie 'expiry' to be optional; r=ato https://hg.mozilla.org/integration/autoland/rev/cd2147b5bc3d Cover non session cookies in web platform tests; r=ato

Sorry, had to back this out again, this time out for eslint failure at testing/marionette/cookie.js:219: ["expiry"] is better written in dot notation: https://hg.mozilla.org/integration/autoland/rev/c654b0e6969ca20eda800bc16bd3b4f5ee6f4f00 https://hg.mozilla.org/integration/autoland/rev/ee9ac079ab0ecb1e6e011094cb227e5c36fe5dcf https://hg.mozilla.org/integration/autoland/rev/48aa910c888f8400372d73a377476ab56592d52a https://hg.mozilla.org/integration/autoland/rev/6e6cacf776f3dbd81f5d527b86b11e3078244ebe Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139226479&repo=autoland TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/marionette/cookie.js:219:16 | ["expiry"] is better written in dot notation. (dot-notation) Please fix the issue and submit updated patches. Thank you.

I have fixed the issue and submitted updated patches.

Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a7e5f49326e9 Do not return expiry key for session cookies; r=ato https://hg.mozilla.org/integration/autoland/rev/0ffc068daf9b Clear all cookies from session before running test; r=ato https://hg.mozilla.org/integration/autoland/rev/a671aceefe2d Allows cookie 'expiry' to be optional; r=ato https://hg.mozilla.org/integration/autoland/rev/d3930e9399c8 Cover non session cookies in web platform tests; r=ato

https://hg.mozilla.org/mozilla-central/rev/a7e5f49326e9 https://hg.mozilla.org/mozilla-central/rev/0ffc068daf9b https://hg.mozilla.org/mozilla-central/rev/a671aceefe2d https://hg.mozilla.org/mozilla-central/rev/d3930e9399c8

Øyvind, thank you a lot for working on this bug. If you want to learn more, and wants to get your hands on another issue please let us know, or find a bug yourself.

(In reply to Henrik Skupin (:whimboo) from comment #62) > Øyvind, thank you a lot for working on this bug. If you want to learn more, > and wants to get your hands on another issue please let us know, or find a > bug yourself. Glad I could contribute with something. And thank _you_ for the help. I would definitely be interested in working on another bug. So if you know of a good one I'd be happy to work on it.