-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[4.0] email cloaking without inline script #19089
The head ref may contain hidden characters: "\u00A74.0-dev-email-cloaking"
Conversation
Seems good to me. Looking forward to the event listener for Ajax rendered content as I rely on this myself. |
@C-Lodder check @Fedik 's comment: #18843 (comment) |
@C-Lodder can you test this one against some ajax created email addresses? |
Can someone from the Security Team confirm if simple encoding is ok here or if we need a stronger key-value approach? |
Sure, I'll test this tomorrow |
Just wondering: Does it even make sense to cloak emails? Afaik Google runs JavaScript when they index the sites, so they will index the email addresses as well. |
@Bakual thats true 😂 |
Looks good to me |
please fix code style issues |
I think only a complex bots, |
@@ -0,0 +1,43 @@ | |||
(() => { | |||
class JoomlaHiddenMail extends HTMLElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit OCD of me but could we change the class to JoomlaCloakEmail
or JoomlaEmailCloak
? Apart from being better wording, it ties in more with the PlgContentEmailcloak
PHP class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @C-Lodder has a good point,
it will be more clear, "where the thing are come from" 😉
JHtmlEmailCloak
and JoomlaEmailCloak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, makes sense, I will change it
newEl.setAttribute('href', 'mailto:' + window.atob(this.getAttribute('first')) + '@' + window.atob(this.getAttribute('last'))); | ||
|
||
// Get all of the original element attributes, and pass them to the link | ||
for(var i = 0, l = this.attributes.length; i < l; ++i){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (...) {
(spacing)
newEl.setAttribute('href', 'mailto:' + window.atob(this.getAttribute('first')) + '@' + window.atob(this.getAttribute('last'))); | ||
|
||
// Get all of the original element attributes, and pass them to the link | ||
for(var i = 0, l = this.attributes.length; i < l; ++i){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (...) {
(spacing)
@C-Lodder did you manage to try this one? |
Ah crap, will test now |
Tested and seem to work perfectly with content rendered via Ajax |
I have tested this item ✅ successfully on d6f2d72 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19089. |
please fix the CS issues and don't comment code just remove it |
@C-Lodder can you test this with the sample data I put in the description? |
Correction: I understand these screenshots do not really show source as they are picked from Web Developper Inspector. In source, the mail adress is obsfucated as should. |
@infograf768 I knew you'll find some bugs here that's the reason I've asked you test! Thanks, will patch things in a bit |
@infograf768 now should be ok, both failing cases: |
I have tested this item ✅ successfully on 385af9c Question: any way to also add test for utf8 mails ? This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19089. |
Php tests or js tests? |
No idea. Something in PlgContentEmailcloakTest.php I guess. |
@infograf768 added two more tests for UTF-8, all tests pass now |
I have tested this item ✅ successfully on 5ec825e This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19089. |
@infograf768 can you please retest? |
@franz-wohlkoenig he doesn't have to, the last commit was to fix the tests, didn't touch the rest of the code 😏 |
Ready to Commit after two successful tests. |
@dgt41 Thanks! |
@wilsonge why did you merge this with drone / unit tests failing? All Joomla 4 PR's are now failing. @dgt41 The error looks like to come from this line https://github.com/joomla/joomla-cms/pull/19089/files#diff-a5e4a8ab7a94472482aa9109f381223fR61 as |
Pull Request for Issue # .
This PR is part of the series of PRs that try to eliminate the inline scripts from Joomla. (The reason is CSP, please watch @wilsonge 's video from JWC17...)
Summary of Changes
Testing Instructions
Edit an article and add
email: something@somewhere.com
Navigate to the article and check if the email is visible.
Disable javascript and reload the page (email shouldn't be visible)
Go to Admin->plugins->Content - Email Cloaking and change the option Mode then revisit the article, the email shouldn't be a link
Set the editor to codemirror and edit the article
popular articles
from the sample data.Replace the contents of the article with:
Check in the front end that article, with and without javascript enabled!
Expected result
All the possible cases as described in the tests:
Actual result
Documentation Changes Required
Yes