Open Bug 1895528 Opened 2 years ago Updated 2 months ago

Consider removing security.data_uri.block_toplevel_data_uri_navigations pref

Categories

(Core :: DOM: Security, task)

Product:
Core
Component:
DOM: Security
Type:
task

Tracking

()

Status:
ASSIGNED

People

(Reporter: gregp, Assigned: Joe)

References

(Blocks 3 open bugs)

Details

(Keywords: leave-open, Whiteboard: [domsecurity-backlog], [wptsync upstream])

Attachments

(1 file)

Bug 1895528 - Stop relying on top-level data: URI navigations in tests. r?gregp!
48 bytes, text/x-phabricator-request
Details | Review

It's been enabled by default for about 7 years now (bug 1401895). Ignoring tests, is this pref still necessary?

If we need to chunk this down we could remove the pref from StaticPrefs (and thus about:config) but use the dynamic pref service to check for a non-default value. That would keep the tests working, and data: navigations are extremely uncommon (because we block them by default!) so the slower lookup wouldn't add any performance trouble.

If we don't want to do that but want to completely remove the pref, we must carefully check all the tests that set the value to false rather than simply toss them. Some might be obsolete, but a lot are regression testing bugs in data: documents that we still want to test. Those could perhaps be rewritten to put the data: document in a frame.

Severity: -- → N/A
Whiteboard: [domsecurity-backlog]

Hello,

I'm a new contributor and I've worked on a solution for the first option proposed by Daniel Veditz for this bug, i.e. to "remove the pref from StaticPrefs (and thus about:config) but use the dynamic pref service to check for a non-default value." Are there any tests that I should run before submitting this patch?

Or do we prefer the second option from Daniel Veditz, i.e. completely removing the pref and adjusting all testing accordingly?

Flags: needinfo?(gregp)

Hello! I definitely prefer the second option of completely removing the pref and adjusting all tests accordingly.

I will warn you though, this is a pretty involved task that will require fixing about two dozen tests across multiple components. If you're up for it, that's great!
https://searchfox.org/firefox-main/search?q=block_toplevel_data_uri_navigations&path=&case=false&regexp=false

Flags: needinfo?(gregp)

Hey Gregory,

I've completed the removal of security.data_uri.block_toplevel_data_uri_navigations as requested and will send the patch through Phabricator next. Just leaving here a quick overview:

Core Implementation:

  • Removed pref check from nsContentSecurityManager.cpp (blocking is now always enabled)
  • Removed pref definition from StaticPrefList.yaml
  • Removed pref from privacy preferences UI (privacy.js)

Test Updates:

  • Deleted 6 obsolete tests that relied on pref toggling
  • Created 16 new HTML test files to replace inline data: URIs (which would now be blocked)
  • Updated 34 tests to remove pref manipulation calls
  • Converted tests from data: URI approach to proper file-based approach through HTTP
Assignee: nobody → jow13_122
Status: NEW → ASSIGNED
Blocks: 1961667
See Also: → 2014500
Pushed by gp3033@protonmail.com: https://github.com/mozilla-firefox/firefox/commit/08f53ac888ff https://hg.mozilla.org/integration/autoland/rev/0ac087dedae1 Removing security.data_uri.block_toplevel_data_uri_navigations pref r=gregp,extension-reviewers,devtools-reviewers,sessionstore-reviewers,akulyk,nchevobbe,sthompson,robwu,dom-core,firefox-build-system-reviewers,smaug,ahochheiden,tschuster
Pushed by amarc@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/66354d03a75f https://hg.mozilla.org/integration/autoland/rev/5bda61e04acd Revert "Bug 1895528 - Removing security.data_uri.block_toplevel_data_uri_navigations pref r=gregp,extension-reviewers,devtools-reviewers,sessionstore-reviewers,akulyk,nchevobbe,sthompson,robwu,dom-core,firefox-build-system-reviewers,smaug,ahochheiden,tschuster" for causing multiple failures

Backed out for causing multiple failures

Flags: needinfo?(jow13_122)

Sorry about the breakage. These 6 tests didn't reference the pref directly. They depended on it being set to false globally in testing/profiles/unittest-required/user.js, so I missed them when searching for pref usage. Working on fixes now.

Flags: needinfo?(jow13_122)

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:Joe, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Flags: needinfo?(jow13_122)
Flags: needinfo?(gregp)
Flags: needinfo?(gregp)

Rebased the patch to current main, updated D274022, and did a fresh try push:

https://treeherder.mozilla.org/jobs?repo=try&revision=5340063cb8ac371b75e26230930e218707c5f9cd

I checked the remaining non-green jobs and they appear unrelated to this bug. The revision was Accepted before in Phabricator, but after
the update it is back in “Needs Review”, and I don’t appear to have the permissions/UI needed to land it myself.

Could someone with landing permissions please land D274022?

Flags: needinfo?(jow13_122) → needinfo?(gregp)

