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

[#33948] fix email validation, allowing email with new domain name extension #4135

Closed
wants to merge 8 commits into from

Conversation

rvbgnu
Copy link
Contributor

@rvbgnu rvbgnu commented Aug 19, 2014

J2.5 email addresses with new TLD extensions are not valid upon installation

Steps to reproduce the issue

  1. Install a new site with Joomla! 2.5.x (I believe it is in all versions).

  2. Provide an admin email address with a new type of extension (e.g. .ninja .expert .company .cool .sexy etc.)

  3. Test also any kind of email following these rules:

    The local part of an email (text before the "@") may include any of the following characters
    Uppercase and lowercase English letters (a–z, A–Z)
    Digits 0 to 9
    These special characters: ! # $ % & ’ * + - / = ? ^ _ ` { | } ~
    Character . (dot, period, full stop) (ASCII: 46) provided that it is not the first or last character, and provided also that it does not appear consecutively (e.g. John..Doe@example.com is not allowed).

Expected result

Any email address from current and scheduled domain name should be accepted.

Actual result

Address is not accepted as valid.

System information (as much as possible)

Initial report on Joomla 2.5.20 - confirmed in 2.5.28-dev

Additional comments

Initial issue Submitted By: Ben Shomer - Open Date: 2014-07-14
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33948
fix email validation, allowing email with new domain name extension (more than 4 characters) like new gTLDs

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Aug 19, 2014

But what about the new extensions to come, or already live?

Like restaurant, photography, healthcare...
http://newgtlds.icann.org/en/program-status/sunrise-claims-periods?search_api_views_fulltext=&sunrise_open&sunrise_close&field_gtld_sunrise_close[2]=2&order=field_trademark_claims_close&sort=asc

Is there already a plan to cover them as soon as there are available?

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Oct 17, 2014

No interest in this??

I've rechecked the current list, export the data and the maximum length would be 15 strings for
xn--i1b6b1a6a2e http://newgtlds.icann.org/en/program-status/sunrise-claims-periods?search_api_views_fulltext=xn--i1b6b1a6a2e&sunrise_open%5Bdate%5D=&sunrise_close%5Bdate%5D=&field_gtld_sunrise_close%5B2%5D=2
and non-IDN is international (13 strings)

So I am setting the max to 15.
Please review it and test it to remove this annoying bug!

@infograf768
Copy link
Member

Our regex in 3.x is now
$regex = '^[a-zA-Z0-9.!#$%&’*+/=?^_{|}~-]+@[a-zA-Z0-9-]+(?:.[a-zA-Z0-9-]+)*$'`

Shall we use the same?

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Oct 18, 2014

Well, maybe, but I do not use regex everyday, so I will track down the commits and comments to double check before doing a blind copy and paste 😎

@rvbgnu rvbgnu changed the title [#33948] fix email validation, allowing email with domain name extension of 8 cha... [#33948] fix email validation, allowing email with new domain name extension Oct 18, 2014
@rvbgnu
Copy link
Contributor Author

rvbgnu commented Oct 18, 2014

Now a email address like contact@me.international works, as well as me.internationalandmore...

So after scratching my head on this tonight, I still don't know exactly what was the purpose of the new regex, and there are many changes in the js folder in J3.3 compare to current J2.5.28-dev, plus the use of punycode.toASCII() here

value = punycode.toASCII(value);

So I would prefer that some devs who authored and committed (and hopefully understand ;-) the code involved, review this to help to back port it from J3.3, if possible.

If this is too complicated or not possible, using the 15 strings limit from my previous PR may be enough for J2.5. What do you think?

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

@infograf768
Copy link
Member

The new regex was specially crafted to take care of longer domains in the platform and ported to 3.x
See
#685
and
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_id=8103&tracker_item_id=29997
that was already before implementing punycode

Therefore this PR is OK for me (with the limitations concerning INtl domains in 2.5, which is OK).

@infograf768
Copy link
Member

hmm, weird
let me look somehting
looks like we had a corruption
my diff was:
$regex = '^[a-zA-Z0-9.!#$%&’+/=?^`{|}-]+@[a-zA-Z0-9-]+(?:.[a-zA-Z0-9-]+)$';
and we have in core
$regex = '^[a-zA-Z0-9.!#$%&’
+/=?^_{|}
-]+@[a-zA-Z0-9-]+(?:.[a-zA-Z0-9-]+)_$'
Looks like we had a corruption when merging

@infograf768
Copy link
Member

We need to correct 3.x

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

@infograf768
Copy link
Member

Please see and test (for 3.x)
#4894

And please change your PR to use that regex

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Oct 22, 2014

Waouh, well done Jean-Marie @infograf768 for the head up!! I should have check with Regex from the code, and not only the only pasted in the comments. Lesson learnt!!

