1521051 - Get rid of nsICookiePermission.ACCESS_ALLOW_FIRST_PARTY_ONLY and nsICookiePermission.ACCESS_LIMIT_THIRD_PARTY

Status: RESOLVED FIXED

(Core :: Permission Manager, enhancement, P3)

Description

[Andrea Marchesini [:baku]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - ACCESS_LIMIT_THIRD_PARTY

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

Comment 3

[Johann Hofmann [:johannh]]

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.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

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?

Comment 5

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 6

[Andrea Marchesini [:baku]]

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 7

[Valentin Gosu [:valentin] (he/him)]

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 :)

Comment 8

[Johann Hofmann [:johannh]]

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?

Comment 9

[Johann Hofmann [:johannh]]

(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 :)

Comment 10

[Andrea Marchesini [:baku]]

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.

Comment 11

[Johann Hofmann [:johannh]]

(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 12

[Francesco Lodolo [:flod]]

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).

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - ACCESS_ALLOW_FIRST_PARTY_ONLY

Let's exclude not supported permission capabilities.

Comment 14

[Johann Hofmann [:johannh]]

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" :)

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

[Razvan Maries]

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5d2f8b1292ce
https://hg.mozilla.org/mozilla-central/rev/a18e28e26934