Make WordPress Core

Opened 14 years ago

Last modified 4 years ago

#16101 new defect (bug)

Numeric term fields are strings

Reported by: foofy's profile foofy Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: needs-unit-tests has-patch
Focuses: Cc:

Description

The numeric fields (term_id, parent, etc.) on term objects are strings. I only noticed this because term_exists() uses is_int() to determine if the $term value is an ID or slug.

Only ticket I could find about this is #5381.

sanitize_term() should fix this, but it bails early in the "raw" context. sanitize_term_field() sanitizes the numeric fields in every context. I don't see a reason for sanitize_term() to bail early so I made a patch that takes that out. The patch also adds some missing fields to the list of those to be sanitized and changes term_exists() to use is_numeric() (which will correctly identify strings containing only numbers) instead of is_int().

Attachments (2)

sanitize-numeric-term-fields.patch (1.4 KB) - added by foofy 14 years ago.
16101.diff (1.6 KB) - added by Howdy_McGee 4 years ago.
Changing to ctype_data() to allow string based integers.

Download all attachments as: .zip

Change History (16)

#1 @scribu
14 years ago

From the PHP manual on is_numeric():

Finds whether the given variable is numeric. Numeric strings consist of optional sign, any number of digits, optional decimal part and optional exponential part. Thus +0123.45e6 is a valid numeric value. Hexadecimal notation (0xFF) is allowed too but only without sign, decimal and exponential part.

This could cause subtle bugs.

#2 @foofy
14 years ago

  • Cc foofy@… added

I think it would be fine to leave that as is_int() since the problem goes away if the term objects are properly sanitized.

#3 follow-up: @mdawaffe
14 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to Future Release

Can use ctype_digit( $term ) without making changes to sanitize_term*().

Casting everything to (int) could change the output of XML-RPC requests which can break clients, so we need to be super careful there.

#4 in reply to: ↑ 3 @mdawaffe
14 years ago

Replying to mdawaffe:

Can use ctype_digit( $term ) without making changes to sanitize_term*().

Nacin tells me some hosts disable Ctype, which is ridiculous (on the hosts' parts, not on Nacin's).

#5 @iandunn
12 years ago

  • Cc ian_dunn@… added
  • Keywords changed from has-patch dev-feedback to has-patch dev-feedback

This just bit me in the ass, because I took the term_id from get_terms() and passed it to get_term_link() as $term. get_term_link() assumed that $term was a slug because it failed is_int(), and since there's no term named "237", it returned a WP_Error().

It seems to me like casting the values to int in get_term*() is the proper fix, even if it requires patching/testing XML-RPC code. If there's a consensus behind that approach, I'll try to find time to write and test a new patch over the next couple weeks.

Version 1, edited 12 years ago by iandunn (previous) (next) (diff)

#7 @wonderboymusic
11 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

#8 @SergeyBiryukov
10 years ago

#29756 was marked as a duplicate.

#9 @boonebgorges
10 years ago

  • Keywords needs-unit-tests added

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


9 years ago

This ticket was mentioned in Slack in #docs by majemedia. View the logs.


9 years ago

#12 @majemedia
9 years ago

Can the return values for term_exists have a note added to documentation?

http://codex.wordpress.org/Function_Reference/term_exists

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


4 years ago

@Howdy_McGee
4 years ago

Changing to ctype_data() to allow string based integers.

#14 @Howdy_McGee
4 years ago

  • Keywords has-patch added; needs-patch removed

It seems to me that every instance of is_int() in taxonomy.php is testing against passed input. I agree with Michael that we could use ctype_digit() for all these instances. I suppose the only drawback is that term slugs specifically cannot be integers which seems like a fair trade for more flexible user input.

Timothy pointed out in slack that ctype_digit() is already used in a few places already so the previous mention of host support may be moot.

The latest patch converts 6 instances of is_int() to ctype_digit().

Note: See TracTickets for help on using tickets.