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

CSP: Hide wrapper span with class instead of style attribute #581

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

Conversation

JohanWinther
Copy link

@JohanWinther JohanWinther commented Jun 22, 2022

CSP nonces need to be added to any elements with a style attribute

This PR fixes the issue found in #570 (comment)

CSP nonces need to be added to any elements with a style attribute
@choonkeat
Copy link

With this patch, I'm still seeing the csp errors in console of Safari, Firefox, Chrome. I believe for style-src, the nonce attribute only works for <style> elements; not meant for style= attributes

@JohanWinther
Copy link
Author

JohanWinther commented Jun 23, 2022

Oh, so there's no way to use inline style attributes with nonces?
Then I guess the only option left is to use the hash. Since the hash of display: none; never changes, you should be able to add it to your style-src with "unsafe-hashes".

Reference: https://content-security-policy.com/examples/allow-inline-style/ (the section about inline style attribute).

@JohanWinther JohanWinther deleted the nonce-fix branch June 23, 2022 15:06
@choonkeat
Copy link

Then I guess the only option left is to use the hash... "unsafe-hashes"

I'm vendoring this repo and patching it :-/ instead of explaining "unsafe-hashes" to whom it may concern 😆

@JohanWinther JohanWinther restored the nonce-fix branch June 25, 2022 09:40
@JohanWinther
Copy link
Author

JohanWinther commented Jun 25, 2022

I realised that perhaps the function can omit the style attribute if you use a nonce, so you will have to use the class to hide the span. I'm reopening this and then I'll push these changes.

Edit: I still think for most users is easier to add unsafe-hashes rather than introducing a new stylesheet to the CSP, especially if you only use elm-css.

@JohanWinther JohanWinther reopened this Jun 25, 2022
@JohanWinther JohanWinther changed the title Add nonce to styled span Hide wrapper span with class instead of style attribute Jun 25, 2022
@JohanWinther
Copy link
Author

JohanWinther commented Jun 25, 2022

@choonkeat
I think my latest commit 4c1b33d fixes all of the issues. By adding the style display: none; to the wrapper class internally we can

  1. avoid the style attribute,
  2. avoid having to force the user to add the style and
  3. still allow a user to override the style with the span.elm-css-style-wrapper selector

I saw that there is no nonce for the global function so I will try to implement that too (in a separate PR). Right now the <style> element that is created through global is represented as an Unstyled node, which means that toNonceUnstyled will not add the nonce to the <style> inside. I will have to explore a bit to solve it though.

@choonkeat
Copy link

|> styleToDeclaration (getCssTemplate [ Css.display Css.none ]) styleWrapperClass

nice solution!

I saw that there is no nonce for the global function

yes, I have a quick patch for local use for now

type Nonce
    = Nonce String

noncedGlobal : Nonce -> List Snippet -> Html.Styled.Html msg
noncedGlobal (Nonce nonce) snippets =
    snippets
        |> Preprocess.stylesheet
        |> Resolve.compile
        |> VirtualDom.text
        |> List.singleton
        |> VirtualDom.node "style" [ VirtualDom.attribute "nonce" nonce ]
        |> List.singleton
        |> VirtualDom.node "span"
            [ VirtualDom.attribute "class" "elm-css-style-wrapper"
            ]
        |> VirtualDom.Styled.unstyledNode

not particularly joyful to have 2 Nonce types

@JohanWinther JohanWinther changed the title Hide wrapper span with class instead of style attribute CSP: Hide wrapper span with class instead of style attribute Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants