WordPress.org

Make WordPress Core

#39125 closed defect (bug) (fixed)

Customize: Video Header YouTube field has issues when whitespace is inserted at beginning or end of URL

Reported by: davidakennedy Owned by: westonruter
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch has-unit-tests commit fixed-major
Focuses: Cc:

Description

If the YouTube text field for video headers has white space at the beginning or end of the string, it causes issues. To reproduce:

  1. Go to Header Media in the Customizer with Twenty Seventeen activated.
  2. Insert a YouTube URL, with a space at the beginning of the string.
  3. In the preview, a refresh happens, but the video isn't there. No errors are output as to why that might be.
  4. Save & Publish.
  5. On the front end, nothing is output.

The reverse:

  1. Go to Header Media in the Customizer with Twenty Seventeen activated.
  2. Insert a YouTube URL, with a space at the end of the string.
  3. In the preview, a refresh happens, and a YouTube iframe is there, but the video errors out.
  4. Save & Publish.
  5. On the front end, a YouTube iframe is there, but the video errors out.

We should handle errors better in the Customizer for this type of thing, and potentially trim whitespace if it's not too aggressive.

Attachments (2)

39125.diff (1.1 KB) - added by tyxla 13 months ago.
Apply trim() to sanitization process of the external header video value.
39125.2.diff (2.2 KB) - added by tyxla 12 months ago.
Update patch to include a unit test.

Download all attachments as: .zip

Change History (12)

#1 @celloexpressions
13 months ago

  • Milestone changed from Awaiting Review to 4.7.1

We should be able to add trim to the sanitization or validation callbacks. I think it's using esc_url now, which I'm surprised doesn't already do that. Moving to 4.7.1 for investigation since it's new in 4.7.

@tyxla
13 months ago

Apply trim() to sanitization process of the external header video value.

#2 @tyxla
13 months ago

  • Keywords has-patch added

The current sanitization is using esc_url_raw(), which converts the spaces to %20.

The diff I've attached introduces a new sanitization callback for the external_header_video theme mod value. That callback will first trim() the value, before escaping it with esc_url_raw().

#3 @celloexpressions
12 months ago

  • Version set to 4.7

39125.diff makes sense to me. It would be nice if we could trim all inputs before sanitizing them, but that could potentially cause a lot of unexpected behavior. This targeted fix probably makes the most sense for 4.7.1.

#4 @joemcgill
12 months ago

  • Keywords needs-unit-tests added

+1 for the approach in 39125.diff. Adding a small unit test will ensure we don't reintroduce this bug later.

@tyxla
12 months ago

Update patch to include a unit test.

#5 @tyxla
12 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

I've uploaded an updated patch - 39125.2.diff includes a unit test which assures sanitization will also trim any whitespace characters.

#6 @westonruter
12 months ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


12 months ago

#8 @westonruter
12 months ago

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

In 39560:

Customize: Trim whitespace for URLs supplied for external_header_video to prevent esc_url_raw() from making them invalid.

Props tyxla.
See #38172.
Fixes #39125.

#9 @westonruter
12 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.7.1

#10 @dd32
12 months ago

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

In 39573:

Customize: Trim whitespace for URLs supplied for external_header_video to prevent esc_url_raw() from making them invalid.

Props tyxla.
See #38172.
Merges [39560] to the 4.7 branch.
Fixes #39125.

Note: See TracTickets for help on using tickets.