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

ob_end_clean does not reset Content-Encoding header #8218

Closed
manuelm opened this issue Mar 18, 2022 · 11 comments
Closed

ob_end_clean does not reset Content-Encoding header #8218

manuelm opened this issue Mar 18, 2022 · 11 comments

Comments

@manuelm
Copy link
Contributor

manuelm commented Mar 18, 2022

Description

Follow up from: #7953

The following code:

<?php
ob_start('ob_gzhandler');
ob_end_clean();
echo 'Hello World';

Resulted in this output:

HTTP/1.1 200 OK
Date: ...
Server: Apache
Content-Encoding: gzip
Vary: Accept-Encoding
Connection: Upgrade
Transfer-Encoding: chunked
Content-Type: text/html; charset=UTF-8

Hello World

Please note that the Content-Encoding header is set to gzip but the output is plain text.

This breaks the export + download functionality of phpMyAdmin. We fixed the export function by adding an additional header_remove("Content-Encoding") after cleaning and ending the output buffering. Is this new behavior intended and PMA should fix it or a bug in PHP?

PHP Version

8.0.17 + 8.1.4

Operating System

No response

@kamil-tekiela
Copy link
Member

This breaks the export + download functionality of phpMyAdmin.

Can you explain how does it break it? Would you be able to link to a commit in PMA where you added the hack? I can't see any recent changes in PMA GitHub.

@manuelm
Copy link
Contributor Author

manuelm commented Mar 18, 2022

I'm no PMA dev. We fixed it locally for our clients. Then I reported the bug report :-)

The export function is broken. Browser says it cannot understand the "download content" as its plain text but the header is set to gzip. So the browser expects gzip-encoded content.

edit:
For reference this is our PMA fix we rolled out:

--- a/libraries/classes/Controllers/ExportController.php        2022-03-18 14:35:05.436069330 +0100
+++ b/libraries/classes/Controllers/ExportController.php        2022-03-18 14:34:33.419300421 +0100
@@ -305,6 +305,7 @@
                     $hasBuffer = false;
                 }
             } while ($hasBuffer);
+            header_remove("Content-Encoding");
         }

         $tables = [];

I'll create a PMA bug report if the new behavior is intended.

@damianwadley
Copy link
Member

If it wants to send plain text then why is it enabling gzipped output buffering in the first place?

@manuelm
Copy link
Contributor Author

manuelm commented Mar 18, 2022

Didn't look at the code in-depth. I assume it's the default code path and always enables output buffering. PMA only disables the buffering right before sending the content as downloadable content.
There's also the possibility to show the sql dump inline. Inside a HTML textfield. Same code path but output buffering enabled.

Since PMA is widely used and the new behavior clearly breaks an important part of PMA I wanted to check-in here first.

@kamil-tekiela
Copy link
Member

@cmb69 Hope you don't mind, I am assigning this bug to you. I did some analysis and I agree that calling ob_end_clean without any prior flush should not add the headers. This seems like a regression after your previous bug.

@arcanisgk
Copy link

arcanisgk commented Mar 24, 2022

It seems to me that there is some kind of confusion; the target of ob_end_clean(); is to clear the output buffer; but not to clean headers (correct me if I'm wrong); ideally ob_end_clean(); respect the current header; any data output after ob_end_clean(); will carry the initially configured header unless it is changed.

https://www.php.net/manual/en/function.ob-end-clean.php

ob_end_clean — Clean (erase) the output buffer and turn off output buffering

if you previously called ob_start('ob_gzhandler'); this will start the compressed capture; that is ob_end_clean, it will automatically activate gzip in the header; therefore it lacks gzip disable;

https://www.php.net/manual/en/function.ob-gzhandler.php

ob_gzhandler — ob_start callback function to gzip output buffer

ob_gzhandler() is intended to be used as a callback function for ob_start() to help facilitate sending gz-encoded data to web browsers that support compressed web pages. Before ob_gzhandler() actually sends compressed data, it determines what type of content encoding the browser will accept ("gzip", "deflate" or none at all) and will return its output accordingly. All browsers are supported since it's up to the browser to send the correct header saying that it accepts compressed web pages. If a browser doesn't support compressed pages this function returns false.

which is what is really needed: a callback to ob_end_clean that allows resetting the entire header, something like ob_end_clean(ob_reset_header);

@kamil-tekiela
Copy link
Member

Correct, ob_end_clean() shouldn't remove the headers, but my point is that it also shouldn't add them. The headers should only be added when there is any output during output buffering, such as ob_flush. If there is no output, and we end clean then there's no need to add the headers.

@langemeijer
Copy link

If it wants to send plain text then why is it enabling gzipped output buffering in the first place?

Note that ob_start('ob_gzhandler') is not the only way to get hit with this regression, also any user with zlib.output_compression = on in php.ini will be affected.

@DanielnetoDotCom
Copy link

I'm no PMA dev. We fixed it locally for our clients. Then I reported the bug report :-)

The export function is broken. Browser says it cannot understand the "download content" as its plain text but the header is set to gzip. So the browser expects gzip-encoded content.

edit: For reference this is our PMA fix we rolled out:

--- a/libraries/classes/Controllers/ExportController.php        2022-03-18 14:35:05.436069330 +0100
+++ b/libraries/classes/Controllers/ExportController.php        2022-03-18 14:34:33.419300421 +0100
@@ -305,6 +305,7 @@
                     $hasBuffer = false;
                 }
             } while ($hasBuffer);
+            header_remove("Content-Encoding");
         }

         $tables = [];

I'll create a PMA bug report if the new behavior is intended.

That is what I did as a work around

function doesPHPVersioHasOBBug(){
    if (version_compare(phpversion(), "8.1.4", "==")) {
        return true;
      } else if (version_compare(phpversion(), "8.0.17", "==")) {
        return true;
      } else {
        return false;
      }
}

function _ob_end_clean(){
    ob_end_clean();
    if(!doesPHPVersioHasOBBug()){
        header_remove("Content-Encoding");
    }
}

@arcanisgk
Copy link

@DanielnetoDotCom Thank you so much; but I have a curiosity, we do not know if this is going to be solved in future versions; It would be convenient to always clean the header.

function _ob_end_clean(){
    ob_end_clean();
    header_remove("Content-Encoding");
}

@DanielnetoDotCom
Copy link

A

@DanielnetoDotCom Thank you so much; but I have a curiosity, we do not know if this is going to be solved in future versions; It would be convenient to always clean the header.

function _ob_end_clean(){
    ob_end_clean();
    header_remove("Content-Encoding");
}

I agree

also I just changed my code to always clean the header and I could confirm it is working in all php versions I have tested

cmb69 added a commit to cmb69/php-src that referenced this issue Apr 12, 2022
The fix for phpGH-7953 introduced a regression by being to deliberate
adding the respective headers.  These must only be added, if the
handler starts, but is not finalizing.
@cmb69 cmb69 closed this as completed in 30f4c72 Apr 25, 2022
cmb69 added a commit that referenced this issue Apr 25, 2022
* PHP-8.0:
  Fix GH-8218: ob_end_clean does not reset Content-Encoding header
cmb69 added a commit that referenced this issue Apr 25, 2022
* PHP-8.1:
  Fix GH-8218: ob_end_clean does not reset Content-Encoding header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants
@manuelm @langemeijer @arcanisgk @cmb69 @kamil-tekiela @damianwadley @DanielnetoDotCom and others