PR updated now. Please test

@infograf768
Copy link
Member

I have corrected an error
6ba7987
and found out that our helper was not in sync with the validation...
#4899

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Nov 4, 2014

Thanks @infograf768 , I have updated ReGex and backported the helper as well (without the punnyCode of course).

Please Test!

@infograf768
Copy link
Member

Have you also backported
#4905

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Nov 5, 2014

Double checking now. Oops, wrong ReGex in validate.js, corrected now.

But we do not have the $tld test on Joomla 2.5, so there is not similar part with this line IMHO
https://github.com/infograf768/joomla-cms/blob/2d33d95df58f6da7a2d6ea32a345c02648c5fb2b/libraries/joomla/form/rule/email.php#L56

Otherwise, backported, please Test!

@docschmitti
Copy link

Hi,
is there any progress to make the new tlds work for joomla 2.5?
Regards,
Dan

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

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Nov 11, 2014

Hi Dan @docschmitti
If you can test the patch with the instructions provided at the beginning, that would help us a lot!! I have added more details about which email address is considered as valid, as it is not a Joomla specific rule.

Thanks,
Hervé

@docschmitti
Copy link

Hi,
I tried with changing:
libraries/joomla/form/rules/email.php
media/system/js/validate-uncompressed.js
media/system/js/validate.js
But did not work for me. Tlds with >4 characters are not accepted (.reviews).
Regards,
Daniel

@rvbgnu
Copy link
Contributor Author

rvbgnu commented Nov 17, 2014

Hi @docschmitti,
Thank you for your reply. You said "I tried with changing (files name)", but I am a bit worry about how did you test this patch. Did you use the Pull Request as explained here http://docs.joomla.org/Git_for_Testers_and_Trackers ?

Kind regards,
Hervé

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

@docschmitti
Copy link

HI, sorry. I am not familiar with programming. Can only do things a bit with copy/paste. Cannot handle all that Github stuff:-( I am afraid I cannot help... besides someone can give me a zip-file and I can install it... :-(
Regards,
Daniel

@zero-24
Copy link
Contributor

zero-24 commented Nov 17, 2014

@docschmitti

I am afraid I cannot help... besides someone can give me a zip-file and I can install it...

One of our easiest things we can do 😄
Zip can be found here: https://github.com/rvbgnu/joomla-cms/archive/2.5.x_fix_33948.zip

@docschmitti
Copy link

ok, thank you. Looks like a complete package of joomla to me. What does it do? Or what is it? A new beta Joomla 2.5? Regards,
Daniel

@zero-24
Copy link
Contributor

zero-24 commented Nov 17, 2014

@docschmitti this is a joomla 2.5 with the changed files by this PR. You can install it as you install joomla normaly and after this test all the things that need to be tested. Thanks 👍

@docschmitti
Copy link

ok, so I should NOT install it on a running website? Only for testing?

@zero-24
Copy link
Contributor

zero-24 commented Nov 18, 2014

ok, so I should NOT install it on a running website? Only for testing?

@docschmitti NO please do not install any patches on a live/productive site. If the patch here have 2 test it get into the next update.

The zip is for testing in a testing envoirment 😄

@infograf768 infograf768 added this to the Joomla! 2.5.28 milestone Dec 6, 2014
@infograf768
Copy link
Member

Test OK. Merging.

infograf768 pushed a commit that referenced this pull request Dec 6, 2014
@infograf768 infograf768 closed this Dec 6, 2014
@davidmdawson
Copy link

You say don't put on a production site... however I am stuck using Joomla 2.5 as our component is not yes ready to move away from an old version of virtuemart.

Now I have the problem on multiple sites that people can't register.

Will there be a patch for joomla 2.5 that we can use?


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

@brianteeman
Copy link
Contributor

Simple answer is no. There is no official support for Joomla 2.5 any more

On 6 August 2015 at 09:58, davidmdawson notifications@github.com wrote:

You say don't put on a production site... however I am stuck using Joomla
2.5 as our component is not yes ready to move away from an old version of
virtuemart.

Now I have the problem on multiple sites that people can't register.

Will there be a patch for joomla 2.5 that we can use?

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/4135
http://issues.joomla.org/tracker/joomla-cms/4135.


Reply to this email directly or view it on GitHub
#4135 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@infograf768
Copy link
Member

You say don't put on a production site

If this is related to the use of https://github.com/rvbgnu/joomla-cms/archive/2.5.x_fix_33948.zip, it is no need. This patch has been merged into 2.5.28 in December 2014

@davidmdawson
Copy link

oh, I still have the problem. I'll look elsewhere then. Maybe an issue with the registration page in virtuemart


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

@rvbgnu rvbgnu deleted the 2.5.x_fix_33948 branch October 14, 2015 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants