Ppsl: final step Input_Instance lazy reading php://input

[Illirgway]

As in 9d6f4e6 , in subsequent \Requests with php 5.6+ we have redundant re-reading 'php://input' one time per request (simple example is request to non-exist page that leads to forge 2 Request instances - for initial request and for 404 == 2 times 'php://input' reading) , and this commit fix this by reading php://input on demand 1 time to class' static var, not object attr + retains what was achieved by previous commit

NOTE: this can potentially break functionality of Input_Instance's child user's classes

[WanWizard]

Because of your NOTE, I am not in favour of this.

It might be better to add this to the constructor, in case an Input_Instance was passed:

$this->input_raw = $input->input_raw;

like is done with all other global inputs...

[Illirgway]

we use $this->raw() inside Input_Instance every time we create object of this class with Input_Instance $input === null

Would it better then to make private static variable $php_input_cache (or siimilar name), fill it with php://input content 1 time inside Input_Instance::_init and lazy copy this var into object' variable $this->input_raw = static::$php_input_cache ?

Due to the fact that if we autoload Input_Instance, then we need at least 1 reading of 'php://input' (main request), so we can do that in Input_Instance::_init fn without unnecessary reading and re-reading. And such a change doesn't break child classes.

[WanWizard]

I don't see the point.

The first time $this->raw() is called, is from hydrate(), which is called from the constructor, for the first instance of Input_Instance created. Subsequent requests will always pass a parent instance to Input, so it will not call hyrate again, instead it copies over the values from the parent. php://input is read only once.

[Illirgway]

When requesting an non-exists controller

https://github.com/fuel/fuel/blob/1.9/develop/public/index.php#L172

try
{
	require APPPATH.'bootstrap.php';
	$response = $routerequest();
}
catch (HttpNotFoundException $e)
{
	$response = $routerequest('_404_', $e);
}

==> call 2 times https://github.com/fuel/fuel/blob/1.9/develop/public/index.php#L131

$routerequest = function($request = null, $e = false)
{
	Request::reset_request(true);		// ATN (1) this
	...
	elseif ($e === false)
	{
		$response = Request::forge()->execute()->response();
	}
	elseif ($route)
	{
		$response = Request::forge($route, false)->execute(array($e))->response();
	}
	...

call first time with $options = true and second with $options = false https://github.com/fuel/core/blob/1.9/develop/classes/request.php#L60 ($options['driver'] is empty)

	...
	$request = new static($uri, isset($options['route']) ? $options['route'] : true, $method);
	if (static::$active)	// no active request first time, parent === null // protected $parent = null;
	{
		$request->parent = static::$active;
		static::$active->children[] = $request;
	}
	...

which create 2 times instance of \Request => forge 2 times \Input with Request $new = $this (this new object of class \Request)
https://github.com/fuel/core/blob/1.9/develop/classes/request.php#L278

		...
		// forge a new input instance for this request
		$this->input = \Input::forge($this, static::$active ? static::$active->input() : null);
		...

which leads to direct create 2 objects of Input_Instance (not reuse prev instance)
https://github.com/fuel/core/blob/1.9/develop/classes/input.php#L44

		if ($new)
		{
			return new \Input_Instance($new, $input);
		}

taking into account this
https://github.com/fuel/core/blob/1.9/develop/classes/request.php#L391

		...
		// Make the current request active
		static::$active = $this;
		if ( ! $this->route)
		{
			static::reset_request();
			throw new \HttpNotFoundException();
		}
		...

and this
https://github.com/fuel/core/blob/1.9/develop/classes/request.php#L148

	public static function reset_request($full = false)
	{
		// Let's make the previous Request active since we are done executing this one.
		static::$active and static::$active = static::$active->parent();	// $parent === null on first request, what actually reset $active request to null
		...

and taking into account above ATN (1), we always have $input === null at least in subsequent HttpNotFoundException which actually leads to double call $this->hydrate() at least when 404
https://github.com/fuel/core/blob/1.9/develop/classes/input/instance.php#L91

		...
		if ($input)
		{
			...
		}
		else
		{
			// fetch global input data
			$this->hydrate();
		}
		...

https://github.com/fuel/core/blob/1.9/develop/classes/input/instance.php#L460

		// fetch the raw input data
		$php_input = $this->raw();

so we actually really re-read php://input one more time

[WanWizard]

Ah, that is what you mean. Catching the exception in index.php does indeed create a new "primary" request.

I can't be to bothered with that to be honest, in apps it shouldn't really happen, as it is an exception, and only as a fallback in case if your application doesn't deal with the exception.

[Illirgway]

But with great value of nginx.client_max_body_size (e.g. I admin project with 128M client_max_body_size), we can get a potential blind (i.e. attacker should not to know what exactly endpoint shoul be used, any of non-existent is enough https://github.com/fuel/core/blob/1.9/develop/classes/request.php#L391) DDoS with overmemallocation ~== size of forged huge request on e.g. small vps, and I think we should protect from such type of attack here, isn't it?

		if ( ! $this->route)
		{
			static::reset_request();
			throw new \HttpNotFoundException(); // always thrown when no route found
		}

Sorry for bad english

[WanWizard]

If your worry is DDoS because of memory allocation, then you have a problem, as there are a lot of ways to achieve that.

If you allow 128M as body size, you should have the power to process that, as you could easily have legitimate requests with that body size, which can also swamp your server? The code can't (without specific code for that) determine what is a legitimate request, and what isn't?

edit: Also, when it gets to that point in index.php, isn't the old request object, and it's linked objects not discarded?

[Illirgway]

It was just an example that such settings are possible.

And finally what should I do with this commit: make new with proposed private static var, turn it into issue, revoke it or nothing? )

[Illirgway]

I wrote a wrapper class for this issue which dont break users' child classes

class Input_Instance extends Fuel\Core\Input_Instance
{
	protected static $raw_input_cache;

	// If we autoload this class, that we have at least one Request (main) and should 1 time read php://input
	public static function _init()
	{
		static::$raw_input_cache = file_get_contents('php://input');
	}

	public function raw()
	{
		if ($this->raw_input === null)
		{
			// get php raw input
			$this->raw_input = static::$raw_input_cache;
		}

		return $this->raw_input;
	}
}

Feel free to join it inside a core Input_Instance if you will