Skip to content

Commit e08437a

Browse files
authored
Replace session key in cache integration with placeholder (#1009)
1 parent 242c8c4 commit e08437a

File tree

2 files changed

+147
-11
lines changed

2 files changed

+147
-11
lines changed

src/Sentry/Laravel/Features/CacheIntegration.php

Lines changed: 67 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use Illuminate\Cache\Events;
66
use Illuminate\Contracts\Events\Dispatcher;
7+
use Illuminate\Contracts\Session\Session;
78
use Illuminate\Redis\Events as RedisEvents;
89
use Illuminate\Redis\RedisManager;
910
use Illuminate\Support\Str;
@@ -86,11 +87,13 @@ public function handleCacheEventsForBreadcrumbs(Events\CacheEvent $event): void
8687
return;
8788
}
8889

90+
$displayKey = $this->replaceSessionKey($event->key);
91+
8992
Integration::addBreadcrumb(new Breadcrumb(
9093
Breadcrumb::LEVEL_INFO,
9194
Breadcrumb::TYPE_DEFAULT,
9295
'cache',
93-
"{$message}: {$event->key}",
96+
"{$message}: {$displayKey}",
9497
$event->tags ? ['tags' => $event->tags] : []
9598
));
9699
}
@@ -109,15 +112,17 @@ public function handleCacheEventsForTracing(Events\CacheEvent $event): void
109112
: $event->keys
110113
);
111114

115+
$displayKeys = $this->replaceSessionKeys($keys);
116+
112117
$this->pushSpan(
113118
$parentSpan->startChild(
114119
SpanContext::make()
115120
->setOp('cache.get')
116121
->setData([
117-
'cache.key' => $keys,
122+
'cache.key' => $displayKeys,
118123
])
119124
->setOrigin('auto.cache')
120-
->setDescription(implode(', ', $keys))
125+
->setDescription(implode(', ', $displayKeys))
121126
)
122127
);
123128
}
@@ -129,30 +134,34 @@ public function handleCacheEventsForTracing(Events\CacheEvent $event): void
129134
: $event->keys
130135
);
131136

137+
$displayKeys = $this->replaceSessionKeys($keys);
138+
132139
$this->pushSpan(
133140
$parentSpan->startChild(
134141
SpanContext::make()
135142
->setOp('cache.put')
136143
->setData([
137-
'cache.key' => $keys,
144+
'cache.key' => $displayKeys,
138145
'cache.ttl' => $event->seconds,
139146
])
140147
->setOrigin('auto.cache')
141-
->setDescription(implode(', ', $keys))
148+
->setDescription(implode(', ', $displayKeys))
142149
)
143150
);
144151
}
145152

146153
if ($event instanceof Events\ForgettingKey) {
154+
$displayKey = $this->replaceSessionKey($event->key);
155+
147156
$this->pushSpan(
148157
$parentSpan->startChild(
149158
SpanContext::make()
150159
->setOp('cache.remove')
151160
->setData([
152-
'cache.key' => [$event->key],
161+
'cache.key' => [$displayKey],
153162
])
154163
->setOrigin('auto.cache')
155-
->setDescription($event->key)
164+
->setDescription($displayKey)
156165
)
157166
);
158167
}
@@ -177,7 +186,7 @@ public function handleRedisCommands(RedisEvents\CommandExecuted $event): void
177186
// 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
178187
// This is not a perfect solution but it's the best we can do without understanding the command that was executed
179188
if (!empty($event->parameters[0]) && is_string($event->parameters[0]) && !Str::contains($event->parameters[0], "\n")) {
180-
$keyForDescription = $event->parameters[0];
189+
$keyForDescription = $this->replaceSessionKey($event->parameters[0]);
181190
}
182191

183192
$context->setDescription(rtrim(strtoupper($event->command) . ' ' . $keyForDescription));
@@ -189,7 +198,7 @@ public function handleRedisCommands(RedisEvents\CommandExecuted $event): void
189198
];
190199

191200
if ($this->shouldSendDefaultPii()) {
192-
$data['db.redis.parameters'] = $event->parameters;
201+
$data['db.redis.parameters'] = $this->replaceSessionKeys($event->parameters);
193202
}
194203

195204
if ($this->isTracingFeatureEnabled('redis_origin')) {
@@ -213,7 +222,7 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo
213222

214223
if ($finishedSpan !== null && count($finishedSpan->getData()['cache.key'] ?? []) === 1) {
215224
$finishedSpan->setData([
216-
'cache.hit' => $event instanceof Events\CacheHit,
225+
'cache.hit' => $event instanceof Events\CacheHit
217226
]);
218227
}
219228

@@ -228,7 +237,7 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo
228237

229238
if ($finishedSpan !== null) {
230239
$finishedSpan->setData([
231-
'cache.success' => $event instanceof Events\KeyWritten,
240+
'cache.success' => $event instanceof Events\KeyWritten
232241
]);
233242
}
234243

@@ -245,6 +254,53 @@ private function maybeHandleCacheEventAsEndOfSpan(Events\CacheEvent $event): boo
245254
return false;
246255
}
247256

257+
/**
258+
* Retrieve the current session key if available.
259+
*/
260+
private function getSessionKey(): ?string
261+
{
262+
try {
263+
/** @var Session $sessionStore */
264+
$sessionStore = $this->container()->make('session.store');
265+
266+
// It is safe for us to get the session ID here without checking if the session is started
267+
// because getting the session ID does not start the session. In addition we need the ID before
268+
// the session is started because the cache will retrieve the session ID from the cache before the session
269+
// is considered started. So if we wait for the session to be started, we will not be able to replace the
270+
// session key in the cache operation that is being executed to retrieve the session data from the cache.
271+
return $sessionStore->getId();
272+
} catch (\Exception $e) {
273+
// We can assume the session store is not available here so there is no session key to retrieve
274+
// We capture a generic exception to avoid breaking the application because some code paths can
275+
// result in an exception other than the expected `Illuminate\Contracts\Container\BindingResolutionException`
276+
return null;
277+
}
278+
}
279+
280+
/**
281+
* Replace a session key with a placeholder.
282+
*/
283+
private function replaceSessionKey(string $value): string
284+
{
285+
return $value === $this->getSessionKey() ? '{sessionKey}' : $value;
286+
}
287+
288+
/**
289+
* Replace session keys in an array of keys with placeholders.
290+
*
291+
* @param string[] $values
292+
*
293+
* @return mixed[]
294+
*/
295+
private function replaceSessionKeys(array $values): array
296+
{
297+
$sessionKey = $this->getSessionKey();
298+
299+
return array_map(static function ($value) use ($sessionKey) {
300+
return is_string($value) && $value === $sessionKey ? '{sessionKey}' : $value;
301+
}, $values);
302+
}
303+
248304
/**
249305
* Normalize the array of keys to a array of only strings.
250306
*

test/Sentry/Features/CacheIntegrationTest.php

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@
99

1010
class CacheIntegrationTest extends TestCase
1111
{
12+
protected $defaultSetupConfig = [
13+
'session.driver' => 'array',
14+
];
15+
1216
public function testCacheBreadcrumbForWriteAndHitIsRecorded(): void
1317
{
1418
Cache::put($key = 'foo', 'bar');
@@ -51,6 +55,32 @@ public function testCacheBreadcrumbIsNotRecordedWhenDisabled(): void
5155
$this->assertEmpty($this->getCurrentSentryBreadcrumbs());
5256
}
5357

58+
public function testCacheBreadcrumbReplacesSessionKeyWithPlaceholder(): void
59+
{
60+
// Start a session properly in the test environment
61+
$this->startSession();
62+
$sessionId = $this->app['session']->getId();
63+
64+
// Use the session ID as a cache key
65+
Cache::put($sessionId, 'session-data');
66+
67+
$breadcrumb = $this->getLastSentryBreadcrumb();
68+
$this->assertEquals('Written: {sessionKey}', $breadcrumb->getMessage());
69+
70+
Cache::get($sessionId);
71+
72+
$breadcrumb = $this->getLastSentryBreadcrumb();
73+
$this->assertEquals('Read: {sessionKey}', $breadcrumb->getMessage());
74+
}
75+
76+
public function testCacheBreadcrumbDoesNotReplaceNonSessionKeys(): void
77+
{
78+
Cache::put('regular-key', 'value');
79+
80+
$breadcrumb = $this->getLastSentryBreadcrumb();
81+
$this->assertEquals('Written: regular-key', $breadcrumb->getMessage());
82+
}
83+
5484
public function testCacheGetSpanIsRecorded(): void
5585
{
5686
$this->markSkippedIfTracingEventsNotAvailable();
@@ -147,6 +177,56 @@ public function testCacheRemoveSpanIsRecorded(): void
147177
$this->assertEquals(['foo'], $span->getData()['cache.key']);
148178
}
149179

180+
public function testCacheSpanReplacesSessionKeyWithPlaceholder(): void
181+
{
182+
$this->markSkippedIfTracingEventsNotAvailable();
183+
184+
$this->startSession();
185+
$sessionId = $this->app['session']->getId();
186+
187+
$span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) {
188+
Cache::get($sessionId);
189+
});
190+
191+
$this->assertEquals('cache.get', $span->getOp());
192+
$this->assertEquals('{sessionKey}', $span->getDescription());
193+
$this->assertEquals(['{sessionKey}'], $span->getData()['cache.key']);
194+
}
195+
196+
public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void
197+
{
198+
$this->markSkippedIfTracingEventsNotAvailable();
199+
200+
// Start a session properly in the test environment
201+
$this->startSession();
202+
$sessionId = $this->app['session']->getId();
203+
204+
$span = $this->executeAndReturnMostRecentSpan(function () use ($sessionId) {
205+
Cache::get([$sessionId, 'regular-key', $sessionId . '_another']);
206+
});
207+
208+
$this->assertEquals('cache.get', $span->getOp());
209+
$this->assertEquals('{sessionKey}, regular-key, ' . $sessionId . '_another', $span->getDescription());
210+
$this->assertEquals(['{sessionKey}', 'regular-key', $sessionId . '_another'], $span->getData()['cache.key']);
211+
}
212+
213+
public function testCacheOperationDoesNotStartSessionPrematurely(): void
214+
{
215+
$this->markSkippedIfTracingEventsNotAvailable();
216+
217+
// Don't start a session to ensure it's not started
218+
219+
$span = $this->executeAndReturnMostRecentSpan(function () {
220+
Cache::get('some-key');
221+
});
222+
223+
// Check that session was not started
224+
$this->assertFalse($this->app['session']->isStarted());
225+
226+
// And the key should not be replaced
227+
$this->assertEquals('some-key', $span->getDescription());
228+
}
229+
150230
private function markSkippedIfTracingEventsNotAvailable(): void
151231
{
152232
if (class_exists(RetrievingKey::class)) {

0 commit comments

Comments
 (0)