fix: move cookie sameSite initializer to constructor to prevent OverflowError#41575
fix: move cookie sameSite initializer to constructor to prevent OverflowError#41575wyattwalter wants to merge 1 commit intoreleasefrom
Conversation
…OverflowError addCookieInitializer() was called on every setSessionId() invocation, permanently chaining Consumer.andThen() on the singleton bean's initializer field. After enough session-setting requests, the chain depth exceeded the JVM stack size, crashing SSO login with a StackOverflowError. Move the sameSite(Lax) initializer to the constructor so it's set once. Leave addCookieInitializers(exchange) as a no-op hook for EE subclasses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughA cookie configuration refactoring in the session ID resolver moves SameSite(LAX) attribute initialization from per-request hooks to the constructor, converting the per-request method to an explicit no-op with documentation to prevent unbounded Consumer chaining. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java (1)
35-46: Javadoc should clarify that per-request customization requires overridingsetSessionId, not using theaddCookieInitializershook.The hook is invoked before
super.setSessionId()(line 31–32), but that's when response cookies are actually built and set. The Javadoc warns against callingaddCookieInitializerhere (correct), then says "Instead, modify cookies on the response aftersuper.setSessionId()builds them" — but that action is impossible from within this hook. The guidance implicitly describes overridingsetSessionIditself, which should be made explicit to prevent confusion.EE currently does not override this hook, so no migration action is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java` around lines 35 - 46, The Javadoc is misleading about how to perform per-request cookie customization: update the comment on CustomCookieWebSessionIdResolverCE::addCookieInitializers to explicitly state that per-request modifications must be done by overriding setSessionId (not by calling addCookieInitializer here), because addCookieInitializers is invoked before super.setSessionId() builds/sets response cookies and therefore cannot modify the final response cookies; reference addCookieInitializer, addCookieInitializers and setSessionId in the text so readers know which methods to use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java`:
- Around line 35-46: The Javadoc is misleading about how to perform per-request
cookie customization: update the comment on
CustomCookieWebSessionIdResolverCE::addCookieInitializers to explicitly state
that per-request modifications must be done by overriding setSessionId (not by
calling addCookieInitializer here), because addCookieInitializers is invoked
before super.setSessionId() builds/sets response cookies and therefore cannot
modify the final response cookies; reference addCookieInitializer,
addCookieInitializers and setSessionId in the text so readers know which methods
to use.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java
| * {@link StackOverflowError}. Instead, modify cookies on the response after | ||
| * {@code super.setSessionId()} builds them. | ||
| */ | ||
| protected void addCookieInitializers(ServerWebExchange exchange) { |
There was a problem hiding this comment.
In the EE pr this method has been completely removed. Might as well remove from here unless it needs to be overridden.
3 participants
addCookieInitializer() was called on every setSessionId() invocation, permanently chaining Consumer.andThen() on the singleton bean's initializer field. After enough session-setting requests, the chain depth exceeded the JVM stack size, crashing SSO login with a StackOverflowError.
Move the sameSite(Lax) initializer to the constructor so it's set once. Leave addCookieInitializers(exchange) as a no-op hook for EE subclasses.
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=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit