1465063 - browser.cookies.getall() returns only non-FPI cookies when domain argument is set with firstPartyDomain set to null

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[ysard]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180516032328

Steps to reproduce:

I am an extension developer who tried to build a cookie manager (https://github.com/ysard/cookie-quick-manager).

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180516032328

Steps to reproduce:

- Deactivate First Party Isolation in about:config
privacy.firstparty.isolate = false

- Go to http://www.whatarecookies.com/cookietest.asp or similar; this will create a non FPI cookie:
    {
    "name": "dta",
    "value": "vcount%3D1%2Cprev%3D1527530922199",
    "domain": "www.whatarecookies.com",
    "hostOnly": true,
    "path": "/",
    "secure": false,
    "httpOnly": false,
    "session": false,
    "firstPartyDomain": "",
    "expirationDate": 1527532122,
    "storeId": "firefox-default"
    }

- Then, activate FPI:
privacy.firstparty.isolate = true

- Go to http://www.whatarecookies.com/cookietest.asp or similar; this will create a FPI cookie:
    {
    "name": "dta",
    "value": "vcount%3D0%2Cprev%3D1527530961000",
    "domain": "www.whatarecookies.com",
    "hostOnly": true,
    "path": "/",
    "secure": false,
    "httpOnly": false,
    "session": false,
    "firstPartyDomain": "whatarecookies.com",
    "expirationDate": 1527532161,
    "storeId": "firefox-default"
    }

For the following tests, the FPI feature is enabled.

- Try to get all cookies :
browser.cookies.getAll({storeId: "firefox-default", firstPartyDomain: null})
Get all cookies (FPI and not FPI) as expected.

- Try to get cookies without firstPartyDomain argument:
browser.cookies.getAll({domain: "www.whatarecookies.com", storeId: "firefox-default"})
Get an error as expected:
First-Party Isolation is enabled, but the required 'firstPartyDomain' attribute was not set.

- Try to get FPI cookies for a domain:
browser.cookies.getAll({domain: "www.whatarecookies.com", storeId: "firefox-default", firstPartyDomain: "whatarecookies.com"})
Get only FPI cookies as expected.

- Try to get all cookies (FPI and not FPI) for a specific domain:
browser.cookies.getAll({domain: "www.whatarecookies.com", storeId: "firefox-default", firstPartyDomain: null"})
Get only not FPI cookies.
This is not expected, because when firstPartyDomain is set to null, I should get the cookies whatever their flag


Actual results:

The api seems to have a bug when you want to get cookies from a domain whatever their firstpartydomain flag; It only returns non-FPI cookies. 
I think it's not the expected behavior and, it's quite annoying.


Expected results:

The API should return FPI AND not FPI cookies when domain filtering is enabled with firstpartydomain set to null.

Thanks for the feedback.

Comment 1

[Rob Wu [:robwu]]

Attachment: fpdcookies.zip

Thanks for this report. This is not behaving as intended.

Automated test case for QA.

1. Load the extension in a new Firefox profile (it is going to clear existing cookies and flip the FPI prefs)
2. Look at the output of the opened tab.

The results are shown below.

The lines marked with ">" are incorrect. We should see:
    {domain: example.com, firstPartyDomain: example.com}
    {domain: example.com, firstPartyDomain: }
but we get this instead:
    {domain: example.com, firstPartyDomain: }



    Cleaning up cookies for a clean state

    Disabling FPI and setting cookie

    Enabling FPI and setting cookie

    Cookies while FPI is enabled

    Result for {firstPartyDomain: null} is
    {domain: example.com, firstPartyDomain: example.com}
    {domain: example.com, firstPartyDomain: }

    Error for {domain: example.com}
    Error: First-Party Isolation is enabled, but the required 'firstPartyDomain' attribute was not set.

    Result for {domain: example.com, firstPartyDomain: example.com} is
    {domain: example.com, firstPartyDomain: example.com}

    Result for {domain: example.com, firstPartyDomain: null} is
>   {domain: example.com, firstPartyDomain: }

    Result for {domain: example.com, firstPartyDomain: undefined} is
>   {domain: example.com, firstPartyDomain: }

    Cookies while FPI is disabled

    Result for {firstPartyDomain: null} is
    {domain: example.com, firstPartyDomain: example.com}
    {domain: example.com, firstPartyDomain: }

    Result for {domain: example.com} is
    {domain: example.com, firstPartyDomain: }

    Result for {domain: example.com, firstPartyDomain: example.com} is
    {domain: example.com, firstPartyDomain: example.com}

    Result for {domain: example.com, firstPartyDomain: null} is
>   {domain: example.com, firstPartyDomain: }

    Result for {domain: example.com, firstPartyDomain: undefined} is
>   {domain: example.com, firstPartyDomain: }

    Finished tests. Now check the results.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1465063 - Add await before assertRejects

Wait for the promise resolution before moving to the next line in the tests.

Review commit: https://reviewboard.mozilla.org/r/250276/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250276/

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1465063 - Add tests for get/getAll w/o firstPartyDomain

The test coverage for cookies.get was incomplete, as scenarios
with empty, null and undefined firstPartyDomain were not checked.

There is a distinction between "empty" and "w/o firstPartyDomain".

Review commit: https://reviewboard.mozilla.org/r/250278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250278/

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1465063 - Return FP cookies from cookies.getAll even if domain/url is set

`browser.cookies.getAll(details)` is expected to return all cookies if the
"details.firstPartyDomain" key is set to undefined/null.
This usually works, because `getCookiesWithOriginAttributes` is used,
which iterates over all cookies and only checks origin attributes that
exist in the specified pattern.

If a "url" or "domain" are set, then `getCookiesFromHost` was used,
which uses a look-up table keyed by host and OriginAttributes, with
undefined origin attributes initialized at their default value.
Consequently, only non-FP cookies were returned.

This commit fixes the issue by using `getCookiesWithOriginAttributes`
when the `firstPartyDomain` does not have an explicit (string) value.

Review commit: https://reviewboard.mozilla.org/r/250280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250280/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984430 [details]
Bug 1465063 - Add await before assertRejects

https://reviewboard.mozilla.org/r/250276/#review256572

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984431 [details]
Bug 1465063 - Add tests for get/getAll w/o firstPartyDomain

https://reviewboard.mozilla.org/r/250278/#review256578

We should also double-check the related doc section on MDN: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/cookies#First-party_isolation

e.g. it doesn't seem to explicitly mention which is the behaviors expected by cookies.getAll when firstPartyDomain is undefined and when it is an empty string, and also when this API is expected to reject.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984432 [details]
Bug 1465063 - Return FP cookies from cookies.getAll even if domain/url is set

https://reviewboard.mozilla.org/r/250280/#review256584

This looks good to me, follows some comments, but the most important ones are:

- the `domain` property missing from the getAll tests with FPI on, 
- test explicitly the behaviors with the `url` property (instead of the `domain` property) in the getAll API calls (with FPI off and on).

I'm clearing the review in the mean time, mostly because of the missing `domain` property in the getAll tests with FPI on, but besides that this looks almost ready.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:86
(Diff revision 1)
>      assertExpectedCookies([null], [cookie], "get: FPI off, w/ null firstPartyDomain, no cookie");
>      cookie = await browser.cookies.get({url, name: "foo2", firstPartyDomain: undefined});
>      assertExpectedCookies([null], [cookie], "get: FPI off, w/ undefined firstPartyDomain, no cookie");
>  
>      // getAll
> -    cookies = await browser.cookies.getAll({});
> +    for (let domain of [undefined, firstPartyDomain]) {

We should also verify the behaviors with `url` instead of `domain` in the API calls options, based on the current implementation I would expect it to behave exactly like `domain`, but it would be better to explicitly verify it for completeness.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:91
(Diff revision 1)
> -    cookies = await browser.cookies.getAll({});
> +    for (let domain of [undefined, firstPartyDomain]) {
> +      const prefix = domain ? "getAll with domain" : "getAll";
> +      cookies = await browser.cookies.getAll({domain});
> -    assertExpectedCookies([
> +      assertExpectedCookies([
> -      {name: "foo1", value: "bar1", firstPartyDomain: ""},
> +        {name: "foo1", value: "bar1", firstPartyDomain: ""},
> -    ], cookies, "getAll: FPI off, w/ empty firstPartyDomain, non-FP cookies");
> +      ], cookies, prefix + ": FPI off, w/ empty firstPartyDomain, non-FP cookies");

Nit, do you mind to change the "string compositions" into "template strings"?

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:97
(Diff revision 1)
> -    cookies = await browser.cookies.getAll({firstPartyDomain: null});
> +      cookies = await browser.cookies.getAll({domain, firstPartyDomain: null});
> -    assertExpectedCookies([
> +      assertExpectedCookies([
> -      {name: "foo1", value: "bar1", firstPartyDomain: ""},
> +        {name: "foo1", value: "bar1", firstPartyDomain: ""},
> -      {name: "foo2", value: "bar2", firstPartyDomain},
> +        {name: "foo2", value: "bar2", firstPartyDomain},
> -    ], cookies, "getAll: FPI off, w/ null firstPartyDomain, all cookies");
> -    cookies = await browser.cookies.getAll({firstPartyDomain: undefined});
> +      ], cookies, prefix + ": FPI off, w/ null firstPartyDomain, all cookies");
> +      cookies = await browser.cookies.getAll({domain, firstPartyDomain: undefined});

Let's also add the test case for firstPartyDomain: "" with FPI off, mostly for completeness.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:169
(Diff revision 1)
>  
>      // getAll
> +    for (let domain of [undefined, firstPartyDomain]) {
> +      const prefix = domain ? "getAll with domain" : "getAll";
> -    await browser.test.assertRejects(
> +      await browser.test.assertRejects(
> -      browser.cookies.getAll({}),
> +        browser.cookies.getAll({}),

`domain` seems to be currently missing from the options in this subset of the getAll tests with FPI on.

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 8984431 [details]
Bug 1465063 - Add tests for get/getAll w/o firstPartyDomain

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250278/diff/1-2/

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 8984432 [details]
Bug 1465063 - Return FP cookies from cookies.getAll even if domain/url is set

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250280/diff/1-2/

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 8984432 [details]
Bug 1465063 - Return FP cookies from cookies.getAll even if domain/url is set

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250280/diff/2-3/

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8984432 [details]
Bug 1465063 - Return FP cookies from cookies.getAll even if domain/url is set

https://reviewboard.mozilla.org/r/250280/#review256610

Just a small fix to one of the messages logged by these test cases, besides that this looks good to me.

You may want to push it to try by also including "test-verify-e10s", just to double check that the test file modified in these patches still passes the "--verify" test mode.

r=me on green try (and the logged message mentioned below fixed).

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:91
(Diff revisions 2 - 3)
> -    for (let domain of [undefined, firstPartyDomain]) {
> -      const prefix = domain ? "getAll with domain" : "getAll";
> -      cookies = await browser.cookies.getAll({domain});
> +    for (let extra of [{}, {url}, {domain: firstPartyDomain}]) {
> +      const prefix = `getAll(${JSON.stringify(extra)})`;
> +      cookies = await browser.cookies.getAll({...extra});
>        assertExpectedCookies([
>          {name: "foo1", value: "bar1", firstPartyDomain: ""},
> -      ], cookies, prefix + ": FPI off, w/ empty firstPartyDomain, non-FP cookies");
> +      ], cookies, `${prefix}: FPI off, w/ empty firstPartyDomain, non-FP cookies`);

it looks that the message should say "w/o firstPartyDomain" here, the one for "w/ empty firstPartyDomain" should be the one at line 95.

Comment 12

[Rob Wu [:robwu]]

Comment on attachment 8984431 [details]
Bug 1465063 - Add tests for get/getAll w/o firstPartyDomain

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250278/diff/2-3/

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 8984432 [details]
Bug 1465063 - Return FP cookies from cookies.getAll even if domain/url is set

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250280/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3e5e1651b49
Add await before assertRejects r=rpl
https://hg.mozilla.org/integration/autoland/rev/dccf43cba794
Add tests for get/getAll w/o firstPartyDomain r=rpl
https://hg.mozilla.org/integration/autoland/rev/8d32de537989
Return FP cookies from cookies.getAll even if domain/url is set r=rpl

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b3e5e1651b49
https://hg.mozilla.org/mozilla-central/rev/dccf43cba794
https://hg.mozilla.org/mozilla-central/rev/8d32de537989