Add a console log origins that have their cookies partitioned by dFPI
Categories
(Core :: Privacy: Anti-Tracking, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox79 | --- | fixed |
People
(Reporter: englehardt, Assigned: nhnt11)
References
(Depends on 2 open bugs, Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
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:
- 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.
- 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. - That the partition messages aren't overly noisy. We can consider only showing one per third-party origin on each page visit.
Assignee | ||
Comment 1•7 months ago
|
||
Updated•7 months ago
|
Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7ff5210dae5e Add a console log origins that have their cookies partitioned by dFPI. r=timhuang
Comment 3•7 months ago
|
||
Backed out for build bustages.
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
Assignee | ||
Updated•7 months ago
|
Assignee | ||
Comment 4•7 months ago
|
||
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.
Assignee | ||
Comment 5•7 months ago
|
||
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.
This is why the use of long is deprecated. it is different lengths that are platform dependent.
Assignee | ||
Comment 8•7 months ago
|
||
(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
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•7 months ago
|
||
But then not change anything other than which bit is used looks like there are many unused ones.
Comment 11•7 months ago
|
||
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•7 months ago
|
||
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.
Assignee | ||
Comment 13•7 months ago
|
||
(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
Assignee | ||
Comment 14•7 months ago
|
||
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.
Assignee | ||
Comment 15•7 months ago
|
||
Updated•7 months ago
|
Comment 16•7 months ago
|
||
(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?
Assignee | ||
Comment 17•7 months ago
|
||
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.
Assignee | ||
Comment 18•7 months ago
|
||
Filed bug 1648346 to investigate the underlying issue.
Comment 19•7 months ago
|
||
Pushed by nhnt11@gmail.com: 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•7 months ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2f466a4e580
https://hg.mozilla.org/mozilla-central/rev/2586226d649a
Description
•