Binary string detection is broken (introduced with 1.9.0)

[driehle]

PHP version: all versions

Description With #126 and #128 a change was introduced that is supposed to prevent binary messages from being dumped. In FullHttpMessageFormatter line 89 the following code is used to detect binary strings:

        if (preg_match('/([\x00-\x1F\x7F])/', $data)) {
            return $message.'[binary stream omitted]';
        }

Unfortunatly, this regex includes common whitespaces like \t, \r and \n as well. In consequence, all output is hidden and replaced with [binary stream omitted], even if the document is a simple HTML or XML document. I doubt that this is the intentioned behaviour, since linebreaks a by far an indicator for a binary string and moreover very common in HTML or XML documents.

Possible Solution The code could be changed as follows, see https://stackoverflow.com/questions/25343508/detect-if-string-is-binary:

        if (preg_match('~[^\x20-\x7E\t\r\n]~', $data)) {
            return $message.'[binary stream omitted]';
        }

However, this would still be problematic regarding non-English languages, i.e. special characters in French, Spanish, Italian or German would still trigger falsely the binary detection. Therefore, I would suggest to check for all non-prinable ASCII characters as well as \ (\x7F) and explicitly exclude \ (\x0D), \ (\x0A) and \ (\x0B).

        // all non-printable ASCII characters and <DEL> except for \t, \r, \n
        if (preg_match('/([\x00-\x09\x0C\x0E-\x1F\x7F])/', $data)) {
            return $message.'[binary stream omitted]';
        }

The same change should probably be applied to CurlCommandFormatter as well.

What do you think?

[dbu]

oh snap. thanks for the analysis! i would be happy with a PR to change the regex. even in case it is not perfect, it will be an improvement, please do a pull request if you have the time to do it.

[driehle]

@dbu I submitted a PR. Not sure what's wrong with PHPStan, but unit tests are fine.