Closed Bug 1396066 Opened 7 years ago Closed 7 years ago

Avoid exposing :-moz-system-metric stuff in content pages.

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- affected

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(5 files)

Bug 1396066: Restrict :-moz-system-metric to chrome and ua sheets.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1396066: Restrict system-metric media features to UA and chrome sheets only.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1396066: Remove -moz-physical-home-button.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1396066: Restrict -moz-is-glyph to UA and chrome only.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
See also bug 1396048. We've been without them for a while in stylo and no bug reports so far. Code-search in github also returns no results for it, and it looks like a fingerprinting vector (see bug 541386). We should consider unshipping it from content pages after 57.
I sent https://groups.google.com/forum/#!topic/mozilla.dev.platform/RVttfrQkXLU, let's use that thread if discussion is needed.
Assignee: nobody → emilio
See Also: → 1396073
Priority: -- → P3
Blocks: 1405311
I don't think your intent to unship email in comment 1 covers patches after the first one. Could you send another intent to unship mentioning all of them?
Flags: needinfo?(emilio)
Oh, actually, I guess that one probably intended to cover all the media queries although didn't name all of them explicitly. -moz-is-glyph isn't a system metric anyway...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #8) > Oh, actually, I guess that one probably intended to cover all the media > queries although didn't name all of them explicitly. Right, I posted separate patches for the ones that don't explicitly use `GetSystemMetric`, but I think they all fall in the same category. > -moz-is-glyph isn't a system metric anyway... That's true, but there was a comment (falsely) claiming it was internal, so I just assumed this was overlooked and fixed it. Let me know if you really want another email.
Flags: needinfo?(emilio)
Comment on attachment 8914736 [details] Bug 1396066: Restrict :-moz-system-metric to chrome and ua sheets. https://reviewboard.mozilla.org/r/186018/#review191252
Attachment #8914736 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8914737 [details] Bug 1396066: Restrict system-metric media features to UA and chrome sheets only. https://reviewboard.mozilla.org/r/186020/#review191254 r=me with the issues addressed. ::: layout/style/nsCSSParser.cpp:3505 (Diff revision 1) > // case insensitive from CSS - must be lower cased > nsContentUtils::ASCIIToLower(mToken.mIdent); > nsDependentString featureString(mToken.mIdent, 0); > uint8_t satisfiedReqFlags = 0; > > + if (EnabledState() & (CSSEnabledState::eInUASheets | CSSEnabledState::eInChrome)) { This line is too long. Please have it less than 80 chars. Probably add a line break after `|`. ::: layout/style/nsMediaFeatures.cpp:740 (Diff revision 1) > { > &nsGkAtoms::_moz_touch_enabled, > nsMediaFeature::eMinMaxNotAllowed, > nsMediaFeature::eBoolInteger, > - nsMediaFeature::eNoRequirements, > + nsMediaFeature::eUserAgentAndChromeOnly, > { &nsGkAtoms::touch_enabled }, > GetSystemMetric > }, I'm a bit concerned about this one. From search result, it seems to be widely used as a way to detect touch screen, and is mentioned even in published book. We should probably add a telemetry before removing this, or at least, we should not unship this until we implement the standard equivalent in bug 1035774. ::: layout/style/nsMediaFeatures.cpp:760 (Diff revision 1) > }, > { > &nsGkAtoms::_moz_windows_theme, > nsMediaFeature::eMinMaxNotAllowed, > nsMediaFeature::eIdent, > + // TODO(emilio): Unship. You may not want to remove the comment when you haven't touched their restriction? It probably doesn't matter a lot, though. ::: layout/style/nsMediaFeatures.cpp:784 (Diff revision 1) > { > &nsGkAtoms::_moz_physical_home_button, > nsMediaFeature::eMinMaxNotAllowed, > nsMediaFeature::eBoolInteger, > - nsMediaFeature::eNoRequirements, > + nsMediaFeature::eUserAgentAndChromeOnly, > { &nsGkAtoms::physical_home_button }, > GetSystemMetric > }, This is something we can probably remove directly. It is likely a leftover from B2G. I see there is not even any internal code using it. ::: layout/style/test/test_media_queries.html (Diff revision 1) > - expression_should_be_parseable("-moz-scrollbar-start-backward"); > - expression_should_be_parseable("-moz-scrollbar-start-forward"); > - expression_should_be_parseable("-moz-scrollbar-end-backward"); > - expression_should_be_parseable("-moz-scrollbar-end-forward"); > - expression_should_be_parseable("-moz-scrollbar-thumb-proportional"); > - expression_should_be_parseable("-moz-overlay-scrollbars"); > - expression_should_be_parseable("-moz-windows-default-theme"); > - expression_should_be_parseable("-moz-mac-graphite-theme"); > - expression_should_be_parseable("-moz-mac-yosemite-theme"); > - expression_should_be_parseable("-moz-windows-accent-color-in-titlebar"); > - expression_should_be_parseable("-moz-windows-compositor"); > - expression_should_be_parseable("-moz-windows-classic"); > - expression_should_be_parseable("-moz-windows-glass"); > - expression_should_be_parseable("-moz-touch-enabled"); > - expression_should_be_parseable("-moz-swipe-animation-enabled"); > - > - expression_should_be_parseable("-moz-scrollbar-start-backward: 0"); > - expression_should_be_parseable("-moz-scrollbar-start-forward: 0"); > - expression_should_be_parseable("-moz-scrollbar-end-backward: 0"); > - expression_should_be_parseable("-moz-scrollbar-end-forward: 0"); > - expression_should_be_parseable("-moz-scrollbar-thumb-proportional: 0"); > - expression_should_be_parseable("-moz-overlay-scrollbars: 0"); > - expression_should_be_parseable("-moz-windows-default-theme: 0"); > - expression_should_be_parseable("-moz-mac-graphite-theme: 0"); > - expression_should_be_parseable("-moz-mac-yosemite-theme: 0"); > - expression_should_be_parseable("-moz-windows-accent-color-in-titlebar: 0"); > - expression_should_be_parseable("-moz-windows-compositor: 0"); > - expression_should_be_parseable("-moz-windows-classic: 0"); > - expression_should_be_parseable("-moz-windows-glass: 0"); > - expression_should_be_parseable("-moz-touch-enabled: 0"); > - expression_should_be_parseable("-moz-swipe-animation-enabled: 0"); > - > - expression_should_be_parseable("-moz-scrollbar-start-backward: 1"); > - expression_should_be_parseable("-moz-scrollbar-start-forward: 1"); > - expression_should_be_parseable("-moz-scrollbar-end-backward: 1"); > - expression_should_be_parseable("-moz-scrollbar-end-forward: 1"); > - expression_should_be_parseable("-moz-scrollbar-thumb-proportional: 1"); > - expression_should_be_parseable("-moz-overlay-scrollbars: 1"); > - expression_should_be_parseable("-moz-windows-default-theme: 1"); > - expression_should_be_parseable("-moz-mac-graphite-theme: 1"); > - expression_should_be_parseable("-moz-mac-yosemite-theme: 1"); > - expression_should_be_parseable("-moz-windows-accent-color-in-titlebar: 1"); > - expression_should_be_parseable("-moz-windows-compositor: 1"); > - expression_should_be_parseable("-moz-windows-classic: 1"); > - expression_should_be_parseable("-moz-windows-glass: 1"); > - expression_should_be_parseable("-moz-touch-enabled: 1"); > - expression_should_be_parseable("-moz-swipe-animation-enabled: 1"); I would probably prefer we keep these tests bug mark them `expression_should_not_be_parseable`. Although I guess removing is also fine...
Attachment #8914737 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8914738 [details] Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only. https://reviewboard.mozilla.org/r/186022/#review191260 Mostly looks good. Cancel r? for test change. ::: layout/style/test/chrome/bug418986-2.js (Diff revision 1) > -// These media queries return value 0 or 1 when the pref is off. > -// When the pref is on, they should not match. > -var suppressed_toggles = [ > - "-moz-mac-graphite-theme", > - // Not available on most OSs. > -// "-moz-maemo-classic", > - "-moz-scrollbar-end-backward", > - "-moz-scrollbar-end-forward", > - "-moz-scrollbar-start-backward", > - "-moz-scrollbar-start-forward", > - "-moz-scrollbar-thumb-proportional", > - "-moz-touch-enabled", > - "-moz-windows-compositor", > - "-moz-windows-default-theme", > - "-moz-windows-glass", > -]; > - > -// Possible values for '-moz-os-version' > -var windows_versions = [ > - "windows-win7", > - "windows-win8", > - "windows-win10", > -]; > - > -// Possible values for '-moz-windows-theme' > -var windows_themes = [ > - "aero", > - "aero-lite", > - "luna-blue", > - "luna-olive", > - "luna-silver", > - "royale", > - "generic", > - "zune" > -]; Majority of the test change should be in the previous part? I would prefer we keep each part self-contained so that we can backout any specific part if necessary. Also if we are not removing `-moz-touch-enabled`, we probably need to keep most of the test code below. Actually I would prefer we don't remove the test code, but treat them as if they are always resisted so that we don't regress. Probably marking them not parseable in the media query test is enough, though... ::: layout/style/test/test_media_queries.html (Diff revision 1) > - expression_should_be_parseable("-moz-windows-theme: aero"); > - expression_should_be_parseable("-moz-windows-theme: aero-lite"); > - expression_should_be_parseable("-moz-windows-theme: luna-blue"); > - expression_should_be_parseable("-moz-windows-theme: luna-olive"); > - expression_should_be_parseable("-moz-windows-theme: luna-silver"); > - expression_should_be_parseable("-moz-windows-theme: royale"); > - expression_should_be_parseable("-moz-windows-theme: generic"); > - expression_should_be_parseable("-moz-windows-theme: zune"); > - expression_should_be_parseable("-moz-windows-theme: garbage"); As before, I would prefer we do `not_be_parseable`.
Attachment #8914738 - Flags: review?(xidorn+moz)
Comment on attachment 8914739 [details] Bug 1396066: Remove -moz-physical-home-button. https://reviewboard.mozilla.org/r/186024/#review191262 ::: layout/style/test/test_media_queries.html (Diff revision 1) > - expression_should_be_parseable("-moz-os-version: windows-win7"); > - expression_should_be_parseable("-moz-os-version: windows-win8"); > - expression_should_be_parseable("-moz-os-version: windows-win10"); `not_be_parseable`
Attachment #8914739 - Flags: review?(xidorn+moz) → review+
Is this disabling the equivalent non-standard media features [1] as well, or just :-moz-system-metric pseudo-classes? [1] https://developer.mozilla.org/en-US/docs/Web/CSS/Mozilla_Extensions#Media_features
(In reply to Kohei Yoshino [:kohei] from comment #14) > Is this disabling the equivalent non-standard media features [1] as well, or > just :-moz-system-metric pseudo-classes? > > [1] > https://developer.mozilla.org/en-US/docs/Web/CSS/ > Mozilla_Extensions#Media_features Both of them, although I'm arguing that we should probably not unship -moz-touch-enabled before bug 1035774. The last patch also unships :-moz-is-glyph in addition to all the system metrics stuff, which I guess is probably fine given there is unlikely any reason people want to use it anywhere.
Comment on attachment 8914740 [details] Bug 1396066: Restrict -moz-is-glyph to UA and chrome only. https://reviewboard.mozilla.org/r/186026/#review191266 It is probably fine that we don't test anything around `:-moz-is-glyph`... if it doesn't work properly, it should be easily caught by refests.
Attachment #8914740 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15) > Both of them, although I'm arguing that we should probably not unship > -moz-touch-enabled before bug 1035774. Thanks for your clarification. I was actually thinking about it.
Comment on attachment 8914738 [details] Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only. https://reviewboard.mozilla.org/r/186022/#review191260 > Majority of the test change should be in the previous part? > > I would prefer we keep each part self-contained so that we can backout any specific part if necessary. > > Also if we are not removing `-moz-touch-enabled`, we probably need to keep most of the test code below. > > Actually I would prefer we don't remove the test code, but treat them as if they are always resisted so that we don't regress. Probably marking them not parseable in the media query test is enough, though... I think keeping them in both this test and media queries test is probably more preferable, since if we need to re-enable anything for webcompat, we wouldn't forget that it should still be surpressed when resisting fingerprinting.
Comment on attachment 8914737 [details] Bug 1396066: Restrict system-metric media features to UA and chrome sheets only. https://reviewboard.mozilla.org/r/186020/#review191418 C/C++ static analysis didn't find any defects in this patch. Hooray!
Comment on attachment 8914738 [details] Bug 1396066: Restrict -moz-windows-theme and -moz-os-version to UA and chrome only. https://reviewboard.mozilla.org/r/186022/#review191640
Attachment #8914738 - Flags: review?(xidorn+moz) → review+
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e5b9aed44133 Restrict :-moz-system-metric to chrome and ua sheets. r=xidorn https://hg.mozilla.org/integration/autoland/rev/87b44ea10407 Restrict system-metric media features to UA and chrome sheets only. r=xidorn https://hg.mozilla.org/integration/autoland/rev/b9d8e4588584 Restrict -moz-windows-theme and -moz-os-version to UA and chrome only. r=xidorn https://hg.mozilla.org/integration/autoland/rev/19c9ea492f5e Restrict -moz-is-glyph to UA and chrome only. r=xidorn https://hg.mozilla.org/integration/autoland/rev/5b1997aeaead Remove -moz-physical-home-button. r=xidorn
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 74a0c5bdb21c -d a701f7c8334f: rebasing 424496:74a0c5bdb21c "Bug 1396066: Restrict :-moz-system-metric to chrome and ua sheets. r=xidorn" note: rebase of 424496:74a0c5bdb21c created no changes to commit rebasing 424497:7d8e0e0215f2 "Bug 1396066: Restrict system-metric media features to UA and chrome sheets only. r=xidorn" merging layout/style/nsMediaFeatures.cpp merging layout/style/test/chrome/bug418986-2.js merging layout/style/test/test_media_queries.html warning: conflicts while merging layout/style/nsMediaFeatures.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
So autocomplete-item.css worries me. Why is the test failing for that? It should be loaded from a chrome:// URI, I would think...
Flags: needinfo?(emilio)
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a22efa8df6b followup. Fix the expected message to be the right non-ASCII thing, admit that devtools don't use UA-only media features.
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00 another followup. Load input.css from a chrome uri, so it can use :-moz-system-metric. This requires exposing the reftest chrome package to content, but that should be OK. r=bzbarsky
windows/autocomplete-item.css contains -moz-windows-default-theme, so that should probably be available on chrome (it is even possible we need to keep exposing it to content, since it seems to be in an XBL.) That should definitely not be whitelisted. The whitelist for svg.css also looks too loose. It should probably be specific to only the violating ones that are ua-only.
(In reply to Pulsebot from comment #37) > Pushed by bzbarsky@mozilla.com: > https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00 > another followup. Load input.css from a chrome uri, so it can use > :-moz-system-metric. This requires exposing the reftest chrome package to > content, but that should be OK. r=bzbarsky I suspect that it wouldn't work. IIRC availability of chrome-only features depends on document url rather than stylesheet url?
Just figured out that there are several media features that we still need to expose because chrome code uses them in matchMedia. Also there are some features that can be removed completely, because they are no longer used.
Yeah, that still doesn't explain why https://hg.mozilla.org/integration/autoland/rev/e41f9295ee00 doesn't work, but anyway, Xidorn and I discussed and we're going to back this out for now, and reland more incrementally.
Flags: needinfo?(emilio)
Backout by philringnalda@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4279ffa1e0da Backed out 9 changesets for Windows reftest failures
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2c404f982003 Add a mechanism to make media features chrome / UA-only. r=xidorn
Keywords: leave-open
Depends on: 1406631
Depends on: 1408838
Depends on: 1408839
Depends on: 1410074
Bug 1410074 takes care of the last ones, yay!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Keywords: leave-open
I've documented this; first of all take a look at the Fx 58 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/58#CSS_2 All of the pages I've touched are listed there, and they match the pages listed in Kohei's compat article. On each of the pages I've added a note stating that they are no longer available to web content in Firefox 58. I've also updated the Mozilla CSS extensions page, moving the affected pseudo-classes to this section: https://developer.mozilla.org/en-US/docs/Web/CSS/Mozilla_Extensions#Mozilla-only_properties_and_pseudo-classes_(avoid_using_on_websites) Let me know if that look OK.
Keywords: dev-doc-needed → dev-doc-complete
Is it intentional that these should be unavailable to userChrome.css too? Not being able to define styles per-os and per-theme can be quite limiting.
(In reply to Alex Vallat from comment #47) > Is it intentional that these should be unavailable to userChrome.css too? > Not being able to define styles per-os and per-theme can be quite limiting. I think you should be able to still style per theme with :-moz-lwtheme-* pseudoclasses, but... Per [1], it looks these are loaded with file:// URIs, so they're not loaded as "chrome" stylesheets. Also, there's nothing in the CSS system that makes these available to user sheets... I can see no reason why they shouldn't though, mind filing a bug and needinfo me? [1]: https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/layout/style/nsLayoutStylesheetCache.cpp#443
Sorry, I meant windows theme, like -moz-windows-classic, not -moz-lwtheme themes. I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1418963 as requested.
Depends on: 1418963
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: