Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

seanbudd
Copy link
Member

@seanbudd seanbudd commented Jan 6, 2025

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:

  • Tested from console by calling ui.browseableMessage
import ui
import nh3
clean = lambda x: nh3.clean(x, tags={"i"})
ui.browseableMessage("<b><i>test</i></b>", "title example", isHtml=True, sanitizeHtmlFunc=clean)

Confirm only italic formatting is shown.

Known issues with pull request:

None

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@coderabbitai summary

@seanbudd seanbudd marked this pull request as ready for review January 6, 2025 00:17
@seanbudd seanbudd requested a review from a team as a code owner January 6, 2025 00:17
@seanbudd seanbudd requested a review from SaschaCowley January 6, 2025 00:17
@AppVeyorBot
Copy link

See test results for failed build of commit 2d1a334176

Copy link
Member

@SaschaCowley SaschaCowley left a 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

source/ui.py Outdated Show resolved Hide resolved
source/ui.py Outdated Show resolved Hide resolved
user_docs/en/changes.md Outdated Show resolved Hide resolved
@SaschaCowley SaschaCowley added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 6, 2025
@seanbudd seanbudd requested a review from SaschaCowley January 7, 2025 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-breaking-change conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sanitize browsableMessage HTML
3 participants