Customizer assumes url is passed with replaceState and pushState

[Christian1012]

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".

[Christian1012]

customize-preview-patch001.diff

[westonruter]

@Christian1012 good catch!

[Christian1012]

@westonruter Glad I could help!

[westonruter]

@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​.

[Christian1012]

@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.

[Christian1012]

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

[westonruter]

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

[Christian1012]

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

[westonruter]

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.

[westonruter]

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.

[Christian1012]

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

[dd32]

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.