Closed Bug 1465063 Opened Last year Closed Last year

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

Categories

(WebExtensions :: General, defect)

60 Branch
x86_64
All
defect
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: ysard_git, Assigned: robwu)

Details

Attachments

(4 files)

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.
OS: Unspecified → All
Hardware: Unspecified → x86_64
Flags: needinfo?(rob)
Attached file 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.
Assignee: nobody → rob
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(rob)
Comment on attachment 8984430 [details]
Bug 1465063 - Add await before assertRejects

https://reviewboard.mozilla.org/r/250276/#review256572
Attachment #8984430 - Flags: review?(lgreco) → review+
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.
Attachment #8984431 - Flags: review?(lgreco) → review+
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.
Attachment #8984432 - Flags: review?(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.
Attachment #8984432 - Flags: review?(lgreco) → review+
Pushed by rob@robwu.nl:
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
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.