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.

Sign up for GitHub

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

Jump to bottom

[fix] Modal's use of setting .style breaks strict Content-Security-Policy #1926

Open
TomNUSDS opened this issue Mar 23, 2022 · 6 comments · May be fixed by #2761
Open

[fix] Modal's use of setting .style breaks strict Content-Security-Policy #1926

TomNUSDS opened this issue Mar 23, 2022 · 6 comments · May be fixed by #2761
Assignees
@werdnanoslen
Labels
type: bug Something isn't working like it's supposed to type: security Pull requests that address a security vulnerability

Comments

@TomNUSDS
Copy link

TomNUSDS commented Mar 23, 2022 •
edited
Loading

ReactUSWDS Version & USWDS Version:

ReactUSWDS: 2.7.2
USWDS: 2.13.0

Describe the bug
A CSP (content-security-policy) is good requirement for government sites. See https://content-security-policy.com/

A starting point for a CSP includes style-src 'self' This means that using <element style= is forbidden as well as a page having a <style> header. Only class names and external stylesheets.

To Reproduce
Steps to reproduce the behavior:

  1. Have a static build project (generates a static html site).

  2. Add a CSP to the index.html as a <meta> (this is an alternative to modifying server headers)

    <meta
        http-equiv="Content-Security-Policy"
        content="style-src 'self'; "/>

(DISCLAIMER: this is the minimal CSP to demonstrate this bug an actual CSP would include a lot more restrictions)

  1. Add code to display a react-uswds Modal dialog

  2. Load page with the Modal dialog
    Result: the debug console shows

Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self' ...
t.getScrollbarWidth | @ | index.js:2

Discussion

The Modal's getScrollbarWidth() function to measure the width of the scrollbar breaks this policy. Specifically

outer.setAttribute(
      'style',
      'visibility: hidden; overflow: scroll; ms-overflow-style: scrollbar'
    )

This should be a relatively easy fix using a class name (see: https://stackoverflow.com/questions/986937/how-can-i-get-the-browsers-scrollbar-sizes mpen answer which includes a link to David Walsh's blog:).

Device and Browser Information (please complete the following information if describing a UI bug):

  • OS: Mac OS
  • Browser: Chrome v99
@TomNUSDS TomNUSDS added the type: bug Something isn't working like it's supposed to label Mar 23, 2022
@hgarfinkle
Copy link
Contributor

When this gets fixed, I think it'd be a great idea to also include a CI check that a CSP wouldn't get violated by future changes.

@hgarfinkle
Copy link
Contributor

@TomNUSDS I'm thinking of using this package https://www.npmjs.com/package/scrollbar-width to solve this issue, rather than having react-uswds implement its own width checker. I'm unfamiliar with CSPs, but I want react-uswds to be as compliant as possible. Will this little package suffice?

@TomNUSDS
Copy link
Author

TomNUSDS commented Apr 7, 2022 •
edited
Loading

Thanks for looking into this issue and making security a priority!

I checked out that package and unfortunately, it seems to manipulate styles directly:

    div1 = document.createElement('div');
    div2 = document.createElement('div');
    div1.style.width = div2.style.width = div1.style.height = div2.style.height = '100px';
    div1.style.overflow = 'scroll';
    div2.style.overflow = 'hidden';

The only solutions that are CSP compliant use class names, put the required styles into that css classes and set the element's className. This is a bit of a pain because it requires the css to be included with the build versus just a pure code approach.

Another possible solution for your Dialog, is to pass in a default scrollbar width as an optional property and if it's set, then the component Dialog doesn't do the measurement. This would allow code that cares about CSP to implement their own safe of measuring it and pass it along.

If you're interested in learning how css style manipulation can be used as an attack vector, here's a good article on invicti.com.

@haworku haworku added the type: security Pull requests that address a security vulnerability label Apr 20, 2022
@werdnanoslen werdnanoslen linked a pull request Feb 13, 2024 that will close this issue
Open
@werdnanoslen werdnanoslen self-assigned this Feb 13, 2024
@werdnanoslen
Copy link
Contributor

Hi @TomNUSDS can you check #2761 to see if it works for your CSP?

@TomNUSDS
Copy link
Author

Your fix LGTM!

Thanks for the follow-up!

@tw-sarah
Copy link

Hi, Is there an update on this? Looks like there is a pull request from February, but it's still sitting in open waiting for approval. The code still does x.setAttribute('style',''visibility: hidden; overflow: scroll; ms-overflow-style: scrollbar''), which violates CSP. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@werdnanoslen werdnanoslen

Labels
type: bug Something isn't working like it's supposed to type: security Pull requests that address a security vulnerability
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Add style as class to fix CSP issue trussworks/react-uswds
5 participants
@tw-sarah @werdnanoslen @hgarfinkle @haworku @TomNUSDS