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)
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•5 years ago
|
||
Updated•5 years ago
|
Comment 3•5 years 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•5 years ago
|
Assignee | ||
Comment 4•5 years 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•5 years 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•5 years 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•5 years ago
|
||
But then not change anything other than which bit is used looks like there are many unused ones.
Comment 11•5 years 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•5 years 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•5 years 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•5 years 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•5 years ago
|
||
Updated•5 years ago
|
Comment 16•5 years 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•5 years 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•5 years ago
|
||
Filed bug 1648346 to investigate the underlying issue.
Comment 19•5 years ago
|
||
Comment 20•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c2f466a4e580
https://hg.mozilla.org/mozilla-central/rev/2586226d649a
Description
•