WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 12 days ago

#50781 new defect (bug)

500 error caused by customize_changeset_uuid for non-authenticated users

Reported by: bacardy4 Owned by:
Milestone: 5.7 Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: has-patch needs-unit-tests needs-testing
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 (7)

50781_1.patch (1.4 KB) - added by sumanm 3 months 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 3 months 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 2 months ago.
Customizer: Redirects URLs to 404 if changeset UUID does not exists for unauthenticated users
50781.3.patch (1.5 KB) - added by sumanm 5 weeks ago.
added additional check for a theme or plugin has made changes
50781.3.2.patch (1.5 KB) - added by sumanm 5 weeks ago.
added additional check for a theme or plugin has made changes
50781.4.patch (1.8 KB) - added by sumanm 5 weeks ago.
50781.diff (5.3 KB) - added by noisysocks 12 days ago.

Download all attachments as: .zip

Change History (19)

#1 @SergeyBiryukov
4 months ago

  • Component changed from General to Customize

#2 follow-up: @dlh
4 months 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 months 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
3 months ago

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

@sumanm
3 months ago

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

@audrasjb
2 months ago

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

#4 @audrasjb
2 months 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
6 weeks 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
5 weeks ago

added additional check for a theme or plugin has made changes

@sumanm
5 weeks ago

added additional check for a theme or plugin has made changes

#6 in reply to: ↑ 5 @sumanm
5 weeks 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 5 weeks ago by sumanm (previous) (diff)

@sumanm
5 weeks ago

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


4 weeks ago

#8 @hellofromTonya
4 weeks 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 weeks ago

  • Keywords dev-feedback added; needs-refresh removed

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


3 weeks ago

#11 follow-up: @noisysocks
12 days 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
12 days ago

#12 in reply to: ↑ 11 @noisysocks
12 days 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.

Note: See TracTickets for help on using tickets.