Make WordPress Core

Opened 5 years ago

Last modified 10 days ago

#45714 new defect (bug)

Allow all valid email formats when editing accounts in the dashboard

Reported by: chrisl27's profile chrisl27 Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests needs-testing needs-refresh changes-requested
Focuses: administration Cc:

Description

When updating an email address in the dashboard wp-admin/includes/user.php:80 uses sanitize_text_field which strips out a substring like "%ed" even though that is valid in the local part of an email address. Eg, the valid email address "user%edition@…" is saved as "userition@…".

This ticket requests to using sanitize_email instead, similar to the REST API that checks using is_email.

(A related, fixed issue is in #18039 which also recommended using sanitize_email)

Attachments (1)

45714.patch (2.3 KB) - added by chrisl27 5 years ago.
Patch

Download all attachments as: .zip

Change History (17)

@chrisl27
5 years ago

Patch

#1 @chrisl27
5 years ago

  • Keywords has-patch has-unit-tests added

#2 @pento
5 years ago

  • Version trunk deleted

#3 @sabernhardt
3 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.5

#4 @sabernhardt
3 months ago

#59961 was marked as a duplicate.

#5 @kebbet
3 months ago

  • Keywords needs-refresh added

I tried to apply the patch, it needs to be updated, as it does not apply at all. Error messages says:

error: wp-admin/includes/user.php: No such file or directory
error: phpunit/tests/user.php: No such file or directory

This ticket was mentioned in PR #5708 on WordPress/wordpress-develop by @kebbet.


3 months ago
#6

  • Keywords needs-refresh removed

Porting original patch to GitHub.

https://core.trac.wordpress.org/ticket/45714

assertInternalType is used in the provided test which is an unidentified method.

#7 @kebbet
3 months ago

  • Keywords needs-refresh added

.

Last edited 3 months ago by kebbet (previous) (diff)

#8 @kebbet
3 months ago

  • Keywords needs-refresh removed

PR is refreshed and assertIsInt is used instead of assertInternalType. Tests seems to pass at GitHub now. But please check that the tests are relevant and correct.

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


3 weeks ago

#10 @rajinsharwar
3 weeks ago

Thanks for your patch @kebbet. If we can get some test reports, and some eyes on this, hopefully, we can get this committed within the cycle. If not, we will have to punt this for 6.6 and check if we can get some early eyes. Keep this in the milestone for now.

#11 @rsiddiqe
2 weeks ago

Test Report

Testing Environment

WordPress: 6.5-beta1
PHP: 8.1.23
Web Server: nginx
Database: MySQL 8.0.16
Browser: Safari Version 16.2 (18614.3.7.1.5)
OS: macOS Ventura 13.1
Theme: Twenty Twenty-Four 1.0
MU Plugins: None activated

Verdict

%ed is still being removed from user id as described in the original ticket.

The scenario was recreated for multiple special characters. Using "&" also behaves unusually, turning the email from "user&id@…" to "user&ampid@…".

See attachments for "&" issue

https://raw.githubusercontent.com/ab-auth/testing/main/photo_2024-02-15_16-29-34%20(2).jpg

https://raw.githubusercontent.com/ab-auth/testing/main/photo_2024-02-15_16-29-34.jpg

#12 follow-up: @swissspidy
12 days ago

  • Keywords needs-refresh added

Test report indicates that the issue is not fixed, unless I'm missing something..?

Unit test can be improved to use a data provider.

#13 in reply to: ↑ 12 @rsiddiqe
11 days ago

Replying to swissspidy:

Test report indicates that the issue is not fixed, unless I'm missing something..?

Unit test can be improved to use a data provider.

Yes. The issue is not resolved. If you'd suggest a data provider, I could redo the unit-test for improvement.

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


10 days ago

#15 @audrasjb
10 days ago

  • Milestone changed from 6.5 to 6.6

As per today's bug scrub:
Since the PR still needs some work, let's move this ticket to milestone 6.6 to give it more time.

#16 @audrasjb
10 days ago

  • Keywords changes-requested added
Note: See TracTickets for help on using tickets.