diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index bef61565..1a2a47a1 100644 --- a/src/Sentry/Laravel/Features/CacheIntegration.php +++ b/src/Sentry/Laravel/Features/CacheIntegration.php @@ -4,6 +4,7 @@ use Illuminate\Cache\Events; use Illuminate\Contracts\Events\Dispatcher; +use Illuminate\Contracts\Session\Session; use Illuminate\Redis\Events as RedisEvents; use Illuminate\Redis\RedisManager; use Illuminate\Support\Str; @@ -86,11 +87,13 @@ public function handleCacheEventsForBreadcrumbs(Events\CacheEvent $event): void return; } + $displayKey = $this->replaceSessionKey($event->key); + Integration::addBreadcrumb(new Breadcrumb( Breadcrumb::LEVEL_INFO, Breadcrumb::TYPE_DEFAULT, 'cache', - "{$message}: {$event->key}", + "{$message}: {$displayKey}", $event->tags ? ['tags' => $event->tags] : [] )); } @@ -109,15 +112,17 @@ public function handleCacheEventsForTracing(Events\CacheEvent $event): void : $event->keys ); + $displayKeys = $this->replaceSessionKeys($keys); + $this->pushSpan( $parentSpan->startChild( SpanContext::make() ->setOp('cache.get') ->setData([ - 'cache.key' => $keys, + 'cache.key' => $displayKeys, ]) ->setOrigin('auto.cache') - ->setDescription(implode(', ', $keys)) + ->setDescription(implode(', ', $displayKeys)) ) ); } @@ -129,30 +134,34 @@ public function handleCacheEventsForTracing(Events\CacheEvent $event): void : $event->keys ); + $displayKeys = $this->replaceSessionKeys($keys); + $this->pushSpan( $parentSpan->startChild( SpanContext::make() ->setOp('cache.put') ->setData([ - 'cache.key' => $keys, + 'cache.key' => $displayKeys, 'cache.ttl' => $event->seconds, ]) ->setOrigin('auto.cache') - ->setDescription(implode(', ', $keys)) + ->setDescription(implode(', ', $displayKeys)) ) ); } if ($event instanceof Events\ForgettingKey) { + $displayKey = $this->replaceSessionKey($event->key); + $this->pushSpan( $parentSpan->startChild( SpanContext::make() ->setOp('cache.remove') ->setData([ - 'cache.key' => [$event->key], + 'cache.key' => [$displayKey], ]) ->setOrigin('auto.cache') - ->setDescription($event->key) + ->setDescription($displayKey) ) ); } @@ -177,7 +186,7 @@ public function handleRedisCommands(RedisEvents\CommandExecuted $event): void // If the first parameter is a string and does not contain a newline we use it as the description since it's most likely a key // This is not a perfect solution but it's the best we can do without understanding the command that was executed if (!empty($event->parameters[0]) && is_string($event->parameters[0]) && !Str::contains($event->parameters[0], "\n")) { - $keyForDescription = $event->parameters[0]; + $keyForDescription = $this->replaceSessionKey($event->parameters[0]); } $context->setDescription(rtrim(strtoupper($event->command) . ' ' . $keyForDescription)); @@ -189,7 +198,7 @@ public function handleRedisCommands(RedisEvents\CommandExecuted $event): void ]; if ($this->shouldSendDefaultPii()) { - $data['db.redis.parameters'] = $event->parameters; + $data['db.redis.parameters'] = $this->replaceSessionKeys($event->parameters); } if ($this->isTracingFeatureEnabled('redis_origin')) { @@ -213,7 +222,7 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo if ($finishedSpan !== null && count($finishedSpan->getData()['cache.key'] ?? []) === 1) { $finishedSpan->setData([ - 'cache.hit' => $event instanceof Events\CacheHit, + 'cache.hit' => $event instanceof Events\CacheHit ]); } @@ -228,7 +237,7 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo if ($finishedSpan !== null) { $finishedSpan->setData([ - 'cache.success' => $event instanceof Events\KeyWritten, + 'cache.success' => $event instanceof Events\KeyWritten ]); } @@ -245,6 +254,53 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo return false; } + /** + * Retrieve the current session key if available. + */ + private function getSessionKey(): ?string + { + try { + /** @var Session $sessionStore */ + $sessionStore = $this->container()->make('session.store'); + + // It is safe for us to get the session ID here without checking if the session is started + // because getting the session ID does not start the session. In addition we need the ID before + // the session is started because the cache will retrieve the session ID from the cache before the session + // is considered started. So if we wait for the session to be started, we will not be able to replace the + // session key in the cache operation that is being executed to retrieve the session data from the cache. + return $sessionStore->getId(); + } catch (\Exception $e) { + // We can assume the session store is not available here so there is no session key to retrieve + // We capture a generic exception to avoid breaking the application because some code paths can + // result in an exception other than the expected `Illuminate\Contracts\Container\BindingResolutionException` + return null; + } + } + + /** + * Replace a session key with a placeholder. + */ + private function replaceSessionKey(string $value): string + { + return $value === $this->getSessionKey() ? '{sessionKey}' : $value; + } + + /** + * Replace session keys in an array of keys with placeholders. + * + * @param string[] $values + * + * @return mixed[] + */ + private function replaceSessionKeys(array $values): array + { + $sessionKey = $this->getSessionKey(); + + return array_map(static function ($value) use ($sessionKey) { + return is_string($value) && $value === $sessionKey ? '{sessionKey}' : $value; + }, $values); + } + /** * Normalize the array of keys to a array of only strings. * diff --git a/test/Sentry/Features/CacheIntegrationTest.php b/test/Sentry/Features/CacheIntegrationTest.php index 52ed81f3..6aae519e 100644 --- a/test/Sentry/Features/CacheIntegrationTest.php +++ b/test/Sentry/Features/CacheIntegrationTest.php @@ -9,6 +9,10 @@ class CacheIntegrationTest extends TestCase { + protected $defaultSetupConfig = [ + 'session.driver' => 'array', + ]; + public function testCacheBreadcrumbForWriteAndHitIsRecorded(): void { Cache::put($key = 'foo', 'bar'); @@ -51,6 +55,32 @@ public function testCacheBreadcrumbIsNotRecordedWhenDisabled(): void $this->assertEmpty($this->getCurrentSentryBreadcrumbs()); } + public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void + { + // Start a session properly in the test environment + $this->startSession(); + $sessionId = $this->app['session']->getId(); + + // Use the session ID as a cache key + Cache::put($sessionId, 'session-data'); + + $breadcrumb = $this->getLastSentryBreadcrumb(); + $this->assertEquals('Written: {sessionKey}', $breadcrumb->getMessage()); + + Cache::get($sessionId); + + $breadcrumb = $this->getLastSentryBreadcrumb(); + $this->assertEquals('Read: {sessionKey}', $breadcrumb->getMessage()); + } + + public function testCacheBreadcrumbDoesNotReplaceNonSessionKeys(): void + { + Cache::put('regular-key', 'value'); + + $breadcrumb = $this->getLastSentryBreadcrumb(); + $this->assertEquals('Written: regular-key', $breadcrumb->getMessage()); + } + public function testCacheGetSpanIsRecorded(): void { $this->markSkippedIfTracingEventsNotAvailable(); @@ -147,6 +177,56 @@ public function testCacheRemoveSpanIsRecorded(): void $this->assertEquals(['foo'], $span->getData()['cache.key']); } + public function testCacheSpanReplacesSessionKeyWithPlaceholder(): void + { + $this->markSkippedIfTracingEventsNotAvailable(); + + $this->startSession(); + $sessionId = $this->app['session']->getId(); + + $span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) { + Cache::get($sessionId); + }); + + $this->assertEquals('cache.get', $span->getOp()); + $this->assertEquals('{sessionKey}', $span->getDescription()); + $this->assertEquals(['{sessionKey}'], $span->getData()['cache.key']); + } + + public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void + { + $this->markSkippedIfTracingEventsNotAvailable(); + + // Start a session properly in the test environment + $this->startSession(); + $sessionId = $this->app['session']->getId(); + + $span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) { + Cache::get([$sessionId, 'regular-key', $sessionId . '_another']); + }); + + $this->assertEquals('cache.get', $span->getOp()); + $this->assertEquals('{sessionKey}, regular-key, ' . $sessionId . '_another', $span->getDescription()); + $this->assertEquals(['{sessionKey}', 'regular-key', $sessionId . '_another'], $span->getData()['cache.key']); + } + + public function testCacheOperationDoesNotStartSessionPrematurely(): void + { + $this->markSkippedIfTracingEventsNotAvailable(); + + // Don't start a session to ensure it's not started + + $span = $this->executeAndReturnMostRecentSpan(function () { + Cache::get('some-key'); + }); + + // Check that session was not started + $this->assertFalse($this->app['session']->isStarted()); + + // And the key should not be replaced + $this->assertEquals('some-key', $span->getDescription()); + } + private function markSkippedIfTracingEventsNotAvailable(): void { if (class_exists(RetrievingKey::class)) {