Skip to content

Commit

Permalink
catch rewrites that pass URI and QUERY STRING as one; related #1551
Browse files Browse the repository at this point in the history 
Some combinations of webserver and php-fpm do this, others dont. The
default in .htaccess caters for the last one only, and we don't want to
throw an exception here if we can correct it for the user.
  • Loading branch information
@WanWizard
WanWizard committed Apr 7, 2017
1 parent 37fd678 commit b873970
Showing 1 changed file with 5 additions and 3 deletions.
8 changes: 5 additions & 3 deletions classes/input/instance.php
View file
Expand Up @@ -182,16 +182,18 @@ public function uri()

// Lets split the URI up in case it contains a ?. This would
// indicate the server requires 'index.php?' and that mod_rewrite
// is not being used.
// is not being used, or that an incorrect rewrite caused the URI
// to include the query string
preg_match('#(.*?)\?(.*)#i', $uri, $matches);

// If there are matches then lets set set everything correctly
if ( ! empty($matches))
{
$uri = $matches[1];

// only reconstruct $_GET if we didn't have a query string
if (empty($_SERVER['QUERY_STRING']))
// only reconstruct $_GET if we didn't have a query string, or
// it contains the URI path as well as the actual query string
if (empty($_SERVER['QUERY_STRING']) or strpos($_SERVER['QUERY_STRING'], '/') == 0)
{
$_SERVER['QUERY_STRING'] = $matches[2];
parse_str($matches[2], $_GET);
Expand Down

9 comments on commit b873970

@icetique
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:
https://domain.com/transactions

For this case, the $matches is empty (because $uri does not contain '?') and does not pass the Security::clean

@WanWizard
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter, because in that case we're sure $_GET is empty, so there is nothing to clean.

@icetique
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not true, see the sample:

			var_dump(Input::get());
			// If there are matches then lets set set everything correctly
			if ( ! empty($matches))
			{
				$uri = $matches[1];
				// only reconstruct $_GET if we didn't have a query string
				if (empty($_SERVER['QUERY_STRING']) or strpos($_SERVER['QUERY_STRING'], '/') == 0)
				{
					$_SERVER['QUERY_STRING'] = $matches[2];
					parse_str($matches[2], $_GET);
					$_GET = \Security::clean($_GET);
				}
			}

Returns:
array(1) { ["/transactions"]=> string(0) "" }

@WanWizard
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you mean GET isn't emptied. Hmm... Good point.

@WanWizard
Copy link
Member Author

@WanWizard WanWizard commented on b873970 Apr 7, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be better:

	// in case of incorrect rewrites, we may need to cleanup and
	// recreate the QUERY_STRING and $_GET
	if (strpos($_SERVER['QUERY_STRING'], '/') == 0)
	{
		// reset $_GET
		$_GET = array();

		// lets split the URI up in case it contains a ?.  This would
		// indicate the server requires 'index.php?'
		preg_match('#(.*?)\?(.*)#i', $uri, $matches);

		// If there are matches then lets set set everything correctly
		if ( ! empty($matches))
		{
			// first bit is the real uri
			$uri = $matches[1];

			// second bit is the real query string
			$_SERVER['QUERY_STRING'] = $matches[2];
			parse_str($matches[2], $_GET);
			$_GET = \Security::clean($_GET);
		}
	}

@icetique
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this one is the winner, I will test it for few days and let you know

@WanWizard
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks. I'll be awaiting your feedback.

@icetique
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I tested it quite well already and can confirm that everything is working as expected on FPM and FastCGI. Of course somebody should test it on other environments.

Thank you for quick fix.

@WanWizard
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll push the change so it can be tested by a wider audience.

Please sign in to comment.