Ensure audio/video urls with specials chars can be embedded

brianhogg commented 2 months ago

Description

Fixes #2749

How has this been tested?

Manually

Checklist:

[x] This PR requires and contains at least one changelog file.
[x] My code has been tested.
[ ] My code passes all existing automated tests.
[x] My code follows the LifterLMS Coding & Documentation Standards.

ideadude commented 2 months ago

The changes here remove the sanitizing of the url. Is parse_url an acceptable way to sanitize the URL?

The new code wouldn't trigger a warning in the Plugin Check Plugin because it's using a get method to get the url instead of $_GET directly.

brianhogg commented 2 months ago

@ideadude When I was looking into why filter_var was failing for valid URLs like those with special chars, it appears filter_var is using an outdated method of validation:

https://www.php.net/manual/en/function.filter-var.php#128462

I added the parse_url just to catch "seriously malformed urls" and instead relying on wp_oembed_get which is validating via wp_safe_remote_get when the embed is discovered and created. Adding an wp_http_validate_url was adding an extra network call plus failing for certain urls which were valid and should return the url wrapped in an iframe.

I did find a method to encode the url and then validate which could provide more validation before handing it off to wp_oembed_get if we'd like use it:

    $path = parse_url( $url, PHP_URL_PATH );
    $encoded_path = array_map( 'urlencode', explode('/', $path ) );
    $url = str_replace( $path, implode( '/', $encoded_path ), $url );

    return filter_var( $url, FILTER_VALIDATE_URL ) ? true : false;

However given FILTER_VALIDATE_URL is not using the up to date spec and the validation is occuring in wp_oembed_get I think we're good as-is.

ideadude commented 2 months ago

I think even a sanitize_text_field would protect from the really bad stuff without breaking valid URLs.

I also think we're good as-is, but the plugin repo guidelines are pretty specific around sanitizing as early as possible and escaping as late as possible to prevent issues from future updates.

gocodebox / lifterlms