Closed Bug 1408962 Opened 7 years ago Closed 7 years ago

Make "Add Cookie" webdriver conforming

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
57 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: whimboo, Assigned: majorpetya)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(1 file)

Bug 1408962 - Make add cookie command webdriver spec comformant
59 bytes, text/x-review-board-request
ato
: review+
whimboo
: review+
Details 
https://w3c.github.io/webdriver/webdriver-spec.html#add-cookie

The current implementation of "Add Cookie" is not webdriver conforming due to the following requirements:

* (1)(2) assert that `cookie` is present and a valid JSON object in parameters (driver.js)

* (6) if `httpOnly` is not `undefined` its value should be asserted to be a boolean. Otherwise it has to default to `false`.

* (6) if `secureOnly` is not `undefined` its value should be asserted to be a boolean. Otherwise it has to default to `false`.

* (6) if `expiry` is not `undefined` its value should be asserted to be an integer (0 - 2^64–1). Otherwise the key has to be left out, and the cookie has to be created as a session cookie.

* (7) if a failure occurs when setting the cookie an error of type "unable to set cookie" has to be raised.
Blocks: 1372582
Will try to look into this this week.
Fantastic! Given your recent patches I will assign this bug already to you.
Assignee: nobody → majorpetya
Status: NEW → ASSIGNED
One minor issue around (6) is that the highest number that JavaScript can accurately represent is 2^53 - 1. If we need to be accurate about this part of the spec then we'll need to internally represent the expiration date as a string and somehow ensure that the XPCOM layer transforms the expiry field accurately. This of course would also mean that the verification would have to be implemented in C++.
Interesting fact Peter! Here is the exact description:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER

Which makes me wonder why we make use of the following line of code:
https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionCookies.jsm#20-21

Tim, you added that code via bug 903398. Can you please have a look if we currently have faulty behavior here?
Flags: needinfo?(ttaubert)
Attachment #8919936 - Flags: review?(ato)
I've had a look at this today (see review for the actual changes), and this is what I came up with:
* (1)(2) -> I think this is already done in cookie.js#fromJSON when as a first step we assert that json is an object.
* (6) -> changed cookie#add to define httpOnly and secure fields with defaults explicitly
* (6) -> the assertion for expiry in fromJSON is still just "positiveInteger", which does not verify the upper bounds of the field due to JS limitations. Let me know if a different approach needs to be used.
* (7) -> this is currently missing, will try to implement this soon...
Actually, what is exactly a legit test case for (7), what is meant to be considered failed for example? Should an incorrect cookie path result in this error for example? Out of curiosity I've tried to send a cookie with expiry: -1, but that resulted in the geckodriver sending back HTTP 400 with a Rust stacktrace in the response..

