🥅 server: fix bad session issue, fingerprint validator errors by code#732
🥅 server: fix bad session issue, fingerprint validator errors by code#732cruzdanilo merged 4 commits intomainfrom
bad session issue, fingerprint validator errors by code#732Conversation
cruzdanilo
requested review from
dieguezguille,
franm91 and
nfmelendez
as code owners
🦋 Changeset detectedLatest commit: 36db52b The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @cruzdanilo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant improvements to how user sessions are managed and how validation errors are reported. By implementing a flexible session ID retrieval strategy that supports both cookies and a custom header, it aims to resolve "bad session" issues and enhance compatibility across different client environments. Concurrently, the integration of code-based fingerprinting for validation errors will streamline debugging and monitoring in Sentry. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThe PR introduces session management via response headers across authentication endpoints, making session_id optional in cookie schemas and adding X-Session-Id header propagation for cross-origin requests. Client-side utilities now extract and reuse session IDs from response headers. Error fingerprinting by code is added to the validator hook for improved Sentry error tracking. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthServer as Auth Server
participant Client2 as Client<br/>(with session)
Client->>AuthServer: GET /authentication
AuthServer->>AuthServer: Create session
AuthServer-->>Client: 200 + Cookie (path=/) +<br/>X-Session-Id Header
Client->>Client: Extract X-Session-Id<br/>from response header
Client2->>AuthServer: POST /authentication<br/>+ X-Session-Id Header
AuthServer->>AuthServer: Read sessionId<br/>from header (or fallback<br/>to cookie)
AuthServer-->>Client2: 200 + Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a session handling issue, likely affecting mobile clients, by introducing a session ID header as a fallback to cookies. The changes are consistent across both client and server implementations. Setting the cookie path to / is a good fix for broader cookie availability. Additionally, the introduction of Sentry error fingerprinting for validation errors is a valuable improvement for monitoring. I've identified a potential runtime error in the stringOrLegacy utility function and provided a suggestion to make it more robust.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
==========================================
- Coverage 67.68% 67.21% -0.48%
==========================================
Files 206 206
Lines 7056 6902 -154
Branches 2217 2143 -74
==========================================
- Hits 4776 4639 -137
+ Misses 2093 2081 -12
+ Partials 187 182 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.changeset/full-doves-bark.md:
- Line 5: The changeset summary is a noun phrase and must be an imperative
lowercase sentence; update the summary line in .changeset/full-doves-bark.md
(the line currently "🥅 fallback session delivery via response header") to
include a present-tense verb and be lowercase, e.g. change it to something like
"add fallback session delivery via response header" or "provide fallback session
delivery via response header".
In `@server/api/auth/registration.ts`:
- Around line 308-309: The error responses in registration.ts (e.g., where
sessionId is read and later error returns) are inconsistent with
authentication.ts: some return { code: "bad session" } while others include the
deprecated legacy field; update all error response shapes in registration.ts
(and mirror in authentication.ts) to follow a consistent transitional
pattern—either include the legacy payload on every error return or remove it
everywhere—so that stringOrLegacy and LegacyAuthentication behavior remains
predictable; search for returns that emit { code: "..."} (notably around the
sessionId check and the error returns near the comments referencing legacy) and
make them emit the chosen canonical shape (include the legacy key with the same
structure used elsewhere if you choose to include it, or remove legacy from all
error responses) and ensure tests/consumers still accept the change.
1 participant
sentry error: https://exactly.sentry.io/issues/7195875235/events/6d2aa6b11f3546328e90412bd89dacea/
Summary by CodeRabbit