Skip to content

Commit a71c6c7

Browse files
huangdijiaCopilot
huangdijia
and
Copilot
authored
fix: improve handling of binary responses in GuzzleHttpClientAspect (#1041)
* fix: improve handling of binary responses in GuzzleHttpClientAspect - Check Content-Type header to determine if response is textual before reading - Return 'Binary Response' placeholder for non-textual content - Avoid unnecessary stream operations on binary responses - Maintain backward compatibility for textual responses This prevents attempting to read or process binary content as text, which could cause memory issues or corrupted logs when dealing with file downloads, images, or other binary API responses. * fix: 修复 GuzzleHttpClientAspect 中对响应有效负载的处理逻辑 * fix: 修复 GuzzleHttpClientAspect 中对流响应的处理逻辑 * fix: 修复 GuzzleHttpClientAspect 中对流响应的处理逻辑 * fix: 改进 GuzzleHttpClientAspect 中对二进制响应的处理逻辑 * fix: 简化 GuzzleHttpClientAspect 中对流响应的处理逻辑 * fix: 修复 GuzzleHttpClientAspect 中对二进制响应的处理逻辑 * fix: 修改 GuzzleHttpClientAspect 中对空字符串响应的处理逻辑 * fix: 修复 GuzzleHttpClientAspect 中对可回溯流的处理逻辑 * Add grpc to textual Content-Type detection Updated the regex in GuzzleHttpClientAspect to include 'application/grpc' as a textual Content-Type. This ensures that gRPC responses are correctly identified as textual for further processing. * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: 修复 GuzzleHttpClientAspect 中对 Content-Type 头的处理逻辑 * fix: 处理 GuzzleHttpClientAspect 中的空响应情况 * fix: 优化 GuzzleHttpClientAspect 中的响应类型声明 * fix: 修复 GuzzleHttpClientAspect 中的响应处理逻辑，确保不再接受空响应 * fix: 修复 GuzzleHttpClientAspect 中的响应内容检查逻辑，确保正确处理空字符串响应 * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: 优化 GuzzleHttpClientAspect 中的响应内容处理，增加响应内容截断逻辑 * fix: 优化 GuzzleHttpClientAspect 中的响应内容截断逻辑，改善代码可读性 * fix: 修正 GuzzleHttpClientAspect 中的常量注释格式，改善代码一致性 * fix: 优化 GuzzleHttpClientAspect 中的响应内容处理逻辑，确保在异常情况下不影响请求流程 * fix: 更新 GuzzleHttpClientAspect 中的内容类型匹配逻辑，支持 gRPC 响应类型 * fix: 简化 GuzzleHttpClientAspect 中的响应内容类型判断逻辑 * fix: 优化 GuzzleHttpClientAspect 中的响应内容处理逻辑，支持 gRPC 和纯文本响应 --------- Co-authored-by: Deeka Wong <8337659+huangdijia@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 96adae5 commit a71c6c7

2 files changed

Lines changed: 90 additions & 29 deletions

File tree

‎src/sentry/src/Tracing/Aspect/GuzzleHttpClientAspect.php‎

Lines changed: 61 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@
1919
use Hyperf\Di\Aop\ProceedingJoinPoint;
2020
use Psr\Container\ContainerInterface;
2121
use Psr\Http\Message\RequestInterface;
22+
use Psr\Http\Message\ResponseInterface;
2223
use Sentry\State\Scope;
2324
use Sentry\Tracing\SpanContext;
2425
use Sentry\Tracing\SpanStatus;
26+
use Throwable;
2527

2628
use function FriendsOfHyperf\Sentry\trace;
2729

@@ -31,6 +33,8 @@
3133
*/
3234
class GuzzleHttpClientAspect extends AbstractAspect
3335
{
36+
private const MAX_RESPONSE_BODY_SIZE = 8192; // 8 KB
37+
3438
public array $classes = [
3539
Client::class . '::transfer',
3640
];
@@ -116,23 +120,9 @@ function (Scope $scope) use ($proceedingJoinPoint, $options, $guzzleConfig, $req
116120
]);
117121

118122
if ($this->feature->isTracingTagEnabled('http.response.body.contents')) {
119-
$isTextual = \preg_match(
120-
'/^(text\/|application\/(json|xml|x-www-form-urlencoded))/i',
121-
$response->getHeaderLine('Content-Type')
122-
) === 1;
123-
$body = $response->getBody();
124-
125-
if ($isTextual && $body->isSeekable()) {
126-
$pos = $body->tell();
127-
$span->setData([
128-
'http.response.body.contents' => \GuzzleHttp\Psr7\Utils::copyToString($body, 8192), // 8KB 上限
129-
]);
130-
$body->seek($pos);
131-
} else {
132-
$span->setData([
133-
'http.response.body.contents' => '[binary omitted]',
134-
]);
135-
}
123+
$span->setData([
124+
'http.response.body.contents' => $this->getResponsePayload($response, $options),
125+
]);
136126
}
137127

138128
$span->setHttpStatus($response->getStatusCode());
@@ -156,4 +146,58 @@ function (Scope $scope) use ($proceedingJoinPoint, $options, $guzzleConfig, $req
156146
->setOrigin('auto.http.client')
157147
);
158148
}
149+
150+
protected function getResponsePayload(ResponseInterface $response, array $options = []): mixed
151+
{
152+
if (isset($options['stream']) && $options['stream'] === true) {
153+
return '[Streamed Response]';
154+
}
155+
156+
// Determine if the response is textual based on the Content-Type header.
157+
$contentType = $response->getHeaderLine('Content-Type');
158+
$isTextual = $contentType === '' || \preg_match(
159+
'/^(text\/|application\/(json|xml|x-www-form-urlencoded|grpc))/i',
160+
$contentType
161+
) === 1;
162+
163+
// If the response is not textual or the stream is not seekable, we will return a placeholder.
164+
if (! $isTextual) {
165+
return '[Binary Omitted]';
166+
}
167+
168+
$stream = $response->getBody();
169+
$pos = null;
170+
171+
try {
172+
if ($stream->isSeekable()) {
173+
$pos = $stream->tell();
174+
$stream->rewind();
175+
}
176+
177+
$content = \GuzzleHttp\Psr7\Utils::copyToString(
178+
$stream,
179+
self::MAX_RESPONSE_BODY_SIZE + 1 // 多读 1 byte 用来判断是否截断
180+
);
181+
182+
if (strlen($content) > self::MAX_RESPONSE_BODY_SIZE) {
183+
return substr(
184+
$content,
185+
0,
186+
self::MAX_RESPONSE_BODY_SIZE
187+
) . '… [truncated]';
188+
}
189+
190+
return $content === '' ? '[Empty-String Response]' : $content;
191+
} catch (Throwable $e) {
192+
return '[Error Retrieving Response Content]';
193+
} finally {
194+
if ($pos !== null) {
195+
try {
196+
$stream->seek($pos);
197+
} catch (Throwable) {
198+
// ignore: tracing must not break the request flow
199+
}
200+
}
201+
}
202+
}
159203
}

‎src/telescope/src/Aspect/GuzzleHttpClientAspect.php‎

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public function process(ProceedingJoinPoint $proceedingJoinPoint)
6060

6161
// Add or override the on_stats option to record the request duration.
6262
$onStats = $options['on_stats'] ?? null;
63-
$proceedingJoinPoint->arguments['keys']['options']['on_stats'] = function (TransferStats $stats) use ($onStats) {
63+
$proceedingJoinPoint->arguments['keys']['options']['on_stats'] = function (TransferStats $stats) use ($onStats, $options) {
6464
try {
6565
$request = $stats->getRequest();
6666
$response = $stats->getResponse();
@@ -75,7 +75,7 @@ public function process(ProceedingJoinPoint $proceedingJoinPoint)
7575
$content['response_status'] = $response->getStatusCode();
7676
$content['response_headers'] = $response->getHeaders();
7777
$content['response_reason'] = $response->getReasonPhrase();
78-
$content['response_payload'] = $this->getResponsePayload($response);
78+
$content['response_payload'] = $this->getResponsePayload($response, $options);
7979
}
8080

8181
Telescope::recordClientRequest(IncomingEntry::make($content));
@@ -91,9 +91,26 @@ public function process(ProceedingJoinPoint $proceedingJoinPoint)
9191
return $proceedingJoinPoint->process();
9292
}
9393

94-
public function getResponsePayload(ResponseInterface $response)
94+
public function getResponsePayload(ResponseInterface $response, array $options = []): mixed
9595
{
96+
if (isset($options['stream']) && $options['stream'] === true) {
97+
return 'Streamed Response';
98+
}
99+
100+
// Determine if the response is textual based on the Content-Type header.
101+
$contentType = $response->getHeaderLine('Content-Type');
102+
$isTextual = $contentType === '' || \preg_match(
103+
'/^(text\/|application\/(json|xml|x-www-form-urlencoded|grpc))/i',
104+
$contentType
105+
) === 1;
106+
107+
// If the response is not textual, we will return a placeholder.
108+
if (! $isTextual) {
109+
return 'Binary Response';
110+
}
111+
96112
$stream = $response->getBody();
113+
97114
try {
98115
if ($stream->isSeekable()) {
99116
$stream->rewind();
@@ -102,23 +119,25 @@ public function getResponsePayload(ResponseInterface $response)
102119
$content = $stream->getContents();
103120

104121
if (is_string($content)) {
122+
// Check if the content is within the size limits.
105123
if (! $this->contentWithinLimits($content)) {
106124
return 'Purged By Hyperf Telescope';
107125
}
126+
// Try to decode JSON responses and hide sensitive parameters.
108127
if (
109128
is_array(json_decode($content, true))
110129
&& json_last_error() === JSON_ERROR_NONE
111130
) {
112-
return $this->contentWithinLimits($content) /* @phpstan-ignore-line */
113-
? $this->hideParameters(json_decode($content, true), Telescope::$hiddenResponseParameters)
114-
: 'Purged By Hyperf Telescope';
115-
}
116-
if (Str::startsWith(strtolower($response->getHeaderLine('content-type') ?: ''), 'text/plain')) {
117-
return $this->contentWithinLimits($content) ? $content : 'Purged By Hyperf Telescope'; /* @phpstan-ignore-line */
131+
return $this->hideParameters(json_decode($content, true), Telescope::$hiddenResponseParameters);
118132
}
133+
// Return gRPC responses and plain text responses as is.
119134
if (Str::contains($response->getHeaderLine('content-type'), 'application/grpc') !== false) {
120135
return TelescopeContext::getGrpcResponsePayload() ?: 'Purged By Hyperf Telescope';
121136
}
137+
// Return plain text responses as is.
138+
if (Str::startsWith(strtolower($response->getHeaderLine('content-type') ?: ''), 'text/plain')) {
139+
return $content;
140+
}
122141
}
123142

124143
if (empty($content)) {
@@ -127,9 +146,7 @@ public function getResponsePayload(ResponseInterface $response)
127146
} catch (Throwable $e) {
128147
return 'Purged By Hyperf Telescope: ' . $e->getMessage();
129148
} finally {
130-
if ($stream->isSeekable()) {
131-
$stream->rewind();
132-
}
149+
$stream->isSeekable() && $stream->rewind();
133150
}
134151

135152
return 'HTML Response';

0 commit comments

Comments
 (0)