browser.cookies fails to get/remove cookies by domain/url when privacy.firstparty.isolate = true

RESOLVED FIXED in Firefox 59

Status

()

Toolkit
WebExtensions: General
P3
normal
RESOLVED FIXED
6 months ago
6 days ago

People

(Reporter: Kestrel, Assigned: cfu)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla59
dev-doc-needed
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fix-optional, firefox59 fixed)

Details

(Whiteboard: [OA])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

6 months ago
STR:

1. Set privacy.firstparty.isolate = true.
2. Visit "example.com" to create new cookies with first party isolation.
3. browser.cookies.getAll({}) correctly returns all cookies for domain.
4. browser.cookies.getAll({domain: "example.com"}) returns zero cookies.

Seems like Services.cookies.getCookiesFromHost() might need originAttributes.firstPartyDomain.
(Reporter)

Updated

6 months ago
Blocks: 1299996
(Reporter)

Comment 1

6 months ago
cookies.remove also fails to remove cookies and cookies.set saves cookies without firstPartyDomain origin attribute.
Summary: browser.cookies.getAll fails to find domain/url cookies when privacy.firstparty.isolate = true → browser.cookies fails to get/remove cookies by domain/url when privacy.firstparty.isolate = true

Updated

6 months ago
Assignee: nobody → ettseng

Updated

6 months ago
Priority: -- → P3

Updated

6 months ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [fingerprinting][fp:backlog]

Comment 2

5 months ago
This bug even happens with privacy.firstparty.isolate = false for safebrowsing cookies (but this cookie should be hidden from the API according to bug 1362834).

STR, to emulate the implementation of chrome.cookies.getAll (https://searchfox.org/mozilla-central/rev/765cc1b925c5d32d05111c364257a0b79bf2952e/toolkit/components/extensions/ext-cookies.js#190-192):
1. Start new Firefox profile, enable add-on debugging to enable Chrome debugging.
2. Clear all cookies.
3. Wait a few minutes until Firefox sends a request to Google's Safebrowsing service.
4. Run the following code in the global JS console (Ctrl-Shift-J):

enumm = Services.cookies.getCookiesWithOriginAttributes('{"userContextId":0}');
while (enumm.hasMoreElements()) {
  cook = enumm.getNext().QueryInterface(Ci.nsICookie2);
  console.log(cook.host + ' ' + cook.name + ' ' + cook.originAttributes.firstPartyDomain);
}

// Result:
.google.com NID  
.google.com NID safebrowsing.66a2a99e-7a96-11e7-933e-a71bbf6a2b3e.mozilla




STR, to emulate the behavior of chrome.cookies.set/get/remove (https://searchfox.org/mozilla-central/rev/765cc1b925c5d32d05111c364257a0b79bf2952e/toolkit/components/extensions/ext-cookies.js#176-189):
(continuing from the previous test case)
5. Run the following code in the global JS console:

enumm = Services.cookies.getCookiesFromHost('google.com', {"userContextId": 0});
while (enumm.hasMoreElements()) {
  cook = enumm.getNext().QueryInterface(Ci.nsICookie2);
  console.log(cook.host + ' ' + cook.name + ' ' + cook.originAttributes.firstPartyDomain);
}

// Result:
.google.com NID 
(note: The cookie with safebrowsing.66a2a99e-7a96-11e7-933e-a71bbf6a2b3e is missing)

Updated

5 months ago
See Also: → bug 1362834

Updated

5 months ago
Whiteboard: [fingerprinting][fp:backlog] → [OA]

Comment 3

5 months ago
This bug already blocks bug 1299996, but I'd like to add an explicit note too.

In the TOR browser, the privacy.firstparty.isolate preference is set to true by default.
Also, in the TOR browser, private browsing mode is enabled by default.

As a result, no WebExtension-based extension can edit cookies in the TOR browser because of this bug.

Comment 4

4 months ago
Is this fix-optional for FF 57?  Ethan, please triage appropriately.  Thanks!
Flags: needinfo?(ettseng)

Comment 5

4 months ago
For the record, I wrote down the implementation plan at https://bugzilla.mozilla.org/show_bug.cgi?id=1362834#c30

The discussion happened on that other bug, but the outcome would actually solve this bug, and not "fix" the problem as stated in the summary of the other bug, i.e. "prevent browser.cookies APIs from interacting with internal cookies". On the contrary, with my proposal the "internal" cookies would be exposed (IMO a good thing, because add-ons can then delete Google's NID cookie).

Updated

4 months ago
status-firefox57: --- → fix-optional
Flags: needinfo?(ettseng)
Copying Rob's comment #30 from bug 1362834 because I am going to mark that bug as wontfix and I don't want his comment to get lost...

(In reply to Tom Ritter [:tjr] from comment #29)
> Hey Rob, what would be the mechanism to move this forward? (At least to the
> point of "Well we know what we want to do, even if it's going to wait to be
> implemented.) Should we have a brainstorm session realtime or something?

Depends on the use cases that we want to support.
- At the very least, a cookie manager should be able to operate correctly when FPI is enabled.
- A nice-to-have is to allow add-ons to assist users in migrating cookies from non-FPI to FPI and vice versa.

I think that all cookie APIs (cookies.get/getAll/remove/set methods, cookies.Cookie type) should be extended with the firstPartyDomain attribute.

When FPI is disabled (default):
- The firstPartyDomain attribute is optional in get/remove/set, defaulting to an empty string.
- Consequently, cookies with a non-empty firstPartyDomain are not shown by default.
- The behavior for getAll is:
  * If firstPartyDomain is not set, default to empty string.
  * If firstPartyDomain is set to a string, use it.
  * If firstPartyDomain is set to null or undefined, do not filter the list of cookies by firstPartyDomain value.
  (this can be implemented by declaring "optional": "omit-key-if-missing" in extensions/schemas/cookies.json).

When FPI is enabled:
- The firstPartyDomain attribute is mandatory in get/remove/set and should be set to a string.
- The firstPartyDomain attribute in getAll should be a string or null/undefined (it cannot be unset).
- Cookies without a firstPartyDomain should have an empty string as a value for firstPartyDomain.
- Optional: cookies.set rejects the request to create new cookies without a non-empty firstPartyDomain value.

Implementation notes:
- The cookies.json schema should declare firstPartyDomain as optional for the methods.
- The implementation in ext-cookies.js must check the firstPartyDomain value according to the FPI setting (with the above semantics).
- getAll in cookies.json must use "optional": "omit-key-if-missing" instead of "optional":true, to allow the implementation to distinguish a set value and an unset value (otherwise the schema validator will fill in null (or another default value) when the value is not set).


I have designed the API in this way, because:
- Extensions without explicit support for FPI won't see FP cookies.
- Extensions without explicit support for FPI won't work when the browser has enabled FPI.
- Extensions are able to support both FP and FPI cookies at the same time if they want to.
(Assignee)

Comment 7

3 months ago
Hi Rob,

Thank you very much for the great design.  I'm implementing it and have some questions.

(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> Copying Rob's comment #30 from bug 1362834 because I am going to mark that
> bug as wontfix and I don't want his comment to get lost...
> 
> (In reply to Tom Ritter [:tjr] from comment #29)
> > Hey Rob, what would be the mechanism to move this forward? (At least to the
> > point of "Well we know what we want to do, even if it's going to wait to be
> > implemented.) Should we have a brainstorm session realtime or something?
> 
> Depends on the use cases that we want to support.
> - At the very least, a cookie manager should be able to operate correctly
> when FPI is enabled.
> - A nice-to-have is to allow add-ons to assist users in migrating cookies
> from non-FPI to FPI and vice versa.
> 
> I think that all cookie APIs (cookies.get/getAll/remove/set methods,
> cookies.Cookie type) should be extended with the firstPartyDomain attribute.
> 
> When FPI is disabled (default):
> - The firstPartyDomain attribute is optional in get/remove/set, defaulting
> to an empty string.
> - Consequently, cookies with a non-empty firstPartyDomain are not shown by
> default.
> - The behavior for getAll is:
>   * If firstPartyDomain is not set, default to empty string.
>   * If firstPartyDomain is set to a string, use it.
>   * If firstPartyDomain is set to null or undefined, do not filter the list
> of cookies by firstPartyDomain value.
>   (this can be implemented by declaring "optional": "omit-key-if-missing" in
> extensions/schemas/cookies.json).

Does "firstPartyDomain is set or not" mean the caller assign it a value or not?  For example,
- browser.cookies.getAll({});                                // not set
- browser.cookies.getAll({firstPartyDomain: "example.com"}); // set

If this understanding is correct, then the "not set" case:
- if firstPartyDomain is declared as "optional": true,                  details.firstPartyDomain will be null
- if firstPartyDomain is declared as "optional": "omit-key-if-missing", details.firstPartyDomain will be undefined ("firstPartyDomain" is not in details)

So I'm wondering if the semantic should be

    if (details.firstPartyDomain == null) {
      /* do not filter the list of cookies */
    } else {
      /* use it */
    }

> When FPI is enabled:
> - The firstPartyDomain attribute is mandatory in get/remove/set and should
> be set to a string.

Should these functions return rejected promises when details.firstPartyDomain is null?

> - The firstPartyDomain attribute in getAll should be a string or
> null/undefined (it cannot be unset).

Similar to above, should getAll return a rejected promise when details.firstPartyDomain is undefined (the unset case)?

> - Cookies without a firstPartyDomain should have an empty string as a value
> for firstPartyDomain.
> - Optional: cookies.set rejects the request to create new cookies without a
> non-empty firstPartyDomain value.

How about get and remove?

> Implementation notes:
> - The cookies.json schema should declare firstPartyDomain as optional for
> the methods.
> - The implementation in ext-cookies.js must check the firstPartyDomain value
> according to the FPI setting (with the above semantics).
> - getAll in cookies.json must use "optional": "omit-key-if-missing" instead
> of "optional":true, to allow the implementation to distinguish a set value
> and an unset value (otherwise the schema validator will fill in null (or
> another default value) when the value is not set).
> 
> 
> I have designed the API in this way, because:
> - Extensions without explicit support for FPI won't see FP cookies.
> - Extensions without explicit support for FPI won't work when the browser
> has enabled FPI.
> - Extensions are able to support both FP and FPI cookies at the same time if
> they want to.

If I understand correctly, browser.cookies.getAll({domain: "example.com"}) still cannot get cookies with firstPartyDomain unless it is explicitly assign firstPartyDomain like brower.cookies.getAll({domain: "example.com", firstPartyDomain: "example.com"}), right?

Thanks again!
Flags: needinfo?(rob)

Comment 8

3 months ago
(In reply to Chung-Sheng Fu [:cfu] from comment #7)
> Hi Rob,
> 
> Thank you very much for the great design.  I'm implementing it and have some
> questions.
> 
> (In reply to Bob Silverberg [:bsilverberg] from comment #6)
> > Copying Rob's comment #30 from bug 1362834 because I am going to mark that
> > bug as wontfix and I don't want his comment to get lost...
> > 
> > (In reply to Tom Ritter [:tjr] from comment #29)
> > > Hey Rob, what would be the mechanism to move this forward? (At least to the
> > > point of "Well we know what we want to do, even if it's going to wait to be
> > > implemented.) Should we have a brainstorm session realtime or something?
> > 
> > Depends on the use cases that we want to support.
> > - At the very least, a cookie manager should be able to operate correctly
> > when FPI is enabled.
> > - A nice-to-have is to allow add-ons to assist users in migrating cookies
> > from non-FPI to FPI and vice versa.
> > 
> > I think that all cookie APIs (cookies.get/getAll/remove/set methods,
> > cookies.Cookie type) should be extended with the firstPartyDomain attribute.
> > 
> > When FPI is disabled (default):
> > - The firstPartyDomain attribute is optional in get/remove/set, defaulting
> > to an empty string.
> > - Consequently, cookies with a non-empty firstPartyDomain are not shown by
> > default.
> > - The behavior for getAll is:
> >   * If firstPartyDomain is not set, default to empty string.
> >   * If firstPartyDomain is set to a string, use it.
> >   * If firstPartyDomain is set to null or undefined, do not filter the list
> > of cookies by firstPartyDomain value.
> >   (this can be implemented by declaring "optional": "omit-key-if-missing" in
> > extensions/schemas/cookies.json).
> 
> Does "firstPartyDomain is set or not" mean the caller assign it a value or
> not?  For example,
> - browser.cookies.getAll({});                                // not set
> - browser.cookies.getAll({firstPartyDomain: "example.com"}); // set
> 
> If this understanding is correct, then the "not set" case:
> - if firstPartyDomain is declared as "optional": true,                 
> details.firstPartyDomain will be null
> - if firstPartyDomain is declared as "optional": "omit-key-if-missing",
> details.firstPartyDomain will be undefined ("firstPartyDomain" is not in
> details)
> 
> So I'm wondering if the semantic should be
> 
>     if (details.firstPartyDomain == null) {
>       /* do not filter the list of cookies */
>     } else {
>       /* use it */
>     }

Yes, your understanding is correct.


> > When FPI is enabled:
> > - The firstPartyDomain attribute is mandatory in get/remove/set and should
> > be set to a string.
> 
> Should these functions return rejected promises when
> details.firstPartyDomain is null?

No, because rejecting null would prevent the use case of migrating cookies to a FPI-enabled browser.


> > - The firstPartyDomain attribute in getAll should be a string or
> > null/undefined (it cannot be unset).
> 
> Similar to above, should getAll return a rejected promise when
> details.firstPartyDomain is undefined (the unset case)?

Note that there is a difference between "unset" and "value is undefined"
('firstPartyDomain' in details) is true iff the property is set (provided that the "omit-key-if-missing" schema flag is set).
I specified the difference because I believed that an extension without explicit support for FPI should not be able to access cookies through the API.

For example:

let firstPartyDomain = details.firstPartyDomain;
if (!("firstPartyDomain" in details)) {
  if (some_function_that_returns_true_if_fpi_is_enabled()) {
    return Promise.reject({ message: "FPI is enabled, but 'firstPartyDomain' was not provided" });
  }
  firstPartyDomain = ""; // Default to "" (because without FPI, cookies have the "" FPD value).
}

> > - Cookies without a firstPartyDomain should have an empty string as a value
> > for firstPartyDomain.
> > - Optional: cookies.set rejects the request to create new cookies without a
> > non-empty firstPartyDomain value.
> 
> How about get and remove?

It should always be possible to read/write cookies, even if created without a FP. So don't reject it.
Actually, do not implement this optional suggestion, because that prevents non-FP cookies from being deleted when remove does not work (bug 1388873 / https://github.com/Rob--W/cookie-manager/commit/d2f7bf8266cb815dd0e03088b9c467462c450e77).



> > Implementation notes:
> > - The cookies.json schema should declare firstPartyDomain as optional for
> > the methods.
> > - The implementation in ext-cookies.js must check the firstPartyDomain value
> > according to the FPI setting (with the above semantics).
> > - getAll in cookies.json must use "optional": "omit-key-if-missing" instead
> > of "optional":true, to allow the implementation to distinguish a set value
> > and an unset value (otherwise the schema validator will fill in null (or
> > another default value) when the value is not set).
> > 
> > 
> > I have designed the API in this way, because:
> > - Extensions without explicit support for FPI won't see FP cookies.
> > - Extensions without explicit support for FPI won't work when the browser
> > has enabled FPI.
> > - Extensions are able to support both FP and FPI cookies at the same time if
> > they want to.
> 
> If I understand correctly, browser.cookies.getAll({domain: "example.com"})
> still cannot get cookies with firstPartyDomain unless it is explicitly
> assign firstPartyDomain like brower.cookies.getAll({domain: "example.com",
> firstPartyDomain: "example.com"}), right?

Yes. When FPI is enabled, the extension needs to be explicit about the context they're working on.
Flags: needinfo?(rob)

Updated

3 months ago
Assignee: ettseng → cfu
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 months ago
Here is how it works:

         | w/o firstPartyDomain   | w/ firstPartyDomain
---------+------------------------+------------------------
 FPI off | set: non-FP cookie     | set: can set FP cookie
         | get: non-FP cookie     |      but return null
         | getAll: non-FP cookies | get: null
         | remove: non-FP cookie  | getAll: empty array
         |                        | remove: null
---------+------------------------+------------------------
 FPI on  | set: non-FP cookie     | set: FP cookie
         | get: non-FP cookie     | get: FP cookie
         | getAll: rejected       | getAll: FP cookies
         | remove: non-FP cookie  | remove: FP cookie

FPI off w/o firstPartyDomain and FPI on w/ firstPartyDomain are simple.

When FPI is off, set with firstPartyDomain can set a FP cookie, but it calls get at the end of the function to get the newly added cookie and cookies with non-empty firstPartyDomain should not be shown, so it will return null.

When FPI is on, set/get/remove can still access non-FP cookies without firstPartyDomain.

Besides, non-FP and FP cookies set when FPI is off can be access when FPI is on.  Also, non-FP cookies set when FPI is on can be access when FPI is off.

Could you please help review the patches and verify if the behaviors are expected?  Thank you very much!
(Assignee)

Updated

2 months ago
Attachment #8925815 - Flags: review?(rob)
Attachment #8925816 - Flags: review?(rob)

Comment 12

2 months ago
(In reply to Chung-Sheng Fu [:cfu] from comment #11)
> Here is how it works:
> 
>          | w/o firstPartyDomain   | w/ firstPartyDomain
> ---------+------------------------+------------------------
>  FPI off | set: non-FP cookie     |#set: can set FP cookie
>          | get: non-FP cookie     |      but return null
>          | getAll: non-FP cookies |#get: null
>          | remove: non-FP cookie  |#getAll: empty array
>          |                        |#remove: null
> ---------+------------------------+------------------------
>  FPI on  |#set: non-FP cookie     | set: FP cookie
>          |#get: non-FP cookie     | get: FP cookie
>          | getAll: rejected       | getAll: FP cookies
>          |#remove: non-FP cookie  | remove: FP cookie

I haven't looked at the patched in detail (nit: line 180 of ext-cookies.js is missing a trailing comma), because your table does not exhibit the behavior that I specified (I marked the differences with #) (thanks for the table by the way, it makes pre-review much easier).

Non-FP cookies do not make sense when FPI is enabled, hence I said:

> When FPI is enabled:
> - The firstPartyDomain attribute is mandatory in get/remove/set and should be set to a string.

So the FPI on + w/o firsyPartyDomain combination should all result in a rejection.


I also wrote:

> I have designed the API in this way, because:
> - Extensions without explicit support for FPI won't see FP cookies.
> - Extensions without explicit support for FPI won't work when the browser has enabled FPI.
> - Extensions are able to support both FP and FPI cookies at the same time if they want to.

so the FPI off + w/ firstPartyDomain combination should also return FP cookies (opposed to nothing), in case an extension wants to migrate cookies back. Another use case is to be able to read/write safebrowsing cookies (bug 1362834).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (typo)
(Assignee)

Comment 16

2 months ago
Thank you very much for clarifying this!  Now it works like:

         | w/o firstPartyDomain   | w/ firstPartyDomain
---------+------------------------+---------------------
 FPI off | set: non-FP cookie     | set: FP cookie
         | get: non-FP cookie     | get: FP cookie
         | getAll: non-FP cookies | getAll: FP cookies
         | remove: non-FP cookie  | remove: FP cookie
---------+------------------------+---------------------
 FPI on  | set: rejection         | set: FP cookie
         | get: rejection         | get: FP cookie
         | getAll: rejection      | getAll: FP cookies
         | remove: rejection      | remove: FP cookie

Please take a look again, thanks!

Comment 17

2 months ago
mozreview-review
Comment on attachment 8925815 [details]
Bug 1381197 - browser.cookies APIs support firstPartyDomain.

https://reviewboard.mozilla.org/r/196998/#review202894

Looks good in terms of functionality. Please fix the failing tests from the try run.
Also, to conserve resources, restrict the tests to a few specific platforms: (-p linux64,macosx64,win64,android-api-16 ).

::: toolkit/components/extensions/ext-cookies.js:300
(Diff revision 2)
> -          let allowed = ["url", "name", "domain", "path", "secure", "session", "storeId"];
> +          if (!("firstPartyDomain" in details)) {
> +            if (Services.prefs.getBoolPref("privacy.firstparty.isolate")) {
> +              return Promise.reject({message: "FPI is enabled, but 'firstPartyDomain' was not provided"});
> +            }
> +            details.firstPartyDomain = ""; // Default to "" (because without FPI, cookies have the "" FPD value).
> +          }

The same validation was copied and pasted four times.
I suggest to reduce code duplication by creating a new function that performs the validation:

```
function normalizeFirstPartyDomain(details) {
  if (details.firstPartyDomain == null) {
    if (Services.prefs.getBoolPref("privacy.firstparty.isolate")) {
      throw new ExtensionError("First-Party Isolation is enabled, but the required 'firstPartyDomain' attribute was not set.");
    }

    // When FPI is disabled, the "firstPartyDomain" attribute is optional
    // and defaults to the empty string.
    details.firstPartyDomain = "";
  }
}
```

(`ExtensionError` above needs to be imported from `ExtensionUtils` before you can use it)


and then in every function, replace your current `if`-check with `normalizeFirstPartyDomain(details);`

In `getAll`, use

```
if (!("firstPartyDomain" in details)) {
   normalizeFirstPartyDomain(details);
}
// firstPartyDomain may be set to null or undefined to not filter by FPD.
```

::: toolkit/components/extensions/schemas/cookies.json:40
(Diff revision 2)
>            "secure": {"type": "boolean", "description": "True if the cookie is marked as Secure (i.e. its scope is limited to secure channels, typically HTTPS)."},
>            "httpOnly": {"type": "boolean", "description": "True if the cookie is marked as HttpOnly (i.e. the cookie is inaccessible to client-side scripts)."},
>            "session": {"type": "boolean", "description": "True if the cookie is a session cookie, as opposed to a persistent cookie with an expiration date."},
>            "expirationDate": {"type": "number", "optional": true, "description": "The expiration date of the cookie as the number of seconds since the UNIX epoch. Not provided for session cookies."},
> -          "storeId": {"type": "string", "description": "The ID of the cookie store containing this cookie, as provided in getAllCookieStores()."}
> +          "storeId": {"type": "string", "description": "The ID of the cookie store containing this cookie, as provided in getAllCookieStores()."},
> +          "firstPartyDomain": {"type": "string", "optional": true, "description": "The first-party domain of the cookie."}

Remove `"optional": true`. This property is always set (to an empty string if the cookie is a non-FPI cookie).

::: toolkit/components/extensions/schemas/cookies.json:75
(Diff revision 2)
>              "description": "Details to identify the cookie being retrieved.",
>              "properties": {
>                "url": {"type": "string", "description": "The URL with which the cookie to retrieve is associated. This argument may be a full URL, in which case any data following the URL path (e.g. the query string) is simply ignored. If host permissions for this URL are not specified in the manifest file, the API call will fail."},
>                "name": {"type": "string", "description": "The name of the cookie to retrieve."},
> -              "storeId": {"type": "string", "optional": true, "description": "The ID of the cookie store in which to look for the cookie. By default, the current execution context's cookie store will be used."}
> +              "storeId": {"type": "string", "optional": true, "description": "The ID of the cookie store in which to look for the cookie. By default, the current execution context's cookie store will be used."},
> +              "firstPartyDomain": {"type": "string", "optional": true, "description": "The first-party domain which the cookie to retrieve is associated."}

Add: "This attribute is required if First-Party Isolation is enabled." (also to `set` and `remove`).

::: toolkit/components/extensions/schemas/cookies.json:107
(Diff revision 2)
>                "domain": {"type": "string", "optional": true, "description": "Restricts the retrieved cookies to those whose domains match or are subdomains of this one."},
>                "path": {"type": "string", "optional": true, "description": "Restricts the retrieved cookies to those whose path exactly matches this string."},
>                "secure": {"type": "boolean", "optional": true, "description": "Filters the cookies by their Secure property."},
>                "session": {"type": "boolean", "optional": true, "description": "Filters out session vs. persistent cookies."},
> -              "storeId": {"type": "string", "optional": true, "description": "The cookie store to retrieve cookies from. If omitted, the current execution context's cookie store will be used."}
> +              "storeId": {"type": "string", "optional": true, "description": "The cookie store to retrieve cookies from. If omitted, the current execution context's cookie store will be used."},
> +              "firstPartyDomain": {"type": "string", "optional": "omit-key-if-missing", "description": "Restricts the retrieved cookies to those whose first-party domain match or are subdomains of this one."}

Add: "This attribute is required if First-Party Isolation is enabled. To not filter by a specific first-party domain, use `null` or `undefined`."
Attachment #8925815 - Flags: review?(rob)

Comment 18

2 months ago
mozreview-review
Comment on attachment 8925816 [details]
Bug 1381197 - Add tests for browser.cookies APIs with first-party isolation.

https://reviewboard.mozilla.org/r/197000/#review202912

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:12
(Diff revision 2)
> +<script>
> +const tests = [{
> +  firstPartyIsolation: false,
> +  background: async () => {
> +    const url = "http://example.org/";
> +    const firstPartyDomain = "example.org";

These constants are declared multiple times in local scopes, but the constants must actually be identical in all tests.

Move these two constants to the global scope to show that relationship.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:28
(Diff revision 2)
> +    cookie = await browser.cookies.getAll({});
> +    browser.test.log(JSON.stringify(cookie));
> +    browser.test.assertEq(1, cookie.length, "FPI disabled, getAll without firstPartyDomain");
> +    cookie = await browser.cookies.getAll({firstPartyDomain});
> +    browser.test.log(JSON.stringify(cookie));
> +    browser.test.assertEq(1, cookie.length, "FPI disabled, getAll with firstPartyDomain");

I'd like to see a stronger test, where you actually validate that the cookie is the one that you're expecting (this applies to every API call in your test, in particular the `getAll` calls).

Consider creating a helper function that serializes the cookie (e.g. the values of name,value,firstPartyDomain) and then use `browser.test.assertEq`. That also removes the need for using `browser.test.log` to dump a cookie.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:34
(Diff revision 2)
> +    cookie = await browser.cookies.remove({url, name: "foo1"});
> +    browser.test.assertTrue(cookie !== null, "FPI disabled, remove non-FP cookie");
> +    cookie = await browser.cookies.remove({url, name: "foo2", firstPartyDomain});
> +    browser.test.assertTrue(cookie !== null, "FPI disabled, remove FP cookie");
> +
> +    // Test when FPI enabled.

Only after reading the rest of the test, I understood these two lines.

This line is not testing anything, it is just preparing a cookie for use in the next test.

Please expand the comment to make that statement more explicit.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:50
(Diff revision 2)
> +
> +    let cookie;
> +    browser.test.assertRejects(
> +        browser.cookies.set({url, name: "foo3", value: "bar3"}),
> +        kExpectedError,
> +        "FPI enabled, set non-FP cookie");

The description is not explanatory.

Try something like "FPI enabled, setting FP cookie without explicit firstPartyDomain attribute."
Attachment #8925816 - Flags: review?(rob)

Updated

2 months ago
Flags: needinfo?(rob)
(Assignee)

Comment 19

2 months ago
(In reply to Rob Wu [:robwu] from comment #18)
> :::
> toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.
> html:12
> (Diff revision 2)
> > +<script>
> > +const tests = [{
> > +  firstPartyIsolation: false,
> > +  background: async () => {
> > +    const url = "http://example.org/";
> > +    const firstPartyDomain = "example.org";
> 
> These constants are declared multiple times in local scopes, but the
> constants must actually be identical in all tests.
> 
> Move these two constants to the global scope to show that relationship.

Thanks for reviewing.  I'm afraid we can't do this because the background functions seem to be invoked in a different context, so they are not able to access variables outside their function scopes.

I will fix other issues very soon, thank you very much!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8925815 - Flags: review?(rob)
Attachment #8927699 - Flags: review?(rob)
Attachment #8925816 - Flags: review?(rob)
(Assignee)

Updated

2 months ago
Attachment #8925815 - Flags: review?(rob)
(Assignee)

Updated

2 months ago
Attachment #8925816 - Flags: review?(rob)
(Assignee)

Comment 23

2 months ago
The test is totally updated.  It now tests the name, value, and firstPartyDomain of the cookies.  And I rewrote it with another approach, so the extension only has to be loaded once and we don't have to define the constants and functions several times.  Please take a look again, thanks!

Comment 25

2 months ago
mozreview-review
Comment on attachment 8925815 [details]
Bug 1381197 - browser.cookies APIs support firstPartyDomain.

https://reviewboard.mozilla.org/r/196998/#review204064

::: toolkit/components/extensions/ext-cookies.js:12
(Diff revision 3)
>                                    "resource://gre/modules/Services.jsm");
>  
> +var {
> +  ExtensionError,
> +} = ExtensionUtils;
> +

Move this two lines lower, below `/* globals ... */`.

::: toolkit/components/extensions/ext-cookies.js:270
(Diff revision 3)
>      }
>  
> +    if (("firstPartyDomain" in details) &&
> +        (details.firstPartyDomain !== cookie.originAttributes.firstPartyDomain)) {
> +      return false;
> +    }

Remove this check. The enumerator should already have filtered the cookies because of the non-undefined "firstPartyDomain" key in `originAttributes`.
Attachment #8925815 - Flags: review?(rob)

Comment 26

2 months ago
mozreview-review
Comment on attachment 8927699 [details]
Bug 1381197 - Fix failures on existent cookies API tests.

https://reviewboard.mozilla.org/r/198542/#review204074
Attachment #8927699 - Flags: review?(rob) → review+

Comment 27

2 months ago
mozreview-review
Comment on attachment 8925816 [details]
Bug 1381197 - Add tests for browser.cookies APIs with first-party isolation.

https://reviewboard.mozilla.org/r/197000/#review204066

Please take a look at the linting errors (from the try run, or locally with `mach eslint`) and fix them.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:17
(Diff revision 3)
> +  const firstPartyDomain = "ext-cookie-first-party.mochi.test";
> +  const kExpectedError = "First-Party Isolation is enabled, but the required 'firstPartyDomain' attribute was not set.";
> +
> +  const assertExpected = (cookie, expected, message) => {
> +    if (cookie === null) {
> +      browser.test.assertTrue(false, "null - " + message);

Use `browser.test.fail("null - ...")`

Similarly for other uses of `browser.test.assertTrue(false, ...)`

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:20
(Diff revision 3)
> +  const assertExpected = (cookie, expected, message) => {
> +    if (cookie === null) {
> +      browser.test.assertTrue(false, "null - " + message);
> +      return;
> +    }
> +    browser.test.assertTrue(true, JSON.stringify(cookie) + " - " + message);

If for logging only, you could use `browser.test.log` here. However, what is the purpose of this line? It can be removed since you are checking (and logging) the (presumably) important values at the next few lines.

Similarly for other locations where you use `assertTrue(true` for logging.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:51
(Diff revision 3)
> +          expected.pop();
> +          break;
> +        }
> +      }
> +      if (!found) {
> +        browser.test.assertTrue(true, JSON.stringify(cookie) + " is unexpected - " + message);

This line will never trigger a test failure.
Did you mean `.assertTrue(false`, `.assertFalse(true` or just simply `.fail(` ?

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:117
(Diff revision 3)
> +    assertExpectedArray(
> +        cookie, [
> +          {name: "foo1", value: "bar1", firstPartyDomain: ""},
> +          {name: "foo2", value: "bar2", firstPartyDomain},
> +          {name: "foo4", value: "bar4", firstPartyDomain},
> +        ], "getAll: FPI on, w/ null firstPartyDomain, all cookies");

Duplicate these expectations for
`cookie = browser.cookies.getAll({firstPartyDomain: undefined});` to get test coverage for the `undefined` value.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:147
(Diff revision 3)
> +    assertExpected(cookie, {name: "foo4", value: "bar4", firstPartyDomain}, "get: FPI off, w/ firstPartyDomain, FP cookie (set when FPI on)");
> +    cookie = await browser.cookies.remove({url, name: "foo4", firstPartyDomain});
> +    assertExpected(cookie, {url, name: "foo4", firstPartyDomain}, "remove: FPI off, w/ firstPartyDomain, FP cookie (set when FPI on)");
> +
> +    // Clean up.
> +    await browser.cookies.remove({url, name: "foo1"});

Consider adding a check that confirms that `getAll` returns an empty list?
Attachment #8925816 - Flags: review?(rob)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 31

2 months ago
(In reply to Rob Wu [:robwu] from comment #27)
> :::
> toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.
> html:20
> (Diff revision 3)
> > +  const assertExpected = (cookie, expected, message) => {
> > +    if (cookie === null) {
> > +      browser.test.assertTrue(false, "null - " + message);
> > +      return;
> > +    }
> > +    browser.test.assertTrue(true, JSON.stringify(cookie) + " - " + message);
> 
> If for logging only, you could use `browser.test.log` here. However, what is
> the purpose of this line? It can be removed since you are checking (and
> logging) the (presumably) important values at the next few lines.
> 
> Similarly for other locations where you use `assertTrue(true` for logging.

This kind of code are rewritten as

    browser.test.assertTrue(cookie !== null, ...);
    if (cookie === null) {
      return;
    }

I intended to avoid checking the condition twice and the code became weird...
I think it looks more reasonable now :p
Please take a look again, thanks!

Comment 32

2 months ago
mozreview-review
Comment on attachment 8925815 [details]
Bug 1381197 - browser.cookies APIs support firstPartyDomain.

https://reviewboard.mozilla.org/r/196998/#review206520
Attachment #8925815 - Flags: review?(rob) → review+

Comment 33

2 months ago
mozreview-review
Comment on attachment 8925816 [details]
Bug 1381197 - Add tests for browser.cookies APIs with first-party isolation.

https://reviewboard.mozilla.org/r/197000/#review206528

All of the three patches LGTM. You should now trigger another try run to make sure that there are no test or linter issues, and request a final review from a WebExtension peer before landing the patches.
Attachment #8925816 - Flags: review?(rob) → review+
(Assignee)

Updated

2 months ago
Attachment #8925815 - Flags: review?(kmaglione+bmo)
Attachment #8927699 - Flags: review?(kmaglione+bmo)
Attachment #8925816 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 34

2 months ago
Hi kmag,

We would like to extend the browser.cookies APIs with supports of firstPartyDomain, which was proposed by robwu in comment 6.  Here are the expected results

         | w/o firstPartyDomain   | w/ firstPartyDomain
---------+------------------------+---------------------
 FPI off | set: non-FP cookie     | set: FP cookie
         | get: non-FP cookie     | get: FP cookie
         | getAll: non-FP cookies | getAll: FP cookies
         | remove: non-FP cookie  | remove: FP cookie
---------+------------------------+---------------------
 FPI on  | set: rejection         | set: FP cookie
         | get: rejection         | get: FP cookie
         | getAll: rejection      | getAll: FP cookies
         | remove: rejection      | remove: FP cookie

Could you please help review the patches, or recommend someone whom we can ask for help?  Thanks.

Updated

2 months ago
Attachment #8925815 - Flags: review?(kmaglione+bmo) → review?(lgreco)
Attachment #8927699 - Flags: review?(kmaglione+bmo) → review?(lgreco)
Attachment #8925816 - Flags: review?(kmaglione+bmo) → review?(lgreco)

Comment 35

2 months ago
Luca, Kris has a huge backlog. Could you take a look at this set of patches and give a peer review?

Comment 36

a month ago
mozreview-review
Comment on attachment 8925815 [details]
Bug 1381197 - browser.cookies APIs support firstPartyDomain.

https://reviewboard.mozilla.org/r/196998/#review210982

::: toolkit/components/extensions/ext-cookies.js:312
(Diff revision 4)
>  
>          getAll: function(details) {
> +          if (!("firstPartyDomain" in details)) {
> +            normalizeFirstPartyDomain(details);
> +          }
> +          // firstPartyDomain may be set to null or undefined to not filter by FPD.

Nit, this inline comment seems to be related to the if statement at line 315, it would probably better to move it right above it.
Attachment #8925815 - Flags: review?(lgreco) → review+

Comment 37

a month ago
mozreview-review
Comment on attachment 8927699 [details]
Bug 1381197 - Fix failures on existent cookies API tests.

https://reviewboard.mozilla.org/r/198542/#review210990

The applied changes looks good, but it seems that this test file had a couple of existent issues that it would be great if we could fix now while we are touching this test.

r+ 

(r++ if we also verify and fix the issues described :-))

::: commit-message-2f590:1
(Diff revision 2)
> +Bug 1381197 - Fix test failures.

Nit, how about "Fix failures on existent cookies API tests" as a more detailed commit message?

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:90
(Diff revision 2)
>  
>      cookie = await browser.cookies.get({url: TEST_URL, name: "name1"});
>      assertExpected(expected, cookie);
>  
>      let cookies = await browser.cookies.getAll({name: "name1"});
>      browser.test.assertEq(cookies.length, 1, "one cookie found for matching name");

It is unrelated to your changes, but it looks that in this test there are a number of `browser.test.assertEq` API call where the  parameters are in the wrong order (in browser.test.assertEq the first parameter should be the expected value and the second one the actual value).

Do you mind to fix it? (so that the errors raised by this assertions on a failure would not be confusing).

(Same issue seems to be also at line 30, 94, 98, 105, 108, 112, 154, 158, 173, 177, 192, 199)

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:213
(Diff revision 2)
>      browser.test.assertEq(false, cookie.httpOnly, "non-httpOnly cookie set");
>  
>      details = await browser.cookies.remove({url: TEST_URL, name: "name1"});
> -    assertExpected({url: TEST_URL, name: "name1", storeId: STORE_ID}, details);
> +    assertExpected({url: TEST_URL, name: "name1", storeId: STORE_ID, firstPartyDomain: ""}, details);
>  
>      cookie = await browser.cookies.set({url: TEST_URL});

It looks that here at line 213 a new cookie is set, but it doesn't look that it is removed anywhere in this test, and so I guess that if this test file is executed alone (which would run it twice, in oop mode and non-oop mode) or using --verify (which runs the same test multiple times), this cookie would make this test to fail because the assertion at line 94 which queries the cookies by domain would return 2 cookies instead of the expected 1. 

This failure doesn't seem to happen on try (and if the issue is there as it seems, it doesn't seem that it has been added by these changes), do you mind to verify if I'm right (e.g. by try to run locally `./mach mochitest toolkit/components/extensions/test/mochitest/test_ext_cookies.html`) and this test fails when executed like described above? Given that we are fixing this test for the new failures related to the FPI changes, it would be great to also fix the issue if it is there.

(One way to fix this test failure could be to just remove the cookie near line 217, e.g. by adding an additional `await browser.cookies.remove({url: TEST_URL, name: ""});` which would ensure that the cookie is removed before the test exits).
Attachment #8927699 - Flags: review?(lgreco) → review+

Comment 38

a month ago
mozreview-review
Comment on attachment 8925816 [details]
Bug 1381197 - Add tests for browser.cookies APIs with first-party isolation.

https://reviewboard.mozilla.org/r/197000/#review210998

This test could use some small additional tweaks before calling it done.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:12
(Diff revision 4)
> +<script>
> +"use strict";
> +
> +async function background() {
> +  const url = "http://ext-cookie-first-party.mochi.test/";
> +  const firstPartyDomain = "ext-cookie-first-party.mochi.test";

In the JSON schema, in the description of the `firstPartyDomain` property related to the `getAll` API method we are mentioning that:

`Restricts the retrieved cookies to those whose first-party domain match or are subdomains of this one.`

But it seems that we have no test cases which is testing the "subdomains" scenario.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:13
(Diff revision 4)
> +"use strict";
> +
> +async function background() {
> +  const url = "http://ext-cookie-first-party.mochi.test/";
> +  const firstPartyDomain = "ext-cookie-first-party.mochi.test";
> +  const kExpectedError = "First-Party Isolation is enabled, but the required 'firstPartyDomain' attribute was not set.";

Nit, do you mind to rename this from `kExpectedError` into just `expectedError`?

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:26
(Diff revision 4)
> +    Object.keys(expected).forEach((key) => {
> +      browser.test.assertEq(expected[key], cookie[key], key + " - " + message);
> +    });
> +  };
> +
> +  const assertExpectedArray = (cookies, expected, message) => {

It seems that we don't really need to define both `assertExpected` and `assertExpectedArray`, we could just add a check to ensure that the cookie is defined into the `assertExpectedArray` helper, and then use it to assert both a single cookie and an array of cookies (e.g. using something like `assertExpectedArray([cookie], [expected], message)`).

We should also change it so that expected is the first parameter (like for `browser.test.assertEq` which is used inside the helper function).

It could be also makes sense to rename this test helper to `assertExpectedCookies`.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:35
(Diff revision 4)
> +          return false;
> +        }
> +      }
> +      return true;
> +    };
> +    browser.test.assertEq(expected.length, cookies.length, JSON.stringify(cookies) + " - " + message);

uhm... I don't really like this `JSON.stringify(cookies)` as part of the assertion message, how about something like `${message} - got the expected number of cookies`?

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:39
(Diff revision 4)
> +    };
> +    browser.test.assertEq(expected.length, cookies.length, JSON.stringify(cookies) + " - " + message);
> +    if (cookies.length !== expected.length) {
> +      return;
> +    }
> +    for (let cookie of cookies) {

I think that we can simplify this helper and make it simpler to read, e.g. something like the following:

```
    for (let expect of expected) {
      let foundCookies = cookies.filter(c => matches(c, expect));
      browser.test.assertEq(1, foundCookies.length, 
                            `${message} - Expected cookie found: ${JSON.stringify(expect)}`);
    }

```

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:57
(Diff revision 4)
> +      }
> +    }
> +  };
> +
> +  // Test when FPI is disabled.
> +  const test1 = async () => {

`test1`, `test2` and `test3` are not very descriptive names, we should renamed them (and the related test messages strings) to be a bit more descriptive (e.g. something like `test_fpi_disabled`, `test_fpi_enabled` and `test_fpd_cookies_on_fpi_disabled`).

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:58
(Diff revision 4)
> +    }
> +  };
> +
> +  // Test when FPI is disabled.
> +  const test1 = async () => {
> +    let cookie;

we are using this "cookie" variable for both a single cookie and an array of cookies, and I think that it is a bit confusing.

I think that it would be better to "cookie" only for the single cookies and declare another "cookies" variable for an array of cookies

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:67
(Diff revision 4)
> +    assertExpected(cookie, {name: "foo2", value: "bar2", firstPartyDomain}, "set: FPI off, w/ firstPartyDomain, FP cookie");
> +    cookie = await browser.cookies.get({url, name: "foo1"});
> +    assertExpected(cookie, {name: "foo1", value: "bar1", firstPartyDomain: ""}, "get: FPI off, w/o firstPartyDomain, non-FP cookie");
> +    cookie = await browser.cookies.get({url, name: "foo2", firstPartyDomain});
> +    assertExpected(cookie, {name: "foo2", value: "bar2", firstPartyDomain}, "get: FPI off, w/ firstPartyDomain, FP cookie");
> +    cookie = await browser.cookies.getAll({});

Nit, I would like an empty line here (and also at line 81), just to help the readability.

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:70
(Diff revision 4)
> +    cookie = await browser.cookies.get({url, name: "foo2", firstPartyDomain});
> +    assertExpected(cookie, {name: "foo2", value: "bar2", firstPartyDomain}, "get: FPI off, w/ firstPartyDomain, FP cookie");
> +    cookie = await browser.cookies.getAll({});
> +    assertExpectedArray(cookie, [{name: "foo1", value: "bar1", firstPartyDomain: ""}], "getAll: FPI off, w/o firstPartyDomain, non-FP cookies");
> +    cookie = await browser.cookies.getAll({firstPartyDomain: null});
> +    assertExpectedArray(

Nit, do you mind it to reformat these `assertExpectedArray` into something like the following snippet?

```
assertExpectedArray(cookie, [
  {...},
  {...},
], "...");
```

::: toolkit/components/extensions/test/mochitest/test_ext_cookies_first_party.html:96
(Diff revision 4)
> +  };
> +
> +  // Test when FPI is enabled.
> +  const test2 = async () => {
> +    let cookie;
> +    browser.test.assertRejects(

Nit, do you mind to reformat these `browser.test.assertRejects` into something like the following?

```
browser.test.assertRejects(browser.cookies.set({...}),
                           expectedError,
                           "...");
```

Comment 39

a month ago
mozreview-review
Comment on attachment 8927699 [details]
Bug 1381197 - Fix failures on existent cookies API tests.

https://reviewboard.mozilla.org/r/198542/#review211098

::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:213
(Diff revision 2)
>      browser.test.assertEq(false, cookie.httpOnly, "non-httpOnly cookie set");
>  
>      details = await browser.cookies.remove({url: TEST_URL, name: "name1"});
> -    assertExpected({url: TEST_URL, name: "name1", storeId: STORE_ID}, details);
> +    assertExpected({url: TEST_URL, name: "name1", storeId: STORE_ID, firstPartyDomain: ""}, details);
>  
>      cookie = await browser.cookies.set({url: TEST_URL});

I just noticed that this issue have been workarounde d in https://hg.mozilla.org/mozilla-central/rev/3c566123d616
(which confirms that I was right and this test was actually failing when running twice).

And so there is no need to verify that the issue actually exists or fix it again in this patch (because we already got a confirmation and a fix from Bug 1416984).
(Assignee)

Comment 40

a month ago
(In reply to Luca Greco [:rpl] from comment #37)
> ::: toolkit/components/extensions/test/mochitest/test_ext_cookies.html:90
> (Diff revision 2)
> >  
> >      cookie = await browser.cookies.get({url: TEST_URL, name: "name1"});
> >      assertExpected(expected, cookie);
> >  
> >      let cookies = await browser.cookies.getAll({name: "name1"});
> >      browser.test.assertEq(cookies.length, 1, "one cookie found for matching name");
> 
> It is unrelated to your changes, but it looks that in this test there are a
> number of `browser.test.assertEq` API call where the  parameters are in the
> wrong order (in browser.test.assertEq the first parameter should be the
> expected value and the second one the actual value).
> 
> Do you mind to fix it? (so that the errors raised by this assertions on a
> failure would not be confusing).
> 
> (Same issue seems to be also at line 30, 94, 98, 105, 108, 112, 154, 158,
> 173, 177, 192, 199)

Yes I also noticed this problem :p
I'm glad to fix it
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 44

a month ago
(In reply to Luca Greco [:rpl] from comment #38)
> In the JSON schema, in the description of the `firstPartyDomain` property
> related to the `getAll` API method we are mentioning that:
> 
> `Restricts the retrieved cookies to those whose first-party domain match or
> are subdomains of this one.`
> 
> But it seems that we have no test cases which is testing the "subdomains"
> scenario.

After a further trace of the implementation of origin attribute, the subdomain case seems not to be supported.

From GetCookiesFromHost, used as a key of hash map:
https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/netwerk/cookie/nsCookieService.cpp#4796
https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/netwerk/cookie/nsCookieKey.h#51
https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/caps/OriginAttributes.cpp#132

From GetCookiesWithOriginAttributes, used for pattern matching:
https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/netwerk/cookie/nsCookieService.cpp#4856
https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/caps/OriginAttributes.h#143

So I just removed the misleading description from the JSON schema.

Please take a look again, thanks!
Flags: needinfo?(lgreco)

Comment 45

14 days ago
mozreview-review
Comment on attachment 8925816 [details]
Bug 1381197 - Add tests for browser.cookies APIs with first-party isolation.

https://reviewboard.mozilla.org/r/197000/#review215490

Thanks!

It looks good to me.

The first two patches in this mozreview request will need to be rebased on a recent tip, to fix some small conflicts with the changes landed in the meantime on the ext-cookies.js module and toolkit/components/extensions/test/mochitest/test_ext_cookies.html.

On the test case there is also an additional change needed around line 151 (on the new assertion added to the test case to remove cookie with name "name1", as mentioned in Comment 39):

```
diff --git a/toolkit/components/extensions/test/mochitest/test_ext_cookies.html b/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
--- a/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
@@ -140,17 +140,18 @@ add_task(async function test_cookies() {

     cookies = await browser.cookies.getAll({url: changePort(TEST_URL, 1)});
     browser.test.assertEq(cookies.length, 1, "Found cookie using getAll with port");
     assertExpected(expected, cookies[0]);

     // .remove should return the URL of the API call, so the port is included in the return value.
     const TEST_URL_TO_REMOVE = changePort(TEST_URL, 1023);
     details = await browser.cookies.remove({url: TEST_URL_TO_REMOVE, name: "name1"});
-    assertExpected({url: TEST_URL_TO_REMOVE, name: "name1", storeId: STORE_ID}, details);
+    assertExpected({url: TEST_URL_TO_REMOVE, name: "name1", storeId: STORE_ID, firstPartyDomain: ""},
+                   details);

     cookie = await browser.cookies.get({url: TEST_URL, name: "name1"});
     browser.test.assertEq(null, cookie, "removed cookie not found");

     let stores = await browser.cookies.getAllCookieStores();
     browser.test.assertEq(1, stores.length, "expected number of stores returned");
     browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
     browser.test.assertEq(1, stores[0].tabIds.length, "one tabId returned for store");
```
Attachment #8925816 - Flags: review?(lgreco) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 49

13 days ago
Thank you very much for the kind instruction! I rebased the patches to the latest code base and fixed the failure in test_ext_cookies.html caused by some newly added tests. Try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=a53fedf993e0658dc6d36350f8ec9c8daae547de
Flags: needinfo?(lgreco)
Keywords: checkin-needed

Comment 50

13 days ago
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/156d158ae4c3
browser.cookies APIs support firstPartyDomain. r=robwu,rpl
https://hg.mozilla.org/integration/autoland/rev/36bccd1ca96f
Fix failures on existent cookies API tests. r=robwu,rpl
https://hg.mozilla.org/integration/autoland/rev/6eb01df118de
Add tests for browser.cookies APIs with first-party isolation. r=robwu,rpl
Keywords: checkin-needed

Comment 51

13 days ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/a5869587aa9d
browser.cookies APIs support firstPartyDomain. r=robwu,rpl
https://hg.mozilla.org/mozilla-central/rev/fc655d3940fc
Fix failures on existent cookies API tests. r=robwu,rpl
https://hg.mozilla.org/mozilla-central/rev/99438fbc74a5
Add tests for browser.cookies APIs with first-party isolation. r=robwu,rpl

Comment 52

13 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a5869587aa9d
https://hg.mozilla.org/mozilla-central/rev/fc655d3940fc
https://hg.mozilla.org/mozilla-central/rev/99438fbc74a5
Status: NEW → RESOLVED
Last Resolved: 13 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 54

9 days ago
When will the documentation on MDN be updated? I'd rather not read through all the comments to find out how to actually use this.

Comment 55

9 days ago
Adding the dev-doc-needed keyword to request an update for MDN.

Comment #34 summarizes the expected behavior of the enhanced cookies API. More details are in comment #6.
Everything has been implemented, except for "Optional: cookies.set rejects the request to create new cookies without a non-empty firstPartyDomain value." (not implementing this allows extensions to migrate FP cookies to non-FP cookies if desired).
Keywords: dev-doc-needed

Comment 56

8 days ago
Thanks, that clears some things (aside from Cookie::firstPartyDomain)

I have a bug to report tho:
I'm visiting www.youtube.com, but the firstPartyDomain for its cookies are set to youtube.com instead. If this is intended, I need a way to get the corrected firstPartyDomain for any given URL.
Flags: needinfo?(cfu)
(Assignee)

Comment 57

7 days ago
(In reply to Lusito from comment #56)
> Thanks, that clears some things (aside from Cookie::firstPartyDomain)
> 
> I have a bug to report tho:
> I'm visiting www.youtube.com, but the firstPartyDomain for its cookies are
> set to youtube.com instead. If this is intended, I need a way to get the
> corrected firstPartyDomain for any given URL.

Thank you very much for helping with this!

It is because the browser uses top-level domains. For example, both "www.developer.mozilla.org" and "developer.mozilla.org" will be "mozilla.org". You can refer to this for more information
https://searchfox.org/mozilla-central/rev/03877052c151a8f062eea177f684a2743cd7b1d5/netwerk/dns/nsIEffectiveTLDService.idl#40

Please ni? me again if you need some further details.
Flags: needinfo?(cfu)

Comment 58

7 days ago
I think there is a flaw in that system.. getPublicSuffix has this to say:

> For example, the public suffix for "www.bbc.co.uk" is "co.uk",
>     * because the .uk TLD does not allow the registration of domains at the
>     * second level ("bbc.uk" is forbidden).

Yet Wikipedia states something different: (https://en.wikipedia.org/wiki/.uk)

> New registrations directly under .uk have been accepted by Nominet since 10 June 2014 08:00 BST

So, either the documentation of getPublicSuffix is outdated, or your code might run into issues with bbc.uk and alike.

I've read about other multi-part TLD doing the same some time ago.
If you still have something in place for it to work correctly, please let me know how.
Do you have some sort of list to identify the multi-part TLD suffixes?
Flags: needinfo?(cfu)

Comment 59

7 days ago
I found some libraries on NPM, which can do the job, like this one: https://github.com/peerigon/parse-domain

But since those libraries are huge (210KB, mostly because of the huge list of TLDs) and thus slow (according to some complaints), I wonder if it would be possible to access firefoxes (probably faster?) getBaseDomain using a web-extensions API.
(Assignee)

Comment 60

6 days ago
(In reply to Lusito from comment #58)
> I think there is a flaw in that system.. getPublicSuffix has this to say:
> 
> > For example, the public suffix for "www.bbc.co.uk" is "co.uk",
> >     * because the .uk TLD does not allow the registration of domains at the
> >     * second level ("bbc.uk" is forbidden).
> 
> Yet Wikipedia states something different: (https://en.wikipedia.org/wiki/.uk)
> 
> > New registrations directly under .uk have been accepted by Nominet since 10 June 2014 08:00 BST
> 
> So, either the documentation of getPublicSuffix is outdated, or your code
> might run into issues with bbc.uk and alike.

It seems to me that this doc is outdated. I tried https://www.nominet.uk/ and it just worked fine.

> I've read about other multi-part TLD doing the same some time ago.
> If you still have something in place for it to work correctly, please let me
> know how.
> Do you have some sort of list to identify the multi-part TLD suffixes?

I found this list https://searchfox.org/mozilla-central/source/netwerk/dns/effective_tld_names.dat
It is then encoded (compressed?) and used in nsEffectiveTLDService::GetBaseDomainInternal, which is invoked by getPublicSuffix, getBaseDomain and other similar functions.
Flags: needinfo?(cfu)

Comment 61

6 days ago
Created attachment 8941852 [details]
cookies.PNG

There are the results I get when using the steps provided in https://bugzilla.mozilla.org/show_bug.cgi?id=1381197#c2 while testing on Firefox 59.0a1 (20180111100722) on Windows 10 64 bit.
Is this the desired result and if I need to do more testing please provide an extension or STR.
Flags: needinfo?(cfu)

Comment 62

6 days ago
Comment 2 only demonstrated why FPI cookies are not shown, it does not show whether the feature works.

I will add support for first-party cookies in my cookie manager (https://github.com/Rob--W/cookie-manager) and if there are any issues, report back.
Flags: needinfo?(cfu) → qe-verify-
You need to log in before you can comment on or make changes to this bug.