FCGI improvements. #9733
FCGI improvements. #9733
Conversation
sbordet
commented
May 4, 2023
sbordet
commented
- Better handling for HTTPS parameter on server side.
- Better handling of unknown frame types.
* Better handling for HTTPS parameter on server side. * Better handling of unknown frame types. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
| notifyFailure(headerParser.getRequest(), x); | ||
| buffer.position(buffer.limit()); |
There was a problem hiding this comment.
Swap these two lines. i.e consume the buffer before notifying the user
| private void notifyFailure(int request, Throwable failure) | ||
| { | ||
| listener.onFailure(request, failure); | ||
| } | ||
|
|
| else | ||
| processField(field); | ||
| } | ||
|
|
||
| public void onHeaders() | ||
| { | ||
| String pathQuery = URIUtil.addPathQuery(_path, _query); | ||
| // TODO https? | ||
| MetaData.Request request = new MetaData.Request(_method, HttpScheme.HTTP.asString(), hostPort, pathQuery, HttpVersion.fromString(_version), _headers, -1); | ||
| HttpScheme scheme = _secure != null ? HttpScheme.HTTPS : HttpScheme.HTTP; |
There was a problem hiding this comment.
What is the value of _secure for? Could it be "false" i.e. saying that it is not secure?
There was a problem hiding this comment.
RFC 3875 (https://datatracker.ietf.org/doc/html/rfc3875) does not define HTTPS, but apparently for PHP it must be set to a non-empty value to indicate that the request was secure (https://www.php.net/manual/en/reserved.variables.server.php).
We use HTTPS=on for compatibility with PHP applications such as WordPress, but other applications require true or 1, see for example: https://build.trac.wordpress.org/browser/branches/6.2/wp-includes/load.php#L1473
So HTTPS=false as per PHP spec would still mean that the request is secure because it's non-empty, but it really depends on the application.
🤷🏼
There was a problem hiding this comment.
Maybe we should have a list of affirmative values: "true", "1", "on", "secure", "https". Anything else is not secure.
At the very least can we exclude the empty string?
There was a problem hiding this comment.
-1 for the list, +1 for non-empty.
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
