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

EZEE-3030: Links containing 'ezlocation` or 'ezcontent' keywords break RTE validation on paste #1283

Merged
merged 3 commits into from
Mar 18, 2020

Conversation

konradoboza
Copy link
Member

Question Answer
Tickets EZEE-3030
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

The recently introduced solution #1245 is missing proper checks for ezcontent and zezlocation keywords that results in http protocol wrongly added, hence broken validation. Added proper conditions and flatten existing ones to increase readability. I will create a PR in ezplatform-richtext for 3.0 once this solution is approved.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@konradoboza konradoboza force-pushed the EZEE-3030-protocol_wrongly_added branch from 457f533 to 73f992b Compare March 17, 2020 08:25
Copy link
Contributor

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA Approved on eZ Platform EE v2.5.9 (patch) and on eZ Platform EE 2.5 with diff.

@lserwatka lserwatka merged commit 75ff822 into 1.5 Mar 18, 2020
@lserwatka lserwatka deleted the EZEE-3030-protocol_wrongly_added branch March 18, 2020 15:00
@lserwatka
Copy link
Member

You can merge it up.

@konradoboza
Copy link
Member Author

Done.

@andrerom
Copy link
Contributor

@konradoboza Doesn't these links also use scheme format, i.e. ezlocation://
If so, maybe the code should rather detect existing scheme before adding one using regex?
Secondly, what about relative urls?

@konradoboza
Copy link
Member Author

@andrerom all the occurrences of ezlocation or ezcontent are validated already, ref: https://github.com/ezsystems/ezplatform-admin-ui/pull/1283/files#diff-ab42d417cb34dabef37b3c2d017e4e8bR420. That being said, scheme formats that you mentioned should be fine. What scenario of relative URLs you have in mind? As far as I can see all URLs like test.com or content/location/67 are prefixed with the protocol on publishing anyway.

@andrerom
Copy link
Contributor

OK, then we should be fine, I was just unsure if this keyword matching might have still some uncovered cases or url types.

@maikfr
Copy link

maikfr commented Apr 30, 2020

Hello all,

we are using the latest 1.5.x tag of this project. Unfortunately the merge-commit from this pull request is not available there, but only in the branch itself. Would it be possible to release a new 1.5 tag with the latest changes of the appropriate branch, to be able to receive the bugfix from this pull request?

Thanks for your effort and help!

@konradoboza
Copy link
Member Author

Hi @maikfr. The ezplatform-admin-ui package will be released most likely as a part of the new 2.5.10 release of eZ Platform. Not sure when this happens though, ping @lserwatka.

For now, you can apply the following diff to take advantage of this fix:

diff --git a/src/bundle/Resources/public/js/scripts/fieldType/base/base-rich-text.js b/src/bundle/Resources/public/js/scripts/fieldType/base/base-rich-text.js
index 9bc8bbd58..dbe15db35 100644
--- a/src/bundle/Resources/public/js/scripts/fieldType/base/base-rich-text.js
+++ b/src/bundle/Resources/public/js/scripts/fieldType/base/base-rich-text.js
@@ -398,17 +398,31 @@
             const links = container.querySelectorAll('a');
             const anchorPrefix = '#';
             const protocolPrefix = 'http://';
+            const restrictedKeywords = ['ezcontent', 'ezlocation'];
 
             links.forEach((link) => {
                 const href = link.getAttribute('href');
                 const protocolPattern = /^https?:\/\//i;
+                const protocolHref = protocolPrefix.concat(href);
 
-                if (href && href.indexOf(anchorPrefix) !== 0 && !protocolPattern.test(href)) {
-                    const protocolHref = protocolPrefix.concat(href);
+                if (!href) {
+                    return;
+                }
+
+                if (href.indexOf(anchorPrefix) === 0) {
+                    return;
+                }
 
-                    link.setAttribute('href', protocolHref);
-                    link.setAttribute('data-cke-saved-href', protocolHref);
+                if (protocolPattern.test(href)) {
+                    return;
                 }
+
+                if (restrictedKeywords.some((keyword) => href.includes(keyword))) {
+                    return;
+                }
+
+                link.setAttribute('href', protocolHref);
+                link.setAttribute('data-cke-saved-href', protocolHref);
             });
         }
     };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants