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

Spin.js dependency throws style-src-elem inline CSP warning #14873

Closed
1 task
alexgibson opened this issue Jul 24, 2024 · 7 comments · Fixed by #14880
Closed
1 task

Spin.js dependency throws style-src-elem inline CSP warning #14873

alexgibson opened this issue Jul 24, 2024 · 7 comments · Fixed by #14880
Labels
Papercuts 💸 Tech debt payoff

Comments

@alexgibson
Copy link
Member

alexgibson commented Jul 24, 2024

Description

It looks like an old dependency we have, spin.js, violates style-src-elem inline CSP.

https://mozilla.sentry.io/share/issue/731d31b3e0a8411088056e46031c3906/

It looks like it's used in two places:

I think we could remove it as a dependency, and create a simpler loading indicator of sorts.


Success Criteria

  • spin.js is removed as a dependency
@janbrasna
Copy link
Contributor

janbrasna commented Jul 24, 2024

It should be CSP-compliant (or at least have an option to):

I see it's being statically vendored (since 8/2014) instead of dependent on npm package, so it's not getting any updates. My guess it's v2.0.x here, the CSP is fixed since v3.0.0 and current is v4.1.x…

EDIT, yup it's 2.0.1:

//fgnass.github.com/spin.js#v2.0.1

I'll see what it takes to move to the newer syntax.

@janbrasna
Copy link
Contributor

Removed hwaccel and trail options that would need describing otherwise. Besides that the API is the same. (Will also need looking into the CSS animation definition that we may have to add somewhere.)

But the upgrade sounds completely doable. Slowly looking into it…

@janbrasna
Copy link
Contributor

But yea it's probably too overkill when you can just show/hide an SVG with all the keyframes already defined eg:

spin

<svg viewBox="0 0 800 800" xmlns="http://www.w3.org/2000/svg">
    <style>
    @keyframes spin {
      to {
        transform: rotate(360deg);
      }
    }
    @keyframes spin2 {
      0% {
        stroke-dasharray: 1, 800;
        stroke-dashoffset: 0;
      }
      50% {
        stroke-dasharray: 400, 400;
        stroke-dashoffset: -200px;
      }
      100% {
        stroke-dasharray: 800, 1;
        stroke-dashoffset: -800px;
      }
    }
    .spin2 {
      transform-origin: center;
      animation: spin2 1.5s ease-in-out infinite,
        spin 2s linear infinite;
      animation-direction: alternate;
    }
    </style>
    <circle class="spin2" cx="400" cy="400" fill="none"
      r="200" stroke-width="50" stroke="#E387FF"
      stroke-dasharray="700 1400"
      stroke-linecap="round" />
</svg>

@janbrasna
Copy link
Contributor

janbrasna commented Jul 24, 2024

FYI: The spinner_color passed around all the included email_newsletter_form instances is a no-op it seems — looking through the form includes, I can't seem to find the container for the spinner there. So unless I'm missing something, it's not even displayed while the form submits (as it got removed here in f34616d and I'm not having success finding some JS or another way it would bind to the form…).

In which case the spinner_color would need to be documented as no-op and just left out…

The only place the spinner really is displayed is the newsletter-management page, and the send_to_device forms.

Why I'm mentioning it is that if this is simplified to a bare animated SVG, that would again have inline styles so can't be inlined, thus can't inherit any currentColor etc., so any colorising is quite off the table with CSP tightening… (I'll look into SMIL that's mostly attributes and won't need unsafe inlines, can be inlined in the markup, but then the question is whether this won't be better as CSS background on the target element if all the usage has the bundles set, I'll investigate…)

EDIT: It is being set to some brand colours on the forms that don't display the spinner at all now, so no loss there, just needs cleaning up since it does nothing. The forms that use the spinner all set the default black so no loss there either.

@janbrasna
Copy link
Contributor

janbrasna commented Jul 29, 2024

For now I went with the smallest change surface keeping the original markup (as it is also used in tests etc.) and chose SVG+CSS via background in #14880

I will open the more meta spinner_color issue (more related to all email_newsletter_form instances than this) separately later, and will resolve that accordingly. Here it is made a no-op as well for now, just using the setting always used for these forms.

@alexgibson
Copy link
Member Author

I will open the more meta spinner_color issue (more related to all email_newsletter_form instances than this) separately later, and will resolve that accordingly. Here it is made a no-op as well for now, just using the setting always used for these forms.

@janbrasna there's no need to open an issue for this I think. The spinner is no longer used in email forms, it was removed quite a long time ago. If there's any left-over code related to it then that can just be removed. Thanks

@janbrasna
Copy link
Contributor

@alexgibson Alright, if it's not coming back it should just be removed from all the macros and docs and the param completely removed from the helper. I'll comb through it at some point to wipe the leftovers 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Papercuts 💸 Tech debt payoff
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants