Closed Bug 1465063 Opened 6 years ago Closed 6 years ago

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

Tracking

(firefox62 fixed)

Status:

RESOLVED FIXED

Milestone:

mozilla62

Tracking Flags:

Tracking

Status

firefox62

---

fixed

People

(Reporter: ysard_git, Assigned: robwu)

Details

Attachments

(4 files)

fpdcookies.zip 6 years ago Rob Wu [:robwu] 1.85 KB, application/zip		Details
Bug 1465063 - Add await before assertRejects 6 years ago Rob Wu [:robwu] 59 bytes, text/x-review-board-request	rpl : review+	Details
Bug 1465063 - Add tests for get/getAll w/o firstPartyDomain 6 years ago Rob Wu [:robwu] 59 bytes, text/x-review-board-request	rpl : review+	Details
Bug 1465063 - Return FP cookies from cookies.getAll even if domain/url is set 6 years ago Rob Wu [:robwu] 59 bytes, text/x-review-board-request	rpl : review+	Details

ysard

Reporter

Description

•

6 years ago

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.

ysard

Reporter

Updated

•

6 years ago

OS: Unspecified → All

Hardware: Unspecified → x86_64

David Durst [:ddurst]

Updated

•

6 years ago

Flags: needinfo?(rob)

Rob Wu [:robwu]

Assignee

Comment 1

•

6 years ago

Attached file fpdcookies.zip — Details

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

`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/

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

Comment 5

•

6 years ago

mozreview-review

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

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

Attachment #8984430 - Flags: review?(lgreco) → review+

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

Comment 6

•

6 years ago

mozreview-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+

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

Comment 7

•

6 years ago

mozreview-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 hidden (mozreview-request)

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 hidden (mozreview-request)

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 hidden (mozreview-request)

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/

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

Comment 11

•

6 years ago

mozreview-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/#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+

Comment hidden (mozreview-request)

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 hidden (mozreview-request)

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/

Pulsebot

Comment 14

•

6 years ago

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

Bogdan Tara[:bogdan_tara | bogdant]

Comment 15

•

6 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox62: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla62

marius.santa

Updated

•

6 years ago

Flags: qe-verify-

BMO Automation

Updated

•

6 years ago

Product: Toolkit → WebExtensions

You need to log in before you can comment on or make changes to this bug.