Closed Bug 1407695 Opened 7 years ago Closed 7 years ago

Get Cookie should not return expiry key for session cookies

Categories

(Remote Protocol :: Marionette, defect)

57 Branch
defect
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: whimboo, Assigned: insulaventus, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(4 files, 2 obsolete files)

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
Assignee: nobody → insula.ventus
Status: NEW → ASSIGNED
Keywords: good-first-bug
OS: Unspecified → All
Hardware: Unspecified → All
(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.
Assignee: insula.ventus → nobody
Status: ASSIGNED → NEW
(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.
Attachment #8918688 - Attachment description: Do not return expiry key for session cookies → Bug 1407695 - Do not return expiry key for session cookies
(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.
Attachment #8918688 - Flags: review?(hskupin) → feedback+
Assignee: nobody → insula.ventus
Status: NEW → ASSIGNED
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.
Attachment #8919069 - Flags: review?(hskupin)
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.
Attachment #8919070 - Flags: review?(hskupin)
Attachment #8919069 - Flags: review?(hskupin)
Attachment #8919070 - Flags: review?(hskupin) → review?(james)
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.
Attachment #8918688 - Attachment is obsolete: true
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.
Attachment #8919069 - Flags: review-
Attachment #8919069 - Flags: review?(hskupin)
Attachment #8919070 - Flags: review?(hskupin) → review?(james)
Attachment #8919070 - Flags: review?(mjzffr)
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.
Attachment #8919069 - Flags: review+
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.
Attachment #8919070 - Flags: review?(mjzffr)
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?
Attachment #8919069 - Attachment is obsolete: true
(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.
Attachment #8919070 - Flags: review?(ato) → review-
Attachment #8919070 - Flags: review?(james)
Attachment #8919070 - Flags: review?(hskupin)
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?
Attachment #8921213 - Flags: review?(ato) → review-
Comment on attachment 8919070 [details]
Bug 1407695 - Do not return expiry key for session cookies;

https://reviewboard.mozilla.org/r/189978/#review197424
Attachment #8919070 - Flags: review?(ato) → review+
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
Attachment #8921213 - Flags: review?(ato) → review+
Comment on attachment 8921349 [details]
Bug 1407695 - Allows cookie 'expiry' to be optional;

https://reviewboard.mozilla.org/r/192384/#review197504
Attachment #8921349 - Flags: review?(ato) → review+
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.
(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?
Flags: needinfo?(insula.ventus)
Ø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
Attachment #8921433 - Flags: review?(ato) → review+
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.
Flags: needinfo?(insula.ventus)
I have fixed the issue and submitted updated patches.
Flags: needinfo?(insula.ventus)
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
Ø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.
Attachment #8919070 - Flags: review?(hskupin)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: