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.

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

fix: Use Spring's native CSRF protection, and fix login page #37292

Draft
wants to merge 12 commits into
base: release
Choose a base branch
from
2 changes: 2 additions & 0 deletions app/client/cypress/support/commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ Cypress.Commands.add("LoginFromAPI", (uname, pword) => {
url: "api/v1/login",
headers: {
origin: baseURL,
"X-Requested-By": "Appsmith",
sharat87 marked this conversation as resolved.
Show resolved Hide resolved
},
followRedirect: true,
body: {
Expand Down Expand Up @@ -939,6 +940,7 @@ Cypress.Commands.add("SignupFromAPI", (uname, pword) => {
url: "api/v1/users",
headers: {
"content-type": "application/json",
"X-Requested-By": "Appsmith",
},
followRedirect: false,
form: true,
Expand Down
8 changes: 8 additions & 0 deletions app/client/src/pages/UserAuth/CsrfTokenInput.tsx
Original file line number Diff line number Diff line change
@@ -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} />;
}
3 changes: 3 additions & 0 deletions app/client/src/pages/UserAuth/Login.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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] || "";
Expand Down Expand Up @@ -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)}
Expand Down
2 changes: 2 additions & 0 deletions app/client/src/pages/UserAuth/SignUp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)}
Expand Down
2 changes: 2 additions & 0 deletions app/client/src/pages/setup/DetailsForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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 {}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import com.appsmith.server.constants.Url;
import com.appsmith.server.domains.User;
import com.appsmith.server.dtos.ResponseDTO;
import com.appsmith.server.filters.CSRFFilter;
import com.appsmith.server.filters.ConditionalFilter;
import com.appsmith.server.filters.LoginRateLimitFilter;
import com.appsmith.server.helpers.RedirectHelper;
Expand Down Expand Up @@ -112,6 +111,9 @@ public class SecurityConfig {
@Autowired
private CustomOauth2ClientRepositoryManager oauth2ClientManager;

@Autowired
private CsrfConfig csrfConfig;

@Value("${appsmith.internal.password}")
private String INTERNAL_PASSWORD;

Expand Down Expand Up @@ -164,10 +166,9 @@ public SecurityWebFilterChain securityWebFilterChain(ServerHttpSecurity http) {
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
Expand All @@ -185,7 +186,6 @@ public SecurityWebFilterChain securityWebFilterChain(ServerHttpSecurity http) {
.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"),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
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";

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 (X_REQUESTED_BY_VALUE.equals(headers.getFirst(X_REQUESTED_BY_NAME))) {
// If `X-Request-By: Appsmith` header is present, CSRF check isn't needed.
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;
}
}

This file was deleted.

Loading
Loading