WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#49991 closed defect (bug) (fixed)

PHP Warnings on invalid requests to wp-json/oembed/1.0/embed

Reported by: dd32 Owned by: TimothyBlynJacobs
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.4
Component: Embeds Keywords: has-patch
Focuses: rest-api Cc:

Description

The url parameter can cause PHP Warnings on invalid requests. Such requests shouldn't be made, but Vulnerability scanners have a tendancy to fuzz the API since it's linked to from most pages/posts.

https://...../wp-json/oembed/1.0/embed?url[]=example

PHP Warning: ltrim() expects parameter 1 to be string, array given in wp-includes/formatting.php on line 4297

This happens as the url fields sanitize_callback is set to esc_url_raw which doesn't handle invalid inputs well.
Adding a validate_callback to check it's a string works properly.

Change History (10)

#2 @dd32
5 months ago

PR #236

For some reason adding is_string as the validate_callback didn't help. (Which is why it's an anonymous function wrapping it)

It looks like the type => string parameter still causes a PHP Notice as it's simply run through strval() and not actually validated/rejected.

Last edited 5 months ago by dd32 (previous) (diff)

#3 @TimothyBlynJacobs
5 months ago

Hm, the type should be sufficient. rest_validate_value_from_schema does an is_string check. That whole check should be identical to:

{
  "type": "string",
  "format": "uri"
}

#4 @TimothyBlynJacobs
5 months ago

Yeah, when I apply that change, and I make the example request, I get this error and no PHP warnings.

{
  "code": "rest_invalid_param",
  "message": "Invalid parameter(s): url",
  "data": {
    "status": 400,
    "params": {
      "url": "url is not of type string."
    }
  }
}

#5 @dd32
5 months ago

@TimothyBlynJacobs Thanks for following up, Are there any build steps that need to be done?

It doesn't look like rest_validate_request_arg() runs at all in my test environments with just type specified.
Using { type: string, validate_callback: rest_validate_request_arg } appears to work though.

For some reason adding is_string as the validate_callback didn't help. (Which is why it's an anonymous function wrapping it)

For reference, it probably didn't work for me as is_string() needs 1 param and using it here as it was, passed 3.

It looks like the type => string parameter still causes a PHP Notice as it's simply run through strval() and not actually validated/rejected.

I can't duplicate the PHP Notice now, I'm not sure what combination of things I set to trigger that.

#6 @TimothyBlynJacobs
5 months ago

Ah, you'd need to drop the sanitize_callback as well.

Routes that extend WP_REST_Controller and use get_endpoint_args_for_item_schema automatically apply rest_(sanitize|validate)_request_arg for the sanitize and validate callbacks.

The oembed controller predates that. Instead, WP_REST_Request::sanitize_params will set rest_parse_request_arg as the sanitization callback if one isn't set, and that will apply rest_sanitize_value_from_schema and rest_validate_value_from_schema.

So the definition should be.

array(
        'description' => __( 'The URL of the resource for which to fetch oEmbed data.' ),
        'type'        => 'string',
        'format'      => 'uri',
        'required'    => true,
)

#7 @dd32
5 months ago

ahah! that explains it, thanks @TimothyBlynJacobs

I've updated the PR and tested it locally, it seems to do the trick :)

(I'll throw this over to you to commit if you want)

#8 @TimothyBlynJacobs
5 months ago

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

In 47755:

REST API: Validate that the oembed url parameter is a string.

This prevents a PHP warning from being issued by esc_url_raw when a non-string value is provided.

Props dd32.
Fixes #49991.

#10 @TimothyBlynJacobs
5 months ago

  • Milestone changed from Awaiting Review to 5.5
  • Version set to 4.4
Note: See TracTickets for help on using tickets.