Patch is updated with the remaining nit fixes and the revision is accepted. I have try access, but I don’t appear to have Lando/landing permissions. Could someone with landing permissions please land D274022?

Flags: needinfo?(hskupin)

landed

Flags: needinfo?(hskupin)
Flags: needinfo?(gregp)
Pushed by gp3033@protonmail.com: https://github.com/mozilla-firefox/firefox/commit/c7739b5a65da https://hg.mozilla.org/integration/autoland/rev/bda313108066 Removing security.data_uri.block_toplevel_data_uri_navigations pref r=gregp,extension-reviewers,devtools-reviewers,sessionstore-reviewers,akulyk,nchevobbe,sthompson,robwu,dom-core,firefox-build-system-reviewers,smaug,ahochheiden,tschuster,whimboo
Pushed by agoloman@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/7d26e00ef8e9 https://hg.mozilla.org/integration/autoland/rev/77d0b6f124fd Revert "Bug 1895528 - Removing security.data_uri.block_toplevel_data_uri_navigations pref r=gregp,extension-reviewers,devtools-reviewers,sessionstore-reviewers,akulyk,nchevobbe,sthompson,robwu,dom-core,firefox-build-system-reviewers,smaug,ahochheiden,tschuster,whimboo" for causing bc failures.

Backed out for causing bc failures.

Flags: needinfo?(jow13_122)
Blocks: 2032021
See Also: 2032021 →

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/59524 for changes under testing/web-platform/tests

Whiteboard: [domsecurity-backlog] → [domsecurity-backlog], [wptsync upstream]

Upstream PR merged by moz-wptsync-bot

Hi Joe, after the 3 weeks since the backout of your patch I would like to ask if you want to pick-up the remaining work as needed to get this bug fixed. Please let us know. Thanks!

Hi Henrik, yes I'll pick it up and try again.

Thanks

Flags: needinfo?(jow13_122)
Depends on: 2040023
Attachment #9529072 - Attachment description: Bug 1895528 - Removing security.data_uri.block_toplevel_data_uri_navigations pref r?gregp! → Bug 1895528 - Stop relying on top-level data: URI navigations in tests. r?gregp!

I'm putting the leave-open keyword on this bug; although there is a patch up there, it only partially covers this bug, by fixing up some tests that use the preference. There is quite a bit more work needed before we can declare this bug fixed:

  1. First, we need to work towards setting the security.data_uri.block_toplevel_data_uri_navigations preference to be true by default in unit tests, at https://searchfox.org/firefox-main/rev/e28b34ab33dbf49364999070168cbb7e11e8e5bd/testing/profiles/unittest-required/user.js#208-213
  2. Once no new test failures are flagged, look for remaining test uses of security.data_uri.block_toplevel_data_uri_navigations
  3. Once all test uses of security.data_uri.block_toplevel_data_uri_navigations are removed, then we can finally drop the preference.
Keywords: leave-open
See Also: → 1902635
Pushed by rob@robwu.nl: https://github.com/mozilla-firefox/firefox/commit/98af8586d34d https://hg.mozilla.org/integration/autoland/rev/2ed6b50a6c02 Stop relying on top-level data: URI navigations in tests. r=gregp,extension-reviewers,devtools-reviewers,akulyk,nchevobbe,sthompson,robwu,dom-core,firefox-build-system-reviewers,smaug,ahochheiden,tschuster,firefox-style-system-reviewers,whimboo,emilio
Pushed by ctuns@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/369c6c1cbe4c https://hg.mozilla.org/integration/autoland/rev/ecf9e1d0b5fb Revert "Bug 1895528 - Stop relying on top-level data: URI navigations in tests. r=gregp,extension-reviewers,devtools-reviewers,akulyk,nchevobbe,sthompson,robwu,dom-core,firefox-build-system-reviewers,smaug,ahochheiden,tschuster,firefox-style-system-reviewers,whimboo,emilio" for causing wd failures in execution_order_tentative.py

Backed out for causing wd failures

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: TEST-UNEXPECTED-PASS | /webdriver/tests/bidi/script/add_preload_script/execution_order_tentative.py | test_preload_script_properties_available_immediately[popup-data_url] - expected FAIL
Flags: needinfo?(jow13_122)
Pushed by rob@robwu.nl: https://github.com/mozilla-firefox/firefox/commit/68be5ee2db46 https://hg.mozilla.org/integration/autoland/rev/40a12f28040b Stop relying on top-level data: URI navigations in tests. r=gregp,extension-reviewers,devtools-reviewers,akulyk,nchevobbe,sthompson,robwu,dom-core,firefox-build-system-reviewers,smaug,ahochheiden,tschuster,firefox-style-system-reviewers,whimboo,emilio

(the patch stuck and has not been reverted; clearing needinfo that was added due to the revert)

Flags: needinfo?(jow13_122)

https://hg.mozilla.org/mozilla-central/rev/40a12f28040b

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: