chore: Custom cooke resolver and logout on session invalidation #39695
Conversation
WalkthroughThis pull request introduces a custom session cookie resolver and updates session handling. A new class, Changes
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java (2)
17-17: Consider using an enum for SameSite valuesUsing string constants like this is fine, but for better type safety and IDE support, consider using an enum to represent SameSite values (None, Lax, Strict).
10-14: Document where the cross-site embedding configuration comes fromThe class comment mentions setting SameSite "based on organization configuration" but the implementation doesn't show where this configuration comes from or how it's used.
Add more specific documentation about how the organization configuration will be used, or consider adding a method parameter to make this configurable:
/** * This class is a custom implementation of the CookieWebSessionIdResolver class. * It allows us to set the SameSite attribute of the session cookie. * Currently set to "Lax" by default, but future implementations will * allow customization based on organization-specific embedding requirements. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CustomCookieWebSessionIdResolver.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CustomCookieWebSessionIdResolver.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java (3)
1-15: LGTM - Documentation clearly explains the purpose of this custom resolverThe class documentation accurately describes the purpose of extending the default cookie resolver to support customized SameSite attributes for cross-site embedding scenarios.
19-26: LGTM - Good cookie configuration with helpful commentsSetting a 30-day max age and root path are sensible defaults. The comments explaining the rationale for preventing cookie expiration on browser close are helpful.
28-32: LGTM - Proper method override patternCorrectly calls the custom initializer method before delegating to the parent implementation.
| protected void addCookieInitializers() { | ||
| // Add the appropriate SameSite attribute based on the exchange attribute | ||
| addCookieInitializer((builder) -> builder.sameSite(LAX).secure(true)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Inconsistency between implementation and documentation
The method comment suggests the SameSite attribute should be based on an exchange attribute, but the implementation hard-codes it to "Lax". Either:
- Update the implementation to actually use the exchange attribute
- Update the comment to reflect the current implementation
Additionally, consider adding a comment explaining why "Lax" was chosen over "Strict" or "None" as it has security implications.
protected void addCookieInitializers() {
- // Add the appropriate SameSite attribute based on the exchange attribute
+ // Set SameSite to "Lax" which provides a balance between security and functionality
+ // "Lax" allows the cookie to be sent when users navigate to the site from external links
addCookieInitializer((builder) -> builder.sameSite(LAX).secure(true));
}Action: Update the method documentation to reflect the deliberate use of a fixed SameSite value.
- The implementation is hard-coded to use "Lax" rather than basing the attribute on an exchange value like the comment suggests.
- Update the comment to clearly state that using "Lax" is an intentional choice to balance security (e.g., CSRF protections) with cookie availability during top-level navigations.
- Optionally, add clarification on why "Lax" was preferred over "Strict" or "None" given your security considerations.
protected void addCookieInitializers() {
- // Add the appropriate SameSite attribute based on the exchange attribute
+ // Set SameSite to "Lax" which strikes a balance between security and cross-origin functionality.
+ // "Lax" allows the cookie to be sent on top-level navigations while providing some CSRF protection.
addCookieInitializer((builder) -> builder.sameSite(LAX).secure(true));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected void addCookieInitializers() { | |
| // Add the appropriate SameSite attribute based on the exchange attribute | |
| addCookieInitializer((builder) -> builder.sameSite(LAX).secure(true)); | |
| } | |
| protected void addCookieInitializers() { | |
| // Set SameSite to "Lax" which strikes a balance between security and cross-origin functionality. | |
| // "Lax" allows the cookie to be sent on top-level navigations while providing some CSRF protection. | |
| addCookieInitializer((builder) -> builder.sameSite(LAX).secure(true)); | |
| } |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13813722166
Commit: 888e9da
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 12 Mar 2025 16:07:33 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit