500 error caused by customize_changeset_uuid for non-authenticated users

[bacardy4]

Hello,

I have noticed that if a non-authenticated user visits a URL containing the following get parameter: ?customize_changeset_uuid=SOME_ID_HERE WordPress returns 500 error.

There should be no reason to allow bots to flood someones Apache log with 500 errors by simply adding a get parameter.

If a user is not authenticated and they add the ?customize_changeset_uuid=ID_HERE parameter they should either be redirected or the get parameter should be ignored rather than getting a 500 error.

Thanks for the consideration.

[dlh]

Hi @bacardy4, and welcome to WordPress Trac! Thanks for the report.

I agree: A 500 response does seem out-of-place in this situation. I'm not sure about redirecting the request or ignoring the provided UUID. That isn't what happens when requesting an invalid post ID or term ID, as far as I know. Instead, perhaps the response should be a 404?

Adding this to the 5.6 milestone to see whether a decision can be made.

[bacardy4]

Hi there,

Yes indeed 404 sounds better than 500.

Thank you and regards!

Replying to dlh:

Hi @bacardy4, and welcome to WordPress Trac! Thanks for the report.

I agree: A 500 response does seem out-of-place in this situation. I'm not sure about redirecting the request or ignoring the provided UUID. That isn't what happens when requesting an invalid post ID or term ID, as far as I know. Instead, perhaps the response should be a 404?

Adding this to the 5.6 milestone to see whether a decision can be made.

[sumanm]

This patch sets 404 page instead of 500 error for non existing changeset UUID if unauthenticated user

[sumanm]

In addition to 404 page instead of 500 error, it also adds nocache headers and noindex for robots

[audrasjb]

Customizer: Redirects URLs to 404 if changeset UUID does not exists for unauthenticated users

[audrasjb]

50781.2.diff refreshes the patch against trunk with correct path to the modified file, updated @since mentions and it also fixes few coding standards issues.

[peterwilsoncc]

For 50781.2.diff​:

• to throw a 404 whenever an invalid change set ID is included, regardless of the format, the result of the wp_is_uuid() check on src/wp-includes/class-wp-customize-manager.php#L539 will need to be altered too (see manual testing notes)
• No need for the X-Robots http header, a 404 is basically a supercharged noindex
• Setting a 404 via pre_handle_404 will also set the no cache headers
• It would be good to check if a theme or plugin has made changes in pre_handle_404 before doing anything in customize_changeset_preview_redirect. If a theme or plugin has made changes, defer to it. See WP_Sitemaps::redirect_sitemapxml().

From manual testing, this is what is happening at the moment:

Expected outcomes:

• Valid change sets continue to display preview as expected
• Invalid ID in the expected format (eg, 7d5b3806-b477-4cdc-be3b-53bd4075583f) shows a 404

Unexpected: an invalid format continues to throw a 500 error, eg:

• /?customize_changeset_uuid=7d5b3806-b477-4cdc-be3b-53bd4075583e-nope
• /?customize_changeset_uuid=7d5b3806-
• /?customize_changeset_uuid=7d5b3806-this-brak-esit-53bd4075583e

Some tests would be dandy too.

The customizer is fairly well tested as is so before putting this in it would be good to set up a pull request and run it through the tests matrix just in case it affects an existing test.

[sumanm]

added additional check for a theme or plugin has made changes

[sumanm]

added additional check for a theme or plugin has made changes

[sumanm]

As suggested,
I have created a new patch
50781.4.patch​
Replying to peterwilsoncc:

Added additional check for:

• It would be good to check if a theme or plugin has made changes in pre_handle_404 before doing anything in customize_changeset_preview_redirect. If a theme or plugin has made changes, defer to it. See WP_Sitemaps::redirect_sitemapxml().

Unexpected: an invalid format continues to throw a 500 error, eg:

• /?customize_changeset_uuid=7d5b3806-b477-4cdc-be3b-53bd4075583e-nope
• /?customize_changeset_uuid=7d5b3806-
• /?customize_changeset_uuid=7d5b3806-this-brak-esit-53bd4075583e

Removed:

• to throw a 404 whenever an invalid change set ID is included, regardless of the format, the result of the wp_is_uuid() check on src/wp-includes/class-wp-customize-manager.php#L539 will need to be altered too (see manual testing notes)
• No need for the X-Robots http header, a 404 is basically a supercharged noindex

