Closed Bug 1554805 Opened 5 years ago Closed 5 years ago

feed reader WX (Brief) not working with FPI enabled

Categories

(Firefox :: Extension Compatibility, defect, P2)

69 Branch
x86_64
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 - wontfix
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- verified

People

(Reporter: vtol, Assigned: johannh)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: regression, Whiteboard: [tor])

Attachments

(2 files, 2 obsolete files)

FF N 69 x_64 on W10 Pro v 1903 b 18362.116


With the introduction of bug 1330467 the feed reader WX Brief stopped working, as in not fetching feeds any more and neither discovering available feeds on websites.

mozregression points at bug 1330467 permissions-in-FPI rework.

Blocks: 1330467

Brief's developer here. Looks like the FPI rework breaks the propagation of extension's "storage" and "unlimitedStorage" permissions.

This results in two effects:

  • the extension's call to navigator.storage.persist() is not resolved automatically, but causes a popup when the user opens the Brief tab,
  • the extension's attempt to open the old {storage: "persistent"} database (to import old data from it if there's anything in there) hangs forever (neither onsuccess nor onerror handler is called at all), which is a kinda unexpected failure mode.

(In reply to Denis Lisov from comment #1)

Brief's developer here. Looks like the FPI rework breaks the propagation of extension's "storage" and "unlimitedStorage" permissions.

Thanks for digging into this!

This results in two effects:

  • the extension's call to navigator.storage.persist() is not resolved automatically, but causes a popup when the user opens the Brief tab,

This makes sense; we explicitly decided that because FPI is not an officially-supported feature in Firefox, that although the landing of Bug 1330467 would wipe any permissions that FPI-users had stored, we would not try to migrate them.

  • the extension's attempt to open the old {storage: "persistent"} database (to import old data from it if there's anything in there) hangs forever (neither onsuccess nor onerror handler is called at all), which is a kinda unexpected failure mode.

Does this happen even after you re-grant the permission above?

