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

EREGCSC-2363 -- Prevent frontend from interpreting HTML characters #1502

Merged
merged 5 commits into from
Dec 23, 2024

Conversation

PhilR8
Copy link
Contributor

@PhilR8 PhilR8 commented Dec 16, 2024

Resolves EREGCSC-2363

Description

On the front end of eRegs, we sometimes use Vue's v-html directive to insert raw HTML into an element's innerHTML property. We do this for a few reasons:

  • Table elements in the regs are returned from the eCFR parser as raw HTML, and we need to pass that raw HTML straight to the DOM for it to render and be styled correctly in the Reader View
  • When using the eRegs API to search for a query string (ex: on the Search page), the matched string is returned from the server surrounded by a span with a search-highlight class.
    • Note: we control the eRegs API and can change how the highlighted text is identified in the API response -- Search.gov search results use special characters that can be identified and replaced with span elements on the front end. This would likely still require using the v-html directive to insert any transformed elements into the DOM because we don't use JSX with Vue.

The downside to using the v-html directive is that the developer has to ensure that the content inserted into the DOM via the v-html directive is trusted. We haven't been doing that, and our usage of the v-html directive resulted in an XSS vulnerability pentest finding on 12/13/14.

As such, we need to better ensure that the content inserted into the DOM via the v-html directive is sanitized. This Pull Request adds the DOMPurify library to the project to create a custom Vue directive that sanitizes any raw HTML that needs to be inserted directly into the DOM.

This pull request changes:

  • Adds DOMPurify npm package to project
  • Creates v-sanitize-html custom directive that uses DOMPurify to sanitize any HTML passed to it
  • Uses v-sanitize-html directive instead of v-html throughout entire application

Steps to manually verify this change:

  1. Green check marks
  2. Ensure v-html directive is not used anywhere in codebase
  3. Ensure that v-sanitize-html has replaced all instances of v-html
  4. Visit experimental deployment
  5. Search for "CCIC"
  6. When the Search results page loads, the browser should NOT show an alert dialog. The CCIC search result should be shown in the list of results.
  7. Compare to dev: search for "CCIC" on the dev site. When the Search results page loads, an alert dialog will appear.
  8. Rest of site works as expected

Copy link

✨ See the Django Site in action

Copy link

✨ See the Django Site in action

@PhilR8 PhilR8 marked this pull request as ready for review December 17, 2024 16:57
@PhilR8 PhilR8 requested a review from cgodwin1 as a code owner December 17, 2024 16:57
@PhilR8 PhilR8 added the Needs Review This PR needs a code review label Dec 17, 2024
Copy link

✨ See the Django Site in action

Copy link
Contributor

@cgodwin1 cgodwin1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cgodwin1 cgodwin1 added Approved and removed Needs Review This PR needs a code review labels Dec 23, 2024
@PhilR8 PhilR8 merged commit 6ba08d6 into main Dec 23, 2024
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants