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

Email Cloaking fires when @ in filename (image) #39786 #39932

Merged
merged 7 commits into from
Mar 1, 2023

Conversation

LukasHH
Copy link
Contributor

@LukasHH LukasHH commented Feb 24, 2023

Pull Request for Issue #39786

Summary of Changes

Change the last pattern to exclude email addresses that are in a source attribute. Example in image or pictures tag

Testing Instructions

Please test all variants of email obfuscation. I have a source code for an article for testing at https://github.com/LukasHH/other-test/blob/main/email_cloaking_test_source.html.
Copy the source code and paste this source code into a new article. Save the new article and call it in the frontend.

Or:
Create an article and include an image whose filename contains an @.

Apply the suggested change to the file here:
https://github.com/joomla/joomla-cms/pull/39932/files

You can also use the Patch Tester components for testing. A tutorial is available here: https://docs.joomla.org/Component_Patchtester_for_Testers

Actual result BEFORE applying this Pull Request

Images that have a filename in the form of a valid email address or contain an @ in the filename will be cloaked and not displayed.

Example without this change here under Sample 21
https://j4-test.it-conserv.de/test/issiu

Expected result AFTER applying this Pull Request

These filenames should be excluded from email cloaking and displayed correctly.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 24, 2023

@formfranska please test the PR

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 25, 2023

Sorry, Not sure exactly what I'm supposed to do. I wasn't able to copy the text from your test article (couldn't scroll all the way down). Complete newbie in github. Sorry 😅

You can use the "Raw" button, then the raw data will be displayed. Or the button "Copy", then the source text is copied.
image

Otherwise simply set an article in which an image is included, whose file name contains an @ sign.

@richard67
Copy link
Member

@LukasHH Could you extend your testing instructions so that people paste the test content multiple times? We should have each test object multiple times to make sure that each occurrence of a pattern is cloaked, not only the first one, and that we don’t get into an endless loop with that while loop which we recently discussed at another place.

@formfranska
Copy link

Sorry, Not sure exactly what I'm supposed to do. I wasn't able to copy the text from your test article (couldn't scroll all the way down). Complete newbie in github. Sorry 😅

You can use the "Raw" button, then the raw data will be displayed. Or the button "Copy", then the source text is copied. image

Otherwise simply set an article in which an image is included, whose file name contains an @ sign.

Thank you! OK, so I didn't have the Copy button but the Raw button was present so used that.
Screenshot 2023-02-25 at 10 07 55

I published the article and only Sample 21 looks broken to me:

Sample 21:

Image filename in the form of an email

<img src="/images/demofiles/%3Cjoomla-hidden-mail%20%20is-link="1" is-email="1" first="dGVzdA==" last="aW1hZ2UucG5n" text="dGVzdEBpbWFnZS5wbmc=" base="" >This email address is being protected from spambots. You need JavaScript enabled to view it." width="215" height="121" loading="lazy" data-path="local-images:/demofiles/test@image.png" />

This email address is being protected from spambots. You need JavaScript enabled to view it." alt="" width="215" height="121" loading="lazy" data-path="local-images:/demofiles/test@image.png" />

<img src="/images/demofiles/%3Cjoomla-hidden-mail%20%20is-link="1" is-email="1" first="dGVzdA==" last="aW1hZ2UucG5n" text="dGVzdEBpbWFnZS5wbmc=" base="" >This email address is being protected from spambots. You need JavaScript enabled to view it." width="100" height="100" loading="lazy" data-path="local-images:/demofiles/test@image.svg" />

This email address is being protected from spambots. You need JavaScript enabled to view it." alt="" width="100" height="100" loading="lazy" data-path="local-images:/demofiles/test@image.svg" />

<img src="/images/demofiles/%3Cjoomla-hidden-mail%20%20is-link="1" is-email="1" first="dGVzdA==" last="aW1hZ2UucG5n" text="dGVzdEBpbWFnZS5wbmc=" base="" >This email address is being protected from spambots. You need JavaScript enabled to view it." alt="test@image.svg" width="100" height="100" loading="lazy" data-path="local-images:/demofiles/test@image.svg" />

test@image.svg

This email address is being protected from spambots. You need JavaScript enabled to view it." media="(min-width: 38em)" /> This email address is being protected from spambots. You need JavaScript enabled to view it." /> This email address is being protected from spambots. You need JavaScript enabled to view it." alt="" />

Screenshot 2023-02-25 at 10-15-27 Source Code for Test Email cloaking - TESTAR

Was this helping you in any way? Or did I do it wrong?

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 25, 2023

This is the initial situation without the patch, which allows you to test all variants. You would now have to apply the changes and then it should be displayed correctly.
https://github.com/joomla/joomla-cms/pull/39932/files

You can also use the Patch Tester component for testing: https://docs.joomla.org/Component_Patchtester_for_Testers

@formfranska
Copy link

formfranska commented Feb 25, 2023

This is the initial situation without the patch, which allows you to test all variants. You would now have to apply the changes and then it should be displayed correctly. https://github.com/joomla/joomla-cms/pull/39932/files

You can also use the Patch Tester component for testing: https://docs.joomla.org/Component_Patchtester_for_Testers

OK, thanks for your help every step of the way 🤣

Applied patch
Screenshot 2023-02-25 at 11 47 14

Sample 21 now looks like this (with patch enabled)
Screenshot 2023-02-25 at 11 48 39

Please note that when I try to look at the images I get

404 Not Found The resource requested could not be found on this server!

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 25, 2023

I have just updated and extended the page with the test code. Here I have also added the sample images. I have also added a readme with further explanation.
https://github.com/LukasHH/other-test

The images are currently not displayed for you, because they are not available. It is important that the file name is not cloaked.

@Hackwar Hackwar added the Small A PR which only has a small change label Feb 25, 2023
@formfranska
Copy link

I have just updated and extended the page with the test code. Here I have also added the sample images. I have also added a readme with further explanation. https://github.com/LukasHH/other-test

The images are currently not displayed for you, because they are not available. It is important that the file name is not cloaked.

Thank you! OK, so with this https://github.com/LukasHH/other-test it looks like this:

Screenshot 2023-02-25 at 18 47 58

Is that how it should look?

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 25, 2023

Yes - this is how it should look. In the browser source code you can also see that the images with an @ are not cloaked, while emails are still cloaked by the plugin.

You can also test it with the as you had described in the forum.
https://forum.joomla.org/viewtopic.php?p=3683408#p3683408

@toivo
Copy link
Contributor

toivo commented Feb 26, 2023

I have tested this item ✅ successfully on cd6ae6b

Tested successfully using the sample page in Joomla 4.2.9-dev of 26 February using PHP 8.1.10 in Wampserver


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@ChristineWk
Copy link

@LukasHH

Status now: Everything done OK so far. Also images loaded etc.

But when applying Patchtester, get response:
The file marked for modification does not exist: plugins/content/emailcloak/emailcloak.php

But: Content – Email Cloacking is enabled!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@richard67
Copy link
Member

@ChristineWk Can it be that you have tested on a 4.3-dev installation where that plugin has been converted to the new structure with PR #38466 ? This would explain your problem with the patchtester. This PR here is for the 4.2-dev branch where the file still is at the old place.

@ChristineWk
Copy link

@richard67
Yes, you're right, I had the thought too... sorry.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@richard67
Copy link
Member

@ChristineWk Hmm unfortunately you can't really test this PR on a 4.3-dev. But thanks for trying. Do you have a 4.2.8 or so for testing, too? On that you could test this PR.

@ChristineWk
Copy link

I have a 4.2.9-dev, because for 4.2.8 there was a security release. Will try.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@ChristineWk
Copy link

@LukasHH

Could you check again my test-site please? It seems OK now. Tks


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 26, 2023

@LukasHH

Could you check again my test-site please? It seems OK now. Tks
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

Hello Christine,

I just see on your page that the Sample 17a and Sample 17b versions are not cloaked. They should still be cloaked.
I'll have a look at it again to see if it's the same for me.

image

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 26, 2023

I need to undo the change in line 444.
1e46b07

I have noticed that this change no longer cloaks the versions under Sample 17. Even without the change, this does not affect functionality with images that contain an @ in the filename.

@ChristineWk
Copy link

@LukasHH

Hv reverted Patch, fetched and applied again.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@LukasHH
Copy link
Contributor Author

LukasHH commented Feb 26, 2023

@LukasHH

Hv reverted Patch, fetched and applied again.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

This looks better now.

@ChristineWk
Copy link

I have tested this item ✅ successfully on e033de6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on e033de6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@Quy Quy changed the title Email Cloaking fires when @ in filename (image) #39786 Email Cloaking fires when @ in filename (image) #39786 Feb 28, 2023
@Quy
Copy link
Contributor

Quy commented Feb 28, 2023

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39932.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 28, 2023
@roland-d roland-d merged commit 1995c41 into joomla:4.2-dev Mar 1, 2023
@roland-d
Copy link
Contributor

roland-d commented Mar 1, 2023

Thank you

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 1, 2023
@roland-d roland-d added this to the Joomla! 4.2.9 milestone Mar 1, 2023
@LukasHH LukasHH deleted the LukasHH/issue39786 branch March 2, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Small A PR which only has a small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants