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 - Consider static option to avoid unsafe-inline violation #41

Open
AdamWillden opened this issue Mar 26, 2019 · 3 comments
Open

CSP - Consider static option to avoid unsafe-inline violation #41

AdamWillden opened this issue Mar 26, 2019 · 3 comments

Comments

@AdamWillden
Copy link

AdamWillden commented Mar 26, 2019

Your code injects an inline style into the head on load. I'd like a static option to disable this and ideally for you to provide a CSS file alongside your distributed JS so it can be loaded more appropriately.

I don't think we should be forced to use a CSP value of style-src 'unsafe-inline' when using the plugin. What I am proposing is a non-breaking option to skip the following code block:

$(function(){
        // check for static option and return if set true

        $("head").append([
            "<style>",
                "@-webkit-keyframes loadingoverlay_animation__rotate_right {",
                  "to {",
                    "-webkit-transform : rotate(360deg);",
                            "transform : rotate(360deg);",
                  "}",
                "}",
//... etc

If you don't want the additional overhead of managing a separate CSS file I'd be happy with an option to disable the injection. I'd simply copy the offending CSS to my local file.

@gasparesganga
Copy link
Owner

Thank you for pointing this out. Honestly, I didn't think about that at all!

What do you mean exactly with "a static option"? Since that code gets injected into the head as soon as the document loads, without any interaction with the plugin itself, there is no way to prevent it at the moment.
I can see 2 possible solutions:

  1. CSS does not get injected until the first real call to LoadingOverlay is made. A static method "DisableCSSInjection" (you can choose the name, I'm not good with them...) will be provided, so that calling that before displaying any LoadingOverlay will effectively disable it. Then of course a separate CSS file will be provided.

  2. Releasing a CSSInjectionLess version of the plugin, which does not include the CSS injection, along with the complete one. Again, a separate CSS will be provided too.

Both ways are fine, but I am more inclined toward the first one... it should cause less confusion than having two versions of the main JS plugin file.

What do you think about that?

@AdamWillden
Copy link
Author

@gasparesganga very good of you to get back to me so quickly. It is sincerely appreciated and the sign of a well maintained plugin! Thank you.

I agree you wouldn't want two separate files to maintain.

As for the first option,something that runs on first execution should be perfectly fine and is probably more self-contained.

A static option was something along the lines of window.blah = false or something under your $.LoadingOverlay namespace the relevant code source would have to be loaded before yours in order to be respected. Lets forget the static approach, I'm happier with the approach you mentioned!


I've since realised I don't even use your styles at all so I'd have no need to even reference the CSS! I could just set the new option and do nothing else.

I abuse the fontawesome feature to add my own style class combined with the background class to get the styling I want.

      $.LoadingOverlaySetup({
        backgroundClass: 'o-overlay',
        fade: [150, 150],
        fontawesome: 'fas fa-cog fa-spin o-overlay_spinner',
        image: null
      });

@gasparesganga
Copy link
Owner

This plugin turned out to be quite popular, so I try to maintain the best way I can :)

Yep, I'd reather avoid some code which relies on the loading order, a static method is clearer, more maintainable and cleaner.

I will include this change in the upcoming v3 release and the separate CSS is not a problem, since I compose the dist files using Gulp, I will tweak the gulpfile.
I think it's better to have that optional CSS file, maybe you aren't using it now, but who knows later on? And of course there will be other people willing to use the new feature.

I'll leave this open and close it with a commit which includes the feature. Feel free to add anything else related to this, I'll work on that in a couple of weeks.

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants