Get rid of nsICookiePermission.ACCESS_ALLOW_FIRST_PARTY_ONLY and nsICookiePermission.ACCESS_LIMIT_THIRD_PARTY

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

5 months ago

These 2 cookie permissions are used just for testing. They are not visible in the UI and they are not exposed to addons.

I think it's time to remove them if nobody has objections.

Assignee

Comment 1

5 months ago
Assignee: nobody → amarchesini
Attachment #9037517 - Flags: review?(valentin.gosu)
Attachment #9037517 - Flags: review?(jhofmann)
Assignee

Comment 2

5 months ago
Attachment #9037518 - Flags: review?(valentin.gosu)
Attachment #9037518 - Flags: review?(jhofmann)
Comment on attachment 9037517 [details] [diff] [review]
part 1 - ACCESS_LIMIT_THIRD_PARTY

Review of attachment 9037517 [details] [diff] [review]:
-----------------------------------------------------------------

Seems okay to me. Note that there are things like https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/extensions/cookie/test/unit/test_cookies_thirdparty.js#105 which don't show up when you grep for ACCESS_LIMIT_THIRD_PARTY but you still need to remove. I'm afraid there could be more usage of the magic "10" number in our code...

r=me with the above mentioned test fixed.
Attachment #9037517 - Flags: review?(jhofmann) → review+
Comment on attachment 9037517 [details] [diff] [review]
part 1 - ACCESS_LIMIT_THIRD_PARTY

Review of attachment 9037517 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/nsICookiePermission.idl
@@ -31,5 @@
>     * any methods on this interface.
>     */
>    const nsCookieAccess ACCESS_SESSION = 8;
>    const nsCookieAccess ACCESS_ALLOW_FIRST_PARTY_ONLY = 9;
> -  const nsCookieAccess ACCESS_LIMIT_THIRD_PARTY = 10;

So, these values probably shouldn't appear in any profiles, right, since they're not exposed in the UI?
Otherwise maybe we should leave a comment here, marking them as reserved?
Attachment #9037517 - Flags: review?(valentin.gosu) → review+
Comment on attachment 9037518 [details] [diff] [review]
part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

Review of attachment 9037518 [details] [diff] [review]:
-----------------------------------------------------------------

Are you sure this isn't exposed in the UI? I'm asking since it's localized.
Assignee

Comment 6

5 months ago

Are you sure this isn't exposed in the UI? I'm asking since it's localized.

Yes, it seems so. We don't have any setter for this value in the UI. Johann, can you confirm this?

Comment on attachment 9037518 [details] [diff] [review]
part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

Review of attachment 9037518 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. We might want to consider waiting till after the code freeze. You never know what we might break with this :)
Attachment #9037518 - Flags: review?(valentin.gosu) → review+
Comment on attachment 9037518 [details] [diff] [review]
part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

Review of attachment 9037518 [details] [diff] [review]:
-----------------------------------------------------------------

You need review from an l10n peer for the .ftl change.

I'm slightly worried that extensions were settings these permissions (I found this old answer here: https://superuser.com/questions/569210/allow-only-some-cookies-and-only-as-first-party-ffie/569569#569569) and that they still show up in some old profiles. It's really hard to say how many profiles are affected, however.

So I'm not sure if that warrants commenting out instead of removing the constants, but we may want to avoid throwing an error and instead returning an empty string id, which I think should fail more gracefully (just displaying nothing for that particular permission).

Flod, do you think setting an empty string ID is an acceptable fallback for very unlikely cases of failure?
Attachment #9037518 - Flags: review?(jhofmann)
Attachment #9037518 - Flags: review?(francesco.lodolo)
Attachment #9037518 - Flags: review-

