Make WordPress Core

Opened 6 years ago

Last modified 3 weeks 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.7 Priority: normal
Severity: normal Version:
Component: Users Keywords: needs-patch
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 6 years ago.
Patch

Download all attachments as: .zip

Change History (24)

@chrisl27
6 years ago

Patch

#1 @chrisl27
6 years ago

  • Keywords has-patch has-unit-tests added

#2 @pento
6 years ago

  • Version trunk deleted

#3 @sabernhardt
11 months ago

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

#4 @sabernhardt
11 months ago

#59961 was marked as a duplicate.

#5 @kebbet
11 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.


11 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
11 months ago

  • Keywords needs-refresh added

.

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

#8 @kebbet
11 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.


8 months ago

#10 @rajinsharwar
8 months 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
8 months 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
8 months 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
8 months 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.


8 months ago

#15 @audrasjb
8 months 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
8 months ago

  • Keywords changes-requested added

#17 @sumitsingh
5 months ago

Test Report

Testing Environment
WordPress: 6.5.3
PHP: 8.1.23
Web Server: 7.4.3-4
Database: MySQL 8.0.36
Browser: Google Chrome Version 124.0.6367.60 (Official Build) (64-bit)
OS: Linux 5.15.0-107-generic x86_64
Theme: Twenty Eleven 4.5
MU Plugins: None activated

The scenario was recreated for multiple special characters. Using "%$" also behaves unusually, turning the email from "sumit%$@gmail.com" to "sumit@…".

#18 @hmbashar
4 months ago

Test Report

Description

I've Apply the both Patch 45714.patch and PR PR5708 anyone doesn't solve the issue check attached.

Environment

  • WordPress: 6.6-alpha-57778-src
  • PHP: 8.3.7
  • Server: nginx/1.25.4
  • Database: mysqli (Server: 8.3.0 / Client: mysqlnd 8.3.7)
  • Browser: Firefox 126.0
  • OS: macOS
  • Theme: Twenty Fifteen 3.7
  • MU Plugins: None activated
  • Plugins:
    • FakerPress 0.6.6
    • Test Reports 1.1.0

Actual Results

  1. ✅ Issue resolved with patch.

Screenshots

Here is my view

https://i.ibb.co/34bKGbR/profile-edit.png
https://i.ibb.co/zNxmR9Y/user-list.png

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


4 months ago

#20 @oglekler
4 months ago

  • Milestone changed from 6.6 to 6.7

This ticket was discussed during bug scrub. According to the test report, the issue is not resolved with existing patches, and additional work is needed. I am moving this ticket to the next milestone.

Let's try to solve it in the beginning of the 6.7 to have more time for testing.

Thanks!

add props to @hellofromTonya

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


3 months ago
#21

  • Keywords needs-refresh removed

Removing the wp_filter_kses filter when inserting user.

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

#22 @rajinsharwar
3 months ago

  • Keywords changes-requested removed

I removed the wp_filter_kses, as that filter was converting the "&" to amp for the emails. As the sanitize_email filter is already present, it should remove the non-email elements from the email.

#23 @sudipatel007
3 weeks ago

  • Keywords needs-patch added; has-patch has-unit-tests needs-testing removed

Test Report

Description

Issue has not been resold after patch in wp 6.7

Environment

  • WordPress: 6.7-alpha-59082
  • PHP: 8.2.23
  • Server: Apache/2.4.62 (Debian)
  • Database: mysqli (Server: 11.5.2-MariaDB-ubu2404 / Client: mysqlnd 8.2.23)
  • Browser: Safari 17.6 (macOS)
  • Theme: Twenty Twenty-Four 1.2
  • MU-Plugins: None activated
  • Plugins:
    • WordPress Beta Tester 3.5.6

Screenshot

https://drive.google.com/file/d/1gHymyg-0xX_xN7uTdpS6j_VnZLSvpBXW/view?usp=sharing

Note: See TracTickets for help on using tickets.