Skip to content

Commit

Permalink
Improve header parsing
Browse files Browse the repository at this point in the history
  • Loading branch information
cleptric committed Oct 10, 2023
1 parent 859b789 commit e1c176b
Show file tree
Hide file tree
Showing 6 changed files with 177 additions and 77 deletions.
66 changes: 3 additions & 63 deletions src/HttpClient/HttpClient.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@

namespace Sentry\HttpClient;

use Sentry\Client;
use Sentry\Dsn;
use Sentry\Options;
use Sentry\Util\Http;

/**
* @internal
Expand Down Expand Up @@ -38,7 +37,7 @@ public function sendRequest(string $requestData, Options $options): Response

$curlHandle = curl_init();
curl_setopt($curlHandle, \CURLOPT_URL, $dsn->getEnvelopeApiEndpointUrl());
curl_setopt($curlHandle, \CURLOPT_HTTPHEADER, $this->getRequestHeaders($dsn));
curl_setopt($curlHandle, \CURLOPT_HTTPHEADER, Http::getRequestHeaders($dsn, $this->sdkIdentifier, $this->sdkVersion));
curl_setopt($curlHandle, \CURLOPT_USERAGENT, $this->sdkIdentifier . '/' . $this->sdkVersion);
curl_setopt($curlHandle, \CURLOPT_TIMEOUT, $options->getHttpTimeout());
curl_setopt($curlHandle, \CURLOPT_CONNECTTIMEOUT, $options->getHttpConnectTimeout());
Expand All @@ -47,11 +46,6 @@ public function sendRequest(string $requestData, Options $options): Response
curl_setopt($curlHandle, \CURLOPT_POSTFIELDS, $requestData);
curl_setopt($curlHandle, \CURLOPT_RETURNTRANSFER, true);
curl_setopt($curlHandle, \CURLOPT_HEADER, true);
/**
* If we add support for CURL_HTTP_VERSION_2_0, we need
* case-insensitive header handling, as HTTP 2.0 headers
* are all lowercase.
*/
curl_setopt($curlHandle, \CURLOPT_HTTP_VERSION, \CURL_HTTP_VERSION_1_1);

Check warning on line 49 in src/HttpClient/HttpClient.php

View check run for this annotation

Codecov / codecov/patch

src/HttpClient/HttpClient.php#L38-L49

Added lines #L38 - L49 were not covered by tests

$httpSslVerifyPeer = $options->getHttpSslVerifyPeer();
Expand Down Expand Up @@ -86,64 +80,10 @@ public function sendRequest(string $requestData, Options $options): Response

$statusCode = curl_getinfo($curlHandle, \CURLINFO_HTTP_CODE);
$headerSize = curl_getinfo($curlHandle, \CURLINFO_HEADER_SIZE);
$headers = $this->getResponseHeaders($headerSize, (string) $body);
$headers = Http::getResponseHeaders($headerSize, (string) $body);

Check warning on line 83 in src/HttpClient/HttpClient.php

View check run for this annotation

Codecov / codecov/patch

src/HttpClient/HttpClient.php#L81-L83

Added lines #L81 - L83 were not covered by tests

curl_close($curlHandle);

Check warning on line 85 in src/HttpClient/HttpClient.php

View check run for this annotation

Codecov / codecov/patch

src/HttpClient/HttpClient.php#L85

Added line #L85 was not covered by tests

return new Response($statusCode, $headers, '');

Check warning on line 87 in src/HttpClient/HttpClient.php

View check run for this annotation

Codecov / codecov/patch

src/HttpClient/HttpClient.php#L87

Added line #L87 was not covered by tests
}

/**
* @return string[]
*/
protected function getRequestHeaders(Dsn $dsn): array
{
$headers = [
'Content-Type' => 'application/x-sentry-envelope',
];

$data = [
'sentry_version' => Client::PROTOCOL_VERSION,
'sentry_client' => $this->sdkIdentifier . '/' . $this->sdkVersion,
'sentry_key' => $dsn->getPublicKey(),
];

if (null !== $dsn->getSecretKey()) {
$data['sentry_secret'] = $dsn->getSecretKey();
}

$authHeader = [];
foreach ($data as $headerKey => $headerValue) {
$authHeader[] = $headerKey . '=' . $headerValue;
}

return array_merge($headers, [
'X-Sentry-Auth' => 'Sentry ' . implode(', ', $authHeader),
]);
}

/**
* @TODO(michi) This might need a bit more love,
* but we only really care about X-Sentry-Rate-Limits and Retry-After
*
* @return string[]
*/
protected function getResponseHeaders(?int $headerSize, string $body): array
{
$headers = [];
$rawHeaders = explode("\r\n", trim(substr($body, 0, $headerSize)));

foreach ($rawHeaders as $value) {
if (!str_contains($value, ':')) {
continue;
}
[$name, $value] = explode(':', $value, 2);
$value = trim($value);
$name = trim($name);

$headers[$name] = $value;
}

return $headers;
}
}
29 changes: 23 additions & 6 deletions src/HttpClient/Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ final class Response
private $statusCode;

/**
* @var string[] The HTTP response headers
* @var string[][]
*/
private $headers;

Expand All @@ -25,7 +25,7 @@ final class Response
private $error;

/**
* @param string[] $headers
* @param string[][] $headers
*/
public function __construct(int $statusCode, array $headers, string $error)
{
Expand All @@ -44,14 +44,31 @@ public function isSuccess(): bool
return $this->statusCode >= 200 && $this->statusCode <= 299;
}

public function hasHeader(string $headerName): bool
public function hasHeader(string $name): bool
{
return \array_key_exists($headerName, $this->headers);
return \array_key_exists($name, $this->headers);
}

public function getHeaderLine(string $headerName): string
/**
* @return string[]
*/
public function getHeader(string $header): array
{
return $this->headers[$headerName] ?? '';
if (!$this->hasHeader($header)) {
return [];

Check warning on line 58 in src/HttpClient/Response.php

View check run for this annotation

Codecov / codecov/patch

src/HttpClient/Response.php#L58

Added line #L58 was not covered by tests
}

return $this->headers[$header];
}

public function getHeaderLine(string $name): string
{
$value = $this->getHeader($name);
if (empty($value)) {
return '';

Check warning on line 68 in src/HttpClient/Response.php

View check run for this annotation

Codecov / codecov/patch

src/HttpClient/Response.php#L68

Added line #L68 was not covered by tests
}

return implode(',', $value);
}

public function getError(): string
Expand Down
69 changes: 69 additions & 0 deletions src/Util/Http.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
<?php

declare(strict_types=1);

namespace Sentry\Util;

use Sentry\Client;
use Sentry\Dsn;

/**
* @internal
*/
final class Http
{
/**
* @return string[]
*/
public static function getRequestHeaders(Dsn $dsn, string $sdkIdentifier, string $sdkVersion): array
{
$headers = [
'Content-Type' => 'application/x-sentry-envelope',
];

$data = [
'sentry_version' => Client::PROTOCOL_VERSION,
'sentry_client' => $sdkIdentifier . '/' . $sdkVersion,
'sentry_key' => $dsn->getPublicKey(),
];

if (null !== $dsn->getSecretKey()) {
$data['sentry_secret'] = $dsn->getSecretKey();
}

$authHeader = [];
foreach ($data as $headerKey => $headerValue) {
$authHeader[] = $headerKey . '=' . $headerValue;
}

return array_merge($headers, [
'X-Sentry-Auth' => 'Sentry ' . implode(', ', $authHeader),
]);
}

/**
* @return string[][]
*/
public static function getResponseHeaders(?int $headerSize, string $body): array
{
$headers = [];
$rawHeaders = explode("\r\n", trim(substr($body, 0, $headerSize)));

foreach ($rawHeaders as $value) {
if (!str_contains($value, ':')) {
continue;
}
[$name, $value] = explode(':', $value, 2);
$value = trim($value);
$name = trim($name);

if (isset($headers[$name])) {
$headers[$name][] = $value;

Check warning on line 61 in src/Util/Http.php

View check run for this annotation

Codecov / codecov/patch

src/Util/Http.php#L61

Added line #L61 was not covered by tests
} else {
$headers[$name] = (array) $value;
}
}

return $headers;
}
}
2 changes: 1 addition & 1 deletion tests/Transport/HttpTransportTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ public function testSendFailsDueToExceedingRateLimits(): void

$this->httpClient->expects($this->once())
->method('sendRequest')
->willReturn(new Response(429, ['Retry-After' => '60'], ''));
->willReturn(new Response(429, ['Retry-After' => ['60']], ''));

$transport = new HttpTransport(
new Options(),
Expand Down
14 changes: 7 additions & 7 deletions tests/Transport/RateLimiterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,31 +60,31 @@ public static function handleResponseDataProvider(): \Generator

yield 'Back-off using X-Sentry-Rate-Limits header with single category' => [
Event::createEvent(),
new Response(429, ['X-Sentry-Rate-Limits' => '60:error:org'], ''),
new Response(429, ['X-Sentry-Rate-Limits' => ['60:error:org']], ''),
429,
];

yield 'Back-off using X-Sentry-Rate-Limits header with multiple categories' => [
Event::createEvent(),
new Response(429, ['X-Sentry-Rate-Limits' => '60:error;transaction:org'], ''),
new Response(429, ['X-Sentry-Rate-Limits' => ['60:error;transaction:org']], ''),
429,
];

yield 'Back-off using X-Sentry-Rate-Limits header with missing categories should lock them all' => [
Event::createEvent(),
new Response(429, ['X-Sentry-Rate-Limits' => '60::org'], ''),
new Response(429, ['X-Sentry-Rate-Limits' => ['60::org']], ''),
429,
];

yield 'Back-off using Retry-After header with number-based value' => [
Event::createEvent(),
new Response(429, ['Retry-After' => '60'], ''),
new Response(429, ['Retry-After' => ['60']], ''),
429,
];

yield 'Back-off using Retry-After header with date-based value' => [
Event::createEvent(),
new Response(429, ['Retry-After' => 'Sun, 02 February 2022 00:01:00 GMT'], ''),
new Response(429, ['Retry-After' => ['Sun, 02 February 2022 00:01:00 GMT']], ''),
429,
];
}
Expand All @@ -99,7 +99,7 @@ public function testIsRateLimited(): void

// Events should be rate-limited for 60 seconds, but transactions should
// still be allowed to be sent
$this->rateLimiter->handleResponse(Event::createEvent(), new Response(429, ['X-Sentry-Rate-Limits' => '60:error:org'], ''));
$this->rateLimiter->handleResponse(Event::createEvent(), new Response(429, ['X-Sentry-Rate-Limits' => ['60:error:org']], ''));

$this->assertTrue($this->rateLimiter->isRateLimited(EventType::event()));
$this->assertFalse($this->rateLimiter->isRateLimited(EventType::transaction()));
Expand All @@ -112,7 +112,7 @@ public function testIsRateLimited(): void

// Both events and transactions should be rate-limited if all categories
// are
$this->rateLimiter->handleResponse(Event::createTransaction(), new Response(429, ['X-Sentry-Rate-Limits' => '60:all:org'], ''));
$this->rateLimiter->handleResponse(Event::createTransaction(), new Response(429, ['X-Sentry-Rate-Limits' => ['60:all:org']], ''));

$this->assertTrue($this->rateLimiter->isRateLimited(EventType::event()));
$this->assertTrue($this->rateLimiter->isRateLimited(EventType::transaction()));
Expand Down
74 changes: 74 additions & 0 deletions tests/Util/HttpTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
<?php

declare(strict_types=1);

namespace Sentry\Tests\Util;

use PHPUnit\Framework\TestCase;
use Sentry\Dsn;
use Sentry\Util\Http;

final class HttpTest extends TestCase
{
/**
* @dataProvider getRequestHeadersDataProvider
*/
public function testGetRequestHeaders(Dsn $dsn, string $sdkIdentifier, string $sdkVersion, array $expectedResult): void
{
$this->assertSame($expectedResult, Http::getRequestHeaders($dsn, $sdkIdentifier, $sdkVersion));
}

public static function getRequestHeadersDataProvider(): \Generator
{
yield [
Dsn::createFromString('http://[email protected]/1'),
'sentry.sdk.identifier',
'1.2.3',
[
'Content-Type' => 'application/x-sentry-envelope',
'X-Sentry-Auth' => 'Sentry sentry_version=7, sentry_client=sentry.sdk.identifier/1.2.3, sentry_key=public',
],
];

yield [
Dsn::createFromString('http://public:[email protected]/1'),
'sentry.sdk.identifier',
'1.2.3',
[
'Content-Type' => 'application/x-sentry-envelope',
'X-Sentry-Auth' => 'Sentry sentry_version=7, sentry_client=sentry.sdk.identifier/1.2.3, sentry_key=public, sentry_secret=secret',
],
];
}

/**
* @dataProvider getResponseHeadersDataProvider
*/
public function testGetResponseHeaders(?int $headerSize, string $body, array $expectedResult): void
{
$this->assertSame($expectedResult, Http::getResponseHeaders($headerSize, $body));
}

public static function getResponseHeadersDataProvider(): \Generator
{
yield [
128,
<<<TEXT
HTTP/1.1 200 OK\r\n
Server: nginx\r\n
Date: Tue, 10 Oct 2023 10:00:00 GMT\r\n
Content-Type: application/json\r\n
Content-Length: 41\r\n
\r\n
{"id":"2beb84919c3b4c92855206dd7f911a56"}
TEXT
,
[
'Server' => ['nginx'],
'Date' => ['Tue, 10 Oct 2023 10:00:00 GMT'],
'Content-Type' => ['application/json'],
'Content-Length' => ['41'],
],
];
}
}

0 comments on commit e1c176b

Please sign in to comment.