I know this is not ideal; but it is possible to detect in an extension if the user has FPI enabled using privacy.websites.firstPartyIsolate (https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-privacy.js#281)

Flags: needinfo?(dennis.lissov)

This makes sense; we explicitly decided that because FPI is not an officially-supported feature in Firefox, that although the landing of Bug 1330467 would wipe any permissions that FPI-users had stored, we would not try to migrate them.

Does this also mean that turning FPI on or off nukes extension's IndexedDB storage automatically (or, being more specific, the FPI and non-FPI IndexedDB for an extension are separate and the extension has no way to access the FPI one when FPI is off or vice versa)?

Does this happen even after you re-grant the permission above?

I've done a few experiments and it seems to be orthogonal to the permission above. If this is done from a regular Brief tab (moz-extension:// one), a separate prompt appears asking "Will you allow (uuid) to store data on your computer?" that's separate from the "Will you allow (name) to store data in persistent storage?", but the background page does not cause such a prompt (probably it's not allowed to cause prompts).

Furthermore, if the regular tab is the first one to try opening the db and cause such a prompt and get the confirmation, that db can be opened in any tab, including the background one; however, if the background tab tries opening it first, all the open attempts from that point hang at least until browser restart (not tested, but page reload and/or extension disable-enable do not help).

Flags: needinfo?(dennis.lissov) → needinfo?(tom)

I wish I could contribute on sorting the technicalities but I do not posses the ability (shame).

Being an avid user of FF N with FPI enabled (since it became available) as well as utilizing Brief since long for speedy consumption of information, it would sadden me if both could not coexists any more with the release of FF 69. It would be a hard choice but I would indeed opt out of FPI in favour of this WX.

With that said and the appreciation of the code contribution from the Tor browser project, the FF browser should perhaps retain a healthy balance between privacy protective measures and WX compatibility. It would surprise me if this particular WX will be the only one being impacted by the recent FPI code rework.

It would not make sense for the FF user with FPI affinity to be confronted with the choice - either FPI or WX. In that light it is a bit hard to grasp:

(In reply to Tom Ritter [:tjr] from comment #2)

FPI is not an officially-supported feature in Firefox

Either the feature is there or its not, officially the code exists in the browser's code base and is apparently actively contributed to. There is hardly a point to provision such code if it cannot be utilized however if it starts hampering the WX eco system.

If extensions are not working in the Tor browser because of such incompatibility is another matter and entirely its user-base prerogative.

(In reply to Denis Lisov from comment #3)

This makes sense; we explicitly decided that because FPI is not an officially-supported feature in Firefox, that although the landing of Bug 1330467 would wipe any permissions that FPI-users had stored, we would not try to migrate them.

Does this also mean that turning FPI on or off nukes extension's IndexedDB storage automatically (or, being more specific, the FPI and non-FPI IndexedDB for an extension are separate and the extension has no way to access the FPI one when FPI is off or vice versa)?

Hm. I am not entirely sure. I am pretty near certain that we don't nuke any data when you enable/disable FPI. Your requests to any persistent store will just be silently redirected to a parallel store for FPI-keyed (or non-FPI-keyed) data. HOWEVER that's just for web content stuff: HTTP cache, Cookies, localstorage, even down to Alt-Svc mappings. The question about the extension's data is new to me. I don't know if FPI affects the Web Extension's data storage. My first guess would be no but I am unsure.

Does this happen even after you re-grant the permission above?

I've done a few experiments and it seems to be orthogonal to the permission above. If this is done from a regular Brief tab (moz-extension:// one), a separate prompt appears asking "Will you allow (uuid) to store data on your computer?" that's separate from the "Will you allow (name) to store data in persistent storage?", but the background page does not cause such a prompt (probably it's not allowed to cause prompts).

Furthermore, if the regular tab is the first one to try opening the db and cause such a prompt and get the confirmation, that db can be opened in any tab, including the background one; however, if the background tab tries opening it first, all the open attempts from that point hang at least until browser restart (not tested, but page reload and/or extension disable-enable do not help).

This seems to make sense to me if I understand it right - the extension doesn't handle the situation where you're installed but the permission disappears. But from a fresh install the permission is requested on the extension page and things work... right?

Either the feature is there or its not, officially the code exists in the browser's code base and is apparently actively contributed to.

I'm afraid the situation isn't "the feature is there or it's not, and it's in the code so it's there." Rather the situation is "Is this code supported or is it not" and the answer is "not". There a fair bit of code in Firefox that can be controlled by preferences that isn't officially supported. However we definitely want to have WX and FPI working together. The timeline for when we can last those fixes is uncertain, and there may be rough edges (like deciding not to migrate permissions) - but rest assured the team does want to resolve these issues.

We're going to keep digging into this :)

Flags: needinfo?(tom)

I don't know if FPI affects the Web Extension's data storage. My first guess would be no but I am unsure.

According to my tests looks like it does (after database creation and toggling privacy.firstparty.isolate opening the database creates a new one - or at least runs onupgradeneeded handler). Not sure whether this is intentional or a bug.

The part that feels wrong to me is that extensions lose access to their IndexedDB (or get back an arbitrarily old version) when the user toggles this setting either way.

This seems to make sense to me if I understand it right - the extension doesn't handle the situation where you're installed but the permission disappears. But from a fresh install the permission is requested on the extension page and things work... right?

This idea does not work automatically (Brief opens databases in the background page immediately on startup, which fails), but it can be used to work around the problem by delaying the database open and/or opening the Brief UI tab manually the first time to ask the user for these permissions.

No longer blocks: 1330467
Regressed by: 1330467

With the unlimitedStorage permission your extension should get both persistent storage and old-style {storage: "persistent"} access automatically (I still need to finally remove the latter part).

I don't really understand why you're manually calling navigator.storage.persist(), but the fact that this spawns a popup hints to me that the principal used when calling this might have a different firstParty attribute than the one used when adding the permission here: https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/toolkit/components/extensions/Extension.jsm#1830-1839

I wish I could dive deeper into this but I don't have the time right now. I can put it on my backlog, and I'm happy to assist anyone else who wants to investigate that, maybe Gary once he's back from PTO?

Maybe the difference is just that one principal will have its FP attribute stripped and the other has it set in a first party context? Just a guess in the dark...

Assignee: nobody → xeonchen
Priority: -- → P2
Whiteboard: [fingerprinting][fp-triaged]
Whiteboard: [fingerprinting][fp-triaged] → [tor]
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

(In reply to Johann Hofmann [:johannh] from comment #7)

With the unlimitedStorage permission your extension should get both persistent storage and old-style {storage: "persistent"} access automatically (I still need to finally remove the latter part).

I don't really understand why you're manually calling navigator.storage.persist(), but the fact that this spawns a popup hints to me that the principal used when calling this might have a different firstParty attribute than the one used when adding the permission here: https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/toolkit/components/extensions/Extension.jsm#1830-1839

I wish I could dive deeper into this but I don't have the time right now. I can put it on my backlog, and I'm happy to assist anyone else who wants to investigate that, maybe Gary once he's back from PTO?

Just a few updates of my current investigation.

This is the content of my permissions.sqlite

Origin Type
moz-extension://c77d45ed-ff2c-4e93-a93a-2397a27f983c WebExtensions-unlimitedStorage
moz-extension://c77d45ed-ff2c-4e93-a93a-2397a27f983c indexedDB
moz-extension://c77d45ed-ff2c-4e93-a93a-2397a27f983c persistent-storage
moz-extension://c77d45ed-ff2c-4e93-a93a-2397a27f983c^firstPartyDomain=c77d45ed-ff2c-4e93-a93a-2397a27f983c persistent-storage
moz-extension://c77d45ed-ff2c-4e93-a93a-2397a27f983c^firstPartyDomain=c77d45ed-ff2c-4e93-a93a-2397a27f983c storageAccessAPI

I'm still thinking how to fix this.

Yeah, that confirms my suspicion. I think the problem is that the principal of the WebExtension (used here to add the permissions) does not consider the FPD attribute, see https://searchfox.org/mozilla-central/search?q=symbol:%23createPrincipal&redirect=false

We should change that :)

https://phabricator.services.mozilla.com/D35017

This revision needs review, but there are no reviewers specified.

Is that how it is supposed to be?

Flags: needinfo?(xeonchen)

(In reply to vtol from comment #12)

https://phabricator.services.mozilla.com/D35017

This revision needs review, but there are no reviewers specified.

Is that how it is supposed to be?

Yes, I'm still working on revising this patch, sorry for waiting.
I'll make my coming patch reviewed as soon as possible.

Flags: needinfo?(xeonchen)

Are you planning on uplifting this?

Flags: needinfo?(xeonchen)

(In reply to Kate Hudson :k88hudson from comment #14)

Are you planning on uplifting this?

No, there's no uplifting plan now, thanks for asking.

Flags: needinfo?(xeonchen)

(In reply to Gary Chen [:xeonchen] from comment #13)

Yes, I'm still working on revising this patch, sorry for waiting.
I'll make my coming patch reviewed as soon as possible.

An update for this bug: I'm currently having no cycles to work on this for a while.
If anyone is interested in fixing this, please feel free to take it.

Attachment #9072150 - Attachment description: Bug 1554805 - add first party domain info to OriginAttribute in WebExtension → Bug 1554805 - Add first party domain info to OriginAttributes in WebExtension code;
Assignee: xeonchen → ehsan
See Also: → 1564593
See Also: → 1556212

I posted this comment to phabricator where there has been some discussion; but I'm cross-posting it here: I wonder if we're doing the wrong thing with this patch.

Right now we're adding the firstPartyDomain attribute in a place where it was missing to make everything consistent. When FPI is enabled; WebExtensions always get their permissions (and their storage) with a FPD specified.

But Bug 1564593 illustrates that now we're storing data for WebExtensions differently when FPI is enabled. And I don't think that makes sense. Enabling or disabling FPI shouldn't affect a WebExtensions storage.

I think maybe we need to be doing the opposite for WebExtensions: always strip the firstPartyDomain so that permissions (and storage) do not use it.

(In reply to Tom Ritter [:tjr] from comment #17)

[snip]

But Bug 1564593 illustrates that now we're storing data for WebExtensions
differently when FPI is enabled. And I don't think that makes sense.
Enabling or disabling FPI shouldn't affect a WebExtensions storage.

I think maybe we need to be doing the opposite for WebExtensions: always
strip the firstPartyDomain so that permissions (and storage) do not use it.

We started to use FPI to defend against cross-site tracking done by websites. There is no such risk in the WebExtensions context. Granted there might be corner-cases like the PDF reader where we do want to have FPI for the request to the PDF URL but in general I think you are absolutely right in that there should be no reason to apply the FPI restrictions to privileged contexts, the WebExtension one being one of those. What does being a first party there even mean?

Oh, I guess you could see that differentiated treatment pretty well in the fingerprinting resistance work which is kind of related: there we regularly check whether we are in a privileged context or not and apply our defenses only in the latter (because a) it breaks browser features if not done (as you can see in this and related bugs) and b) because the browser is a trusted context here (which includes extensions)).

The discussion over @ phabricator seems to have stalled without reaching a conclusion of how to proceed futher with this regression, or so it would seem. Also cross referenced bug 1564593 and bug 1556212 appear growing stale and soon, with the 71 branch, it will be the third branch being affected by the regression.

(In reply to Tom Ritter [:tjr] from comment #5)

However we definitely want to have WX and FPI working together. The timeline for when we can last those fixes is uncertain, and there may be rough edges (like deciding not to migrate permissions) - but rest assured the team does want to resolve these issues.

We're going to keep digging into this :)

Sorry, I didn't realize this is still assigned to me. I think I'm probably not the best person to work on it, given the discussion on phabricator I think someone more familiar with the web extension code may be a better fit. Sorry about the lag. :(

Assignee: ehsan → nobody
Status: ASSIGNED → NEW
Attachment #9072150 - Attachment is obsolete: true

For the future assignee, the discussion from this point on is relevant.

That is now the second assignee throwing in the towel inside of 3 months, incl. the one having introduced the regression with bug 1330467.

In about 3 weeks from now FX 69 is scheduled to be released and going to hit the broad FX userbase with not only this WX suffering from the regression.

(In reply to vtol from comment #23)

That is now the second assignee throwing in the towel inside of 3 months, incl. the one having introduced the regression with bug 1330467.

Friendly reminder about the bugzilla etiquette, specifically the "no obligation" part.

In about 3 weeks from now FX 69 is scheduled to be released and going to hit the broad FX userbase with not only this WX suffering from the regression.

It has been clear from comment 15 that there was never any plans to backport the fix here to Firefox 69. First party isolation isn't a supported configuration and while people go above and beyond to keep it working as much as possible most of the time, sometimes it breaks for a while and it takes time to fix it. This feature is developed for the usage of Tor Browser primarily therefore regressions in its functionality are targeted to be fixed by the next Firefox ESR version (which is 74 AFAIK.)

See Also: 1564593

So the two things I think we could do here are:

  • Gary's original patch which dynamically adjusted the fPD attribute on the permission principal based on whether FPI is enabled or not
  • Categorically strip all known OAs (specifically fPD for now) for moz-extension principals in the permission manager.

I'm leaning towards the latter option, it sounds like a cleaner way forward in the long term. Tom also mentioned he would be on board with that. Ehsan, do you have any concerns about that idea? If not, I can probably take this bug...

Flags: needinfo?(ehsan)

That sounds good to me!

Flags: needinfo?(ehsan)
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Attachment #9090519 - Attachment description: Bug 1554805 - Strip firstPartyDomain attribute when creating moz-extension StoragePrincipals. r=ehsan!,rpl! → Bug 1554805 - Add tests for stripping the firstPartyDomain attribute for moz-extension principals. r=rpl!
Attachment #9090308 - Attachment is obsolete: true
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a219d05a2791
Never set firstPartyDomain on origin attributes for moz-extension. r=rpl,Ehsan,tjr
https://hg.mozilla.org/integration/autoland/rev/a94015c87faa
Add tests for stripping the firstPartyDomain attribute for moz-extension principals. r=rpl
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71

works for me with change set 3fa65bda1e506a314ea90d936f763c7e840ab98a.

Please consider uplift to beta and current

Thanks for verifying! I would prefer to be cautious with this and avoid uplifting into late Beta. Nightly will merge soon and so we can just let it ride the trains.

Status: RESOLVED → VERIFIED

(In reply to Johann Hofmann [:johannh] from comment #34)

Thanks for verifying! I would prefer to be cautious with this and avoid uplifting into late Beta.

Any particular reason for being cautious here? I am asking because I plan to backport that patch straight to Tor Browser which will be based on ESR 68 soonish.

Flags: needinfo?(jhofmann)

(In reply to Georg Koppen from comment #35)

(In reply to Johann Hofmann [:johannh] from comment #34)

Thanks for verifying! I would prefer to be cautious with this and avoid uplifting into late Beta.

Any particular reason for being cautious here? I am asking because I plan to backport that patch straight to Tor Browser which will be based on ESR 68 soonish.

To clarify, no particular reason to be cautious, just that FPI isn't an available configuration outside of about:config that would be worth risking a late beta uplift (that touches some core security bits) over.

It makes total sense to move this to ESR 68 if it helps Tor Browser (which I can imagine it would).

Flags: needinfo?(jhofmann)

(In reply to Johann Hofmann [:johannh] from comment #36)

(In reply to Georg Koppen from comment #35)

(In reply to Johann Hofmann [:johannh] from comment #34)

Thanks for verifying! I would prefer to be cautious with this and avoid uplifting into late Beta.

Any particular reason for being cautious here? I am asking because I plan to backport that patch straight to Tor Browser which will be based on ESR 68 soonish.

To clarify, no particular reason to be cautious, just that FPI isn't an available configuration outside of about:config that would be worth risking a late beta uplift (that touches some core security bits) over.

Fair enough.

It makes total sense to move this to ESR 68 if it helps Tor Browser (which I can imagine it would).

It would indeed as we would need to carry one patch less. Would you mind filing the respective uplift request to get this into ESR68?

Flags: needinfo?(jhofmann)

Comment on attachment 9096892 [details]
Bug 1554805 - Never set firstPartyDomain on origin attributes for moz-extension. r=rpl!,ehsan!,tjr!

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Patch that only affects FPI and will be useful for Tor Browser
  • User impact if declined: Add-ons have trouble using IndexedDB, so they might break.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a simple patch with tests that should only have an effect on the user if FPI is active (which is only accessible through about:config), so the risk to standard Firefox should be very low. With FPI on, I wouldn't be 100% that we didn't introduce some unexpected edge case somewhere.
  • String or UUID changes made by this patch: None
Flags: needinfo?(jhofmann)
Attachment #9096892 - Flags: approval-mozilla-esr68?
Attachment #9090519 - Flags: approval-mozilla-esr68?

To avoid confusion, I'm going to sit on this until the next cycle so it ships in Firefox at the same time across both channels.

Regressions: 1588916

Comment on attachment 9090519 [details]
Bug 1554805 - Add tests for stripping the firstPartyDomain attribute for moz-extension principals. r=rpl!

Approved for 68.3esr now that the version bump has landed.

Attachment #9090519 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Attachment #9096892 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Backed out changeset d54afc3153e3 (Bug 1554805) for causing xpcshell timeouts on test_ext_indexedDB_principal.js

Backout link: https://hg.mozilla.org/releases/mozilla-esr68/rev/172e58ba16ecfd97f6b1bef2bfe63744daa773b8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&tochange=172e58ba16ecfd97f6b1bef2bfe63744daa773b8&fromchange=99b094c6fa924cfd1f8fac46136e2def1678e382&searchStr=%28x3&selectedJob=272252647

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272252647&repo=mozilla-esr68&lineNumber=2504

[task 2019-10-21T20:57:48.098Z] 20:57:48 INFO - TEST-START | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_indexedDB_principal.js
[task 2019-10-21T21:02:48.098Z] 21:02:48 WARNING - TEST-UNEXPECTED-TIMEOUT | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_indexedDB_principal.js | Test timed out
[task 2019-10-21T21:02:48.099Z] 21:02:48 INFO - TEST-INFO took 300000ms
[task 2019-10-21T21:02:48.412Z] 21:02:48 INFO - xpcshell return code: 143
[task 2019-10-21T21:02:48.433Z] 21:02:48 WARNING - TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
[task 2019-10-21T21:02:48.433Z] 21:02:48 INFO - INFO | Result summary:
[task 2019-10-21T21:02:48.433Z] 21:02:48 INFO - INFO | Passed: 247
[task 2019-10-21T21:02:48.433Z] 21:02:48 WARNING - INFO | Failed: 1
[task 2019-10-21T21:02:48.434Z] 21:02:48 WARNING - One or more unittests failed.
[task 2019-10-21T21:02:48.434Z] 21:02:48 INFO - INFO | Todo: 6
[task 2019-10-21T21:02:48.434Z] 21:02:48 INFO - INFO | Retried: 0
[task 2019-10-21T21:02:48.434Z] 21:02:48 INFO - SUITE-END | took 872s
[task 2019-10-21T21:02:48.474Z] 21:02:48 ERROR - Return code: 1

Flags: needinfo?(jhofmann)

Not sure why Android was timing out on ESR68 but not on central or beta :(

Given the Android test failures and bug 1588916 looking unlikely to be resolved during this cycle, I'm pushing this out to the next cycle for ESR68 reconsideration.
https://hg.mozilla.org/releases/mozilla-esr68/rev/971b8ea8b4ec95929b0cfd1d0ec23317370c192b

Comment on attachment 9090519 [details]
Bug 1554805 - Add tests for stripping the firstPartyDomain attribute for moz-extension principals. r=rpl!

Clearing the ESR68 approval flags for now to get this off the needs-uplift radar. And I think we'll want to take a fresh look at this from a risk perspective anyway.

Attachment #9090519 - Flags: approval-mozilla-esr68+
Attachment #9096892 - Flags: approval-mozilla-esr68+

[Tracking Requested - why for this release]: this didn't make it in 68.4.

Per discussion with Johann, we're going to leave this out of ESR.

Flags: needinfo?(jhofmann)
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: