Change maxSize on RequestHeaderParser

[RuslanHimikEvchev]

Hello!
You make a wonderful project, which we use in production. But we have a moment that has to be constantly maintained and remembered about it. This is the maximum size of the buffer, when parsing the request. Now it's 4096 characters, which is not very convenient. Is it possible to make this parameter configurable or get rid of it?
React\Http\RequestHeaderParser, line 18 on v0.7.4

[WyriHaximus]

Hey why is the 4096 length limit an issue for you? It is only used for the limiting the request header size and not the body. Are your headers exceeding that size and if so by how much?

[clue]

Thanks for filing this! I think it makes sense to provide a way to configure this limit and I suppose we would welcome PRs for this 👍

For the reference: Note that this limit only applies to the total maximum size of the incoming request headers, it does not limit the size of the (much bigger) message bodies or the outgoing response headers.

[RuslanHimikEvchev]

In some cases, we have a more than 4k headers bag. Maximum is 16k

[WyriHaximus]

Dang that is a lot , as @clue suggests would you mind creating a PR for it? :)

[christoph-kluge]

I'm willing to help here but I'm not sure how we should get the stuff to the right place.. any preferences here?

// Option#1 inject via optional parameter in __construct() OR via setter
$config = new ServerConfiguration($options);
$config->setRequestHeaderBufferSize($maxSize);

$server = new Server($callback, $config);
$server->setServerConfiguration($config);

// Option#2 inject server configuration via environment variables (default: 4096)
setenv('REACTPHP_REQUEST_HEADER_BUFFER_SIZE', $maxSize);
$server = new Server($callback);

// Option#3 setter inside the server
$server = new Server($callback);
$server->setRequestHeaderBufferSize($maxSize);

[WyriHaximus]

Tbh I would rather see an extra argument to the constructor with options, like:

$config = [];
$config['max_header_size'] = 1024 * 16;
$server = new Server($callback, $config);

[clue]

Small status update: We've raised the hard-coded default request header size to 8 KIB via #253 for the v0.8.0 release. This means that this will be less of an issue for most consumers of this package.

That being said, I think we all agree that offering a way to configure this limit is the goal here. We will look into coordinating efforts to get this in in one of the next releases

[Manuelacosta98]

Hi i see this issue mark as easy pick so i would love to give it a try.

from the thing i can read the approach would be:

1. Adding second parameter "config" to server class constructor.
2. Adding third parameter "config" to react/http/src/Io/StreamingServer.php constructor.
3. Adding a contructor to react/http/src/Io/RequestHeaderParser.php where the header max size is pass.

or

1. Adding second parameter "config" to server class constructor.
2. Adding a setHeaderMaxSize to react/http/src/Io/StreamingServer.php.
3. Adding a setHeaderMaxSize to react/http/src/Io/RequestHeaderParser.php where the header max size is pass.

The second list i dont know if would work because the streamingServer constructor get the parser to listen right away, so the set would happen after. Thanks would appreciate some feedback

[clue]

This doesn't appear to be a high priority issue ever since we've updated the default to 8 KiB, but I still think it makes sense to expose this configuration.

I'm not sure what this could looks like tho. The StreamingServer is marked internal and I don't think this insignificant feature warrants exposing this class as part of our public API. Likewise, the RequestHeaderParser is also entirely internal only.

Any input and or PRs welcome 👍

[SimonFrings]

Is this issue still relevant, it looks like the original concern has been resolved, or am I overlooking something?

Reference: I am currently going through all issues and pull requests in react to see if they're still open for discussion or got resolved in the meantime.

[clue]

I'm closing this for now as it hasn't received any input in a while and I believe this has been resolved in the meantime. If you feel this is still relevant, please come back with more details and we can always reopen this 👍

Thank you for your effort nonetheless, keep it up! 👍

[fritz-gerneth]

We are running into the same issue over here now. Some other application sets rather large cookies (30kb+) on the domain root, we kind of are forced to receive (running on a subdomain).

If you're ok with it, I'd submit a PR implementing the solution outlined by @Manuelacosta98 :

Adding second parameter "config" to server class constructor.
Adding third parameter "config" to react/http/src/Io/StreamingServer.php constructor.
Adding a contructor to react/http/src/Io/RequestHeaderParser.php where the header max size is pass.

[SimonFrings]

@fritz-gerneth Sounds good, we're always happy about PRs. I could imagine that this involves some bigger adjustments, but if you have a good solution in your mind, I am happy to take a look 👍

[WyriHaximus]

We are running into the same issue over here now. Some other application sets rather large cookies (30kb+) on the domain root, we kind of are forced to receive (running on a subdomain).

If you're ok with it, I'd submit a PR implementing the solution outlined by @Manuelacosta98 :

Adding second parameter "config" to server class constructor.
Adding third parameter "config" to react/http/src/Io/StreamingServer.php constructor.
Adding a contructor to react/http/src/Io/RequestHeaderParser.php where the header max size is pass.

@clue We might have to consider configuration middleware or something at this point. Or a configuration object one can pass into the server class to configure certain things.

[fritz-gerneth]

We might have to consider configuration middleware or something at this point. Or a configuration object one can pass into the server class to configure certain things.

Should this then be merged with the StreamingRequestMiddleware (in a BC way)?

[WyriHaximus]

We might have to consider configuration middleware or something at this point. Or a configuration object one can pass into the server class to configure certain things.

Should this then be merged with the StreamingRequestMiddleware (in a BC way)?

Or a new middleware perhaps