Make WordPress Core

Opened 4 years ago

Last modified 15 months ago

#50781 new defect (bug)

500 error caused by customize_changeset_uuid for non-authenticated users

Reported by: bacardy4's profile bacardy4 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: needs-unit-tests needs-testing needs-patch 2nd-opinion
Focuses: Cc:

Description

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.

Attachments (8)

50781_1.patch (1.4 KB) - added by sumanm 4 years ago.
This patch sets 404 page instead of 500 error for non existing changeset UUID if unauthenticated user
50781.patch (1.7 KB) - added by sumanm 4 years ago.
In addition to 404 page instead of 500 error, it also adds nocache headers and noindex for robots
50781.2.diff (1.7 KB) - added by audrasjb 4 years ago.
Customizer: Redirects URLs to 404 if changeset UUID does not exists for unauthenticated users
50781.3.patch (1.5 KB) - added by sumanm 4 years ago.
added additional check for a theme or plugin has made changes
50781.3.2.patch (1.5 KB) - added by sumanm 4 years ago.
added additional check for a theme or plugin has made changes
50781.4.patch (1.8 KB) - added by sumanm 4 years ago.
50781.diff (5.3 KB) - added by noisysocks 4 years ago.
50781.3.diff (5.3 KB) - added by noisysocks 3 years ago.

Download all attachments as: .zip

Change History (36)

#1 @SergeyBiryukov
4 years ago

  • Component changed from General to Customize

#2 follow-up: @dlh
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.6
  • Version changed from 5.4.2 to 4.7

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.

#3 in reply to: ↑ 2 @bacardy4
4 years ago

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
4 years ago

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

@sumanm
4 years ago

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

@audrasjb
4 years ago

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

#4 @audrasjb
4 years ago

  • Keywords has-patch added; needs-patch removed

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.

#5 follow-up: @peterwilsoncc
4 years ago

  • Keywords needs-refresh needs-unit-tests added

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
4 years ago

added additional check for a theme or plugin has made changes

@sumanm
4 years ago

added additional check for a theme or plugin has made changes

#6 in reply to: ↑ 5 @sumanm
4 years ago

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
Last edited 4 years ago by sumanm (previous) (diff)

@sumanm
4 years ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#8 @hellofromTonya
4 years ago

  • Keywords needs-testing added
  • Milestone changed from 5.6 to 5.7

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.

#9 @sumanm
4 years ago

  • Keywords dev-feedback added; needs-refresh removed

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


4 years ago

#11 follow-up: @noisysocks
4 years ago

  • Keywords dev-feedback removed

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
4 years ago

#12 in reply to: ↑ 11 @noisysocks
4 years ago

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#14 @peterwilsoncc
3 years ago

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

#15 @noisysocks
3 years ago

50781.3.diff switches to 404.

#16 @dlh
3 years ago

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?

This ticket was mentioned in Slack in #core by dlh. View the logs.


3 years ago

#18 @peterwilsoncc
3 years ago

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.

#19 @lukecarbis
3 years ago

  • Milestone changed from 5.7 to 5.8

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#22 @hellofromTonya
3 years ago

  • Keywords needs-patch added; has-patch removed

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

#23 @JeffPaul
3 years ago

  • Milestone changed from 5.8 to Future Release

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.

#24 @joostdevalk
20 months ago

  • Milestone changed from Future Release to 6.2

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.

If you want to prevent browser cache from getting annoying when you log in after, we could send a vary: cookie header.

Last edited 20 months ago by joostdevalk (previous) (diff)

This ticket was mentioned in Slack in #core by costdev. View the logs.


16 months ago

#26 @costdev
16 months ago

  • Keywords 2nd-opinion added

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.

#27 @westonruter
15 months ago

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.

#28 @costdev
15 months ago

  • Milestone changed from 6.2 to Future Release

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

Note: See TracTickets for help on using tickets.