Closed Bug 1645896 Opened 8 months ago Closed 7 months ago

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

Categories

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

Product:
Core
Component:
Privacy: Anti-Tracking
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla79
Tracking Flags:
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)

Bug 1645896 - Add a console log origins that have their cookies partitioned by dFPI. r=timhuang!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1645896 - Cast state constants to uint32_t in switch block. r=timhuang!
47 bytes, text/x-phabricator-request
Details | Review

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.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
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
Flags: needinfo?(nhnt11)

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.

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.

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

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

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.

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.

(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

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.

Attachment #9158883 - Attachment is obsolete: true

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

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.

Filed bug 1648346 to investigate the underlying issue.

Depends on: 1648346
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

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

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
status-firefox79: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Depends on: 1649280
Depends on: 1660575
You need to log in before you can comment on or make changes to this bug.