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

Errors immediately when loaded in a sandboxed iframe #112

Closed
guanzo opened this issue Mar 29, 2020 · 8 comments · Fixed by #171
Closed

Errors immediately when loaded in a sandboxed iframe #112

guanzo opened this issue Mar 29, 2020 · 8 comments · Fixed by #171
Labels
bug Something isn't working

Comments

@guanzo
Copy link

guanzo commented Mar 29, 2020

Hi, I'm using this lib in a twitch.tv Extension, which runs in a sandboxed iframe with a lot of security rules. https://dev.twitch.tv/docs/extensions/#restrictions-on-content

One of the rules script-src, affects the new Worker() call, causing this error

img

The corresponding code

  function confettiCannon(canvas, globalOpts) {
    var isLibCanvas = !canvas;
    var allowResize = !!prop(globalOpts || {}, 'resize');
    var shouldUseWorker = canUseWorker && !!prop(globalOpts || {}, 'useWorker');
    console.log({ isLibCanvas, shouldUseWorker, canvas, globalOpts })
    console.log('about to create worker?')
    var worker
    try {
        worker = shouldUseWorker ? getWorker() : null;
    } catch (err) {
        console.log('YOOO', err)
    }

This was confusing at first, because I never set the useWorker: true option. Turns out, the lib creates a Worker on initialization, and an error gets thrown. There should be a try/catch somewhere.

Just a suggestion, fix it however you want, but I believe this should be fixed.

I can create a reproduction if you want.

@catdad catdad added the bug Something isn't working label Mar 29, 2020
@catdad
Copy link
Owner

catdad commented Mar 29, 2020

I don't use Twitch. Can you possibly get me the full Content-Security-Policy header they are using to make sure I am testing this properly?

@guanzo
Copy link
Author

guanzo commented Mar 29, 2020

content-security-policy: default-src 'self' https://6rh8h42nhnjmirlyxc0w9lbpmeehi0.ext-twitch.tv; block-all-mixed-content; img-src * data: blob:; media-src * data: blob:; frame-ancestors https://supervisor.ext-twitch.tv https://extension-files.twitch.tv https://*.twitch.tv https://*.twitch.tech https://localhost.twitch.tv:* https://localhost.twitch.tech:* http://localhost.rig.twitch.tv:*; font-src https://6rh8h42nhnjmirlyxc0w9lbpmeehi0.ext-twitch.tv https://fonts.googleapis.com https://fonts.gstatic.com; style-src 'self' 'unsafe-inline' https://6rh8h42nhnjmirlyxc0w9lbpmeehi0.ext-twitch.tv https://fonts.googleapis.com; connect-src https: wss: https://www.google-analytics.com https://stats.g.doubleclick.net; script-src 'self' https://6rh8h42nhnjmirlyxc0w9lbpmeehi0.ext-twitch.tv https://extension-files.twitch.tv https://www.google-analytics.com;

catdad added a commit that referenced this issue Mar 30, 2020
falling back to rendering in the main thread if the worker cannot be created
@catdad
Copy link
Owner

catdad commented Mar 30, 2020

A fix was published in 1.1.3

@catdad catdad closed this as completed Mar 30, 2020
@guanzo
Copy link
Author

guanzo commented Mar 30, 2020

Thanks so much.

@guanzo
Copy link
Author

guanzo commented Apr 1, 2020

Hey, this is great and all, but the error still appears in console. I guess this is the type of error that a try/catch won't silence?

My extension is installed by hundreds of thousands of channels, so I really don't want this error appearing. Even if 99% of users don't see it, it still reflects poorly on me imo.

I'm trying to think of a potential solution. I think the main problem for me, is that you're calling new Worker() automatically, I can't prevent it from my code. Could you perhaps.. not do that?
Then I could call confetti.create($canvas, { useWorker: false }), and bypass the codepath that leads to new Worker(), so the error is never thrown.

As for the warning, could you somehow allow it to be disabled in production builds?

if (typeof process !== undefined && process.env && process.env !== 'production') {
  // console.warn
}

That's just an example, but the point is to somehow silence the warning in production.

To be honest, canvas-confetti is not important for my extension... I just love it a lot and want to share it with my users. If this error can't be silenced, then I'll have to remove the library, and I don't want to do that. Thanks!

EDIT: I actually found an acceptable solution through lazy loading, by only loading the lib when I actually need it. So, disregard this I suppose.

@adamreisnz
Copy link
Contributor

I am also seeing this error, so I prefer this not be disregarded but reopened if possible.
Can the script avoid using service workers altogether unless this is enabled explicitly?

@adamreisnz
Copy link
Contributor

adamreisnz commented Sep 23, 2020

Ideally, as mentioned above, the worker code in getWorker would not self-execute and would only be called if shouldUseWorker is true.

Or any other mechanism that silences the error, because we can't allow for random code blobs in our CSP policy, and now this error is showing up in production for all our tens of thousands of users.

@catdad
Copy link
Owner

catdad commented Sep 23, 2020

I apologize, I meant to comment and I guess I never did. This is not being ignored. Rather, I do not agree that it is desirable for this library.

Workers need to be enabled by default. Without workers, I cannot guarantee that the animation in this library will be smooth. What is worse, without workers, I cannot guarantee that any other work that your page needs to do will not be interrupted or have its performance penalized.

We all have to live with the fact that users simply don't read documentation. I can't bury an option somewhere that says "here is how to turn good performance on". That is irresponsible from the standpoint of a maintainer. Therefore, unless there is a compelling argument, workers will stay on by default. This allows the most users to have the best performance.

Now, since this is an expectation, users need to know when they have inadvertently affected that. It should not be silent. If you copied a content security policy from a random blog because it said it was good but you also expect nothing to change, obviously that is not going to work. You absolutely can choose to trade performance for security in the correct context, but you need to know that that is what you are doing.

Finally, to address the issue of "but my users will open up their console and see an error and think that I am a terrible developer". Frankly, I was hoping not to have to address this, but here we are. It is a pretty rate sight to see a website with a silent console. Most popular websites include plenty of messages. Similar libraries that rely on web workers also use them by default, resulting in an error as well as console warning when web workers cannot be used. Even sites that you actually expect developers to open the console on -- like jsfiddle or codepen -- have some amount of noise in their console. I just don't buy the argument that it affects the popularity of your website, extension, plugin, or what have you. Maybe I am wrong, and would like to hear if you have substantial evidence otherwise. For the time being, I think what I have mentioned above warrants using workers by default, as well as the error and warning you see in the console.

liamcmitchell-sc added a commit to liamcmitchell-sc/canvas-confetti that referenced this issue Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants