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

Backport the prismjs 1.27.0 upgrade to 3.x #58

Closed
stof opened this issue Feb 22, 2022 · 14 comments
Closed

Backport the prismjs 1.27.0 upgrade to 3.x #58

stof opened this issue Feb 22, 2022 · 14 comments

Comments

@stof
Copy link

stof commented Feb 22, 2022

react-syntax-highlighter is still using refractor 3 for now. As prismjs 1.27.0 fixes an XSS vulnerability, would you agree to backport that upgrade to a 3.x version of refractor ?

@wooorm
Copy link
Owner

wooorm commented Feb 22, 2022

  • Where does it say there’s a security fix?
  • Could it become acceptable to a) maintain an unmaintained project, or b) switch away from an unmaintained project?

@72636c
Copy link

72636c commented Feb 25, 2022

Here you go:

I think many of us on 3.x haven't upgraded because our packages/tools are not ESM compatible yet. There's little I can add to the existing discourse on this subject except to say that it shouldn't be trivialised when we have major parts of the ecosystem (think Jest, TypeScript) struggling to land support.

@wooorm
Copy link
Owner

wooorm commented Feb 25, 2022

Thanks

  • TS + ESM should be mostly fine!
  • At some point, you should probably switch to something other than Jest. There are better maintained and faster alternatives out there. See also fix: jest-circus shares events among imports #11483 jestjs/jest#11529 (comment).
  • I’m not saying things are trivial. I hope you understand that me maintaining multiple release lines isn’t either. That will stop at some point.
  • This misses the point that react-syntax-highlighter isn’t maintained.

@wooorm
Copy link
Owner

wooorm commented Feb 25, 2022

Released!

@wooorm wooorm closed this as completed Feb 25, 2022
@stof
Copy link
Author

stof commented Mar 2, 2022

@wooorm I'm not using react-syntax-highlighter myself. I'm using Storybook, which uses react-syntax-highlighter

@stof
Copy link
Author

stof commented Mar 2, 2022

And thanks for this backport.

@wooorm
Copy link
Owner

wooorm commented Mar 2, 2022

Perhaps you can ask Storybook folks, though?

@okonet
Copy link
Contributor

okonet commented May 11, 2022

Storybook will most probably switch from react-syntax-highlighter to something else in the future since we ran into other blockers with it: storybookjs/storybook#18090

@wooorm
Copy link
Owner

wooorm commented May 11, 2022

I don’t exactly get what’s going on there, but it seems to reference Prism actually running in the browser? I don’t think this package, nor react-syntax-highlighter, cause that?
Can you point me to the exact problem there?

@okonet
Copy link
Contributor

okonet commented May 11, 2022

Hmm, I don't know where the "real" Prism JS came from but it was a race condition caused by it for sure. I could spend more time investigating the dependency problem for sure.

@wooorm
Copy link
Owner

wooorm commented May 11, 2022

There is some code to prevent Prism from doing weird stuff:

refractor/lib/core.js

Lines 40 to 56 in c5744cb

const ctx =
typeof globalThis === 'object'
? globalThis
: // @ts-expect-error
typeof self === 'object'
? // @ts-expect-error
self
: // @ts-expect-error
typeof window === 'object'
? // @ts-expect-error
window
: typeof global === 'object'
? global
: {}
/* eslint-enable no-undef */
const restore = capture()
.
It could be that that’s not working (on a past version?)?

@okonet
Copy link
Contributor

okonet commented May 11, 2022

Yeah it seem like it. There is another approach which I really like: https://github.com/FormidableLabs/prism-react-renderer/blob/master/patches/prismjs%2B1.26.0.patch

Essentially it removes unused code instead of working around it. It also cuts the bundle size quite significantly.

Maybe this approach could be considered in this repo as well?

wooorm added a commit that referenced this issue May 12, 2022
Related-to: GH-58.
Closes GH-61.

Co-authored-by: Andrey Okonetchnikov <andrey@okonet.dev>
@wooorm
Copy link
Owner

wooorm commented May 13, 2022

Landed that idea, thanks!
react-syntax-highlighter isn’t maintained actively though. You might still want to do that in house (and also potentially switch to lowlight/starry-night/etc)

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

No branches or pull requests

4 participants