1645896 - Add a console log origins that have their cookies partitioned by dFPI

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, enhancement, P1)

Description

[Steven Englehardt [:englehardt]]

When storage access is blocked by ETP we log the following console message:

Request to access cookie or storage on “https://social-track-digest256.dummytracker.org/cookie_access_test.html?test_origin=senglehardt.com” was blocked because it came from a tracker and content blocking is enabled. [Learn more]

The "Learn More" link leads to one of the errors on this page.

We should add in a similar console message when request to access partitioned storage occurs, e.g.,:

Partitioned cookie or storage access was provided to “https://social-track-digest256.dummytracker.org/cookie_access_test.html?test_origin=senglehardt.com” because it is loaded in the third-party context and storage partitioning is enabled. [Learn more]

We should also ensure:

1. That there is a grouping message (not sure if these need to be added manually). e.g., Partitioned cookie or storage access was provided to “<URL>” because it is loaded in the third-party context and storage partitioning is enabled.
2. That the "[Learn more]" link has a relevant MDN page. In fact, I think we should move the current index page for errors one directory up, i.e., https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Privacy/Storage_access_policy/Errors/ --> https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Privacy/Errors/. Then we can add a new page for the partitioning message and add it to that index.
3. That the partition messages aren't overly noisy. We can consider only showing one per third-party origin on each page visit.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1645896 - Add a console log origins that have their cookies partitioned by dFPI. r=timhuang!

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ff5210dae5e
Add a console log origins that have their cookies partitioned by dFPI. r=timhuang

Comment 3

[Razvan Maries]

Backed out for build bustages.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedTaskRun=JZWYUAg1TxysKpImnwbPdA.0&resultStatus=testfailed%2Cbusted%2Cexception&revision=dcf0f9b8d89da9d20c0bfc7bed4613df0d880922

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307352680&repo=autoland&lineNumber=33354

Backout: https://hg.mozilla.org/integration/autoland/rev/d1a5cda117eee6d9671046afb338cb76b3c9baf9

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1645896 - Use uint64_t everywhere for representing content blocking rejected reason. r=timhuang!

We've been assigning nsIWebProgressListener::STATE_COOKIES_foo values, which are unsigned longs,
to uint32_t variables. This overflow was revealed when adding a check for STATE_COOKIES_PARTITIONED_FOREIGN,
which is the first comparison to a value that exceeds uint32_t space. This patch replaces all occurrances.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dffa2ad39a55f39a9100d3678945b837f41f6304

Comment 6

[mac198442]

I think this fix is overly complicated I think all that is required is to replace the long in the idl with uint-32. The value assigned is a perfectly good uint-32 value the issue is with the idl file having it defined as being a signed 32 bit value.

Comment 7

[mac198442]

This is why the use of long is deprecated. it is different lengths that are platform dependent.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to mac198442 from comment #6)

I think this fix is overly complicated I think all that is required is to replace the long in the idl with uint-32. The value assigned is a perfectly good uint-32 value the issue is with the idl file having it defined as being a signed 32 bit value.

Yeah, the fix doesn't work. Windows compiler is interpreting the const as a signed value. But it's declared as an unsigned long: https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/uriloader/base/nsIWebProgressListener.idl#351

Comment 9

[mac198442]

oh yes it does say unsigned long and not just long. but I wonder if saying uint32 rather than long will avoid the issue because it will not think it is converting anything if it is uint32 everywhere.

Comment 10

[mac198442]

But then not change anything other than which bit is used looks like there are many unused ones.

Comment 11

[mac198442]

There are 31 bits here that could be used without running into this issue and i do not see where all 31 of the ones that would not cause an issue are being used.

Comment 12

[mac198442]

my suggestion if there is another bit that could be used would be use it and add a comment where they are defined warinng about using the top bit.

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to mac198442 from comment #12)

my suggestion if there is another bit that could be used would be use it and add a comment where they are defined warinng about using the top bit.

I'm very attracted to this idea, but I don't want to go for it without fully understanding what exactly is going on here, i.e. why is the compiler on Windows interpreting the const as a signed integer? There are three other cases in m-c where we have a const unsigned long value of 0x80000000 in an IDL [1]. They all seem to be unused in code, however. I'm going to dig for a while and report back here when I have more info.

[1] https://searchfox.org/mozilla-central/search?q=0x80000000&path=.idl

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Talked to a few different people about this. There seems to be a combination of issues here: something weird happening on Windows where the enum generated by the xpidl parser is given a signed underlying type, and also the parser not being smart enough to emit : unsigned long on the enum declaration. Simplest fix is to cast the constant to uint32_t in the switch cases. Patch coming.

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1645896 - Cast state constants to uint32_t in switch block. r=timhuang!

Comment 16

[mac198442]

(In reply to Nihanth Subramanya [:nhnt11] from comment #14)

Talked to a few different people about this. There seems to be a combination of issues here: something weird happening on Windows where the enum generated by the xpidl parser is given a signed underlying type, and also the parser not being smart enough to emit : unsigned long on the enum declaration. Simplest fix is to cast the constant to uint32_t in the switch cases. Patch coming.

This seems like the right approach. Just seems the patch does not apply cleanly now according to phabricator is this because something changed or is this change dependent on the earlier one in the patch landing first?

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

Try looks ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5fccbf439fcca050171e2d1494c938357ded12e

(In reply to mac198442 from comment #16)

(In reply to Nihanth Subramanya [:nhnt11] from comment #14)

Talked to a few different people about this. There seems to be a combination of issues here: something weird happening on Windows where the enum generated by the xpidl parser is given a signed underlying type, and also the parser not being smart enough to emit : unsigned long on the enum declaration. Simplest fix is to cast the constant to uint32_t in the switch cases. Patch coming.

This seems like the right approach. Just seems the patch does not apply cleanly now according to phabricator is this because something changed or is this change dependent on the earlier one in the patch landing first?

D80675 needs to be applied first, then D80942. The dependency wasn't set in Phabricator, I updated that now.

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Filed bug 1648346 to investigate the underlying issue.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2f466a4e580
Add a console log origins that have their cookies partitioned by dFPI. r=timhuang
https://hg.mozilla.org/integration/autoland/rev/2586226d649a
Cast state constants to uint32_t in switch block. r=timhuang

Comment 20

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/c2f466a4e580
https://hg.mozilla.org/mozilla-central/rev/2586226d649a