The nsICookieManager2.idl#Add method does not document any error scenarios and has void return value, so I'm not quite sure how to trigger this issue.
(In reply to Henrik Skupin (:whimboo) from comment #4)
> Interesting fact Peter! Here is the exact description:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Number/MAX_SAFE_INTEGER
> 
> Which makes me wonder why we make use of the following line of code:
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/SessionCookies.jsm#20-21
> 
> Tim, you added that code via bug 903398. Can you please have a look if we
> currently have faulty behavior here?

Looks like we should probably switch to Number.MAX_SAFE_INTEGER for that const. I copied that line from the cookie code in sessionstore that has been around since Firefox 2. I'm guessing that the author meant to actually use 2^53.
Flags: needinfo?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #8)
> Looks like we should probably switch to Number.MAX_SAFE_INTEGER for that
> const. I copied that line from the cookie code in sessionstore that has been
> around since Firefox 2. I'm guessing that the author meant to actually use
> 2^53.

I filed bug 1410067 for that. Thanks.
(In reply to Peter Major from comment #6)
> I've had a look at this today (see review for the actual changes), and this
> is what I came up with:
> * (1)(2) -> I think this is already done in cookie.js#fromJSON when as a
> first step we assert that json is an object.

Yeah. Looks like I missed that `cmd.parameters` will always be present. So we won't raise an exception in driver.js when calling fromJSON().

> * (6) -> the assertion for expiry in fromJSON is still just
> "positiveInteger", which does not verify the upper bounds of the field due
> to JS limitations. Let me know if a different approach needs to be used.

I think we have to get a webdriver spec issue on file which should update this part of the spec and tell that 2^53-1 is the maximum. Maybe we should just implement that limit on our side already?
(In reply to Peter Major from comment #7)
> Actually, what is exactly a legit test case for (7), what is meant to be
> considered failed for example? Should an incorrect cookie path result in
> this error for example? Out of curiosity I've tried to send a cookie with
> expiry: -1, but that resulted in the geckodriver sending back HTTP 400 with
> a Rust stacktrace in the response..

Good question. I would just wrap the call to `niICookieManager2.add` into try/catch, and if that fails raise the expected error.

Andreas, maybe you know more from former TPAC discussions, and if we really need this error type.
Flags: needinfo?(ato)
(In reply to Peter Major from comment #7)

> Actually, what is exactly a legit test case for (7), what is meant
> to be considered failed for example? Should an incorrect cookie
> path result in this error for example?

An incorrect cookie path might in some browsers cause an error.
I’m not too sure, because this is a completely unspecified area of
the web.

> Out of curiosity I've tried to send a cookie with expiry: -1, but
> that resulted in the geckodriver sending back HTTP 400 with a Rust
> stacktrace in the response..

That would be a separate issue to do with data validation of the
input.  We need to address this in cookie#add/GeckoDriver#addCookie
as well as in the webdriver crate [1].

> The nsICookieManager2.idl#Add method does not document any error
> scenarios and has void return value, so I'm not quite sure how to
> trigger this issue.

It doesn’t error in Gecko, but might in other user agents.  This
is to cater for a potential error scenario that occurs when adding
the cookie to the internal cookie store, which is completely
undefined by any web standard.

In any case, an unexpected ‘unable to set cookie’ error being
returned should always be treated as an error case in WPT.  It
always expects to be able to set cookies.

  [1] https://searchfox.org/mozilla-central/source/testing/webdriver/README.md
Flags: needinfo?(ato)
Comment on attachment 8919936 [details]
Bug 1408962 - Make add cookie command webdriver spec comformant

https://reviewboard.mozilla.org/r/190878/#review196542

::: testing/marionette/cookie.js:113
(Diff revision 2)
>    assert.string(newCookie.domain, "Cookie domain must be string");
>  
> -  if (typeof newCookie.path == "undefined") {
> -    newCookie.path = "/";
> +  if (typeof newCookie.secure == "undefined") {
> +    newCookie.secure = false;
> +  }
> +  if (typeof newCookie.httpOnly == "undefined") {

One of the shortcomings of having all this post-processing implemented in the add method is that it's not really possible to unit test them (because the add method is "mocked" in test_cookie.js).
Would it be better to change fromJSON to do more than just validating JSON field format?

::: testing/marionette/test_cookie.js:13
(Diff revision 2)
>  
>  cookie.manager = {
>    cookies: [],
>  
>    add: function (domain, path, name, value, secure, httpOnly, session, expiry, originAttributes) {
> +    if (name === "fail") {

Not the prettiest solution (see line 215 for the test), but I couldn't find a better way to trigger this codepath otherwise.

::: testing/web-platform/tests/webdriver/tests/cookies/add_cookie.py:105
(Diff revision 2)
> +    assert "name" in cookie
> +    assert isinstance(cookie["name"], basestring)
> +    assert "value" in cookie
> +    assert isinstance(cookie["value"], basestring)
> +    # Waiting for Bug 1407695
> +    #assert "expiry" not in cookie

Is there a way to disable a python test temporarily? Should I just wait instead for 1407695?
Comment on attachment 8919936 [details]
Bug 1408962 - Make add cookie command webdriver spec comformant

https://reviewboard.mozilla.org/r/190880/#review196774

Looks good so far, but given the overlap with the other bug lets wait for further reviews. Anyway, I made some comments too.

::: testing/marionette/cookie.js:12
(Diff revision 3)
>  const {interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  Cu.import("resource://gre/modules/Services.jsm");
>  
>  Cu.import("chrome://marionette/content/assert.js");
> -const {InvalidCookieDomainError} = Cu.import("chrome://marionette/content/error.js", {});
> +const {InvalidCookieDomainError, UnableToSetCookieError} = Cu.import("chrome://marionette/content/error.js", {});

Lets use the style we use in other files like interactions.js by putting each error type in ist own line.

::: testing/marionette/cookie.js:43
(Diff revision 3)
>   *     are required fields which must be strings.  The <var>path</var> and
>   *     <var>domain</var> fields are optional, but must be a string if
>   *     provided.
> - *     The <var>secure</var>, <var>httpOnly</var>, and
> - *     <var>session</var>fields are similarly optional, but must be
> - *     booleans.  Likewise, the <var>expiry</var> field is optional but
> + *     The <var>secure</var>, and <var>httpOnly</var> are similarly
> + *     optional, but must be booleans.  Likewise, the <var>expiry</var> field
> + *      is optional but must be unsigned integer.

nit: remove the extra space at the beginning of the line.

::: testing/marionette/cookie.js:118
(Diff revision 3)
> +    newCookie.httpOnly = false;
>    }
> -
>    if (typeof newCookie.expiry == "undefined") {
> -    // twenty years into the future
> -    let date = new Date();
> +    // The XPCOM interface requires the expiry field even for session cookies.
> +    newCookie.expiry = new Date(Date.now()).getTime() + 86400;

So from:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookie2

> Note: That expiry time will also be honored for session cookies; thus, whichever is the more restrictive of the two will take effect.

To really ensure those will be session cookies we should use the max safe value, right?

::: testing/marionette/cookie.js:167
(Diff revision 3)
> -      newCookie.httpOnly,
> +        newCookie.httpOnly,
> -      newCookie.session,
> +        newCookie.session,
> -      newCookie.expiry,
> +        newCookie.expiry,
> -      {} /* origin attributes */);
> +        {} /* origin attributes */);
> +  } catch (e) {
> +    throw new UnableToSetCookieError(e);

Does that forward the error message? I assume so.
Attachment #8919936 - Flags: review?(hskupin)
Comment on attachment 8919936 [details]
Bug 1408962 - Make add cookie command webdriver spec comformant

https://reviewboard.mozilla.org/r/190880/#review196774

> Does that forward the error message? I assume so.

Yes it should.
Attachment #8919936 - Flags: review?(ato)
Attachment #8919936 - Flags: review?(ato)
Comment on attachment 8919936 [details]
Bug 1408962 - Make add cookie command webdriver spec comformant

https://reviewboard.mozilla.org/r/190880/#review196798

Aside from whimboo’s comments, this looks good.
Attachment #8919936 - Flags: review?(ato) → review+
Comment on attachment 8919936 [details]
Bug 1408962 - Make add cookie command webdriver spec comformant

https://reviewboard.mozilla.org/r/190880/#review200984

::: testing/marionette/cookie.js:122
(Diff revision 4)
>    }
> -
>    if (typeof newCookie.expiry == "undefined") {
> -    // twenty years into the future
> -    let date = new Date();
> -    let now = new Date(Date.now());
> +    // The XPCOM interface requires the expiry field even for session cookies.
> +    newCookie.expiry = Number.MAX_SAFE_INTEGER;
> +    newCookie.session = true;

So compared to `expiry` the method `cookie.manager.add()` allows `undefined` for `isSession`? We would hit this case for all non-session cookies now.

::: testing/marionette/test_cookie.js:246
(Diff revision 4)
>  });
>  
>  add_test(function test_iter() {
>    cookie.manager.cookies = [];
> +  let tomorrow = new Date();
> +  tomorrow.setDate(tomorrow.getDate() + 1);

`getDate()` returns the day of the month. In case of the last day of the month, the result would be `1`. This should cause problems with the test.

I would propose that you better add 24 hours.
Attachment #8919936 - Flags: review?(hskupin) → review-
Comment on attachment 8919936 [details]
Bug 1408962 - Make add cookie command webdriver spec comformant

https://reviewboard.mozilla.org/r/190880/#review202836

This looks fine to me now. Thanks Peter! Before I'm going to request the landing of the patch, I triggered another try build to make sure it all works.
Attachment #8919936 - Flags: review?(hskupin) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/229d540c474e
Make add cookie command webdriver spec comformant r=ato,whimboo
https://hg.mozilla.org/mozilla-central/rev/229d540c474e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1427807
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: