Skip to content

Replace session key in cache integration with placeholder #1009

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

cleptric
Copy link
Member

A fix was implemented to prevent session keys from appearing as cache keys in Sentry's session overview.

  • Three new private methods were added to src/Sentry/Laravel/Features/CacheIntegration.php:
    • isSessionKey(string $key): bool determines if a given cache key matches the current session ID, handling prefixed keys and ensuring sessions are not prematurely started.
    • replaceSessionKey(string $key): string replaces a detected session key with the placeholder {sessionKey}.
    • replaceSessionKeys(array $keys): array applies the replacement logic to arrays of keys.
  • Existing event handlers in src/Sentry/Laravel/Features/CacheIntegration.php were updated to utilize these new methods:
    • handleCacheEventsForBreadcrumbs now replaces session keys in breadcrumb messages.
    • handleCacheEventsForTracing replaces session keys in span descriptions and cache.key data for cache.get, cache.put, and cache.remove operations.
    • handleRedisCommands also applies the replacement to Redis command descriptions and parameters.
  • Comprehensive tests were added to test/Sentry/Features/CacheIntegrationTest.php to verify session key replacement in breadcrumbs and spans, and to confirm that the integration does not prematurely start sessions.

This ensures session IDs are consistently anonymized as {sessionKey} in Sentry data, improving readability and privacy.

@stayallive stayallive changed the title Fix GitHub issue for Sentry Laravel Replace session key in cache integration with placeholder Jun 21, 2025
@stayallive stayallive marked this pull request as ready for review June 21, 2025 11:20
@stayallive stayallive requested review from stayallive and Copilot June 21, 2025 11:20
Copilot

This comment was marked as outdated.

@cleptric cleptric requested a review from Copilot June 25, 2025 01:41
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures that session IDs are anonymized as {sessionKey} in Sentry breadcrumbs, spans, and Redis command data by replacing any detected session key in cache and Redis integrations.

  • Introduces methods to retrieve the current session ID and replace it (and arrays of keys) with a placeholder.
  • Updates cache and Redis event handlers to apply the replacement logic to breadcrumb messages, span descriptions, and parameters.
  • Adds comprehensive tests to verify placeholder substitution and prevent premature session starts.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
test/Sentry/Features/CacheIntegrationTest.php Added tests for replacing session keys with {sessionKey} in breadcrumbs and spans, and verifying no premature session start.
src/Sentry/Laravel/Features/CacheIntegration.php Added getSessionKey, replaceSessionKey, replaceSessionKeys methods and updated event handlers to use them.
Comments suppressed due to low confidence (1)

test/Sentry/Features/CacheIntegrationTest.php:196

  • There’s no test for session keys stored with a cache prefix. Adding a test case that uses a prefixed session ID (e.g., prefix_{$sessionId}) would ensure the replacement logic covers those scenarios.
    public function testCacheSpanReplacesMultipleSessionKeysWithPlaceholder(): void

@cleptric
Copy link
Member Author

bugbot run

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@getsentry getsentry deleted a comment from seer-by-sentry bot Aug 11, 2025
@getsentry getsentry deleted a comment from seer-by-sentry bot Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants