WordPress.org

Make WordPress Core

Opened 8 months ago

Closed 5 months ago

#40230 closed defect (bug) (fixed)

Is it intended to return 500 for this message: You can't give users that role. or Sorry, you are not allowed to give users that role.

Reported by: tuanmh Owned by: johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch needs-testing
Focuses: administration Cc:

Description

Hey there,

We've performed some hacking tests on our system, basically it tries to change role of an user to higher level (e.g. editor to administrator) by using a non-authorised user. We got the message:
"You can’t give users that role." in 4.6.x or "Sorry, you are not allowed to give users that role." in 4.7 as expected but the HTTP status returned is 500.

Should we return 403 instead of 500? Is it intended?

It has caused false alerts on our system as every time we perform the tests, we got alerts through email - which could easily cause oversights to actual 500 errors.

This should be an easy fix:

  • wp-admin/includes/user.php line 62
  • wp-admin/users.php line 113
  • wp-admin/network/site-users.php line line 143

There are other permission's related messages which should return 403 as well.

Attachments (1)

permission_messages_status_code.diff (4.8 KB) - added by tuanmh 8 months ago.

Download all attachments as: .zip

Change History (11)

#1 @johnbillion
8 months ago

  • Component changed from General to Users
  • Focuses administration added
  • Keywords needs-patch good-first-bug added
  • Version 4.6.4 deleted

Thanks for the ticket @tuanmh. Yes, these responses should provide an HTTP status code of 403 instead of 500.

If you can make a patch for these changes that would be great!

#2 @tuanmh
8 months ago

Great, I'll submit a patch ASAP.

Last edited 8 months ago by tuanmh (previous) (diff)

#3 @tuanmh
8 months ago

  • Keywords has-patch added; needs-patch removed
  • Version set to trunk

#4 @tuanmh
8 months ago

Diff added @johnbillion. Please review.

#5 @johnbillion
8 months ago

  • Keywords needs-testing added; good-first-bug removed
  • Milestone changed from Awaiting Review to 4.8
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Version trunk deleted

Thanks @tuanmh! I'll review it shortly.

(P.S. The Version field in Trac is used to indicate the first version that was affected by the issue. As it's a broad issue and there's no specific first version, we simply remove the value.)

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


6 months ago

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


6 months ago

#8 @obenland
6 months ago

  • Milestone changed from 4.8 to Future Release

#9 @johnbillion
5 months ago

  • Milestone changed from Future Release to 4.9

#10 @johnbillion
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 40940:

Users: Use more appropriate HTTP status codes for errors relating to user management.

Also re-uses one error message string.

Props tuanmh

Fixes #40230

Note: See TracTickets for help on using tickets.