WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 6 months ago

#50781 new defect (bug)

500 error caused by customize_changeset_uuid for non-authenticated users

Reported by: bacardy4 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: needs-unit-tests needs-testing needs-patch
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 15 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 15 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 15 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 14 months ago.
added additional check for a theme or plugin has made changes
50781.3.2.patch (1.5 KB) - added by sumanm 14 months ago.
added additional check for a theme or plugin has made changes
50781.4.patch (1.8 KB) - added by sumanm 14 months ago.
50781.diff (5.3 KB) - added by noisysocks 13 months ago.
50781.3.diff (5.3 KB) - added by noisysocks 10 months ago.

Download all attachments as: .zip

Change History (31)

#1 @SergeyBiryukov
16 months ago

  • Component changed from General to Customize

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

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

@sumanm
15 months ago

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

@audrasjb
15 months ago

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

#4 @audrasjb
15 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
14 months 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
14 months ago

added additional check for a theme or plugin has made changes

@sumanm
14 months ago

added additional check for a theme or plugin has made changes

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

@sumanm
14 months ago

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


13 months ago

#8 @hellofromTonya
13 months 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
13 months ago

  • Keywords dev-feedback added; needs-refresh removed

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


13 months ago

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

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


10 months ago

#14 @peterwilsoncc
10 months 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
10 months ago

#16 @dlh
10 months 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.


10 months ago

#18 @peterwilsoncc
10 months 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
10 months ago

  • Milestone changed from 5.7 to 5.8

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


7 months ago

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


6 months ago

#22 @hellofromTonya
6 months 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
6 months 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.

Note: See TracTickets for help on using tickets.