From 95ddd2d3ca9506c955a018999abf228aa5628da2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 17 Jun 2025 09:01:55 +0000 Subject: [PATCH 1/7] Mask session keys in cache events to protect sensitive information --- .../Laravel/Features/CacheIntegration.php | 98 +++++++++++++++++-- test/Sentry/Features/CacheIntegrationTest.php | 81 +++++++++++++++ 2 files changed, 170 insertions(+), 9 deletions(-) diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index bef61565..6ec04de6 100644 --- a/src/Sentry/Laravel/Features/CacheIntegration.php +++ b/src/Sentry/Laravel/Features/CacheIntegration.php @@ -86,11 +86,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 +111,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 +133,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 +185,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 +197,12 @@ public function handleRedisCommands(RedisEvents\CommandExecuted $event): void ]; if ($this->shouldSendDefaultPii()) { - $data['db.redis.parameters'] = $event->parameters; + // Replace session keys in parameters if present + $parameters = $event->parameters; + if (!empty($parameters[0]) && is_string($parameters[0])) { + $parameters[0] = $this->replaceSessionKey($parameters[0]); + } + $data['db.redis.parameters'] = $parameters; } if ($this->isTracingFeatureEnabled('redis_origin')) { @@ -245,6 +258,73 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo return false; } + /** + * Check if a cache key is the current session key. + * + * @param string $key + * + * @return bool + */ + private function isSessionKey(string $key): bool + { + // Check if the container has a bound request and session + if (!$this->container()->bound('request')) { + return false; + } + + try { + $request = $this->container()->make('request'); + + // Check if the request has a session + if (!$request->hasSession()) { + return false; + } + + $session = $request->session(); + + // Don't start the session if it hasn't been started yet + if (!$session->isStarted()) { + return false; + } + + // Get the session ID and check if the cache key matches + $sessionId = $session->getId(); + + // Check if the key equals the session ID or contains it + // This handles cases where the cache key might be prefixed + return $key === $sessionId || Str::endsWith($key, $sessionId); + } catch (\Exception $e) { + // If anything goes wrong, we assume it's not a session key + return false; + } + } + + /** + * Replace a session key with a placeholder. + * + * @param string $key + * + * @return string + */ + private function replaceSessionKey(string $key): string + { + return $this->isSessionKey($key) ? '{sessionKey}' : $key; + } + + /** + * Replace session keys in an array of keys with placeholders. + * + * @param array $keys + * + * @return array + */ + private function replaceSessionKeys(array $keys): array + { + return array_map(function ($key) { + return $this->replaceSessionKey($key); + }, $keys); + } + /** * 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..707ebe26 100644 --- a/test/Sentry/Features/CacheIntegrationTest.php +++ b/test/Sentry/Features/CacheIntegrationTest.php @@ -51,6 +51,33 @@ public function testCacheBreadcrumbIsNotRecordedWhenDisabled(): void $this->assertEmpty($this->getCurrentSentryBreadcrumbs()); } + public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void + { + // Start a session + $this->app['request']->setLaravelSession($session = $this->app['session.store']); + $session->start(); + $sessionId = $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 +174,60 @@ public function testCacheRemoveSpanIsRecorded(): void $this->assertEquals(['foo'], $span->getData()['cache.key']); } + public function testCacheSpanReplacesSessionKeyWithPlaceholder(): void + { + $this->markSkippedIfTracingEventsNotAvailable(); + + // Start a session + $this->app['request']->setLaravelSession($session = $this->app['session.store']); + $session->start(); + $sessionId = $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 + $this->app['request']->setLaravelSession($session = $this->app['session.store']); + $session->start(); + $sessionId = $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, {sessionKey}', $span->getDescription()); + $this->assertEquals(['{sessionKey}', 'regular-key', '{sessionKey}'], $span->getData()['cache.key']); + } + + public function testCacheOperationDoesNotStartSessionPrematurely(): void + { + $this->markSkippedIfTracingEventsNotAvailable(); + + // Make sure session is not started + $this->assertFalse($this->app['session.store']->isStarted()); + + $span = $this->executeAndReturnMostRecentSpan(function () { + Cache::get('some-key'); + }); + + // Session should still not be started + $this->assertFalse($this->app['session.store']->isStarted()); + + // And the key should not be replaced + $this->assertEquals('some-key', $span->getDescription()); + } + private function markSkippedIfTracingEventsNotAvailable(): void { if (class_exists(RetrievingKey::class)) { From d30f9d7500f9530f64d51e24e1351c5e71e7b914 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 17 Jun 2025 11:27:38 +0000 Subject: [PATCH 2/7] Fix cache integration span data merging and improve session handling in tests --- .../Laravel/Features/CacheIntegration.php | 18 +++++---- test/Sentry/Features/CacheIntegrationTest.php | 37 ++++++++++--------- 2 files changed, 30 insertions(+), 25 deletions(-) diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index 6ec04de6..7abfea18 100644 --- a/src/Sentry/Laravel/Features/CacheIntegration.php +++ b/src/Sentry/Laravel/Features/CacheIntegration.php @@ -225,9 +225,10 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo $finishedSpan = $this->maybeFinishSpan(SpanStatus::ok()); if ($finishedSpan !== null && count($finishedSpan->getData()['cache.key'] ?? []) === 1) { - $finishedSpan->setData([ - 'cache.hit' => $event instanceof Events\CacheHit, - ]); + $finishedSpan->setData(array_merge( + $finishedSpan->getData(), + ['cache.hit' => $event instanceof Events\CacheHit] + )); } return true; @@ -240,9 +241,10 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo ); if ($finishedSpan !== null) { - $finishedSpan->setData([ - 'cache.success' => $event instanceof Events\KeyWritten, - ]); + $finishedSpan->setData(array_merge( + $finishedSpan->getData(), + ['cache.success' => $event instanceof Events\KeyWritten] + )); } return true; @@ -314,9 +316,9 @@ private function replaceSessionKey(string $key): string /** * Replace session keys in an array of keys with placeholders. * - * @param array $keys + * @param string[] $keys * - * @return array + * @return string[] */ private function replaceSessionKeys(array $keys): array { diff --git a/test/Sentry/Features/CacheIntegrationTest.php b/test/Sentry/Features/CacheIntegrationTest.php index 707ebe26..d6b8ddae 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'); @@ -53,10 +57,10 @@ public function testCacheBreadcrumbIsNotRecordedWhenDisabled(): void public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void { - // Start a session - $this->app['request']->setLaravelSession($session = $this->app['session.store']); - $session->start(); - $sessionId = $session->getId(); + // Start a session properly in the test environment + $this->withSession(['test' => 'value']); + + $sessionId = $this->app['session']->getId(); // Use the session ID as a cache key Cache::put($sessionId, 'session-data'); @@ -178,10 +182,10 @@ public function testCacheSpanReplacesSessionKeyWithPlaceholder(): void { $this->markSkippedIfTracingEventsNotAvailable(); - // Start a session - $this->app['request']->setLaravelSession($session = $this->app['session.store']); - $session->start(); - $sessionId = $session->getId(); + // Start a session properly in the test environment + $this->withSession(['test' => 'value']); + + $sessionId = $this->app['session']->getId(); $span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) { Cache::get($sessionId); @@ -196,10 +200,10 @@ public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void { $this->markSkippedIfTracingEventsNotAvailable(); - // Start a session - $this->app['request']->setLaravelSession($session = $this->app['session.store']); - $session->start(); - $sessionId = $session->getId(); + // Start a session properly in the test environment + $this->withSession(['test' => 'value']); + + $sessionId = $this->app['session']->getId(); $span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) { Cache::get([$sessionId, 'regular-key', $sessionId . '_another']); @@ -214,15 +218,14 @@ public function testCacheOperationDoesNotStartSessionPrematurely(): void { $this->markSkippedIfTracingEventsNotAvailable(); - // Make sure session is not started - $this->assertFalse($this->app['session.store']->isStarted()); + // Don't start a session to ensure it's not started $span = $this->executeAndReturnMostRecentSpan(function () { Cache::get('some-key'); }); - - // Session should still not be started - $this->assertFalse($this->app['session.store']->isStarted()); + + // 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()); From 8b547e8a97fcdb6a18caa0186cc8c6df526bfb95 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 17 Jun 2025 11:43:07 +0000 Subject: [PATCH 3/7] Fix session key matching and improve session handling in cache integration --- .../Laravel/Features/CacheIntegration.php | 16 +++++---- test/Sentry/Features/CacheIntegrationTest.php | 34 ++++++++++++++----- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index 7abfea18..1334f54e 100644 --- a/src/Sentry/Laravel/Features/CacheIntegration.php +++ b/src/Sentry/Laravel/Features/CacheIntegration.php @@ -278,23 +278,27 @@ private function isSessionKey(string $key): bool $request = $this->container()->make('request'); // Check if the request has a session - if (!$request->hasSession()) { + if (!method_exists($request, 'hasSession') || !$request->hasSession()) { return false; } $session = $request->session(); // Don't start the session if it hasn't been started yet - if (!$session->isStarted()) { + if (!method_exists($session, 'isStarted') || !$session->isStarted()) { return false; } - // Get the session ID and check if the cache key matches + // Get the session ID and check if the cache key matches exactly $sessionId = $session->getId(); + + // Check for empty session ID + if (empty($sessionId)) { + return false; + } - // Check if the key equals the session ID or contains it - // This handles cases where the cache key might be prefixed - return $key === $sessionId || Str::endsWith($key, $sessionId); + // Check if the key equals the session ID exactly + return $key === $sessionId; } catch (\Exception $e) { // If anything goes wrong, we assume it's not a session key return false; diff --git a/test/Sentry/Features/CacheIntegrationTest.php b/test/Sentry/Features/CacheIntegrationTest.php index d6b8ddae..aac1e885 100644 --- a/test/Sentry/Features/CacheIntegrationTest.php +++ b/test/Sentry/Features/CacheIntegrationTest.php @@ -58,8 +58,8 @@ public function testCacheBreadcrumbIsNotRecordedWhenDisabled(): void public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void { // Start a session properly in the test environment - $this->withSession(['test' => 'value']); - + $this->ensureRequestIsBound(); + $this->startSession(); $sessionId = $this->app['session']->getId(); // Use the session ID as a cache key @@ -183,8 +183,8 @@ public function testCacheSpanReplacesSessionKeyWithPlaceholder(): void $this->markSkippedIfTracingEventsNotAvailable(); // Start a session properly in the test environment - $this->withSession(['test' => 'value']); - + $this->ensureRequestIsBound(); + $this->startSession(); $sessionId = $this->app['session']->getId(); $span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) { @@ -201,8 +201,8 @@ public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void $this->markSkippedIfTracingEventsNotAvailable(); // Start a session properly in the test environment - $this->withSession(['test' => 'value']); - + $this->ensureRequestIsBound(); + $this->startSession(); $sessionId = $this->app['session']->getId(); $span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) { @@ -210,8 +210,8 @@ public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void }); $this->assertEquals('cache.get', $span->getOp()); - $this->assertEquals('{sessionKey}, regular-key, {sessionKey}', $span->getDescription()); - $this->assertEquals(['{sessionKey}', 'regular-key', '{sessionKey}'], $span->getData()['cache.key']); + $this->assertEquals('{sessionKey}, regular-key, ' . $sessionId . '_another', $span->getDescription()); + $this->assertEquals(['{sessionKey}', 'regular-key', $sessionId . '_another'], $span->getData()['cache.key']); } public function testCacheOperationDoesNotStartSessionPrematurely(): void @@ -252,4 +252,22 @@ private function executeAndReturnMostRecentSpan(callable $callable): Span return array_pop($spans); } + + private function ensureRequestIsBound(): void + { + // Ensure we have a request instance + if (!$this->app->bound('request')) { + $this->app->instance('request', $this->app->make(\Illuminate\Http\Request::class)); + } + } + + private function startSession(): void + { + // Start the session + $session = $this->app['session']; + $session->start(); + + // Set the session on the request + $this->app['request']->setLaravelSession($session); + } } From c081654fbd441a9a043ee8f990ea93eb90497526 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Sat, 21 Jun 2025 12:30:51 +0200 Subject: [PATCH 4/7] CR --- .../Laravel/Features/CacheIntegration.php | 59 ++++++------------- test/Sentry/Features/CacheIntegrationTest.php | 39 ++++++------ 2 files changed, 34 insertions(+), 64 deletions(-) diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index 1334f54e..ba5a530e 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\Http\Request; use Illuminate\Redis\Events as RedisEvents; use Illuminate\Redis\RedisManager; use Illuminate\Support\Str; @@ -197,12 +198,7 @@ public function handleRedisCommands(RedisEvents\CommandExecuted $event): void ]; if ($this->shouldSendDefaultPii()) { - // Replace session keys in parameters if present - $parameters = $event->parameters; - if (!empty($parameters[0]) && is_string($parameters[0])) { - $parameters[0] = $this->replaceSessionKey($parameters[0]); - } - $data['db.redis.parameters'] = $parameters; + $data['db.redis.parameters'] = $this->replaceSessionKeys($event->parameters); } if ($this->isTracingFeatureEnabled('redis_origin')) { @@ -261,44 +257,25 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo } /** - * Check if a cache key is the current session key. - * - * @param string $key - * - * @return bool + * Retrieve the current session key if available. */ - private function isSessionKey(string $key): bool + private function getSessionKey(): ?string { - // Check if the container has a bound request and session - if (!$this->container()->bound('request')) { - return false; - } - try { + /** @var Request $request */ $request = $this->container()->make('request'); - // Check if the request has a session - if (!method_exists($request, 'hasSession') || !$request->hasSession()) { + if (!$request->hasSession()) { return false; } $session = $request->session(); - // Don't start the session if it hasn't been started yet - if (!method_exists($session, 'isStarted') || !$session->isStarted()) { + if (!$session->isStarted()) { return false; } - // Get the session ID and check if the cache key matches exactly - $sessionId = $session->getId(); - - // Check for empty session ID - if (empty($sessionId)) { - return false; - } - - // Check if the key equals the session ID exactly - return $key === $sessionId; + return $session->getId(); } catch (\Exception $e) { // If anything goes wrong, we assume it's not a session key return false; @@ -307,28 +284,26 @@ private function isSessionKey(string $key): bool /** * Replace a session key with a placeholder. - * - * @param string $key - * - * @return string */ - private function replaceSessionKey(string $key): string + private function replaceSessionKey(string $value): string { - return $this->isSessionKey($key) ? '{sessionKey}' : $key; + return $value === $this->getSessionKey() ? '{sessionKey}' : $value; } /** * Replace session keys in an array of keys with placeholders. * - * @param string[] $keys + * @param string[] $values * * @return string[] */ - private function replaceSessionKeys(array $keys): array + private function replaceSessionKeys(array $values): array { - return array_map(function ($key) { - return $this->replaceSessionKey($key); - }, $keys); + $sessionKey = $this->getSessionKey(); + + return array_map(static function ($value) use ($sessionKey) { + return $value === $sessionKey ? '{sessionKey}' : $value; + }, $values); } /** diff --git a/test/Sentry/Features/CacheIntegrationTest.php b/test/Sentry/Features/CacheIntegrationTest.php index aac1e885..f26d33cb 100644 --- a/test/Sentry/Features/CacheIntegrationTest.php +++ b/test/Sentry/Features/CacheIntegrationTest.php @@ -58,7 +58,7 @@ public function testCacheBreadcrumbIsNotRecordedWhenDisabled(): void public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void { // Start a session properly in the test environment - $this->ensureRequestIsBound(); + $this->ensureRequestIsBoundWithSession(); $this->startSession(); $sessionId = $this->app['session']->getId(); @@ -66,12 +66,12 @@ public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void Cache::put($sessionId, 'session-data'); $breadcrumb = $this->getLastSentryBreadcrumb(); - $this->assertEquals("Written: {sessionKey}", $breadcrumb->getMessage()); + $this->assertEquals('Written: {sessionKey}', $breadcrumb->getMessage()); Cache::get($sessionId); $breadcrumb = $this->getLastSentryBreadcrumb(); - $this->assertEquals("Read: {sessionKey}", $breadcrumb->getMessage()); + $this->assertEquals('Read: {sessionKey}', $breadcrumb->getMessage()); } public function testCacheBreadcrumbDoesNotReplaceNonSessionKeys(): void @@ -79,7 +79,7 @@ public function testCacheBreadcrumbDoesNotReplaceNonSessionKeys(): void Cache::put('regular-key', 'value'); $breadcrumb = $this->getLastSentryBreadcrumb(); - $this->assertEquals("Written: regular-key", $breadcrumb->getMessage()); + $this->assertEquals('Written: regular-key', $breadcrumb->getMessage()); } public function testCacheGetSpanIsRecorded(): void @@ -183,7 +183,7 @@ public function testCacheSpanReplacesSessionKeyWithPlaceholder(): void $this->markSkippedIfTracingEventsNotAvailable(); // Start a session properly in the test environment - $this->ensureRequestIsBound(); + $this->ensureRequestIsBoundWithSession(); $this->startSession(); $sessionId = $this->app['session']->getId(); @@ -201,7 +201,7 @@ public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void $this->markSkippedIfTracingEventsNotAvailable(); // Start a session properly in the test environment - $this->ensureRequestIsBound(); + $this->ensureRequestIsBoundWithSession(); $this->startSession(); $sessionId = $this->app['session']->getId(); @@ -223,10 +223,10 @@ public function testCacheOperationDoesNotStartSessionPrematurely(): void $span = $this->executeAndReturnMostRecentSpan(function () { Cache::get('some-key'); }); - - // Check that session was not started + + // 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()); } @@ -253,21 +253,16 @@ private function executeAndReturnMostRecentSpan(callable $callable): Span return array_pop($spans); } - private function ensureRequestIsBound(): void + private function ensureRequestIsBoundWithSession(): void { - // Ensure we have a request instance - if (!$this->app->bound('request')) { - $this->app->instance('request', $this->app->make(\Illuminate\Http\Request::class)); - } - } + if ($this->app->bound('request')) { + $request = $this->app['request']; + } else { + $request = $this->app->make(\Illuminate\Http\Request::class); - private function startSession(): void - { - // Start the session - $session = $this->app['session']; - $session->start(); + $this->app->instance('request', $request); + } - // Set the session on the request - $this->app['request']->setLaravelSession($session); + $request->setLaravelSession($this->app['session']->driver()); } } From d48ae6ba1164f763c86f2f5af13ce293cefa6c25 Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Sat, 21 Jun 2025 12:43:30 +0200 Subject: [PATCH 5/7] Fix not replacing session key before session is started --- .../Laravel/Features/CacheIntegration.php | 22 +++++-------------- test/Sentry/Features/CacheIntegrationTest.php | 17 -------------- 2 files changed, 6 insertions(+), 33 deletions(-) diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index ba5a530e..f0f431be 100644 --- a/src/Sentry/Laravel/Features/CacheIntegration.php +++ b/src/Sentry/Laravel/Features/CacheIntegration.php @@ -4,7 +4,7 @@ use Illuminate\Cache\Events; use Illuminate\Contracts\Events\Dispatcher; -use Illuminate\Http\Request; +use Illuminate\Contracts\Session\Session; use Illuminate\Redis\Events as RedisEvents; use Illuminate\Redis\RedisManager; use Illuminate\Support\Str; @@ -262,23 +262,13 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo private function getSessionKey(): ?string { try { - /** @var Request $request */ - $request = $this->container()->make('request'); + /** @var Session $request */ + $request = $this->container()->make('session.store'); - if (!$request->hasSession()) { - return false; - } - - $session = $request->session(); - - if (!$session->isStarted()) { - return false; - } - - return $session->getId(); + return $request->getId(); } catch (\Exception $e) { - // If anything goes wrong, we assume it's not a session key - return false; + // We can assume the session store is not available here so there is no session key to retrieve + return null; } } diff --git a/test/Sentry/Features/CacheIntegrationTest.php b/test/Sentry/Features/CacheIntegrationTest.php index f26d33cb..6aae519e 100644 --- a/test/Sentry/Features/CacheIntegrationTest.php +++ b/test/Sentry/Features/CacheIntegrationTest.php @@ -58,7 +58,6 @@ public function testCacheBreadcrumbIsNotRecordedWhenDisabled(): void public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void { // Start a session properly in the test environment - $this->ensureRequestIsBoundWithSession(); $this->startSession(); $sessionId = $this->app['session']->getId(); @@ -182,8 +181,6 @@ public function testCacheSpanReplacesSessionKeyWithPlaceholder(): void { $this->markSkippedIfTracingEventsNotAvailable(); - // Start a session properly in the test environment - $this->ensureRequestIsBoundWithSession(); $this->startSession(); $sessionId = $this->app['session']->getId(); @@ -201,7 +198,6 @@ public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void $this->markSkippedIfTracingEventsNotAvailable(); // Start a session properly in the test environment - $this->ensureRequestIsBoundWithSession(); $this->startSession(); $sessionId = $this->app['session']->getId(); @@ -252,17 +248,4 @@ private function executeAndReturnMostRecentSpan(callable $callable): Span return array_pop($spans); } - - private function ensureRequestIsBoundWithSession(): void - { - if ($this->app->bound('request')) { - $request = $this->app['request']; - } else { - $request = $this->app->make(\Illuminate\Http\Request::class); - - $this->app->instance('request', $request); - } - - $request->setLaravelSession($this->app['session']->driver()); - } } From 5b148aaf1437685e1f04a533a958e9ea3a0a900b Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Sat, 21 Jun 2025 13:20:42 +0200 Subject: [PATCH 6/7] Revert `setData` changes, already merges data --- src/Sentry/Laravel/Features/CacheIntegration.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index f0f431be..5d55f93a 100644 --- a/src/Sentry/Laravel/Features/CacheIntegration.php +++ b/src/Sentry/Laravel/Features/CacheIntegration.php @@ -221,10 +221,9 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo $finishedSpan = $this->maybeFinishSpan(SpanStatus::ok()); if ($finishedSpan !== null && count($finishedSpan->getData()['cache.key'] ?? []) === 1) { - $finishedSpan->setData(array_merge( - $finishedSpan->getData(), - ['cache.hit' => $event instanceof Events\CacheHit] - )); + $finishedSpan->setData([ + 'cache.hit' => $event instanceof Events\CacheHit + ]); } return true; @@ -237,10 +236,9 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo ); if ($finishedSpan !== null) { - $finishedSpan->setData(array_merge( - $finishedSpan->getData(), - ['cache.success' => $event instanceof Events\KeyWritten] - )); + $finishedSpan->setData([ + 'cache.success' => $event instanceof Events\KeyWritten + ]); } return true; From 7d0d14436c97882b344c9bd20e7814173ace9bdc Mon Sep 17 00:00:00 2001 From: Alex Bouma Date: Sat, 28 Jun 2025 10:49:12 +0200 Subject: [PATCH 7/7] CR --- .../Laravel/Features/CacheIntegration.php | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/Sentry/Laravel/Features/CacheIntegration.php b/src/Sentry/Laravel/Features/CacheIntegration.php index 5d55f93a..1a2a47a1 100644 --- a/src/Sentry/Laravel/Features/CacheIntegration.php +++ b/src/Sentry/Laravel/Features/CacheIntegration.php @@ -260,12 +260,19 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo private function getSessionKey(): ?string { try { - /** @var Session $request */ - $request = $this->container()->make('session.store'); - - return $request->getId(); + /** @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; } } @@ -283,14 +290,14 @@ private function replaceSessionKey(string $value): string * * @param string[] $values * - * @return string[] + * @return mixed[] */ private function replaceSessionKeys(array $values): array { $sessionKey = $this->getSessionKey(); return array_map(static function ($value) use ($sessionKey) { - return $value === $sessionKey ? '{sessionKey}' : $value; + return is_string($value) && $value === $sessionKey ? '{sessionKey}' : $value; }, $values); }