Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 52 additions & 1 deletion src/v4/Exception/BringApiException.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,60 @@ public static function fromResponse(ResponseInterface $response, ?\Throwable $pr
}

if ($errors === []) {
$errors[] = new BringApiError('', sprintf('HTTP %d (no parsable error body)', $status), null);
// No recognised error envelope. Surface a snippet of the raw body
// so the actual Bring rejection reason isn't lost — without this,
// unfamiliar 4xx shapes (plain text, HTML, JSON envelopes other
// than errors/messages) collapse to "no parsable error body" and
// the operator has nothing to act on. The body is redacted and
// truncated; the parseable-envelope path is unchanged so the
// "Bring sometimes echoes credentials" guarantee on getMessage()
// still holds for the case it was written for.
$snippet = self::redactBodySnippet($body);
$errors[] = new BringApiError(
'',
$snippet === ''
? sprintf('HTTP %d (empty error body)', $status)
: sprintf('HTTP %d: %s', $status, $snippet),
null,
);
}

return new self($status, $errors, $response, $previous);
}

/**
* Trim, single-line, truncate and redact obvious credential-shaped values
* from a response body so it's safe to embed in an exception message.
*/
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;
}
Comment on lines +130 to +160
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;
    }

}
39 changes: 39 additions & 0 deletions tests/v4/Http/TransportTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,45 @@ public function testApiExceptionMessageEmbedsFirstErrorButNotRawBody(): void
}
}

public function testUnparsableErrorBodyIsSurfacedInMessage(): void
{
// Bring occasionally returns a 4xx with a body that doesn't fit the
// errors/messages envelope (plain text, HTML, a different JSON shape).
// We must still surface enough of it that the operator can act on it
// instead of collapsing to "no parsable error body".
$body = 'Recipient postal code 1642 is not serviced by BUSINESS_PARCEL';
$client = new RecordingClient([new Response(400, ['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 400', $e->getMessage());
self::assertStringContainsString('postal code 1642', $e->getMessage());
self::assertStringNotContainsString('no parsable error body', $e->getMessage());
}
}

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());
}
}
Comment on lines +124 to +141
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());
        }
    }


public function testTransportExceptionWrapsPsr18Failure(): void
{
$networkFail = new class ('boom') extends \RuntimeException implements ClientExceptionInterface {
Expand Down