From 32ed0ac9adba72d0ce6ef079d70a8293f361e33f Mon Sep 17 00:00:00 2001 From: Shrikant Sharat Kandula Date: Sat, 8 Mar 2025 21:01:02 +0530 Subject: [PATCH] fix: Use Spring's native CSRF protection, and fix login page (#37292) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description The login page, and a few other pages are exempted from CSRF today and aren't doing any check at all. This makes our login page vulnerable to CSRF. But it's not really exploitable in its current form, since there's other points in the login flow that patch this hole up. Nevertheless, CSRF being possible on the login form doesn't sound good in any tone and context. This PR fixes this by not exempting _anything_ from CSRF, and doing a stateless CSRF check where necessary. PR summary: 1. Switches from our home-built CSRF filter implementation to Spring's native implementation. 2. Login form and a few others were previously exempted from CSRF checks, and now that exemption is gone. This is why we need the `X-Requested-By: Appsmith` for the login/signup form submission calls from Cypress. 3. Removes the check on `Content-Type: application/json` header. Previously, if a request had this header, it was considered exempt from CSRF check. This has been removed as it appears it's not a safe assumption in today's JSON-dominated web. ⚠️ verify SCIM flow before merging. ## Automation /test all ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: 0873799e2346e58dac3d59b1a3890b86ab17d5b4 > Cypress dashboard. > Tags: `@tag.All` > Spec: >
Thu, 06 Mar 2025 12:13:19 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced a `CsrfTokenInput` component to enhance security during user authentication and signup processes by including a CSRF token. - **Improvements** - Enhanced API request headers for login and signup commands to improve security. - Added cookie validation for successful login to ensure session integrity. - Improved error handling for database operations. - **Bug Fixes** - Removed outdated CSRF filter to streamline CSRF protection handling in the application. - **Tests** - Added comprehensive unit tests for CSRF protection to ensure correct behavior under various scenarios. - Introduced a new test suite for testing CSRF logout and login functionality. --- .../Git/GitSync/MergeViaRemote_spec.ts | 11 +- .../OtherUIFeatures/ApplicationURL_spec.js | 11 +- app/client/cypress/support/Pages/HomePage.ts | 6 + app/client/cypress/support/commands.js | 4 + .../__tests__/apiRequestInterceptors.test.ts | 12 -- .../request/addRequestedByHeader.ts | 13 -- .../request/apiRequestInterceptor.ts | 2 - .../src/api/interceptors/request/index.ts | 2 +- .../src/pages/UserAuth/CsrfTokenInput.tsx | 8 + app/client/src/pages/UserAuth/Login.tsx | 3 + app/client/src/pages/UserAuth/SignUp.tsx | 2 + app/client/src/pages/setup/DetailsForm.tsx | 2 + app/client/src/usagePulse/utils.ts | 5 +- .../server/configurations/CsrfConfig.java | 9 + .../server/configurations/SecurityConfig.java | 12 +- .../configurations/ce/CsrfConfigCE.java | 159 ++++++++++++++++++ .../appsmith/server/filters/CSRFFilter.java | 66 -------- .../server/configurations/CsrfTest.java | 91 ++++++++++ 18 files changed, 311 insertions(+), 107 deletions(-) delete mode 100644 app/client/src/api/interceptors/request/addRequestedByHeader.ts create mode 100644 app/client/src/pages/UserAuth/CsrfTokenInput.tsx create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java create mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CsrfConfigCE.java delete mode 100644 app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java create mode 100644 app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java diff --git a/app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts index 469bafcda0..25ee67ca34 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts +++ b/app/client/cypress/e2e/Regression/ClientSide/Git/GitSync/MergeViaRemote_spec.ts @@ -89,8 +89,15 @@ describe( expect(location.pathname).includes(newPathname); }); - cy.request("PUT", `/api/v1/applications/${applicationId}`, { - applicationVersion: 1, + cy.request({ + method: "PUT", + url: `/api/v1/applications/${applicationId}`, + headers: { + "X-Requested-By": "Appsmith", + }, + body: { + applicationVersion: 1, + }, }); _.gitSync.CreateGitBranch(cleanUrlBranch, true); diff --git a/app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js b/app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js index ffa6ccfaee..d93525a20f 100644 --- a/app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js +++ b/app/client/cypress/e2e/Regression/ClientSide/OtherUIFeatures/ApplicationURL_spec.js @@ -61,8 +61,15 @@ describe("Slug URLs", { tags: ["@tag.AppUrl"] }, () => { }); it("3. Check the url of old applications, upgrades version and compares appsmith.URL values", () => { - cy.request("PUT", `/api/v1/applications/${applicationId}`, { - applicationVersion: 1, + cy.request({ + method: "PUT", + url: `/api/v1/applications/${applicationId}`, + headers: { + "X-Requested-By": "Appsmith", + }, + body: { + applicationVersion: 1, + }, }).then((response) => { const application = response.body.data; expect(application.applicationVersion).to.equal(1); diff --git a/app/client/cypress/support/Pages/HomePage.ts b/app/client/cypress/support/Pages/HomePage.ts index 5d75b7fed0..ea5c7c53ac 100644 --- a/app/client/cypress/support/Pages/HomePage.ts +++ b/app/client/cypress/support/Pages/HomePage.ts @@ -358,6 +358,9 @@ export class HomePage { //Maps to LogOut in command.js public LogOutviaAPI() { + // We explicitly clear the `XSRF-TOKEN` cookie since Cypress often sends a stale cookie value without updating it to + // the correct value. Ref: https://github.com/cypress-io/cypress/issues/25841. + cy.clearCookie("XSRF-TOKEN"); cy.request({ method: "POST", url: "/api/v1/logout", @@ -397,6 +400,9 @@ export class HomePage { } public InvokeDispatchOnStore() { + // We explicitly clear the `XSRF-TOKEN` cookie since Cypress often sends a stale cookie value without updating it to + // the correct value. Ref: https://github.com/cypress-io/cypress/issues/25841. + cy.clearCookie("XSRF-TOKEN"); cy.window().then((win: any) => { if (win && win.store) { cy.window() diff --git a/app/client/cypress/support/commands.js b/app/client/cypress/support/commands.js index 96dba3434e..5d25d292ee 100644 --- a/app/client/cypress/support/commands.js +++ b/app/client/cypress/support/commands.js @@ -184,6 +184,7 @@ Cypress.Commands.add("LoginFromAPI", (uname, pword) => { url: "api/v1/login", headers: { origin: baseURL, + "X-Requested-By": "Appsmith", }, followRedirect: true, body: { @@ -938,6 +939,9 @@ Cypress.Commands.add("SignupFromAPI", (uname, pword) => { cy.request({ method: "POST", url: "api/v1/users", + headers: { + "X-Requested-By": "Appsmith", + }, followRedirect: false, form: true, body: { diff --git a/app/client/src/api/__tests__/apiRequestInterceptors.test.ts b/app/client/src/api/__tests__/apiRequestInterceptors.test.ts index 8a90efc3d4..d8c500f751 100644 --- a/app/client/src/api/__tests__/apiRequestInterceptors.test.ts +++ b/app/client/src/api/__tests__/apiRequestInterceptors.test.ts @@ -2,7 +2,6 @@ import axios from "axios"; import { addGitBranchHeader, blockAirgappedRoutes, - addRequestedByHeader, addEnvironmentHeader, increaseGitApiTimeout, addAnonymousUserIdHeader, @@ -57,17 +56,6 @@ describe("Api request interceptors", () => { axios.interceptors.request.eject(identifier); }); - it("checks if the request config has csrfToken in the headers", async () => { - const url = "v1/actions/execute"; - const identifier = axios.interceptors.request.use(addRequestedByHeader); - const response = await axios.post(url); - - expect(response.config.headers).toHaveProperty("X-Requested-By"); - expect(response.config.headers["X-Requested-By"]).toBe("Appsmith"); - - axios.interceptors.request.eject(identifier); - }); - it("checks if the request config has gitBranch in the headers", async () => { const url = "v1/"; const identifier = axios.interceptors.request.use((config) => { diff --git a/app/client/src/api/interceptors/request/addRequestedByHeader.ts b/app/client/src/api/interceptors/request/addRequestedByHeader.ts deleted file mode 100644 index c64a363dc8..0000000000 --- a/app/client/src/api/interceptors/request/addRequestedByHeader.ts +++ /dev/null @@ -1,13 +0,0 @@ -import type { InternalAxiosRequestConfig } from "axios"; - -export const addRequestedByHeader = (config: InternalAxiosRequestConfig) => { - config.headers = config.headers || {}; - - const methodUpper = config.method?.toUpperCase(); - - if (methodUpper && methodUpper !== "GET" && methodUpper !== "HEAD") { - config.headers["X-Requested-By"] = "Appsmith"; - } - - return config; -}; diff --git a/app/client/src/api/interceptors/request/apiRequestInterceptor.ts b/app/client/src/api/interceptors/request/apiRequestInterceptor.ts index 4a9d2ed633..c91e6dc595 100644 --- a/app/client/src/api/interceptors/request/apiRequestInterceptor.ts +++ b/app/client/src/api/interceptors/request/apiRequestInterceptor.ts @@ -6,7 +6,6 @@ import { isAirgapped } from "ee/utils/airgapHelpers"; import type { InternalAxiosRequestConfig } from "axios"; import getQueryParamsObject from "utils/getQueryParamsObject"; -import { addRequestedByHeader } from "./addRequestedByHeader"; import { increaseGitApiTimeout } from "./increaseGitApiTimeout"; import { getCurrentGitBranch } from "selectors/gitSyncSelectors"; import { addVersionHeader as _addVersionHeader } from "./addVersionHeader"; @@ -61,7 +60,6 @@ const addVersionHeader = (config: InternalAxiosRequestConfig) => { export const apiRequestInterceptor = (config: InternalAxiosRequestConfig) => { const interceptorPipeline = compose( blockAirgappedRoutes, - addRequestedByHeader, addVersionHeader, addGitBranchHeader, increaseGitApiTimeout, diff --git a/app/client/src/api/interceptors/request/index.ts b/app/client/src/api/interceptors/request/index.ts index 6f9957f00e..8086faeb54 100644 --- a/app/client/src/api/interceptors/request/index.ts +++ b/app/client/src/api/interceptors/request/index.ts @@ -1,6 +1,6 @@ export { addGitBranchHeader } from "./addGitBranchHeader"; export { blockAirgappedRoutes } from "./blockAirgappedRoutes"; -export { addRequestedByHeader } from "./addRequestedByHeader"; +export { addVersionHeader } from "./addVersionHeader"; export { addEnvironmentHeader } from "./addEnvironmentHeader"; export { apiRequestInterceptor } from "./apiRequestInterceptor"; export { increaseGitApiTimeout } from "./increaseGitApiTimeout"; diff --git a/app/client/src/pages/UserAuth/CsrfTokenInput.tsx b/app/client/src/pages/UserAuth/CsrfTokenInput.tsx new file mode 100644 index 0000000000..3620d4f736 --- /dev/null +++ b/app/client/src/pages/UserAuth/CsrfTokenInput.tsx @@ -0,0 +1,8 @@ +import React from "react"; + +export default function CsrfTokenInput() { + const csrfToken: string = + document.cookie.match(/\bXSRF-TOKEN=([-a-z0-9]+)/i)?.[1] ?? ""; + + return ; +} diff --git a/app/client/src/pages/UserAuth/Login.tsx b/app/client/src/pages/UserAuth/Login.tsx index fcac3e210b..9b0de89119 100644 --- a/app/client/src/pages/UserAuth/Login.tsx +++ b/app/client/src/pages/UserAuth/Login.tsx @@ -53,6 +53,8 @@ import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; import { getHTMLPageTitle } from "ee/utils/BusinessFeatures/brandingPageHelpers"; import * as Sentry from "@sentry/react"; import { Severity } from "@sentry/react"; +import CsrfTokenInput from "pages/UserAuth/CsrfTokenInput"; + const validate = (values: LoginFormValues, props: ValidateProps) => { const errors: LoginFormValues = {}; const email = values[LOGIN_FORM_EMAIL_FIELD_NAME] || ""; @@ -184,6 +186,7 @@ export function Login(props: LoginFormProps) { {isFormLoginEnabled && ( + handleSubmit(e)} > + +
; retries: number; retryTimeout: number; @@ -25,6 +23,7 @@ export const fetchWithRetry = (config: { credentials: "same-origin", headers: { "Content-Type": "application/json", + "X-Requested-By": "Appsmith", }, body: JSON.stringify(config.payload), keepalive: true, diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java new file mode 100644 index 0000000000..d20a269c14 --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java @@ -0,0 +1,9 @@ +package com.appsmith.server.configurations; + +import com.appsmith.server.configurations.ce.CsrfConfigCE; +import lombok.RequiredArgsConstructor; +import org.springframework.stereotype.Component; + +@Component +@RequiredArgsConstructor +public class CsrfConfig extends CsrfConfigCE {} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java index ecf53ac70d..8143204515 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java @@ -10,7 +10,6 @@ import com.appsmith.server.constants.Url; import com.appsmith.server.domains.User; import com.appsmith.server.dtos.ResponseDTO; import com.appsmith.server.exceptions.AppsmithErrorCode; -import com.appsmith.server.filters.CSRFFilter; import com.appsmith.server.filters.ConditionalFilter; import com.appsmith.server.filters.LoginRateLimitFilter; import com.appsmith.server.helpers.RedirectHelper; @@ -120,6 +119,9 @@ public class SecurityConfig { @Autowired private ProjectProperties projectProperties; + @Autowired + private CsrfConfig csrfConfig; + @Value("${appsmith.internal.password}") private String INTERNAL_PASSWORD; @@ -172,10 +174,9 @@ public class SecurityConfig { ServerAuthenticationEntryPointFailureHandler failureHandler = new ServerAuthenticationEntryPointFailureHandler(authenticationEntryPoint); + csrfConfig.applyTo(http); + return http.addFilterAt(this::sanityCheckFilter, SecurityWebFiltersOrder.FIRST) - // The native CSRF solution doesn't work with WebFlux, yet, but only for WebMVC. So we make our own. - .csrf(csrfSpec -> csrfSpec.disable()) - .addFilterAt(new CSRFFilter(objectMapper), SecurityWebFiltersOrder.CSRF) // Default security headers configuration from // https://docs.spring.io/spring-security/site/docs/5.0.x/reference/html/headers.html .headers(headerSpec -> headerSpec @@ -193,7 +194,6 @@ public class SecurityConfig { .authorizeExchange(authorizeExchangeSpec -> authorizeExchangeSpec // The following endpoints are allowed to be accessed without authentication .matchers( - ServerWebExchangeMatchers.pathMatchers(HttpMethod.GET, Url.LOGIN_URL), ServerWebExchangeMatchers.pathMatchers(HttpMethod.GET, Url.HEALTH_CHECK), ServerWebExchangeMatchers.pathMatchers(HttpMethod.POST, USER_URL), ServerWebExchangeMatchers.pathMatchers(HttpMethod.POST, USER_URL + "/super"), @@ -309,7 +309,7 @@ public class SecurityConfig { } // 3. Check Appsmith version, if present. Not making this a mandatory check for now, but reconsider later. - final String versionHeaderValue = headers.getFirst("X-Appsmith-Version"); + final String versionHeaderValue = headers.getFirst(CsrfConfig.VERSION_HEADER); final String serverVersion = projectProperties.getVersion(); if (versionHeaderValue != null && !serverVersion.equals(versionHeaderValue)) { final ErrorDTO error = new ErrorDTO( diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CsrfConfigCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CsrfConfigCE.java new file mode 100644 index 0000000000..25ddbe78cd --- /dev/null +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ce/CsrfConfigCE.java @@ -0,0 +1,159 @@ +package com.appsmith.server.configurations.ce; + +import lombok.NonNull; +import lombok.RequiredArgsConstructor; +import org.apache.commons.lang3.ObjectUtils; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpMethod; +import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.security.config.Customizer; +import org.springframework.security.config.web.server.SecurityWebFiltersOrder; +import org.springframework.security.config.web.server.ServerHttpSecurity; +import org.springframework.security.web.server.csrf.CookieServerCsrfTokenRepository; +import org.springframework.security.web.server.csrf.CsrfServerLogoutHandler; +import org.springframework.security.web.server.csrf.CsrfToken; +import org.springframework.security.web.server.csrf.DefaultCsrfToken; +import org.springframework.security.web.server.csrf.ServerCsrfTokenRepository; +import org.springframework.security.web.server.csrf.ServerCsrfTokenRequestAttributeHandler; +import org.springframework.security.web.server.util.matcher.ServerWebExchangeMatcher; +import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.WebFilter; +import org.springframework.web.server.WebFilterChain; +import reactor.core.publisher.Mono; + +import java.util.UUID; + +/** + * This component has three purposes: + *
    + *
  1. The CSRFSpec customizer, or in English, responsible for configuring CSRF for our API. Handled by the {@link #customize} method. + *
  2. A request matcher, responsible for deciding CSRF token should be checked. Handled by the {@link #matches} method. + *
  3. A WebFilter, that ensures the CSRF token Mono actually gets subscribed to. Handled by the {@link #filter} method. + *
+ * References: + *
    + *
  • Spring Security on CSRF + *
  • Spring Security on CSRF with WebFlux + *
  • + * OWASP on CSRF + */ +@RequiredArgsConstructor +public class CsrfConfigCE implements Customizer, ServerWebExchangeMatcher, WebFilter { + + @SuppressWarnings("UastIncorrectHttpHeaderInspection") + private static final String X_REQUESTED_BY_NAME = "X-Requested-By"; + + private static final String X_REQUESTED_BY_VALUE = "Appsmith"; + + @SuppressWarnings("UastIncorrectHttpHeaderInspection") + public static final String VERSION_HEADER = "X-Appsmith-Version"; + + public void applyTo(ServerHttpSecurity http) { + http.csrf(this).addFilterAfter(this, SecurityWebFiltersOrder.CSRF); + } + + @Override + public void customize(@NonNull ServerHttpSecurity.CsrfSpec spec) { + spec.requireCsrfProtectionMatcher(this) + .csrfTokenRepository(new Repository()) + // TODO: This shouldn't be necessary. This is apparently weaker than the default and recommended option, + // `XorServerCsrfTokenRequestAttributeHandler`. Figure out a way to switch to the default. + .csrfTokenRequestHandler(new ServerCsrfTokenRequestAttributeHandler()); + } + + @Override + public Mono matches(@NonNull ServerWebExchange exchange) { + final ServerHttpRequest request = exchange.getRequest(); + final HttpMethod method = request.getMethod(); + + if (HttpMethod.GET.equals(method) || HttpMethod.HEAD.equals(method)) { + // Semantically, GET requests should be handled as read-only requests, and assuming that is true, they are + // safe from CSRF. While it looks like it's no-harm doing the CSRF token check for "GET" requests also, it + // means we can't simply open these endpoints in the browser and see the response. This can seriously + // inhibit troubleshooting and developer convenience. + // This is why it's important to ensure GET handlers don't have significant side effects. + // Ref: + // https://docs.spring.io/spring-security/reference/features/exploits/csrf.html#csrf-protection-read-only + return ServerWebExchangeMatcher.MatchResult.notMatch(); + } + + if (HttpMethod.POST.equals(method) + && isUrlExemptedFromCsrf(request.getPath().value())) { + // We put this check exactly here, so that only POST requests can ever be exempted. This is intentional. + return ServerWebExchangeMatcher.MatchResult.notMatch(); + } + + final HttpHeaders headers = request.getHeaders(); + + if (headers.containsKey(VERSION_HEADER)) { + // If `X-Appsmith-Version` header is present, CSRF check isn't needed. + return ServerWebExchangeMatcher.MatchResult.notMatch(); + } + + if (X_REQUESTED_BY_VALUE.equals(headers.getFirst(X_REQUESTED_BY_NAME))) { + // If `X-Request-By: Appsmith` header is present, CSRF check isn't needed. + // Using this header for requests in new code is discouraged. Consider using `X-Appsmith-Version` instead. + return ServerWebExchangeMatcher.MatchResult.notMatch(); + } + + // Require a CSRF token check for any request that falls through here. + return ServerWebExchangeMatcher.MatchResult.match(); + } + + @Override + public Mono filter(@NonNull ServerWebExchange exchange, @NonNull WebFilterChain chain) { + // It _looks_ like we aren't doing anything here, but we very much are. + // The CSRF Token Mono needs to be subscribed to, for the CSRF token to be added to a `Set-Cookie` header. If + // this Mono is not subscribed to, the token won't be available to the client. + final Mono csrfTokenMono = + ObjectUtils.defaultIfNull(exchange.getAttribute(CsrfToken.class.getName()), Mono.empty()); + return csrfTokenMono.then(chain.filter(exchange)); + } + + /** + * This little Repository class might just as well be the default {@link CookieServerCsrfTokenRepository} itself. + * Two of three methods just delegate to an instance of that class. We delegate instead of extending because it's a + * final class. + *

    + * Problem: On logging out, the {@link CsrfServerLogoutHandler} calls the repository's {@link #saveToken} method, + * with a {@code null} token. This clears the {@code XSRF-TOKEN} cookie on the browser. Because of this, the client + * SPA is unable to load set a {@code _csrf} hidden input in the login form that shows up just after logout. The + * user has to refresh the page to get a new token in the cookie, and for the login form to work again. + *

    + * To solve for this, we override the behaviour of {@link #saveToken} when a {@code null} token is passed, and + * instead of clearing, we generate a new token to be saved in the cookie. With this, the login form that shows up + * just after a logout is able to capture the CSRF token, and so the form works fine. + */ + public static class Repository implements ServerCsrfTokenRepository { + private final CookieServerCsrfTokenRepository delegate = CookieServerCsrfTokenRepository.withHttpOnlyFalse(); + + @Override + public Mono generateToken(ServerWebExchange exchange) { + return delegate.generateToken(exchange); + } + + @Override + public Mono saveToken(ServerWebExchange exchange, CsrfToken token) { + if (token == null) { + // Called by CsrfLogoutHandler, which tries to clear the token. We want to regenerate, not clear. + token = new DefaultCsrfToken( + // Values taken from CookieServerCsrfTokenRepository + "X-XSRF-TOKEN", "_csrf", UUID.randomUUID().toString()); + } + + return delegate.saveToken(exchange, token); + } + + @Override + public Mono loadToken(ServerWebExchange exchange) { + return delegate.loadToken(exchange); + } + } + + /** + * Override to add exemptions. + */ + protected static boolean isUrlExemptedFromCsrf(@NonNull String urlPath) { + return false; + } +} diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java deleted file mode 100644 index fa289670ba..0000000000 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java +++ /dev/null @@ -1,66 +0,0 @@ -package com.appsmith.server.filters; - -import com.appsmith.server.constants.Url; -import com.appsmith.server.dtos.ResponseDTO; -import com.fasterxml.jackson.core.JsonProcessingException; -import com.fasterxml.jackson.databind.ObjectMapper; -import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; -import org.springframework.http.HttpMethod; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.http.server.reactive.ServerHttpRequest; -import org.springframework.http.server.reactive.ServerHttpResponse; -import org.springframework.web.server.ServerWebExchange; -import org.springframework.web.server.WebFilter; -import org.springframework.web.server.WebFilterChain; -import reactor.core.publisher.Mono; - -import java.util.Set; - -@Slf4j -@RequiredArgsConstructor -public class CSRFFilter implements WebFilter { - - private static final Set EXEMPT = Set.of( - Url.LOGIN_URL, - Url.USER_URL, // For signup request - Url.USER_URL + "/super", // For superuser signup request - Url.USER_URL + "/verifyEmailVerificationToken"); - - private static final String X_REQUESTED_BY_NAME = "X-Requested-By"; - private static final String X_REQUESTED_BY_VALUE = "Appsmith"; - - private final ObjectMapper objectMapper; - - @Override - public Mono filter(ServerWebExchange exchange, WebFilterChain chain) { - final ServerHttpRequest request = exchange.getRequest(); - final HttpMethod method = request.getMethod(); - final boolean isGetOrHead = HttpMethod.GET.equals(method) || HttpMethod.HEAD.equals(method); - - if (!isGetOrHead && !EXEMPT.contains(request.getPath().value())) { - // For POST requests, either a `X-Requested-By: Appsmith` header or a `Content-Type: application/json` - // is required. If neither is present, reject the request. This is to prevent CSRF attacks. - if (MediaType.APPLICATION_JSON.equals(request.getHeaders().getContentType()) - || X_REQUESTED_BY_VALUE.equals(request.getHeaders().getFirst(X_REQUESTED_BY_NAME))) { - return chain.filter(exchange); - } - - log.error("CSRF header requirements not satisfied to {}. Rejecting request.", request.getPath()); - ServerHttpResponse response = exchange.getResponse(); - response.setStatusCode(HttpStatus.FORBIDDEN); - response.getHeaders().set("Content-Type", MediaType.APPLICATION_JSON_VALUE); - try { - final byte[] bytes = objectMapper.writeValueAsBytes( - new ResponseDTO<>(HttpStatus.FORBIDDEN.value(), null, "Forbidden", false)); - return response.writeWith( - Mono.just(exchange.getResponse().bufferFactory().wrap(bytes))); - } catch (JsonProcessingException e) { - return Mono.error(e); - } - } - - return chain.filter(exchange); - } -} diff --git a/app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java b/app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java new file mode 100644 index 0000000000..c585096d35 --- /dev/null +++ b/app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java @@ -0,0 +1,91 @@ +package com.appsmith.server.configurations; + +import com.appsmith.server.constants.Url; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.web.reactive.context.ReactiveWebApplicationContext; +import org.springframework.http.HttpHeaders; +import org.springframework.http.MediaType; +import org.springframework.test.web.reactive.server.WebTestClient; +import org.springframework.web.reactive.function.BodyInserters; + +import java.util.ArrayList; +import java.util.List; +import java.util.UUID; +import java.util.stream.Stream; + +import static org.springframework.security.test.web.reactive.server.SecurityMockServerConfigurers.springSecurity; + +@SpringBootTest +public class CsrfTest { + private WebTestClient webTestClient; + + @BeforeEach + void setup(ReactiveWebApplicationContext context) { + webTestClient = WebTestClient.bindToApplicationContext(context) + .apply(springSecurity()) + .build(); + } + + @ParameterizedTest + @MethodSource("testParams") + void testCsrf(TestParams t) { + final String tokenValue = UUID.randomUUID().toString(); + + final WebTestClient.RequestBodySpec spec = webTestClient + .post() + .uri(t.url) + .header(HttpHeaders.ORIGIN, "localhost") + .contentType(MediaType.APPLICATION_FORM_URLENCODED); + + final BodyInserters.FormInserter body = + BodyInserters.fromFormData("username", "user@example.com").with("password", "password"); + if (t.formParam != null) { + body.with(t.formParam, tokenValue); + } + + if (t.cookieName != null) { + spec.cookie(t.cookieName, tokenValue); + } + + final WebTestClient.ResponseSpec response = spec.body(body).exchange(); + + if ("_csrf".equals(t.formParam) && "XSRF-TOKEN".equals(t.cookieName)) { + // Redirects to error because the username/password are incorrect, and that's okay. The fact that it + // attempted a login is test enough here. + response.expectStatus() + .is3xxRedirection() + .expectHeader() + .valueMatches(HttpHeaders.LOCATION, ".*\\?error=.*"); + } else { + response.expectStatus().isForbidden(); + } + } + + private record TestParams(String url, String formParam, String cookieName) {} + + private static Stream testParams() { + final List urls = List.of(Url.LOGIN_URL, Url.USER_URL, Url.USER_URL + "/super"); + + final List formParams = new ArrayList<>(List.of("_csrf", "_wrong_csrf")); + formParams.add(null); + + final List cookieNames = new ArrayList<>(List.of("XSRF-TOKEN", "SOMETHING-ELSE")); + cookieNames.add(null); + + List args = new ArrayList<>(); + + for (final String url : urls) { + for (final String formParam : formParams) { + for (final String cookieName : cookieNames) { + args.add(Arguments.of(new TestParams(url, formParam, cookieName))); + } + } + } + + return args.stream(); + } +}