Skip to content

Surface raw body in BringApiException when error envelope is unparsable#60

Merged
crakter merged 1 commit into
masterfrom
claude/brave-bardeen-S4zZM
Jun 1, 2026
Merged

Surface raw body in BringApiException when error envelope is unparsable#60
crakter merged 1 commit into
masterfrom
claude/brave-bardeen-S4zZM

Conversation

@crakter
Copy link
Copy Markdown
Owner

@crakter crakter commented Jun 1, 2026

Summary

Bring returns several 4xx shapes beyond the documented errors/messages JSON envelope — plain text, HTML pages, JSON variants from upstream gateways. In those cases the parser silently dropped the body and callers saw only HTTP 400 (no parsable error body), with no clue what Bring was actually complaining about.

Now the synthetic fallback error carries a redacted, single-line, 500-char-capped snippet of the body so the real rejection reason reaches the operator. Credential-shaped values (api_key, apikey, x-mybring-api-key, authorization, password, secret, token) are redacted in both "key":"value" JSON-property and key=value form-value shapes before embedding.

The parseable-envelope path is unchanged: getMessage() still embeds only the first parsed error, preserving the existing guarantee (covered by testApiExceptionMessageEmbedsFirstErrorButNotRawBody) that Bring's potentially credential-echoing response bodies don't leak via getMessage().

Motivation

Triggered by a /book 400 in purchase whose user-facing error was just Bring API returned HTTP 400 (-: HTTP 400 (no parsable error body)). The product-direction bug that caused the original rejection was fixed separately in crakter/purchase#251, but the diagnostic blind spot remains — and the next non-envelope 400 from Bring would land the same way.

Test plan

  • vendor/bin/phpunit tests/v4/Http/TransportTest.php — 4 filtered, all green
  • vendor/bin/phpunit — 198 tests, all green
  • vendor/bin/phpstan analyse src/v4/Exception/BringApiException.php — no errors
  • New tests:
    • testUnparsableErrorBodyIsSurfacedInMessage — plain-text 400 body shows up in getMessage()
    • testUnparsableErrorBodyRedactsCredentialShapedValuesapi_key/Authorization values stripped before embedding

Generated by Claude Code

Bring returns several 4xx shapes beyond the documented
errors/messages JSON envelope — plain text, HTML pages, JSON
variants from upstream gateways. In those cases the parser
silently dropped the body and the caller saw only
"HTTP 400 (no parsable error body)", with no clue what Bring was
actually complaining about.

Now the fallback synthetic error carries a redacted, single-line,
500-char-capped snippet of the body so the real rejection reason
reaches the operator. Credential-shaped values (api_key, apikey,
x-mybring-api-key, authorization, password, secret, token) are
redacted in both JSON-property and form-value shapes.

The parseable-envelope path is unchanged: getMessage() still
embeds only the first parsed error, preserving the existing
guarantee that Bring's potentially credential-echoing response
bodies don't leak via getMessage().
@crakter crakter merged commit d3d0db9 into master Jun 1, 2026
17 checks passed
@crakter crakter deleted the claude/brave-bardeen-S4zZM branch June 1, 2026 20:11
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances error handling in BringApiException by extracting and redacting a snippet of the raw response body when a standard error envelope is missing, preventing valuable debugging information from being lost. The reviewer identified security gaps in the redaction regex, noting that it fails to match prefixed keys (like access_token) and colon-separated headers (like Authorization: Bearer), and provided robust code suggestions to improve both the redaction logic and its corresponding unit tests.

Comment on lines +130 to +160
private static function redactBodySnippet(string $body): string
{
$body = trim($body);
if ($body === '') {
return '';
}

// Collapse whitespace so multi-line HTML/JSON doesn't blow up logs.
$body = (string) preg_replace('/\s+/u', ' ', $body);

// Redact common credential shapes: JSON "key":"value", form key=value,
// and Authorization-style header values. Match key names case-insensitively.
$sensitive = '(?:api[_-]?key|apikey|x-mybring-api-key|authorization|password|secret|token)';
$body = (string) preg_replace(
'/"(' . $sensitive . ')"\s*:\s*"[^"]*"/i',
'"$1":"[REDACTED]"',
$body,
);
$body = (string) preg_replace(
'/\b(' . $sensitive . ')\s*=\s*[^&\s"\']+/i',
'$1=[REDACTED]',
$body,
);

$limit = 500;
if (mb_strlen($body) > $limit) {
$body = mb_substr($body, 0, $limit) . '…';
}

return $body;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The current redaction logic has a few gaps that could leak sensitive credentials into exception messages and application logs:

  1. Prefixes on sensitive keys: The use of \b before api_key or token prevents matching keys with prefixes like access_token, refresh_token, or my_api_key because _ is a word character (meaning there is no word boundary between _ and t or a).
  2. Header/YAML format: Plain text or gateway errors often return credentials in Key: Value format (e.g., Authorization: Bearer <token> or api_key: <key>). These are not matched by either the JSON regex (which expects quotes) or the form-urlencoded regex (which expects =).

We can make the redaction significantly more robust by:

  • Allowing optional alphanumeric/hyphen/underscore prefixes before the sensitive keywords.
  • Adding a third regex to match colon-separated values (like HTTP headers or YAML).
    private static function redactBodySnippet(string $body):
    {
        $body = trim($body);
        if ($body === '') {
            return '';
        }

        // Collapse whitespace so multi-line HTML/JSON doesn't blow up logs.
        $body = (string) preg_replace('/\\s+/u', ' ', $body);

        // Redact common credential shapes: JSON "key":"value", form key=value,
        // and Authorization-style header values. Match key names case-insensitively.
        $sensitive = '[a-z0-9_.-]*(?:api[_-]?key|authorization|password|secret|token)';
        $body = (string) preg_replace(
            '/"(' . $sensitive . ')"\\s*:\\s*"[^"]*"/i',
            '"$1":"[REDACTED]"',
            $body,
        );
        $body = (string) preg_replace(
            '/\\b(' . $sensitive . ')\\s*=\\s*[^&\\s"\']+/i',
            '$1=[REDACTED]',
            $body,
        );
        $body = (string) preg_replace(
            '/\\b(' . $sensitive . ')\\s*:\\s*([^\\s",]+(?:\\s+[^\\s",]+)?)/i',
            '$1: [REDACTED]',
            $body,
        );

        $limit = 500;
        if (mb_strlen($body) > $limit) {
            $body = mb_substr($body, 0, $limit) . '';
        }

        return $body;
    }

Comment on lines +124 to +141
public function testUnparsableErrorBodyRedactsCredentialShapedValues(): void
{
// If the unparsable body happens to echo credential-shaped values,
// they must be redacted before being embedded in getMessage().
$body = '{"detail":"unauthorized","api_key":"secretKey","Authorization":"Basic abc"}';
$client = new RecordingClient([new Response(401, ['Content-Type' => 'application/json'], $body)]);
$transport = new Transport($client, $this->factory, $this->factory, $this->factory, new NullAuthorization());

try {
$transport->send(new PostalCodeEndpoint('0150'));
self::fail('expected BringApiException');
} catch (BringApiException $e) {
self::assertStringContainsString('HTTP 401', $e->getMessage());
self::assertStringContainsString('unauthorized', $e->getMessage());
self::assertStringNotContainsString('secretKey', $e->getMessage());
self::assertStringNotContainsString('Basic abc', $e->getMessage());
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Update the test to verify that the more robust redaction patterns (such as plain text headers like Authorization: Bearer ... and prefixed keys like access_token) are correctly redacted.

    public function testUnparsableErrorBodyRedactsCredentialShapedValues(): void
    {
        // If the unparsable body happens to echo credential-shaped values,
        // they must be redacted before being embedded in getMessage().
        $body = 'Authorization: Bearer abc-123, access_token=xyz987, api_key: secretKey';
        $client = new RecordingClient([new Response(401, ['Content-Type' => 'text/plain'], $body)]);
        $transport = new Transport($client, $this->factory, $this->factory, $this->factory, new NullAuthorization());

        try {
            $transport->send(new PostalCodeEndpoint('0150'));
            self::fail('expected BringApiException');
        } catch (BringApiException $e) {
            self::assertStringContainsString('HTTP 401', $e->getMessage());
            self::assertStringNotContainsString('abc-123', $e->getMessage());
            self::assertStringNotContainsString('xyz987', $e->getMessage());
            self::assertStringNotContainsString('secretKey', $e->getMessage());
        }
    }

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants