guzzle: Exception Message truncation isn't so friendly

Hit the same issue…

FOUND perfect and simple WORKAROUND!

We could use: ‘http_errors’ => false,

$guzzleClient->post( $url, [ 'headers' => $headers, 'http_errors' => false, 'json' => $json, ] );

It’s looks like it works with all request GET, POST, PUT, DELETE!

Now you can simply use “$response->getStatusCode()” and “$response->getBody()->getContents()”

Unfortunately exceptions end up in all sorts of places and there is no guarantee that a logger will truncate the output.

This topic has been debated several times, please read the linked issues and PRs.

Basically, HTTP message body being part of the exception message is bad to begin with. HTTP exceptions shouldn’t even exist. Users should handle errors, but people are lazy.

Also, there is no guarantee that an HTTP body returns a human readable message. It’s 2019: we have protobuf and thrift.

There are tons of other reasons, why dumping the HTTP message body into the exception message is a terrible idea, but this is what the community wants, so it’s there. That said, it’s a potential security issue, so truncation was introduced.

2021 and I’m still thinking. WHY on earth there is not a simple parameter to disable message truncation. I really like Guzzle. It solves quite a lot of problems. But at least for development purposes I just want to say: “Leave that decision for the developers”…

Exactly! This is really annoying.

Hardware and storage do not cost nothing today, but I can undestand that need for somebody. So let’s do that by configuration for easier using by other people. For example env variable loading would be great.

We have to do this 🤦‍♂️

<?php declare(strict_types=1);

namespace Trisbee\Shoptet\Trisbee;

use GuzzleHttp\ClientInterface;
use GuzzleHttp\Exception\RequestException;
use Trisbee\Shoptet\Trisbee\Exception\ApiException;

class HttpClient
{


    /**
     * @var ClientInterface
     */
    private $client;

    public function __construct(ClientInterface $client)
    {
        $this->client = $client;
    }

    public function post($uri, $options = [])
    {
        return $this->sendRequest(function ($uri, $options) {
            return $this->client->post($uri, $options);
        }, $uri, $options);
    }

    public function get($uri, $options = [])
    {
        return $this->sendRequest(function ($uri, $options) {
            return $this->client->get($uri, $options);
        }, $uri, $options);
    }

    public function patch($uri, $options = [])
    {
        return $this->sendRequest(function ($uri, $options) {
            return $this->client->patch($uri, $options);
        }, $uri, $options);
    }

    public function put($uri, $options = [])
    {
        return $this->sendRequest(function ($uri, $options) {
            return $this->client->put($uri, $options);
        }, $uri, $options);
    }

    public function delete($uri, $options = [])
    {
        return $this->sendRequest(function ($uri, $options) {
            return $this->client->delete($uri, $options);
        }, $uri, $options);
    }

    private function sendRequest(callable $callFunction, $uri, $options)
    {
        try {
            return $callFunction($uri, $options);
        } catch (RequestException $e) {
            if ($e->getResponse() !== null) {
                throw new ApiException('Api error: ' . $e->getResponse()->getBody()->getContents(), $e->getCode(), $e);
            }

            throw $e;
        }
    }
}

Just wanted to chime in here since this issue is still open. Ran into this problem myself recently when dealing with the MailChimp API, which returns errors like this:

{"type":"http://developer.mailchimp.com/documentation/mailchimp/guides/error-glossary/","title":"Invalid Resource","status":400,"detail":"The \"name\" parameter must be unique but a segment named \"XXXXXXX\" already exists.","instance":"0fac1777-b697-4f65-89e9-xxxxxxxxxx"}

With this truncating, I only get this:

Client error: 'PATCH https://us18.api.mailchimp.com/3.0/lists/xxxxxxxxx/segments/xxxxx' resulted in a '400 Bad Request' response{"type":"http://developer.mailchimp.com/documentation/mailchimp/guides/error-glossary/","title":"Invalid Resourc ... (truncated)

I’m using Laravel and Bugsnag, so error handling and reporting is pretty much out of the box. I tried catching the Guzzle RequestException in the handler, but then when I throw a new exception, I lose the trace.

So my only option at this point is to wrap all the Guzzle requests throughout my app, or at least in the places I anticipate needing a longer message body. It seems like a lot of work, when a simple increase in the truncated limit from 120 to 240 would be sufficient.

Disabling truncation is something that was not implemented.

It is. There is even a BodySummarizerInterface which allows you to truncate at any length, no truncate or remove every vowel if you so desire.

Also, documentation lacks sufficient information about this issue.

I know, that is why I suggested you to update the documentation.

Hey @pculka,

Your message is neither inspiring nor productive. It is also clear to me that you have not read this thread before posting. The issue was fixed last year.

If you want to show appreciation for the thousands of hours spent on maintaining guzzle over the past decade, you may help by sending a PR to improve the documentation around this issue.

Why Obviously I don’t want to log that 10k JSON?

I know it’s already 2018, but people still make the mistake of storing logs on servers with limited disks without rotation. If I have to modify that statement, I would say logging the body of HTTP responses in an uncontrolled manner poses a very real and horrifying risk. From a maintainer point of view I have to be on the defensive side of things on this one.

A configuration option could be a solution, but the thing is it does not protect you from the above thing if you just globally turn that option into infinity.

That being said, we might consider adding an option.

Till then, here are a few (better) options:

1. I strongly believe that the proper way to put the body into the exception is catching it and manually returning another exception. That’s just simply superior to anything, because you consciously decide how to handle errors.
2. Guzzle exposes an interface that it implements. Unfortunately it’s not used widely, but implementing that custom interface and doing whatever you want with the exception and then rethrowing it can be a solution.
3. Directly depending on Guzzle is an architecture mistake anyway, so you should implement a wrapper interface of your own (or use something like HTTPlug/PSR-18). Again, inside that interface, you do whatever you want.
4. You can create a middleware, catch the interface and then rehtrow it in a single point.

Ultimately I believe this issue only occurs because people don’t use Guzzle in a way they should. The above solutions should all provide a solution. As mentioned before, security MUST be a first priority in this case. That said, I will leave this issue open and consider adding an option, but this isn’t a high priority issue for now. I hope it helps.

@sagikazarmark The PR looks OK form certain perspective, but I’m not at that point sadly. Regarding the various logging solutions you are right, but its their space of responsibility, so they should do truncation. Why would Guzzle put itself in the responsibility of logging? It’s neither what is was build for nor specializes in.

For me, 1) is not a solution. It’s essentially the problem. I already have a exception handler library attached to a logging library that will log any exception for me. The library won’t treat guzzle exceptions differently and I don’t want to fork the library and do that. I also don’t want to catch guzzle exceptions because I believe I should catch it if I want to handle it. I should not be forced to catch an exception just to see the message.

1. is not a solution for the same reason as 1. Every exception class has a method called getMessage and an http exception message is in the body. The interface could have been a solution if Guzzle offered an inversion of control container. I would be able to implement my custom exception class and then tell Guzzle to use my class instead of the default one.

3 is unnecessary overhead. Yes, I would be able to have the try catch and rethrow in only one place by doing 3 and I might try to do that. Unfortunately I see it as a tremendous overhead because it brings 0 benefit for my needs if Guzzle could just let me see the exception message in my logging system.

I haven’t pursued 4) because from the linked issue (#1722) I understood you were against doing exactly that. I will try to learn more about Guzzle Middleware to try and accomplish that.

For your last statement, if you think people should always catch Guzzle exception if they want to see the message, then I have to agree with you that people don’t use Guzzle correctly. Honestly, I don’t want to use Guzzle like that. I understand and respect the decision for security first, but dictating that getting the response body is unsafe in any situation is just incorrect. I know a lot of people can misuse the ability to configure the size and end up with a flaw, but that’s an explicit developer decision and Guzzle should not be concerned about that. I would even argue that you could still limit the configuration saying nobody can instruct Guzzle to decode more than 512KB worth of data. But 120 characters isn’t enough for most MySQL error messages, which are the most basic error messages there is.

It looks like this is how to avoid truncation in Guzzle 7.2:

$stack = new HandlerStack(Utils::chooseHandler());
$stack->push(Middleware::httpErrors(new BodySummarizer(PHP_INT_MAX)), 'http_errors');
$stack->push(Middleware::redirect(), 'allow_redirects');
$stack->push(Middleware::cookies(), 'cookies');
$stack->push(Middleware::prepareBody(), 'prepare_body');
$client = new Client(['handler' => $stack /* other options here */ ]);

The above code is based on HandlerStack::create. Note, that if the list of default middlewares changes then this code needs to be updated. I would say that we should re-open this issue as it is still not fully solved.

If anyone knows how to get rid of the truncation in a simpler way (not counting $exception->getResponse()->getBody()), I would also appreciate, if you share this information (there is nothing in the documentation, btw).

Main problem is that even though BodySummarizer has the option to set $truncateAt to null, the \GuzzleHttp\Psr7\Message::bodySummary still uses 120 as default. Not sure, if that’s intentional or not. Maybe @GrahamCampbell can comment on this? It is not obvious at least. I would expect to not have any truncation when $trancateAt is null in the BodySummarizer, but that’s not possible without changing the truncation code in \GuzzleHttp\Psr7\Message::bodySummary.

Yeah, doesn’t matter how bad it is the exception message now, your best bet is to use another http client.

We use Guzzle for functional tests and a truncated message is as useful as a simple “Client reported 400 Bad request”. Wrapping the HTTP client again just to get a clear error message in tests is just one more part that can break.

I’m a bit surprised because I fell for this again. This is such an unintuitive behavior, either be generic with the exception message and wrap it up or give me everything in the message.

Is there any reason why the PHP setting log_errors_max_len is not enough?

Hi guys,

So is anyone satisfied by that “middle-ware” options (#2795) ? Because that’s not what I would call a solution, definitely not.

To me it sounds way too complicated. This is a far more complex workaround than catching the RequestException to fetch the response body contents as mentioned in the first place.

Whereas a simple error_message_maxlenght configuration option would be simpler to use (and, for what I’ve seen, to implement in guzzle too) developpers willing to have full error messages must reinvent the wheel by rewriting the whole middle-ware stack, in other words creating a custom guzzle implementation, that’s just a non-sense.

Checking @mtdowling comment here… I am trying to think what would have happened if we allowed to log all the exception message and at the same time an issue exists like this (check the related PR here for even more info).

Although the above linked issue seems as a bug… and therefore it must be fixed… the point is that allowing to log all the response message makes it unsafe.

Friendly speaking… this is not under any roadmap… and I don’t see it coming any time soon 😃

Would an env var be an option? Something like GUZZLE_ anything, as I suppose no other script in the world would use this prefix, it’s as easy to implement (check, not present = default) and would give great flexibility to everyone?

If one sets this var and has memory or any type of issue that’s one’s problem, these are a few lines of code and would help in a way that words can’t suffice to explain