Make WordPress Core

Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#22324 closed enhancement (wontfix)

sanitize_post_field() forgets some integer fields

Reported by: geertdd's profile GeertDD Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: Priority: normal
Severity: minor Version: 3.4.2
Component: General Keywords: has-patch commit
Focuses: Cc:

Description

The post_author and comment_count properties of the $post object should be sanitized to integer values instead of strings (the default data type).

Both those fields are stored in BIGINT(20) columns in the posts table. They can only contain integer values.

Attachments (3)

post.php.diff (532 bytes) - added by GeertDD 12 years ago.
22324.ut.diff (1.1 KB) - added by kovshenin 12 years ago.
22324.fix-xmlrpc.diff (2.6 KB) - added by markoheijnen 12 years ago.
Be sure that post_author is a string when calling the XML-RPC

Download all attachments as: .zip

Change History (25)

@GeertDD
12 years ago

#1 @SergeyBiryukov
12 years ago

  • Keywords commit added
  • Milestone changed from Awaiting Review to Future Release

#2 @SergeyBiryukov
12 years ago

  • Milestone changed from Future Release to 3.6

#3 @nacin
12 years ago

  • Keywords needs-unit-tests added

@kovshenin
12 years ago

#4 @kovshenin
12 years ago

  • Cc kovshenin added

22324.ut.diff checks for integer fields after sanitize_post.

#5 @SergeyBiryukov
12 years ago

In 1193/tests:

sanitize_post() test for integer fields. props kovshenin. see #22324.

#6 @SergeyBiryukov
12 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 23353:

Sanitize post_author and comment_count as integer fields. props GeertDD. fixes #22324.

#7 @bpetty
12 years ago

  • Keywords has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for new unit test failures. The following tests need updates if this is correct:

1) Tests_XMLRPC_mw_getPost::test_valid_post
Failed asserting that 286 is of type "string".   

/wordpress-tests/tests/xmlrpc/mw/getPost.php:57

2) Tests_XMLRPC_mw_getRecentPosts::test_valid_post
Failed asserting that 292 is of type "string".   

/wordpress-tests/tests/xmlrpc/mw/getRecentPosts.php:62

3) Tests_XMLRPC_wp_getPage::test_valid_page
Failed asserting that 368 is of type "string".

/wordpress-tests/tests/xmlrpc/wp/getPage.php:56

4) Tests_XMLRPC_wp_getPost::test_valid_post
Failed asserting that 387 is of type "string".

/wordpress-tests/tests/xmlrpc/wp/getPost.php:51

#8 @markoheijnen
12 years ago

The tests shouldn't be updated. We talking about the XML-RPC here so we probably should update the code then.
I'm looking into it what changed.

#9 @markoheijnen
12 years ago

Looking into it this never should have been committed as it is. Changing the data type is really dangerous and should be tested well and also the external API's should be checked. This since the app can crash now due their strictness of data types.

@markoheijnen
12 years ago

Be sure that post_author is a string when calling the XML-RPC

#10 @markoheijnen
12 years ago

  • Keywords has-patch commit added

Added the fix for the XML-RPC

#12 @SergeyBiryukov
12 years ago

wp.getPage() description in Codex appears to be inaccurate:
http://codex.wordpress.org/XML-RPC_wp#wp.getPage

It states that userid is an int, whereas it was actually a string. Related: ticket:18869:4.

#13 @SergeyBiryukov
12 years ago

  • Keywords needs-unit-tests removed

Updated the Codex page. 22324.fix-xmlrpc.diff passes the unit tests.

#14 @SergeyBiryukov
12 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 23359:

Cast post_author to string in XML-RPC methods. props markoheijnen. fixes #22324.

#15 follow-up: @westi
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm not 100% sure this is a good change to make without further review and testing.

One thing that is probably broken by this change is anything that does post author highlighting of comments because get_comments returns straight db data with everything as strings.

We use a value and type === check here - https://core.trac.wordpress.org/browser/trunk/wp-includes/comment-template.php#L316

Which now will fail as one will be an int and one a string.

#16 @gibrown
12 years ago

  • Cc greg@… added

This uncovered a subtle caching bug on wp.com that I think probably affects a lot of plugins, and maybe elsewhere in core. Two cases.

1) Used to work, but with this change started failing.

  • Post content is stored in the cache and so post_author is returned as a string when get_post() is called.
  • Plugin loads the post_author from cache and then === compares against something like comment_author

2) Failed before (very rare though due to caching, so we had never reproduced it), with this change it works.

  • Post content is loaded from db rather than cache by get_post(). post_author is an int.
  • Plugin does a === comparison which fails against things like comment_author.

The second example is a bug that is probably occurring now in many plugins, but so rarely that it is not noticed.

Given that (2) is actually failing, maybe the int coming out of the db should actually be cast to a string in get_post(), etc.

#17 @ryan
12 years ago

This was reverted on wordpress.com for the aforementioned reasons.

#18 in reply to: ↑ 15 @johnbillion
12 years ago

Replying to westi:

I'm not 100% sure this is a good change to make without further review and testing.

One thing that is probably broken by this change is anything that does post author highlighting of comments because get_comments returns straight db data with everything as strings.

We use a value and type === check here - https://core.trac.wordpress.org/browser/trunk/wp-includes/comment-template.php#L316

Which now will fail as one will be an int and one a string.

This does indeed break post author highlighting. Twenty Twelve is no longer highlighting post author comments because it does an identicality check here: http://core.trac.wordpress.org/browser/tags/3.5.1/wp-content/themes/twentytwelve/functions.php#L291

Last edited 12 years ago by johnbillion (previous) (diff)

#19 @nacin
12 years ago

In 23531:

Revert [23359]. The post_author and comment_count post object fields will remain numeric strings for back compat. see #22324.

#20 @nacin
12 years ago

In 1224/tests:

Change unit tests to reflect [23531]. see #22324.

#21 @nacin
12 years ago

  • Milestone 3.6 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed

#22 @nacin
11 years ago

#25092 was marked as a duplicate.

Note: See TracTickets for help on using tickets.