(In reply to Johann Hofmann [:johannh] from comment #8)

Comment on attachment 9037518 [details] [diff] [review]
part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

So I'm not sure if that warrants commenting out instead of removing the
constants, but we may want to avoid throwing an error and instead returning
an empty string id, which I think should fail more gracefully (just
displaying nothing for that particular permission).

I'm referring to the code in permissions.js :)

Status: NEW → ASSIGNED
Priority: -- → P3
Assignee

Comment 10

5 months ago

I'm slightly worried that extensions were settings these permissions (I
found this old answer here:

Note that ACCESS_ALLOW_FIRST_PARTY_ONLY is not exposed to web-extensions. Anything before webextension should not be supported in new firefox versions. Maybe we should show the default value in permission.js.

(In reply to Andrea Marchesini [:baku] from comment #10)

I'm slightly worried that extensions were settings these permissions (I
found this old answer here:

Note that ACCESS_ALLOW_FIRST_PARTY_ONLY is not exposed to web-extensions. Anything before webextension should not be supported in new firefox versions. Maybe we should show the default value in permission.js.

Yup, it still doesn't change the fact that these old extensions polluted profiles with their own custom data (I've seen this before with the geolocation permission) and if it's not too inconvenient we should try to be mindful of it.

Personally I would love to find a way to cleanse the permission manager of all non-default values, but I guess that's pretty hard in practice.

Comment on attachment 9037518 [details] [diff] [review]
part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

Review of attachment 9037518 [details] [diff] [review]:
-----------------------------------------------------------------

Can someone test what happens if the string ID returned is empty? I assume we would warn in Console, and don't break anything, but better safe than sorry.

The alternative would be to have some sort of fallback string ("Option not available", "Option deprecated", etc.), but I'm not sure how fit it would be for this context.

::: python/l10n/fluent_migrations/bug_1491676_preferences_properties.py
@@ -185,5 @@
>          transforms_from(
>              """
>  permissions-capabilities-listitem-allow =
>      .value = { COPY(from_path, "can")}
> -permissions-capabilities-listitem-allow-first-party =

Please don't modify existing migration recipes. These are pieces of code that are valid at the time they land, and are periodically cleaned up (i.e. removed).

This one in particular is being removed as I write this comment (bug 1521695).
Attachment #9037518 - Flags: review?(francesco.lodolo) → review+
Assignee

Comment 13

5 months ago

Let's exclude not supported permission capabilities.

Attachment #9037518 - Attachment is obsolete: true
Attachment #9038475 - Flags: review?(jhofmann)
Comment on attachment 9038475 [details] [diff] [review]
part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

Review of attachment 9038475 [details] [diff] [review]:
-----------------------------------------------------------------

This seems reasonable to me, thank you!

::: browser/components/preferences/permissions.js
@@ +130,5 @@
>        this._addPermissionToList(permission);
>        this.buildPermissionsList();
>      } else if (data == "changed") {
>        let p = this._permissions.get(permission.principal.origin);
> +      // Maybe this item has excluded before because it had an invalid capability.

typo nit: "has been excluded" or "was excluded" :)
Attachment #9038475 - Flags: review?(jhofmann) → review+

Comment 15

5 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/483fa314e45e
nsICookiePermission.ACCESS_LIMIT_THIRD_PARTY, r=valentin, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/90bb620dd870
Get rid of nsICookiePermission.ACCESS_ALLOW_FIRST_PARTY_ONLY, r=johannh, r=flod

Comment 16

5 months ago

Backed out 2 changesets (bug 1521051) for xpcshell fails on extensions/cookie/test/unit/test_cookies_thirdparty.js.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=6a1c4140f4796d03368e9ab4b8f547209422fdd5

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223569342&repo=mozilla-inbound&lineNumber=9197

20:02:01 INFO - TEST-START | extensions/cookie/test/unit/test_cookies_thirdparty.js
20:02:01 WARNING - TEST-UNEXPECTED-FAIL | extensions/cookie/test/unit/test_cookies_thirdparty.js | xpcshell return code: 0
20:02:01 INFO - TEST-INFO took 226ms
20:02:01 INFO - >>>>>>>
20:02:01 INFO - PID 14988 | Unable to load \untrusted-startup-test-dll.dll; LoadLibraryW failed: 126Couldn't convert chrome URL: chrome://branding/locale/brand.properties
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 80
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 80
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 80
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 80
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Failed to get directory to cache.: file z:/build/build/src/security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp, line 80
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file z:/build/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2536
20:02:01 INFO - (xpcshell/head.js) | test MAIN run_test pending (1)
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file z:/build/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 668
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: NSS will be initialized without a profile directory. Some things may not work as expected.: file z:/build/build/src/security/manager/ssl/nsNSSComponent.cpp, line 1489
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: This method is lossy. Use GetCanonicalPath !: file z:/build/build/src/xpcom/io/nsLocalFileWin.cpp, line 3263
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: This method is lossy. Use GetCanonicalPath !: file z:/build/build/src/xpcom/io/nsLocalFileWin.cpp, line 3263
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: This method is lossy. Use GetCanonicalPath !: file z:/build/build/src/xpcom/io/nsLocalFileWin.cpp, line 3263
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: site security information will not be persisted: file z:/build/build/src/security/manager/ssl/nsSiteSecurityService.cpp, line 506
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: NS_ENSURE_TRUE(aChannel) failed: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 172
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED) failed with result 0x80004005: file z:/build/build/src/extensions/cookie/nsPermissionManager.cpp, line 1047
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: NS_ENSURE_TRUE(aChannel) failed: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 172
20:02:01 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_thirdparty.js | run_test - [run_test : 20] 1 == 1
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Found a principal with no URI, assuming third-party request: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 226
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Found a principal with no URI, assuming third-party request: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 226
20:02:01 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_thirdparty.js | run_test - [run_test : 20] 2 == 2
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: NS_ENSURE_TRUE(aChannel) failed: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 172
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: NS_ENSURE_TRUE(aChannel) failed: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 172
20:02:01 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_thirdparty.js | run_test - [run_test : 20] 3 == 3
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Found a principal with no URI, assuming third-party request: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 226
20:02:01 INFO - PID 14988 | [14988, Main Thread] WARNING: Found a principal with no URI, assuming third-party request: file z:/build/build/src/dom/base/ThirdPartyUtil.cpp, line 226
20:02:01 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_thirdparty.js | run_test - [run_test : 20] 4 == 4

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f4bf8af10a1ba026623963950277dac0814360

Flags: needinfo?(amarchesini)

Comment 17

5 months ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2f8b1292ce
nsICookiePermission.ACCESS_LIMIT_THIRD_PARTY, r=valentin, r=johannh
https://hg.mozilla.org/integration/mozilla-inbound/rev/a18e28e26934
Get rid of nsICookiePermission.ACCESS_ALLOW_FIRST_PARTY_ONLY, r=johannh, r=flod
Assignee

Updated

5 months ago
Flags: needinfo?(amarchesini)

Comment 18

5 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Updated

5 months ago
Depends on: 1522776
You need to log in before you can comment on or make changes to this bug.