Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#38506 closed defect (bug) (fixed)

Email length can be less than 6 character

Reported by: mangeshp's profile mangeshp Owned by: dd32's profile dd32
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit
Focuses: Cc:


Email length checked in comment.php need to be modified. Email length can be less than six character (e.g a@b.c) and it's valid email according to the RFC. Same thing is referred here in ticket #38477.

So user should be able to post the a@b.c as a valid email while posting the comment.

Attachments (2)

38506.diff (794 bytes) - added by mangeshp 7 years ago.
38506.2.diff (1.8 KB) - added by rmccue 7 years ago.
Check for email length in REST API validation

Download all attachments as: .zip

Change History (18)

7 years ago

#1 @mangeshp
7 years ago

  • Keywords has-patch added

#2 @dd32
7 years ago

This dates back to [2558] (6 char) and [2556] (7char).

Technically a@b.c is a legitimate email address, however realistically, it's not and will never be encountered in the wild. All ccTLDs are 2+ characters and 3+ characters for gTLDs.
Single-character gTLDs are supposed to be reviewed on a case-by-case basis in non-ascii scripts, but there's said to be very little reasoning on why one would be approved (and to date, as far as I can see no-one has put a request in).

Specifications aside, I see no harm in enforcing that the minimum length is 6 characters - - as that's the shortest possible valid email address available in the foreseeable future, in the event a single-digit-ascii (and not UTF8 IDN TLD) is approved we'll have quite some time to update to support it.

#3 @mangeshp
7 years ago

Thanks for the update @dd32!

It's strange, we are allowing 6 characters email ID via REST API nd not form Comment form.

#4 @dd32
7 years ago

I agree, and believe the REST API should also enforce it. At the end of the day though, no real harm in removing it either.

#5 @mangeshp
7 years ago

I agree, it would be better to maintain the consistency. Will ask @rachelbaker for further guidance.

#6 @rachelbaker
7 years ago

  • Component changed from Comments to REST API
  • Milestone changed from Awaiting Review to 4.7

We currently only enforce 6 character email addresses in wp_handle_comment_submission() and the XML-RPC method wp_newComment()IF the require_name_email option is set to true.

If no 3 character email address could be encountered in the wild, shouldn't we update the is_email() function and properly validate email address in other locations within core, like for user accounts? That just seems to make more sense to me than adding another special validation on top of is_email(), but I am interested in your thoughts @dd32

#7 @danielbachhuber
7 years ago

  • Owner set to dd32
  • Status changed from new to assigned

Assigning to @dd32 for a final decision. Happy to help out with a code patch / tests once we know which direction to head.

This ticket was mentioned in Slack in #core by rmccue. View the logs.

7 years ago

#9 @dd32
7 years ago

Sorry, I missed these pings previously.

I'm all for standardising on is_email(). I personally don't see any (practical) point in treating an email < 6 char as valid, so it makes sense to me for us to add that to the is_email() checks.

I understand the other side of it though, it could potentially be a valid email address, we're just refusing to honor that as an valid email address for comments - which is why it shouldn't be in is_email() but should be within the comment handlers.
At the end of the day though, discussion for keeping it, discarding it, adding it to is_email() is not worth our time, as it's all theoretical, and whomever wants to patch should decide what the most logical option is.

#10 @rmccue
7 years ago

  • Owner changed from dd32 to rmccue

7 years ago

Check for email length in REST API validation

#11 @rmccue
7 years ago

  • Keywords has-unit-tests added

Added a patch to test for this specifically in the REST API, along with a test. While I do think this belongs in is_email(), I'm also wary of changing that while we're in beta, and I'd rather the API have the stronger validation for now.

We can revisit changing is_email() in 4.8, I think.

#12 @rmccue
7 years ago

  • Keywords commit added
  • Owner changed from rmccue to rachelbaker

@rachelbaker Over to you for review/commit. We can open a new ticket for changing is_email in 4.8.

This ticket was mentioned in Slack in #core by rmccue. View the logs.

7 years ago

#14 @rmccue
7 years ago

  • Owner changed from rachelbaker to dd32

Rachel is not around; over to Dion for review.

#15 @rmccue
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39158:

REST API: Require 6 characters for comment email addresses.

The regular comments API requires 6 characters rather than 3, so we need to match this.

Props mangeshp, dd32.
Fixes #38506.

#16 @rmccue
7 years ago

In 39159:

REST API: Embiggen the test email address.

We no longer want a@… to be valid, so let's make it a@… instead.

Props dd32.
See #38506.

Note: See TracTickets for help on using tickets.