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

Bug - Corrupted CommonJS in js-cache #6

Closed
faf opened this issue Jan 27, 2021 · 8 comments
Closed

Bug - Corrupted CommonJS in js-cache #6

faf opened this issue Jan 27, 2021 · 8 comments
Assignees
Labels
1 - 🐞 bug 🐞 An issue with the system. 4 - verified This issue or pull request was verified.
Milestone

Comments

@faf
Copy link

faf commented Jan 27, 2021

Expected behavior

All JS working as intended.

Actual behavior

Syntax error making JS dysfunctional.

How to reproduce

Steps to reproduce the behavior:

  1. Open any Znuny web interface page (either agent, or customer) in browser after upgrade from OTRS 6.0.30.
  2. Open devtools console and see errors starting with Uncaught SyntaxError: missing ) after argument list in CommonJS.

Environment

  • OS: Linux (ALT Linux P9 x86_64)
  • Browser: Chromium (84.0.4147.105)
  • Znuny 6.0.31

Additional information

The problem occurs because of CKEditor, namely because of this line. Seems like during the generation of combined js-file everything after the double slashes and before the line break is treated as comment and is ignored.

The quick solution is to manually fix ckeditor.js by escaping double slashes, so indexOf("file://") becomes indexOf("file:\/\/"). But I'm not sure whether it's the correct way.

@faf faf added 1 - 🐞 bug 🐞 An issue with the system. 4 - clarification The issue or pull requests needs more information. labels Jan 27, 2021
@hanneshal hanneshal added the 4 - verified This issue or pull request was verified. label Jan 28, 2021
@hanneshal
Copy link

Hi @faf

we update the CKEditor right up to 4.16.0 because they released a new security release, yesterday sigh
https://ckeditor.com/cke4/release/CKEditor-4.16.0
A new release is on the way.

For further reference:
We did miss this in our environment because we do not have the JS cache enabled, to make it easier for the development team, when changes to JS files are needed.

From now on we changed this on the QA stage.

Thank you for your help ❤️ Спасибо за вашу помощь :)

@ymyasoedov
Copy link

ymyasoedov commented Jan 29, 2021

I'm almost sure that it's a bug in JavaScript::Minifier. More over ckeditor.js is already minified out of the box. There is no sense to do it twicely. The simplest bug fix would be to ignore the minification stage.
Also, it's would be nice to report about this bug to upstream repository: https://github.com/zoffixznet/JavaScript-Minifier
BTW, there is an alternative package JavaScript::Minifier::XS, that could be used instead of that one. It has stronger syntax analyzer.

@ymyasoedov
Copy link

Opened a bug for JavaScript::Minifier package zoffixznet/JavaScript-Minifier#10.

@hanneshal
Copy link

Thank your for your help. We really appreciate this.
The updated release is available now: https://www.znuny.org/releases/znuny-6-0-32
We had to bump the CKE also due to an security issue on their side.

@zoffixznet
Copy link

Opened a bug for JavaScript::Minifier package

Just a note: I inherited this codebase to be sort of a janitor. I've no idea how the code works, so unfortunately, can't provide the fix. Can only apply and upload one if someone is kind enough to provide it.

Sorry.

@faf
Copy link
Author

faf commented Feb 2, 2021

Seems like it's possible to fix this particular issue in minifier, but it looks like there could be more problems there. Maybe one should switch to JavaScript::Minifier::XS or some other minifier instead.

@zoffixznet
Copy link

I've merged the fix faf submitted (thank you) and pushed updated module to CPAN.

switch to JavaScript::Minifier::XS or some other minifier instead.

That's my recommendation as well. I believe JavaScript::Minifier::XS or something similar is used as an optional dependency by Mojolicious::Plugin::Webpack and Mojolicious::Plugin::AssetPack. I've used Mojolicious::Plugin::AssetPack with a ton of JS files and never had any issues, so whatever it uses under the hood to minify should be fairly solid.

@jepf
Copy link
Contributor

jepf commented Feb 3, 2021

Thank you all very much for your support.

Both solutions work fine for us (JavaScript::Minifier 1.15 and JavaScript::Minifier::XS).

The next release of Znuny will contain the updated version 1.15 of JavaScript::Minifier and will optionally automatically utilize JavaScript::Minifier::XS instead if it is installed.

Same will be the case btw. for CSS::Minifier and CSS::Minifier::XS.

znuny-robo pushed a commit that referenced this issue Feb 24, 2021
…f JavaScript::Minifier::XS if available. Thanks to Fedor A. Fetisov (@faf), @zoffixznet and Yuri Myasoedov (@ymyasoedov). See #6.
@dennykorsukewitz dennykorsukewitz removed the 4 - clarification The issue or pull requests needs more information. label Feb 24, 2021
@dennykorsukewitz dennykorsukewitz added this to the rel-6_0_33 milestone Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - 🐞 bug 🐞 An issue with the system. 4 - verified This issue or pull request was verified.
Development

No branches or pull requests

6 participants