[hellofromTonya]

From scrub today, we are deep into the beta cycle for 5.6. Punting this ticket to 5.7.

If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

[noisysocks]

I'd suggest we use 400 when the UUID is invalid, 404 when the UUID doesn't correspond to any changeset post, and 403 when the user doesn't have permission to do something.

Is displaying the 404 template really necessary? We can solve the original issue that @bacardy4 reported simply by passing a 4xx response code to wp_die().

[noisysocks]

Replying to noisysocks:

Is displaying the 404 template really necessary? We can solve the original issue that @bacardy4 reported simply by passing a 4xx response code to wp_die().

50781.diff​ demonstrates what I mean here. Let me know what you think.

[peterwilsoncc]

@noisysocks I think I'd prefer the 404 for incorrect IDs, it seems closer to what WordPress does for a non-existent post ID such as http://wordpress-develop.local/?p=1234567890

[noisysocks]

50781.3.diff​ switches to 404.

[dlh]

In 50781.3.diff​, is it intentional that $response isn't passed to the wp_die() call within $this->doing_ajax() at the top of the method as well?

[peterwilsoncc]

Sorry @noisysocks, I wasn't clear: I was thinking of showing the 404 template rather than thinking in terms of the http header. As a valid URL can become invalid once the changeset is published, the page may be visited by legitimate users so the page needs to look nice.

As 5.7 is now in beta, it might be worth taking this off the milestone for further consideration.

[hellofromTonya]

As 50781.3.diff​ is being discussed and iterated upon, removing has-patch and adding needs-patch.

[JeffPaul]

With 5.8 Beta 1 next week, discussion stalled on this during the 5.8 release cycle, and no owner set, I'm going to punt this to Future Release. Once discussion picks back up and an approach is confirmed, this ticket can be added back to a numbered milestone.

[joostdevalk]

I actually disagree with the chosen solution and think we should pick the other solution @bacardy4 suggested: this should be a 301 to the URL without the parameter for every non-logged in user, or user with not enough rights to see the changeset, with a filter to prevent that 301.

Throwing 500s or 404s for this kind of stuff is not needed, and just clutters logs in a meaningless way.

[costdev]

This ticket was discussed during the recent bug scrub. It looks like there may be some discussion needed on the path forward. Adding 2nd-opinion to help get this ticket some more attention during this cycle.

[westonruter]

I'm confused by this point in the description:

I have noticed that if a non-authenticated user visits a URL containing the following get parameter: ?customize_changeset_uuid=SOME_ID_HERE WordPress returns 500 error.

A key purpose of changesets is specifically to allow unauthenticated users to preview changes. I can see this indeed works as expected, if I make a change in the Customizer and save the changes as a draft. When I do so, a preview URL is presented in the UI with the customize_changeset_uuid parameter added.

Nevertheless, if I start making changes in the Customizer and I have not explicitly saved a draft yet, then the Customizer will stash the changes in a wp_customize_changeset post with an autosave status. In this case, if I try copying one of the links in the Customizer preview and accessing it as an unauthenticated user, I then will get the described error, but only because the link includes additional parameters, namely customize_autosaved=on and customize_messenger_channel=preview-1. Nevertheless, if I remove those parameters then it loads as expected. So something isn't quite right here.

In the case of an invalid UUID which fails to match wp_is_uuid(), I think a 400 Bad Request response makes the most sense. But a 404 response would be OK (not 200 though! 😉)

In the case of a valid UUID that no longer corresponds to a wp_customize_changeset post, there would be two possible reasons for this:

1. The changeset was published, meaning the changes are now live.
2. The changeset never existed in the first place.

In the first case, a status of 410 Gone would make sense, but since the changeset is deleted there wouldn't be a way to know it used to exist to differentiate it from the second case above. A 301 Moved Permanently would also make sense, but that would miss an opportunity to communicate to the user explicitly that the changes are now live or that the changes couldn't be found.

Another possibility here would be to return a 300 Multiple Choices response. The body here would say that either the changeset has been published, in which case there can be a link with the customize_changeset_uuid parameter removed (in other words a manual redirect). Otherwise, there can also be a note saying that the changeset may not have ever existed in the first place (in other words a manual 404). I think this would be best.

[costdev]

As this ticket still needs some discussion and a decision about the preferred path forward, I'm moving this ticket to Future Release.