WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#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:
PR Number:

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 3 years ago.
Apply trim() to sanitization process of the external header video value.
39125.2.diff (2.2 KB) - added by tyxla 3 years ago.
Update patch to include a unit test.

Download all attachments as: .zip

Change History (12)

#1 @celloexpressions
3 years 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
3 years ago

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

#2 @tyxla
3 years 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
3 years 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
3 years 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
3 years ago

Update patch to include a unit test.

#5 @tyxla
3 years 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
3 years 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.


3 years ago

#8 @westonruter
3 years 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
3 years ago

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

Reopening for 4.7.1

#10 @dd32
3 years 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.