-
-
Notifications
You must be signed in to change notification settings - Fork 652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sanitize browseableMessage html #17581
base: master
Are you sure you want to change the base?
Conversation
See test results for failed build of commit 2d1a334176 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nitpicky suggestions
Co-authored-by: Sascha Cowley <[email protected]>
Do changes need to be made to bundle nh3 with NVDA, or will it automatically be included because it has been imported in |
It should be automatically included - this is proven when running the installer. However, it won't import everything, only what is needed like other dependencies |
Link to issue number:
Fixes #16985
Summary of the issue:
ui.browsableMessage
can inject unsanitized HTML into NVDA.This is an issue if translations are passed in as unsanitized HTML.
Translations are fairly unregulated, translation strings are the only "code" included in NVDA without a direct review from NV Access or as a review as a dependency. If NVDA translations can perform RCE, we have a problem.
Considering no NVDA source code uses the
isHtml
functionality of this function currently, this isn't an active vector.However, if we ever start using
isHtml
, it becomes an active vector, which is something we want to avoid and prevent from becoming a possibility.Description of user facing changes
HTML is now sanitised for browseableMessages. This will only affect add-ons. It may cause some add-ons to lose formatting in browseableMessages that use HTML. To restore certain formatting, authors can update the whitelisting by calling
nh3.clean
with their own desired parameters.Description of development approach
Added a sanitization function to
browseableMessage
.Testing strategy:
ui.browseableMessage
Confirm only italic formatting is shown.
Known issues with pull request:
None
Code Review Checklist:
@coderabbitai summary