Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP Status Codes #4833

Merged
merged 5 commits into from Aug 26, 2018
Merged

Conversation

Sorck
Copy link
Contributor

@Sorck Sorck commented Jul 13, 2018

Makes sure all HTTP status codes provided HTTP response headers are correct for the version of HTTP being used.

Also utilises the centralised function send_http_status in Errors.php to make sure the HTTP version determining code is centralised.

Resolves #4832

…en providing HTTP status headers. Working towards SimpleMachines#4832

Signed-off-by: James Robson <git@sorck.net>
Signed-off-by: James Robson <git@sorck.net>
@@ -44,7 +44,7 @@ function Display()
if (isset($_SERVER['HTTP_X_MOZ']) && $_SERVER['HTTP_X_MOZ'] == 'prefetch')
{
ob_end_clean();
header('HTTP/1.1 403 Prefetch Forbidden');
send_http_status(403, 'Prefetch Forbidden');
Copy link
Collaborator

Choose a reason for hiding this comment

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

To use send_http_status is in my eyes not the right approache,
because this function is not everywhere defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

send_http_status is defined in Errors.php which loaded on every page load (wether from /index.php or /SSI.php). It's also available before any SMF functions are called so it is defined everywhere it's being used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nope see proxy.php
Why you choose to take this risk instead of place the vars ther?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inlining it in over 20 places will be verbose and harder to maintain than a centralised function.

The best solution would be moving send_http_status from Errors.php to Subs.php. Subs.php is a more natural home for the function as send_http_status has a similar use to redirectexit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we use this on many early stages and error cases etc.
i would be for to keep the decentralize concept and use the var methode instead of a function approache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible for SERVER_PROTOCOL to hold a few different values according to RFC 3875 and we need to default to HTTP/1.0 if we don't know the protocol. So the following is not guaranteed to work:
header($_SERVER['SERVER_PROTOCOL'] . " 500 Internal Server Error");

So we need to check the protocol using a preg_match which will look like this each time (if we assume that there could be excess whitespace on the protocol):
header((preg_match('~^\s*(HTTP/[12]\.\d)\s*$~i', $_SERVER['SERVER_PROTOCOL'], $matches) ? $matches[1] : "HTTP/1.0") . " 500 Internal Server Error");
Or
header(preg_match('~^(HTTP/[12]\.\d)$~i', trim($_SERVER['SERVER_PROTOCOL'])) ? $_SERVER['SERVER_PROTOCOL'] : "HTTP/1.0") . " 500 Internal Server Error");

Compared with my proposed version:
send_http_status(500);

With the function in Subs.php it is available in all cases before any SMF function calls. If defined there then it is definitely loaded whether in early stages or in an error cases.

Signed-off-by: James Robson <git@sorck.net>
@jdarwood007
Copy link
Member

https://serverfault.com/questions/442960/nginx-ignoring-clients-http-1-0-request-and-respond-by-http-1-1

According to the mentioned RFCs, this really shouldn't be a issue.

Browsers seem to indicate that they support http2 as well:
https://en.wikipedia.org/wiki/HTTP/1.1_Upgrade_header#Use_with_HTTP/2

I can't find confirmation, but I believe that means that servers should be sending back the proper header info if supported?

proxy.php Outdated
@@ -116,7 +116,7 @@ public function serve()
if ($response === -1)
{
// Throw a 404
header('HTTP/1.0 404 Not Found');
header(send_http_status(404));
exit;
Copy link
Member

Choose a reason for hiding this comment

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

Your sending a double header here.

@jdarwood007
Copy link
Member

Also this function could call http_response_code if its available (5.4 or higher needed) as it can handle most cases better than the built in function.

As well header does have a 3rd param that also works here for sending a status code.

@MissAllSunday
Copy link
Contributor

We have set 5.4.0 as min for SMF 2.1 so we should be on the safe side when using http_response_code() but do a check for http_response_code() availability and leaving the in-house solution doesn't do any harm.

@jdarwood007
Copy link
Member

@Sorck Can you fix the double header issue and we can merge these. Implanting the built in php http_response_code is optional.

Signed-off-by: Jessica <suki@missallsunday.com>
@MissAllSunday
Copy link
Contributor

Double header removed.

@MissAllSunday MissAllSunday merged commit 579d5c8 into SimpleMachines:release-2.1 Aug 26, 2018
jdarwood007 added a commit to jdarwood007/SMF that referenced this pull request Sep 2, 2018
This fixes SimpleMachines#4886 by moving it to the Load.php for guests only.

As well fixed a issue related to SimpleMachines#4833 where maintenance mode can cause a issue with HTTP/2.0 with the spdy protocol.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants