WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

#39175 closed defect (bug) (fixed)

Customizer assumes url is passed with replaceState and pushState

Reported by: Christian1012 Owned by: westonruter
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.8
Component: Customize Keywords: has-patch commit fixed-major
Focuses: javascript, administration Cc:

Description

According to the w3 spec on pushState and replaceState, the third argument url is optional.

In customize-preview.js, injectUrlWithState( url ) assumes url is set, and may append query strings to an undefined url. If the javascript on the page uses replaceState without passing in the optional third url paramenter, this results in preview urls like: https://example.com/undefined?customize_changeset_uuid=de92e920-e85f-41c3-ab0a-ed6713655aee.

When provided an url like this, the end result will be the customizer preview displaying a 404 page. Unless, I assume, you have a page with the slug "undefined" or "null".

Attachments (4)

customize-preview.diff (602 bytes) - added by Christian1012 7 months ago.
customize-preview-patch001.diff
39175.2.diff (1.0 KB) - added by westonruter 7 months ago.
39175.3.diff (1.1 KB) - added by Christian1012 7 months ago.
39175.4.diff (1.1 KB) - added by Christian1012 7 months ago.

Download all attachments as: .zip

Change History (17)

@Christian1012
7 months ago

customize-preview-patch001.diff

#1 @Christian1012
7 months ago

  • Component changed from General to Customize
  • Keywords has-patch added

#2 @westonruter
7 months ago

  • Milestone changed from Awaiting Review to 4.7.1
  • Owner set to westonruter
  • Status changed from new to accepted

@Christian1012 good catch!

#3 @Christian1012
7 months ago

@westonruter Glad I could help!

@westonruter
7 months ago

#4 @westonruter
7 months ago

@Christian1012 given that the HTML spec says of history.replaceState():

Updates the current entry in the session history to have the given data, title, and, if provided and not null, URL.

So if the URL is not being updated, should we not then also do this behavior and only call injectUrlWithState if ! _.isUndefined( url ) && null !== url? See 39175.2.diff.

#5 @Christian1012
7 months ago

@westonruter Yup, looks about right to me. I will add that from a brief look around the web, most tutorials reference the history methods in question as taking 3 parameters and do not always make it apparent the third is optional. In my experience, it's always wise to harden your application as much as possible against incorrect implementations downstream. I can imagine a scenario where someone might attempt:

window.location.replaceState( data, '', false );

This code will not throw an exception nor warn them in the console, but work as expected in a theme. However, as consequence, the Customizer would quietly have the same issues as discussed in this ticket. Is it perhaps wise to provide that extra check here as well? See https://core.trac.wordpress.org/attachment/ticket/39175/39175.3.diff.

Last edited 7 months ago by Christian1012 (previous) (diff)

#6 @Christian1012
7 months ago

@westonruter Strike my last, did some more testing, looks like boolean false is interpreted as a string 'false'.

#7 @westonruter
7 months ago

@Christian1012 so then probably better to explicitly check for a non-empty string like: 'string' === typeof url && url.length > 0?

#8 @Christian1012
7 months ago

I think that covers the bases @westonruter. New diff attached.
https://core.trac.wordpress.org/attachment/ticket/39175/39175.4.diff

#9 @westonruter
7 months ago

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

In 39547:

Customize: Allow (optional) url parameter to be omitted in intercepted calls to history.pushState() and history.replaceState() in customize preview.

Fixes issue where calls without the url parameter erroneously end up rewriting the location path to /undefined.

Props Christian1012, westonruter.
Fixes #39175.

#10 @westonruter
7 months ago

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

Re-opening for 4.7.1.

@Christian1012 I realized we could just pass through url as-is as the third parameter instead of null so that's what I did in [39547]. That will allow undefined, null, and false, and "" to all be passed through—all of which should have the same effect as just passing through null, but maybe an author in the future would have a reason to pass a non-string value into the function if the spec changes, however unlikely.

#11 @Christian1012
7 months ago

Thanks @westonruter, enjoyed working with you on this. Let's do it again it sometime.

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


7 months ago

#13 @dd32
6 months ago

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

In 39574:

Customize: Allow (optional) url parameter to be omitted in intercepted calls to history.pushState() and history.replaceState() in customize preview.

Fixes issue where calls without the url parameter erroneously end up rewriting the location path to /undefined.

Props Christian1012, westonruter.
Merges [39547] to the 4.7 branch.
Fixes #39175.

Note: See TracTickets for help on using tickets.