Make WordPress Core

Opened 20 months ago

Last modified 4 months ago

#49277 new enhancement

Implement email sanitize in REST API

Reported by: spacedmonkey Owned by:
Milestone: 5.9 Priority: normal
Severity: normal Version: 4.4
Component: REST API Keywords: has-patch 2nd-opinion has-unit-tests
Focuses: Cc:


Implement email sanitize in REST API over just using sanitize_text_field

Attachments (1)

49277.diff (2.5 KB) - added by spacedmonkey 20 months ago.

Download all attachments as: .zip

Change History (11)

20 months ago

#1 @TimothyBlynJacobs
20 months ago

  • Keywords 2nd-opinion added

Is this stricter than sanitize_text_field? If so, I think we'd want to make sure it isn't stricter than is_email would allow, right?

Separately, as I understand it, filter_var historically hasn't been used in WordPress since I think it can be disabled and we don't list it as a required extension.

Cc: @SergeyBiryukov, @jrf.

#2 @spacedmonkey
20 months ago

filter_var is used elsewhere in core.

sanitize_text_field is applied to emails first then filter_var is run. The idea is to just remove character that are not valid in an email. It doesn't do a lot of validation that is_email, checking domain length etc.

#3 @spacedmonkey
20 months ago

  • Keywords has-unit-tests added

#4 @TimothyBlynJacobs
20 months ago

Does that line actually get executed? It looks like it prefers regex which I think needs to be available to run WP?

Related #28170, #16867

The idea is to just remove character that are not valid in an email.

Right, what I'm trying to figure out, is if there is a case where the sanitization would not allow an email that would have previously been allowed by is_email(). I understand filter_var doesn't do the complex checking is_email does. But I'm wondering if any of the characters it strips would've been allowed by is_email.

#5 @spacedmonkey
20 months ago

I am not locked to using filter_var. Regex will work just as well.

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.

18 months ago

#7 @kadamwhite
18 months ago

@SergeyBiryukov After discussion in the REST API weekly meeting we'd love to get your eyes on this briefly, it'd make us more confidant in the solution

This ticket was mentioned in PR #786 on WordPress/wordpress-develop by lukaspawlik.

10 months ago

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

@TimothyBJacobs @spacedmonkey this is the refreshed work of patch attached into Trac ticket. Based on your discussion I've ported behaviour of filter_var into PHP code. Additionally I've added an extra check to avoid sanitization of email when is_email function returns true what is most likely an indicator that value is recognized correctly and doesn't need to be sanitized.

Please let me know if you have any questions.

#9 @spacedmonkey
4 months ago

@TimothyBlynJacobs I think the provided patch seems good. Any chance of getting this in 5.8?

#10 @spacedmonkey
4 months ago

  • Milestone changed from Awaiting Review to 5.9
Note: See TracTickets for help on using tickets.