wp_die() triggers block when using ModSecurity Core Rules

[cstrosser]

wp_die() causes ModSecurity (using Core Rules), a commonly used Apache plugin, to throw a 406 error, blocking the message from displaying.

This seems to be triggered by the fact that a 500 error is thrown. I went and changed the default status code to 401 (Unauthorized) and it worked like a charm. I just wonder if there is a better fix for this issue... or simply a better status code to use.

Perhaps making a group of functions to make the error codes more focused.

Example:

wp_die_auth( __('You do not have sufficient permissions to access this page.') );

/**
 * Exits WordPress with Unauthorized status code.
 *
 * @see wp_die()
 */
function wp_die_auth($message, $title = '') {
	wp_die( $message, $title, 401 );
}

[westi]

Looking at ​http://www.owasp.org/index.php/Category:OWASP_ModSecurity_Core_Rule_Set_Project#tab=About these rules are only Alpha quality.

Do you have more info on what the rule is trying to prevent?

[dd32]

I have a feeling its recently released, see ​http://blog.modsecurity.org/

If i recall correctly, Its due to some security people disliking 500 errors for web app error handling, and wanting people to use the 40x error set. But theres not a 400 error code for all cases either.. IMO, the rule is pointless.

[nacin]

See also #11286.

I'm willing to take ownership of this and go through wp_die references and add response codes. I imagine that would mean creating some shortcut wp_die functions different requests, i.e. authentication, forbidden, not found, internal error, (401, 403, 404, 500) etc.

[miqrogroove]

Status 401 is not appropriate, unless someone has been adding a lot of features I didn't know about.

[miqrogroove]

See also #11289.

[aaroncampbell]

Please see also #12341 and #12529. Here's why I don't like 500 errors (pasted from #12529):

Part of the reason I want to do this, is that I've noticed that automated systems trying to break a site seem to see a 500 error as a clue that they are getting closer to their goal. They seem to pound a lot harder on the scripts that return 500 errors, so I'd like to clean these up and only return them when they're actually appropriate.

Currently wp_die is used 278 times in 86 files. However, we only specify an HTTP response code 3 times, which means 99% of these calls return a 500 status code.

[nacin]

Let's do this early 3.1.

[lloydbudd]

Here is another symptom reported by a customer:

"On a blackberry bold if I try to submit a comment that is empty the blackberry displays a message saying that a 500 error was generated instead of the page with the message that the comment was empty. The blackberry stops the page from loading which leaves the user confused."

[solarissmoke]

Here's a patch which proposes a bunch of new functions to call wp_die with different status codes. Meanwhile the default response is changed to 200.

I'm thinking that we need consensus on this first. Then it is just a case of sifting through all existing wp_die() calls and changing them accordingly.

[nacin]

Don't think we should change the default.

[solarissmoke]

Replying to nacin:

Don't think we should change the default.

Okay. How about just adding a wp_forbid function which issues a 403? - that seems the most appropriate for a number of current uses of wp_die - anything more can be added as/when needed in the future.

[westi]

This is a low priority buy worth task that I would like to see us do but it is not a priority for 3.2 so removing 3.2-early

[johnbillion]

#18030 was marked as a duplicate.

[joehoyle]

I'm happy to take ownership of this, this is pretty important for me, as throwing 500s when it isn't really screws up with my monitoring!

For the current patch, I agree with nacin that the default should not be changed. I also don't really care for the new convenience functions. For real convenience, nacin and I discussed the 3 param to wp_die() can handle an integer which is a shortcut for passing array( 'response' => 500 );

Attached patch adds the integer shortcut to wp_die and replaces all instances of "Cheatin Uh?" to pass a 403 response. I'd rather not try tackle 100% of the wp_die calls in one patch here, rather go incremental.

[joehoyle]

Ping nacin, I know we discussed this in wceu - how do you feel about this patch? I know there was talk about the second param of '' being a pain, however - I was thinking over this, I think introducing a similarly named function like wp_die_forbidden or something just increases the amount of inconsistency with having different params.

[nacin]

This function already has a ton of "magic", I'll give you that. Example: for wp_die( $wp_error_object ), that WP_Error object could also have a title in its error data, and it'd magically work.

But we're already giving this function a new prototype. Existing:

wp_die( string|WP_Error $message = '', $title = '', array $args = array() )

New:

wp_die( string|WP_Error $message = '', $title = '', int $response_code = 500 )

I've been convinced to be conservative on method overloading because it really damages IDE usage (example: add_query_arg()) but as long as "standard" arguments are listed (in add_query_arg()'s case, they are not, and func_get_arg() is used), then I'm more for it. Why not an additional prototype of:

wp_die( string|WP_Error $message = '', int $response_code = 500 )

If we had a default message (which we don't), I'd even be OK with even allowing this, too:

wp_die( int $response_code = 500 )

I dunno. I can be convinced either way. Check out 10551.diff​ as a sample.

[joehoyle]

I see what you mean. I think given that the second argument is so seldom used, I can get on board with having the second arg be used for the code.

IMO this is too far: wp_die( int $response_code = 500 ) I'd be worried about it giving unexpected behaviour, e.g.:

wp_die( $request->error_message ); // could unknowingly be an int, and do something unexpected.

+1 for nacin's patch.

[johnbillion]

I think this is fine because it changes the signature of wp_die() but doesn't change it for the various die handler callbacks.

I agree with Joe, wp_die( int $response_code = 500 ) would mean introducing a default error message and we don't want to encourage people to avoid providing their own.

While we're at it, the inline docs for _default_wp_die_handler() need updating because $message can be a WP_Error.

[wonderboymusic]

to be paired with #11286

[johnbillion]

In 30355:

Allow the response code to be passed as a shorthand to the $title or $args parameter of wp_die(), for brevity.

See #10551 and #11286
Props nacin

[johnbillion]

In 30356:

Switch to a 403 response code in places where it is more appropriate than a 500 due to permissions errors.

Fixes #10551
Props nacin

[joehoyle]

Wohoo!

[nacin]

This was mostly joehoyle's leg work and initial patch, props (and thanks) joe!

Replying to joehoyle:

could unknowingly be an int, and do something unexpected

FWIW, I agree this could be awkward, but this is also precisely how die/exit already work, in that they are exit codes when they are ints.

Replying to johnbillion:

would mean introducing a default error message and we don't want to encourage people to avoid providing their own.

OK, that argument definitely resonates with me. Carry on!

[DrewAPicture]

If $title also now accepts an integer as described in [30355], that string|int parameter type should be reflected.

[DrewAPicture]

In 30379:

Ensure the mixed type of string|int is reflected on the $title parameter in wp_die().

The ability to pass an error code as short-hand to the $title and $args parameters was added in r30355. Changes also include cleaned-up formatting and line-wraps for other documentation in the DocBlock.

See [30355]. Fixes #10551.

[johnbillion]

In 30507:

Update the inline docs for wp_die() to reflect parameter changes made in r30355

See #10551

[rachelbaker]

#29829 was marked as a duplicate.