Make WordPress Core

Opened 12 years ago

Closed 11 years ago

#22936 closed defect (bug) (fixed)

XML-RPC WordPress api setOption double escapes args

Reported by: jachzen's profile jachzen Owned by: nacin's profile nacin
Milestone: 3.9 Priority: normal
Severity: major Version:
Component: XML-RPC Keywords:
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Parts of the xml-rpc wordpress api are not usable, as they doublequote strings. e.g. Munich's becomes Munich\\'s.

wp.setOptions($args) escpapes all args and calls update_option() which is then calling mysql_real_escape_string(), leading to a double escaping. To solve this options should not be escaped in wp-setOptions() function.

Here the callStack showing the 2nd escaping:

wp-includes/wp-db.php.wpdb->_real_escape:884
wp-includes/wp-db.php.wpdb->escape_by_ref:950	
wp-includes/wp-db.php.array_walk:0	
wp-includes/wp-db.php.wpdb->prepare:1003	
wp-includes/wp-db.php.wpdb->update:1365	
wp-includes/option.php.update_option:258

Attachments (2)

22936.1.diff (1.3 KB) - added by danielbachhuber 11 years ago.
stripslashes() on option value before updating
22936.2.diff (1.5 KB) - added by danielbachhuber 11 years ago.
Patch with tests for escaped data and non-escaped data.

Download all attachments as: .zip

Change History (12)

#1 @SergeyBiryukov
12 years ago

  • Description modified (diff)
  • Severity changed from blocker to major
  • Summary changed from XML-RPC Wordpress api setOption double escapes args to XML-RPC WordPress api setOption double escapes args

#2 @josephscott
12 years ago

  • Keywords has-patch removed

This ticket was mentioned in IRC in #wordpress-dev by danielbachhuber. View the logs.


11 years ago

@danielbachhuber
11 years ago

stripslashes() on option value before updating

#4 @danielbachhuber
11 years ago

  • Keywords has-patch dev-feedback added
  • Milestone changed from Awaiting Review to 3.9

Thanks for the report, jachzen. Apologies it's taken a while for a response.

Using 22936.1.diff, I get this result against the test case:

    1) Tests_XMLRPC_wp_setOptions::test_set_option_no_escape_strings
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'Mary's Lamb Shop'
    +'Mary's Lamb Shop'

XML-RPC isn't my forte though, so I need an opinion on what's actually intended.

Last edited 11 years ago by SergeyBiryukov (previous) (diff)

#5 @maxcutler
11 years ago

The code part of 22936.1.diff looks correct to me. The reason that the unit test is failing is that the particular option it uses (blog_title/blogname) gets HTML-escaped in sanitize_option, so the single-quote gets converted to its HTML entity encoding.

The test needs to be updated to expect the HTML encoding. Might be worth adding a test case for some other option that doesn't get run through esc_html.

This ticket was mentioned in IRC in #wordpress-dev by maxcutler. View the logs.


11 years ago

@danielbachhuber
11 years ago

Patch with tests for escaped data and non-escaped data.

#7 @danielbachhuber
11 years ago

  • Keywords commit added; dev-feedback removed

#8 @nacin
11 years ago

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

In 27551:

Avoid saving slashed data in XML-RPC's wp.setOptions.

props danielbachhuber.
fixes #22936.

#9 @bpetty
11 years ago

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

This new test never passes in multisite, not when it was added, and still not now.

1) Tests_XMLRPC_wp_setOptions::test_set_option_no_escape_strings
Failed asserting that false matches expected true.

/home/bryan/Projects/wp-vagrant/wordpress/tests/phpunit/tests/xmlrpc/wp/setOptions.php:22

Leaving reopened until fixed, but maybe it's not critical and can be punted.

#10 @nacin
11 years ago

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

In 28067:

Fix a multisite test failure by testing an option that can be updated in multisite.

fixes #22936.

Note: See TracTickets for help on using tickets.