fix: Use Spring's native CSRF protection, and fix login page (#37292)
## 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 ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13697073430> > Commit: 0873799e2346e58dac3d59b1a3890b86ab17d5b4 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13697073430&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 06 Mar 2025 12:13:19 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
parent
03e4470c66
commit
32ed0ac9ad
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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: {
|
||||
|
|
|
|||
|
|
@ -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) => {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
};
|
||||
|
|
@ -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<InternalAxiosRequestConfig>(
|
||||
blockAirgappedRoutes,
|
||||
addRequestedByHeader,
|
||||
addVersionHeader,
|
||||
addGitBranchHeader,
|
||||
increaseGitApiTimeout,
|
||||
|
|
|
|||
|
|
@ -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";
|
||||
|
|
|
|||
8
app/client/src/pages/UserAuth/CsrfTokenInput.tsx
Normal file
8
app/client/src/pages/UserAuth/CsrfTokenInput.tsx
Normal file
|
|
@ -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 <input name="_csrf" type="hidden" value={csrfToken} />;
|
||||
}
|
||||
|
|
@ -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 && (
|
||||
<EmailFormWrapper>
|
||||
<SpacedSubmitForm action={loginURL} method="POST">
|
||||
<CsrfTokenInput />
|
||||
<FormGroup
|
||||
intent={error ? "danger" : "none"}
|
||||
label={createMessage(LOGIN_PAGE_EMAIL_INPUT_LABEL)}
|
||||
|
|
|
|||
|
|
@ -59,6 +59,7 @@ import log from "loglevel";
|
|||
import { SELF_HOSTING_DOC } from "constants/ThirdPartyConstants";
|
||||
import * as Sentry from "@sentry/react";
|
||||
import { Severity } from "@sentry/react";
|
||||
import CsrfTokenInput from "pages/UserAuth/CsrfTokenInput";
|
||||
|
||||
declare global {
|
||||
interface Window {
|
||||
|
|
@ -243,6 +244,7 @@ export function SignUp(props: SignUpFormProps) {
|
|||
method="POST"
|
||||
onSubmit={(e) => handleSubmit(e)}
|
||||
>
|
||||
<CsrfTokenInput />
|
||||
<FormGroup
|
||||
intent={error ? "danger" : "none"}
|
||||
label={createMessage(SIGNUP_PAGE_EMAIL_INPUT_LABEL)}
|
||||
|
|
|
|||
|
|
@ -24,6 +24,7 @@ import { isAirgapped } from "ee/utils/airgapHelpers";
|
|||
import { setFirstTimeUserOnboardingTelemetryCalloutVisibility } from "utils/storage";
|
||||
import RadioButtonGroup from "components/editorComponents/RadioButtonGroup";
|
||||
import { Space } from "./NonSuperUserProfilingQuestions";
|
||||
import CsrfTokenInput from "../UserAuth/CsrfTokenInput";
|
||||
|
||||
export interface DetailsFormValues {
|
||||
firstName?: string;
|
||||
|
|
@ -105,6 +106,7 @@ export default function DetailsForm(
|
|||
className={props.isFirstPage ? "block" : "hidden"}
|
||||
data-testid="formPage"
|
||||
>
|
||||
<CsrfTokenInput />
|
||||
<div className="flex flex-row justify-between w-100">
|
||||
<StyledFormGroup className="!w-52 t--welcome-form-first-name">
|
||||
<FormTextField
|
||||
|
|
|
|||
|
|
@ -10,9 +10,7 @@ import { getInstanceId } from "ee/selectors/organizationSelectors";
|
|||
|
||||
//TODO (Dipyaman): We should return a promise that will get resolved only on success or rejected after the retries
|
||||
export const fetchWithRetry = (config: {
|
||||
// TODO: Fix this the next time the file is edited
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
url: any;
|
||||
url: string;
|
||||
payload: Record<string, unknown>;
|
||||
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,
|
||||
|
|
|
|||
|
|
@ -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 {}
|
||||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
* <ol>
|
||||
* <li>The CSRFSpec customizer, or in English, responsible for configuring CSRF for our API. Handled by the {@link #customize} method.
|
||||
* <li>A request matcher, responsible for deciding CSRF token should be checked. Handled by the {@link #matches} method.
|
||||
* <li>A WebFilter, that ensures the CSRF token Mono actually gets subscribed to. Handled by the {@link #filter} method.
|
||||
* </ol>
|
||||
* References:
|
||||
* <ul>
|
||||
* <li><a href="https://docs.spring.io/spring-security/reference/servlet/exploits/csrf.html">Spring Security on CSRF</a>
|
||||
* <li><a href="https://docs.spring.io/spring-security/reference/reactive/exploits/csrf.html">Spring Security on CSRF with WebFlux</a>
|
||||
* <li><a href="https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html">
|
||||
* OWASP on CSRF</a>
|
||||
*/
|
||||
@RequiredArgsConstructor
|
||||
public class CsrfConfigCE implements Customizer<ServerHttpSecurity.CsrfSpec>, 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<ServerWebExchangeMatcher.MatchResult> 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<Void> 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<CsrfToken> 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.
|
||||
* <p>
|
||||
* 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.
|
||||
* <p>
|
||||
* 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<CsrfToken> generateToken(ServerWebExchange exchange) {
|
||||
return delegate.generateToken(exchange);
|
||||
}
|
||||
|
||||
@Override
|
||||
public Mono<Void> 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<CsrfToken> loadToken(ServerWebExchange exchange) {
|
||||
return delegate.loadToken(exchange);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Override to add exemptions.
|
||||
*/
|
||||
protected static boolean isUrlExemptedFromCsrf(@NonNull String urlPath) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
|
@ -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<String> 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<Void> 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);
|
||||
}
|
||||
}
|
||||
|
|
@ -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<String> 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<Arguments> testParams() {
|
||||
final List<String> urls = List.of(Url.LOGIN_URL, Url.USER_URL, Url.USER_URL + "/super");
|
||||
|
||||
final List<String> formParams = new ArrayList<>(List.of("_csrf", "_wrong_csrf"));
|
||||
formParams.add(null);
|
||||
|
||||
final List<String> cookieNames = new ArrayList<>(List.of("XSRF-TOKEN", "SOMETHING-ELSE"));
|
||||
cookieNames.add(null);
|
||||
|
||||
List<Arguments> 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();
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user