Make WordPress Core

Opened 5 years ago

Last modified 3 years ago

#49277 assigned enhancement

Implement email sanitize in REST API

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

Description

Implement email sanitize in REST API over just using sanitize_text_field

Attachments (1)

49277.diff (2.5 KB) - added by spacedmonkey 5 years ago.

Download all attachments as: .zip

Change History (16)

@spacedmonkey
5 years ago

#1 @TimothyBlynJacobs
5 years 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
5 years 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
5 years ago

  • Keywords has-unit-tests added

#4 @TimothyBlynJacobs
5 years 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
5 years 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.


5 years ago

#7 @kadamwhite
5 years 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.


4 years ago
#8

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 years ago

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

#10 @spacedmonkey
4 years ago

  • Milestone changed from Awaiting Review to 5.9

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


3 years ago

#12 @spacedmonkey
3 years ago

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

adamziel commented on PR #786:


3 years ago
#13

I see a few characters that don't fit my mental model of an email address, e.g. { and }. Is the regex accurate? Also, should the same logic be used for validation?

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


3 years ago

#15 @spacedmonkey
3 years ago

  • Milestone changed from 5.9 to Future Release

As discussed in the REST API meeting, this ticket is being punted another release.

Note: See TracTickets for help on using tickets.