fix: cgi-fcgi status code #6267

PROFeNoM · 2024-02-13T07:55:57Z

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! 😃

bunglegrind · 2024-02-13T13:55:12Z

Well, it sounds deliberated.

jamieburchell · 2024-02-13T16:04:48Z

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 · 2024-02-14T06:41:16Z

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 · 2024-02-14T07:17:57Z

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 · 2024-02-14T07:33:33Z

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 · 2024-02-14T08:02:52Z

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

bunglegrind · 2024-02-14T08:06:51Z

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 · 2024-02-29T09:09:25Z

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

kenjis · 2024-02-29T09:34:55Z

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

@gxgpet Could you merge this?

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 marked this pull request as ready for review February 13, 2024 07:58

PROFeNoM mentioned this pull request Feb 13, 2024

CodeIgniter 3 Compatibility DataDog/dd-trace-php#2515

Merged

2 tasks

fix: cgi-fcgi status code

551b855

PROFeNoM force-pushed the develop branch from ce2e037 to 551b855 Compare February 14, 2024 10:05

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

kenjis approved these changes Feb 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: cgi-fcgi status code #6267

fix: cgi-fcgi status code #6267

PROFeNoM commented Feb 13, 2024 •

edited

bunglegrind commented Feb 13, 2024

jamieburchell commented Feb 13, 2024 •

edited

kenjis commented Feb 14, 2024

jamieburchell commented Feb 14, 2024 •

edited

kenjis commented Feb 14, 2024

bunglegrind commented Feb 14, 2024

bunglegrind commented Feb 14, 2024 •

edited

PROFeNoM commented Feb 29, 2024

kenjis commented Feb 29, 2024

fix: cgi-fcgi status code #6267

Are you sure you want to change the base?

fix: cgi-fcgi status code #6267

Conversation

PROFeNoM commented Feb 13, 2024 • edited

bunglegrind commented Feb 13, 2024

jamieburchell commented Feb 13, 2024 • edited

kenjis commented Feb 14, 2024

jamieburchell commented Feb 14, 2024 • edited

kenjis commented Feb 14, 2024

bunglegrind commented Feb 14, 2024

bunglegrind commented Feb 14, 2024 • edited

PROFeNoM commented Feb 29, 2024

kenjis commented Feb 29, 2024

PROFeNoM commented Feb 13, 2024 •

edited

jamieburchell commented Feb 13, 2024 •

edited

jamieburchell commented Feb 14, 2024 •

edited

bunglegrind commented Feb 14, 2024 •

edited