Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cookies] Correct utility function and tests #12835

Merged
merged 2 commits into from
Sep 6, 2018

Conversation

jugglinmike
Copy link
Contributor

The cookie-helper.sub.js utility script includes
set_prefixed_cookie_via_http_test, a function that defines sub-tests
using the promise_test API. Previously, it included the following
code:

promise_test(t => {
  var postDelete = _ => {
    // (elided)
  };

  if (!options.origin) {
    return postDelete;
  } else {
    // (elided)
  }
});

The promise_test function does not recognize return values which are
functions, so returning the postDelete method had no effect, and as a
result, the generated tests performed zero assertions. Because none of
the consumers of set_prefixed_cookie_via_http_test specified a value
for the origin option, all invocations were effected by this bug.

Correcting the problem surfaced a number of errors in the tests. In the
interest of atomicity, this patch attempts to address them all:

  • The logic intended to defensively remove cookies prior to testing was
    implemented using document.cookie. Because some tests create cookies
    which include the HttpOnly attribute, the DOM API cannot remove
    cookies in all cases. This patch refactors the solution to remove
    such cookies via an HTTP request. It also assumes the environment is
    initially clean and instead expresses the concern via an asynchronous
    "cleanup" function. (This change necessitated an extension to the
    set.py script so that it could be used to expire cookies.)
  • The test name __secure.header.html incorrectly asserted that a
    cookie set with the Secure attribute could be observed in a
    non-secure context. This patch corrects the expectation.
  • The test named __secure.header.https.html incorrectly asserted that
    a cookie set with a foreign Origin attribute could be observed from
    the current origin. This patch corrects the expectation.

This patch does not affect the test results in Firefox or Chrome. The commands:

$ xvfb-run --auto-servernum ./wpt run firefox cookies/prefix
$ xvfb-run --auto-servernum ./wpt run chrome cookies/prefix

...both continue to report the following:

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 95 checks (9 tests, 86 subtests)
Expected results: 95

Some tests which use the `credFetch` utility include assertions only for
the absence of cookies. Because `fetch` does not reject the returned
promise for valid HTTP responses outside of the 2XX range, these tests
could be satisfied by querying non-existent URLs. This is not an issue
in any existing tests, but it has the potential to hide problems in
future patches.

Update the `credFetch` function to report unsuccessful requests as
failures.
The `cookie-helper.sub.js` utility script includes
`set_prefixed_cookie_via_http_test`, a function that defines sub-tests
using the `promise_test` API. Previously, it included the following
code:

    promise_test(t => {
      var postDelete = _ => {
        // (elided)
      };

      if (!options.origin) {
        return postDelete;
      } else {
        // (elided)
      }
    });

The `promise_test` function does not recognize return values which are
functions, so returning the `postDelete` method had no effect, and as a
result, the generated tests performed zero assertions. Because none of
the consumers of `set_prefixed_cookie_via_http_test` specified a value
for the `origin` option, all invocations were effected by this bug.

Correcting the problem surfaced a number of errors in the tests. In the
interest of atomicity, this patch attempts to address them all:

- The logic intended to defensively remove cookies prior to testing was
  implemented using `document.cookie`. Because some tests create cookies
  which include the `HttpOnly` attribute, the DOM API cannot remove
  cookies in all cases. This patch refactors the solution to remove
  such cookies via an HTTP request. It also assumes the environment is
  initially clean and instead expresses the concern via an asynchronous
  "cleanup" function. (This change necessitated an extension to the
  `set.py` script so that it could be used to expire cookies.)
- The test name `__secure.header.html` incorrectly asserted that a
  cookie set with the `Secure` attribute could be observed in a
  non-secure context. This patch corrects the expectation.
- The test named `__secure.header.https.html` incorrectly asserted that
  a cookie set with a foreign `Origin` attribute could be observed from
  the current origin. This patch corrects the expectation.
Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this patch looks correct to me. I very much appreciate you digging in.

LGTM.

@mikewest mikewest merged commit 88d4f4c into web-platform-tests:master Sep 6, 2018
jugglinmike added a commit to bocoup/wpt that referenced this pull request Sep 7, 2018
Previously, tests defined via the `promise_test` API would fail
immediately if their body returned the value `undefined`. The tests
would *not* fail if they returned any other value. Because the
motivation for using `promise_test` is to track resolution with a
"thenable" value (i.e. an object with a "then" method), this was overly
permissive and allowed contributors to write tests which spuriously
passed [1].

Update testharness.js to enforce more stringent criteria on the value
return by `promise_test` bodies: that it is a "thenable" value. This
change is incompatible with an exiting functional test, but it does not
effect any tests in WPT (as verified by a survey using both the Chrome
and Firefox browsers). Update the functional test accordingly.

[1] web-platform-tests#12835
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants