Open Bug 1618557 Opened 5 years ago Updated 8 days ago

It seems that LoadInfo's top-level document constructor sets up the wrong top-level anti-tracking principal

Categories

(Core :: Privacy: Anti-Tracking, defect, P3)

defect

Tracking

()

People

(Reporter: ehsan.akhgari, Unassigned)

References

Details

Attachments

(3 obsolete files)

See this principal. Here innerWindow is the old inner window that the top-level navigation is replacing. It seems wrong to use its principal as the top-level anti-tracking principal.

baku, what do you think?

Flags: needinfo?(amarchesini)

This is related to https://hg.mozilla.org/mozilla-central/rev/d496a070135c. It's possible that stopAtOurLevel is mostly false so we always bail our early here, but it could be that the fix to bug 1577298 wasn't completely right...

Blocks: 1577298

baku said he'll take this.

Assignee: nobody → amarchesini

I tried dropping this write in this try push: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=290729119&revision=9fd39f404f603d7cb668853a20013d7b661600d9

It seems to be fine, but it'd be nice to have a test.

Matt, if you want to work on this, it's great. I can review the code. Let me know.

Flags: needinfo?(amarchesini) → needinfo?(matt.woodrow)

I'm not really sure where to start for adding a test sorry.

I can upload a patch to just remove the offending call if that's useful!

Flags: needinfo?(matt.woodrow)
Priority: -- → P1
Status: NEW → ASSIGNED

Dimi, do we still need to fix this issue? Asking because the fission-porting has changed the code a lot lately.

Flags: needinfo?(dlee)

We still use topLevelPrincipal from LoadInfo to get partitionPrincipal at here. But it is only used when we can't partitionKey from CookieJarSetting in some special cases, so in most of the cases, the value of topLevelPrincipal doesn't matter.

I think the right solution now is to find the cases we don't set partitionKey, and then remove TopLevelPrincipal from LoadInfo entirely to prevent using it in the future. Because this value doesn't work in Fission anyway.

Flags: needinfo?(dlee)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:baku, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(amarchesini)
See Also: → 1673889

The following patch is waiting for review from an inactive reviewer:

ID Title Author Reviewer Status
D79098 Bug 1618557 - Remove top-level principal from LoadInfo, r?dimi baku dimi: Resigned from review

:baku, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

Flags: needinfo?(amarchesini)
Severity: normal → S3

Dimi, do you think this is still relevant? Thanks.

Flags: needinfo?(dlee)
Flags: needinfo?(amarchesini)

(In reply to Tim Huang[:timhuang] from comment #11)

Dimi, do you think this is still relevant? Thanks.

As per discussion with :tim offline, we think removing toplevelprincipal from loadinfo is still the right thing to do, so yes, this is still relevant.

Flags: needinfo?(dlee)
Assignee: amarchesini → amadan
Attachment #9359875 - Attachment is obsolete: true
Attachment #9155603 - Attachment is obsolete: true
Priority: P1 → P3
Priority: P3 → P1
Priority: P1 → P3
Assignee: abhishekmadan2002 → nobody
Status: ASSIGNED → NEW
Attachment #9360250 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: