[Firefox] Disable the ability to change preferences directly from the viewer #16583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… viewer Please note that we've never had any functionality in the viewer itself that *set* preferences, and we've thus only ever read them. For the GENERIC viewer it obviously makes sense for the user to be able to modify preferences, e.g. via the console, but that doesn't really apply to the *built-in* Firefox PDF Viewer since preferences are already accessible via `about:config` there. Hence it does seems somewhat strange to expose, a limited part of, the Firefox preference system in this way when we're not even using it. Note that the unused preference setting-code also include a fair amount of *additional* validation on the platform-side, such as limiting any possible preference changes to the `pdfjs.`-branch and also an explicit white-list of preference names[1], to make sure that this is safe; please see: - https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/toolkit/components/pdfjs/content/PdfStreamConverter.sys.mjs#458-495 - https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/toolkit/modules/AsyncPrefs.sys.mjs#21-48 Assuming that this patch lands, I'll follow-up with a mozilla-central patch to remove the code mentioned above. --- [1] This hard-coded list contains preferences that no longer exist, and also at least one (fairly obvious) typo.
Note how the [`ChromeActions.getPreferences` method](https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/toolkit/components/pdfjs/content/PdfStreamConverter.sys.mjs#497-530) returns the preferences as a string, which we then have to convert back into an Object in the viewer. Back when that code was originally written it wasn't possible to send Objects from the platform-code, however that's no longer the case and we should be able to (eventually) remove this unnecessary string-parsing now. *Please note that in order to prevent breakage we'll need to land these changes in stages:* - Land this patch in mozilla-central, as part of regular the PDF.js updates. - Change the return type in the `ChromeActions.getPreferences` method, in a mozilla-central patch. - Remove the string-handling from the `FirefoxPreferences._readFromStorage` method.
calixteman
approved these changes
Jun 21, 2023
|
/botio integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/8440e7b33bbe151/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/1ede192cb0c52a6/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/1ede192cb0c52a6/output.txt Total script time: 4.49 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/8440e7b33bbe151/output.txt Total script time: 14.29 mins
|
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Jun 24, 2023
…the PDF Viewer. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Differential Revision: https://phabricator.services.mozilla.com/D181878
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Jun 24, 2023
…r preferences. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Depends on D181878 Differential Revision: https://phabricator.services.mozilla.com/D181879
jamienicol
pushed a commit
to jamienicol/gecko
that referenced
this pull request
Jun 27, 2023
…the PDF Viewer. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Differential Revision: https://phabricator.services.mozilla.com/D181878
jamienicol
pushed a commit
to jamienicol/gecko
that referenced
this pull request
Jun 27, 2023
…r preferences. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Depends on D181878 Differential Revision: https://phabricator.services.mozilla.com/D181879
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified-and-comments-removed
that referenced
this pull request
Jul 1, 2023
…the PDF Viewer. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Differential Revision: https://phabricator.services.mozilla.com/D181878 UltraBlame original commit: aa0f336c52a100b99e4aacbdbaa64705cacb650d
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified-and-comments-removed
that referenced
this pull request
Jul 1, 2023
…r preferences. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Depends on D181878 Differential Revision: https://phabricator.services.mozilla.com/D181879 UltraBlame original commit: 21d4902198ae482d7fd52735881f90fb09ffaa07
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-comments-removed
that referenced
this pull request
Jul 1, 2023
…the PDF Viewer. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Differential Revision: https://phabricator.services.mozilla.com/D181878 UltraBlame original commit: aa0f336c52a100b99e4aacbdbaa64705cacb650d
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-comments-removed
that referenced
this pull request
Jul 1, 2023
…r preferences. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Depends on D181878 Differential Revision: https://phabricator.services.mozilla.com/D181879 UltraBlame original commit: 21d4902198ae482d7fd52735881f90fb09ffaa07
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified
that referenced
this pull request
Jul 1, 2023
…the PDF Viewer. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Differential Revision: https://phabricator.services.mozilla.com/D181878 UltraBlame original commit: aa0f336c52a100b99e4aacbdbaa64705cacb650d
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified
that referenced
this pull request
Jul 1, 2023
…r preferences. r=pdfjs-reviewers,robwu Follow-up to mozilla/pdf.js#16583 Depends on D181878 Differential Revision: https://phabricator.services.mozilla.com/D181879 UltraBlame original commit: 21d4902198ae482d7fd52735881f90fb09ffaa07
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Firefox] Disable the ability to change preferences directly from the viewer
Please note that we've never had any functionality in the viewer itself that set preferences, and we've thus only ever read them.
For the GENERIC viewer it obviously makes sense for the user to be able to modify preferences, e.g. via the console, but that doesn't really apply to the built-in Firefox PDF Viewer since preferences are already accessible via
about:configthere. Hence it does seems somewhat strange to expose, a limited part of, the Firefox preference system in this way when we're not even using it.Note that the unused preference setting-code also include a fair amount of additional validation on the platform-side, such as limiting any possible preference changes to the
pdfjs.-branch and also an explicit white-list of preference names[1], to make sure that this is safe; please see:Assuming that this patch lands, I'll follow-up with a mozilla-central patch to remove the code mentioned above.
[1] This hard-coded list contains preferences that no longer exist, and also at least one (fairly obvious) typo.
[Firefox] Avoid unnecessary string-parsing when reading preferences
Note how the
ChromeActions.getPreferencesmethod returns the preferences as a string, which we then have to convert back into an Object in the viewer.Back when that code was originally written it wasn't possible to send Objects from the platform-code, however that's no longer the case and we should be able to (eventually) remove this unnecessary string-parsing now.
Please note that in order to prevent breakage we'll need to land these changes in stages:
ChromeActions.getPreferencesmethod, in a mozilla-central patch.FirefoxPreferences._readFromStoragemethod.