Skip to content

Conversation

mklilley
Copy link

@mklilley mklilley commented Jan 5, 2021

Old version of seedrandom used eval which is not considered safe against XSS attacks (see https://infosec.mozilla.org/guidelines/web_security#content-security-policy). Seedrandom version 3.0.5 fixed this. However, starting in version 3, global Math.seedrandom is no longer available when using require('seedrandom') and so I needed to refactor the code a little.

I also found that gulp build would not work unless I updated the gulp-git dependency.

@mklilley
Copy link
Author

mklilley commented Jan 7, 2023

Hi there, I was just going through my github repos and doing a little tidy up and found a fork that linked to this PR I made some time ago. Just out of curiosity, was there anything wrong with my PR?

@JeffreyArts
Copy link
Collaborator

Hi @mklilley,

Unless I am missing something, this change would disallow the package to be used in a web environment since there won't be support for require. I appreciate the effort to update the problems associated with outdated packaged, but this does not seem to be a complete solution 🙂

@mklilley
Copy link
Author

Hi @JeffreyArts. Thanks for taking the time to look at my PR. Tbh, it's been so long since I did this PR that I can't recall all the details. However, when I look at your comment, I'm not sure I agree. Let me explain, and please do correct me if I've misunderstood you. Before I made my changes, there was already a require statement in the code. So, if your worry is that require is not supported in the browser then I agree, but this was not something that I introduced in my PR. My PR was to fix something very specific around content security policy.

@JeffreyArts
Copy link
Collaborator

Hi @JeffreyArts. Thanks for taking the time to look at my PR. Tbh, it's been so long since I did this PR that I can't recall all the details. However, when I look at your comment, I'm not sure I agree. Let me explain, and please do correct me if I've misunderstood you. Before I made my changes, there was already a require statement in the code. So, if your worry is that require is not supported in the browser then I agree, but this was not something that I introduced in my PR. My PR was to fix something very specific around content security policy.

Could you please pinpoint me to this reference? Cause looking at shuffle-seed.js I can't find a require statement. Also, I agree with @louisremi's comment. His snippet works splendidly. Current version could be updated with that (compiled) snippet, but @webcaetano would need to update the NPM package for it as well.

@mklilley
Copy link
Author

Click on files changed tab in this PR and you'll see the old require references. Let me just say, I've got no vested interest in my PR anymore. It was so long ago that I did it and the world has moved on, so I'm sure other people have better solutions now.

@webcaetano
Copy link
Owner

hi @JeffreyArts. what is your username on npm? So i can invite you as maintainer.
Thanks

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.

3 participants