home_url() used inappropriately in bbp_verify_nonce_request

[bbn]

home_url() is a wordpress function that supports a filter. for instance, a plugin that enables a multi-lingual installation may alter a user's home_url() based on their language preferences.

The bbp_verify_nonce_request function currently uses home_url() to confirm that the request's $_SERVER information matches the installation's url. However if the home_url() function has a filter applied that adds query arguments, then the match fails and the NONCE verification returns false.

I would submit that this is an inappropriate use of the home_url() function, since it can be transformed arbitrarily. A more reliable way of determining the installation's root URL should be employed.

[johnjamesjacoby]

Since these values are stored in the database, and the API to get that data allows for filtering it in several places, it sounds like you're trying to fix the wrong problem.

home_url() is the correct function; how/why something would add query args directly to it is the actual bug.

Can you walk me through what the values are in the post form, what they should be, and where that data lives?

[bbn]

Hi,

The problem is not the POST form in fact. The problem is in the URL comparison. Here's walk through of the code in bbp_verify_nonce_request with discussion of the values returned in the installation I'm working on.

function bbp_verify_nonce_request( $action = '', $query_arg = '_wpnonce' ) {

	// Get the home URL
	$home_url = strtolower( home_url() );

The value stored in $home_url is ​http://forumnet.ircam.fr/?lang=en .

	// Build the currently requested URL
	$scheme        = is_ssl() ? 'https://' : 'http://';
	$requested_url = strtolower( $scheme . $_SERVER['HTTP_HOST'] . $_SERVER['REQUEST_URI'] );

The value stored in $requested_url is ​http://forumnet.ircam.fr/topic/language-choice-suite/?lang=en.

	// Check the nonce
	$result = isset( $_REQUEST[$query_arg] ) ? wp_verify_nonce( $_REQUEST[$query_arg], $action ) : false;

	// Nonce check failed
	if ( empty( $result ) || empty( $action ) || ( strpos( $requested_url, $home_url ) !== 0 ) )
		$result = false;

The nonce check is fine, but because $home_url is not anywhere inside $requested_url, $result is false.

So the bug is that the bbpress function presumes that home_url will not have query arguments. Since there is a hook that allows a filtering transformation of the home_url() output, I believe this is a possibility that should be respected.

[MZAWeb]

Not a solution, but WPML and bbPress play nice together if you have WPML set up to use "language as a folder" (ie: ​http://forumnet.ircam.fr/en) instead of as a parameter.

[johnjamesjacoby]

home_url() shouldn't have any query arguments, since its supposed to be the home URL. We could likely use the get_option() equivalent, but that still runs filters, and would be vulnerable to the same incorrect override of home_url().

Using the querystring method when you're using pretty permalinks is what's causing the comparison to fail. Using either subdomains or folders should work perfectly fine.

It would be annoying to have to strip querystrings off of home_url() on every nonce check, but it would prevent incompatibilies with other plugins that allow for similar breakage.

[bbn]

Hi JJJ,

I respectfully challenge your assertion that "home_url() shouldn't have any query arguments, since it's supposed to be the home URL." Query strings are part of a URL. In our case the home URL includes a query string. If the home_url function wasn't meant to return anything with query strings, then maybe it would be specified in the wordpress documentation, or better yet checked against in the wordpress code after the filter has been run. This is not the case. Feel free to clarify what you mean by "shouldn't."

Using either subdomains or folders does not work "perfectly fine" for us. I don't need to justify this. Suffice it to say that reasons exist and are subtle but important to us.

Yes, writing software is sometimes annoying!

[johnjamesjacoby]

(In [4552]) In bbp_verify_nonce_request(), parse home_url() to remove any strange characters or query-strings that plugins might append to it. Fixes #2070.