Skip to content
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.

Sign up for GitHub

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

chore: Custom cooke resolver and logout on session invalidation #39695

Merged
merged 3 commits into from
Mar 13, 2025
+58 −10
Merged

chore: Custom cooke resolver and logout on session invalidation #39695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ddb534
chore: Custom cooke resolver and logout on session invalidation
nidhi-nair Mar 12, 2025
f8dc52b
Conducive to splitting
nidhi-nair Mar 12, 2025
888e9da
Conducive to splitting
nidhi-nair Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...er/src/main/java/com/appsmith/server/configurations/CustomCookieWebSessionIdResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package com.appsmith.server.configurations;

import com.appsmith.server.configurations.ce.CustomCookieWebSessionIdResolverCE;

/**
* This class is a custom implementation of the CookieWebSessionIdResolver class.
* It allows us to set the SameSite attribute of the session cookie based on
* the organization configuration for cross site embedding.
*/
public class CustomCookieWebSessionIdResolver extends CustomCookieWebSessionIdResolverCE {}
11 changes: 1 addition & 10 deletions ...rver/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,9 @@
import org.springframework.web.server.ServerWebExchange;
import org.springframework.web.server.WebFilterChain;
import org.springframework.web.server.adapter.ForwardedHeaderTransformer;
import org.springframework.web.server.session.CookieWebSessionIdResolver;
import org.springframework.web.server.session.WebSessionIdResolver;
import reactor.core.publisher.Mono;

import java.time.Duration;
import java.util.HashSet;
import java.util.List;

Expand All @@ -73,7 +71,6 @@
import static com.appsmith.server.constants.Url.USAGE_PULSE_URL;
import static com.appsmith.server.constants.Url.USER_URL;
import static com.appsmith.server.constants.ce.UrlCE.CONSOLIDATED_API_URL;
import static java.time.temporal.ChronoUnit.DAYS;

@EnableWebFluxSecurity
@EnableReactiveMethodSecurity
Expand Down Expand Up @@ -271,13 +268,7 @@ public SecurityWebFilterChain securityWebFilterChain(ServerHttpSecurity http) {
*/
@Bean
public WebSessionIdResolver webSessionIdResolver() {
CookieWebSessionIdResolver resolver = new CookieWebSessionIdResolver();
// Setting the max age to 30 days so that the cookie doesn't expire on browser close
// If the max age is not set, some browsers will default to deleting the cookies on session close.
resolver.setCookieMaxAge(Duration.of(30, DAYS));
resolver.addCookieInitializer((builder) -> builder.path("/"));
resolver.addCookieInitializer((builder) -> builder.sameSite("Lax"));
return resolver;
return new CustomCookieWebSessionIdResolver();
}

private User createAnonymousUser() {
Expand Down
38 changes: 38 additions & 0 deletions ...c/main/java/com/appsmith/server/configurations/ce/CustomCookieWebSessionIdResolverCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package com.appsmith.server.configurations.ce;

import org.springframework.web.server.ServerWebExchange;
import org.springframework.web.server.session.CookieWebSessionIdResolver;

import java.time.Duration;

import static java.time.temporal.ChronoUnit.DAYS;

/**
* This class is a custom implementation of the CookieWebSessionIdResolver class.
* It allows us to set the SameSite attribute of the session cookie based on
* the organization configuration for cross site embedding.
*/
public class CustomCookieWebSessionIdResolverCE extends CookieWebSessionIdResolver {

public static final String LAX = "Lax";

public CustomCookieWebSessionIdResolverCE() {
// Set default cookie attributes

// Setting the max age to 30 days so that the cookie doesn't expire on browser close
// If the max age is not set, some browsers will default to deleting the cookies on session close.
this.setCookieMaxAge(Duration.of(30, DAYS));
this.addCookieInitializer((builder) -> builder.path("/"));
}

@Override
public void setSessionId(ServerWebExchange exchange, String id) {
addCookieInitializers();
super.setSessionId(exchange, id);
}

protected void addCookieInitializers() {
// Add the appropriate SameSite attribute based on the exchange attribute
addCookieInitializer((builder) -> builder.sameSite(LAX).secure(true));
}
Comment on lines +34 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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:

  1. Update the implementation to actually use the exchange attribute
  2. 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.

Suggested change
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));
}

}
9 changes: 9 additions & 0 deletions .../appsmith-server/src/main/java/com/appsmith/server/exceptions/GlobalExceptionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ public class GlobalExceptionHandler {

private final SessionUserService sessionUserService;

@ExceptionHandler(IllegalStateException.class)
public Mono<Void> handleSessionInvalidation(ServerWebExchange exchange, IllegalStateException ex) {
if (ex.getMessage().contains("Session was invalidated")) {
exchange.getResponse().setStatusCode(HttpStatus.UNAUTHORIZED);
return exchange.getResponse().setComplete();
}
return Mono.error(ex);
}

/**
* This function only catches the AppsmithException type and formats it into ResponseEntity<ErrorDTO> object
* Ideally, we should only be throwing AppsmithException from our code. This ensures that we can standardize
Expand Down