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.

Sign up for GitHub

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

Jump to bottom

fix: cgi-fcgi status code #6267

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix: cgi-fcgi status code #6267

wants to merge 1 commit into from

Conversation

PROFeNoM
Copy link

@PROFeNoM PROFeNoM commented Feb 13, 2024 •
edited
Loading

Hi 👋

I work at Datadog, and while doing some tests with CodeIgniter 3, I stumbled upon what I found to be strange behavior when using the cgi-fcgi sapi. I want to understand whether this is a bug or a feature, or if there is an existing workaround. I mostly want to foster discussion and understand :)

Consider this controller:

<?php

class Foo extends CI_Controller {
    function index() {
        throw new \Exception('an exception message');
    }
}

When accessing the /foo endpoint, I expect a 500 status code: This is indeed the case in the response header of a curl request, for instance.

However, this is not the case after the exception handler has been called. For instance, if you add the following to error_exception.php, a 200 status code will be displayed:

<p>Status Code: <?php echo http_response_code(); ?></p>

Forcing the status code during the call to header effectively solves this issue, if this is one.

You may ask why do we care? Basically, currently, it would appear that when using CGI SAPIs, requests that should be marked as 500s are marked as 200s on Datadog's UI :(

Thank you for your help! 😃

@PROFeNoM PROFeNoM changed the title fix: Forces the HTTP response code when using *cgi* sapi fix: Forces the HTTP response code when using *cgi* SAPIs Feb 13, 2024
@PROFeNoM PROFeNoM marked this pull request as ready for review February 13, 2024 07:58
@PROFeNoM PROFeNoM mentioned this pull request Feb 13, 2024
Merged
2 tasks
@bunglegrind
Copy link

Well, it sounds deliberated.

@jamieburchell
Copy link
Contributor

jamieburchell commented Feb 13, 2024 •
edited
Loading

I think so too. The code has been that way forever and underneath in other lines it does set the code. For what reason this was omitted for CGI, I have no idea.

@kenjis
Copy link
Contributor

kenjis commented Feb 14, 2024

That 15-year-old code was intended for CGI.
817163a#diff-bb769d1a64c842b43be080746afeb59ac119e1cf74fe9ff41ca5310dc18e587dR327

The line returns Status: {$code} {$text}, and $code means an HTTP status code.
Therefore if the HTTP response code is not $code, it is a bug, or just the cgi-fcgi was not considered at the time of writing.

The Status header field contains a 3-digit integer result code that
indicates the level of success of the script's attempt to handle the
request.
https://datatracker.ietf.org/doc/html/rfc3875#section-6.3.3

CodeIgniter/system/codeigniter/Common.php

Lines 323 to 336 in 817163a

$server_protocol = (isset($_SERVER['SERVER_PROTOCOL'])) ? $_SERVER['SERVER_PROTOCOL'] : FALSE;
if (substr(php_sapi_name(), 0, 3) == 'cgi')
{
header("Status: {$code} {$text}", TRUE);
}
elseif ($server_protocol == 'HTTP/1.1' OR $server_protocol == 'HTTP/1.0')
{
header($server_protocol." {$code} {$text}", TRUE, $code);
}
else
{
header("HTTP/1.1 {$code} {$text}", TRUE, $code);
}

@jamieburchell
Copy link
Contributor

jamieburchell commented Feb 14, 2024 •
edited
Loading

That 15-year-old code was intended for CGI.

Yes, but why was the response code omitted from the response specifically for CGI? It looks like it's intentional given the other lines that do set it.

@kenjis
Copy link
Contributor

kenjis commented Feb 14, 2024

I'm not confirmed, but it seems if the status code is set to header(), a server error occurs with CGI.
So it is safe that adding code only for cgi-fcgi. > @PROFeNoM

Source: https://qiita.com/Enchan/items/a95e6cc3a651aee7c936#%E7%99%BA%E7%94%9F%E5%8E%9F%E5%9B%A0%E3%81%A8%E5%AF%BE%E7%AD%96

<?php
    // (<?phpの前に任意の文字列がないようにしてください)
    header('HTTP', true, 400);
?>
<html><body>
    <h1>400 Bad Request</h1>
    <p>This page is not working properly.</p>
    <p>If this issue occur repeatedly, please contact administrator.</p>
</body></html>
    HTTP/1.1 500 Internal Server Error
    Server: nginx
    Date: Wed, 28 Oct 2020 09:41:38 GMT
    Content-Type: text/html; charset=iso-8859-1
    Content-Length: 531
    Connection: keep-alive
    <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
    <html><head>
    <title>500 Internal Server Error</title>
    </head><body>
    <h1>Internal Server Error</h1>
    <p>The server encountered an internal error or
    misconfiguration and was unable to complete
    your request.</p>
    <p>Please contact the server administrator at 
    [no address given] to inform them of the time this error occurred,
    and the actions you performed just before this error.</p>
    <p>More information about this error may be available
    in the server error log.</p>
    </body></html>

@bunglegrind
Copy link

So, probably the correct fix is on the if condition if (PHP_SAPI === "cgi") or something

@bunglegrind
Copy link

bunglegrind commented Feb 14, 2024 •
edited
Loading

Yes, but why was the response code omitted from the response specifically for CGI? It looks like it's intentional given the other lines that do set it.

After a quick inspection at RFC 3875, it looks like there's not response code except within the Status header.

@PROFeNoM PROFeNoM changed the title fix: Forces the HTTP response code when using *cgi* SAPIs fix: cgi-fcgi status code Feb 14, 2024
@PROFeNoM
Copy link
Author

Hey @kenjis 👋 Thanks for approving 😃 Do you think this PR will get merged, and if so, do you have an ETA?

@kenjis
Copy link
Contributor

kenjis commented Feb 29, 2024

@PROFeNoM Sorry, I'm not a maintainer. So I don't know.

@gxgpet Could you merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kenjis kenjis kenjis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PROFeNoM @bunglegrind @jamieburchell @kenjis