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

Froala feature flag - disabled by default #12416

Merged
merged 5 commits into from Jun 1, 2023

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented May 31, 2023

Q A
Bug fix? (use the a.b branch) [security precaution]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [y]
Automated tests included? [ ]
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes https://mautic.atlassian.net/browse/TPROD-441

Description:

The Froala Javascript library Mautic use has several security vulnerabilities reported:

https://security.snyk.io/package/npm/froala-editor/2.4.2

It's used in the legacy builder but it seems that many community members are still using it:

https://forum.mautic.org/t/is-anyone-still-using-the-legacy-email-and-page-builder/27819

So the solution in between that will disable this vulnerable library for Mautic 5 but allows users to enable it if they need it and are fine with the security risks is to add a new feature flag (= configuration option). This feature flag is OFF by default so all Mautic administrators must enable it and agree with the downsides.

Screenshot 2023-05-31 at 13 56 17

If the GrapesJS builder plugin is disabled and a Mautic user tries to open the legacy builder, they get this warning with instructions about what to do:

Screenshot 2023-05-31 at 13 55 52

Apart from making Mautic 5 a bit safer, it will also help with performance.

With Froala enabled:
Screenshot 2023-05-31 at 14 45 29

With Froala disabled:
Screenshot 2023-05-31 at 14 45 43

This feature flag will save 1.6 MB off of the Mautic administration page load. Saved 27% of loaded size and the loading is 11% faster than before.

The savings will be greater when we remove the Froala library completely. The CSS/LESS files are baked into the library.css file and cannot be easily extracted. So those are still loading all the time.

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Disable the GrapesJS plugin so the email and page builders would use the legacy builder.
  3. Open JS console in your browser dev tools to see if there are any JS errors.
  4. Try to open the legacy builder. <<< You should get the warning.
  5. Go to Global configuration, System settings and enable the Froala library.
  6. Open the legacy builder now. <<< The builder works correctly.

found during testing the legacy builder. Plus:
- jquery.js was defined twice
- using shorter controller syntax
On loading the legacy builder there is alert that the Froala assets must be enabled first
@escopecz escopecz requested a review from mabumusa1 as a code owner May 31, 2023 13:37
@escopecz escopecz added bug Issues or PR's relating to bugs bc-break A BC break PR for major release milestones only builder-legacy Anything related to the legacy email or landing page builders labels May 31, 2023
@escopecz escopecz added this to the 5.0-alpha milestone May 31, 2023
@escopecz escopecz added the ready-to-test PR's that are ready to test label May 31, 2023
@codecov
Copy link

codecov bot commented May 31, 2023

Codecov Report

Merging #12416 (ede9522) into 5.x (0d5b22e) will decrease coverage by 0.03%.
The diff coverage is 62.50%.

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #12416      +/-   ##
============================================
- Coverage     56.32%   56.30%   -0.03%     
- Complexity    35185    35187       +2     
============================================
  Files          2210     2210              
  Lines        105570   105572       +2     
============================================
- Hits          59463    59440      -23     
- Misses        46107    46132      +25     
Impacted Files Coverage Δ
...bundles/EmailBundle/Controller/EmailController.php 27.54% <25.00%> (ø)
...p/bundles/PageBundle/Controller/PageController.php 33.98% <25.00%> (ø)
...s/CoreBundle/Controller/BuilderControllerTrait.php 100.00% <100.00%> (ø)
app/bundles/CoreBundle/Form/Type/ConfigType.php 100.00% <100.00%> (ø)
...undles/CoreBundle/Helper/AssetGenerationHelper.php 56.37% <100.00%> (-0.58%) ⬇️
...pp/bundles/CoreBundle/Twig/Helper/AssetsHelper.php 73.38% <100.00%> (-8.93%) ⬇️

@mabumusa1
Copy link
Member

Warning Message show
image

Warning for enabling is showing
image

Afterwards it loads normally
image

Copy link
Member

@mabumusa1 mabumusa1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go, since this Froala has changed their license model, there is nothing much to do except using it.

@escopecz escopecz merged commit fc21a10 into mautic:5.x Jun 1, 2023
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc-break A BC break PR for major release milestones only bug Issues or PR's relating to bugs builder-legacy Anything related to the legacy email or landing page builders ready-to-test PR's that are ready to test
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants