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

Conversation

sharat87
Copy link
Member

@sharat87 sharat87 commented Nov 8, 2024

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.

Automation

/test all

🔍 Cypress test results

Caution

🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11774970530
Commit: abf1b77
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js
  2. cypress/e2e/Regression/ClientSide/IDE/Focus_retentions_inputs_spec.js
  3. cypress/e2e/Regression/ClientSide/Linting/BasicLint_spec.ts
List of identified flaky tests.
Mon, 11 Nov 2024 09:51:31 UTC

Communication

Should the DevRel and Marketing teams inform users about this change?

  • Yes
  • 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.

Copy link
Contributor

coderabbitai bot commented Nov 8, 2024

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes introduced in this pull request enhance the security and functionality of the application by adding CSRF protection and improving API request handling. Key modifications include the addition of headers in API commands, the introduction of a CSRF token input component in user authentication forms, and the implementation of a CSRF configuration class in the server-side code. Error handling and validation checks have also been improved across various components, ensuring a more robust and secure application.

Changes

File Change Summary
app/client/cypress/support/commands.js Added "X-Requested-By": "Appsmith" header to LoginFromAPI and SignupFromAPI commands; included cookie validation in LoginFromAPI; enhanced error handling in addIndexedDBKey; modified stubPostHeaderReq for origin header; assertions added to createSuperUser.
app/client/src/pages/UserAuth/CsrfTokenInput.tsx Introduced CsrfTokenInput component to retrieve and render a CSRF token input field.
app/client/src/pages/UserAuth/Login.tsx Integrated CsrfTokenInput into the Login component's form, enhancing security without altering existing logic.
app/client/src/pages/UserAuth/SignUp.tsx Added CsrfTokenInput to the SignUp form, maintaining existing submission logic while enhancing security.
app/client/src/pages/setup/DetailsForm.tsx Included CsrfTokenInput in the DetailsForm component, conditionally rendered based on props.isFirstPage.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java Created CsrfConfig class for CSRF protection, implementing necessary interfaces and methods for token management and request matching.
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java Updated SecurityConfig to include CsrfConfig, applying CSRF protection and removing the previous custom filter implementation.
app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java Removed CSRFFilter class, which previously handled CSRF protection.
app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java Added unit tests for CSRF protection, validating various scenarios with CSRF tokens and cookies.

Possibly related PRs

Suggested labels

Critical, Login / Signup

Suggested reviewers

  • nidhi-nair
  • abhvsn

🎉 In the realm of code, new tokens arise,
With CSRF guards, we strengthen the ties.
Headers added, commands refined,
A secure path for users, well-defined.
Through forms and tests, our efforts align,
In the world of Appsmith, security will shine! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the Bug Something isn't working label Nov 8, 2024
@sharat87 sharat87 added the ok-to-test Required label for CI label Nov 8, 2024
@sharat87 sharat87 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Nov 8, 2024
@appsmithorg appsmithorg deleted a comment from github-actions bot Nov 8, 2024
@sharat87 sharat87 changed the title fix: Remove exemptions from CSRF check fix: Use Spring's native CSRF protection, and fix login page Nov 9, 2024
@sharat87 sharat87 marked this pull request as ready for review November 9, 2024 02:54
@sharat87 sharat87 marked this pull request as draft November 9, 2024 02:56
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (9)
app/client/src/pages/UserAuth/CsrfTokenInput.tsx (1)

4-5: Consider using a more robust cookie parsing approach.

While the regex works, consider using a dedicated cookie parsing utility for better maintainability and reliability.

-const csrfToken: string =
-  document.cookie.match(/\bXSRF-TOKEN=([-a-z0-9]+)/i)?.[1] ?? "";
+import Cookies from 'js-cookie';
+
+const csrfToken: string = Cookies.get('XSRF-TOKEN') ?? "";
app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java (3)

26-31: Consider adding security headers verification.

While the setup is good, consider extending it to verify security headers in responses (X-Frame-Options, X-Content-Type-Options, etc.) as part of CSRF protection testing.

 @BeforeEach
 void setup(ReactiveWebApplicationContext context) {
     webTestClient = WebTestClient.bindToApplicationContext(context)
             .apply(springSecurity())
+            .configureClient()
+            .defaultHeaders(headers -> headers.add("X-Requested-By", "Appsmith"))
             .build();
 }

33-66: Add documentation for test scenarios.

The test method effectively covers various CSRF scenarios, but would benefit from explicit documentation of test cases.

 @ParameterizedTest
 @MethodSource("testParams")
+/**
+ * Tests CSRF protection with following scenarios:
+ * 1. Valid CSRF token in both form and cookie
+ * 2. Invalid CSRF token in form
+ * 3. Missing CSRF token in cookie
+ * 4. Mismatched tokens
+ */
 void testCsrf(TestParams t) {

36-36: Consider using SecureRandom for token generation.

Using UUID for CSRF tokens is acceptable but consider using SecureRandom for stronger token generation in security-critical tests.

-final String tokenValue = UUID.randomUUID().toString();
+final String tokenValue = new BigInteger(130, new SecureRandom()).toString(32);
app/client/src/pages/setup/DetailsForm.tsx (1)

109-109: Consider adding error handling for missing CSRF token.

The CsrfTokenInput is rendered unconditionally. Consider adding error handling in case the CSRF token cookie is not present.

-          <CsrfTokenInput />
+          <ErrorBoundary fallback={<div>Failed to load CSRF token. Please refresh the page.</div>}>
+            <CsrfTokenInput />
+          </ErrorBoundary>
app/client/src/pages/UserAuth/Login.tsx (1)

189-189: Consider enhancing form security

While the CSRF protection is a good addition, consider these security enhancements:

  1. Add rate limiting for failed login attempts
  2. Implement password strength validation
  3. Add client-side email validation before form submission

Also applies to: 190-190, 191-191, 192-192, 193-193, 194-194, 195-195, 196-196, 197-197, 198-198, 199-199, 200-200, 201-201, 202-202, 203-203, 204-204, 205-205, 206-206, 207-207, 208-208, 209-209, 210-210, 211-211, 212-212, 213-213, 214-214, 215-215, 216-216, 217-217, 218-218, 219-219, 220-220, 221-221, 222-222, 223-223, 224-224, 225-225, 226-226, 227-227, 228-228, 229-229

app/client/src/pages/UserAuth/SignUp.tsx (1)

247-247: Consider adding error handling for CSRF token validation failures

While the CsrfTokenInput is correctly placed, consider adding error handling for cases where CSRF token validation fails. This would improve the user experience by showing appropriate error messages.

          <SpacedSubmitForm
            action={signupURL.toString()}
            id="signup-form"
            method="POST"
            onSubmit={(e) => handleSubmit(e)}
          >
-           <CsrfTokenInput />
+           <CsrfTokenInput 
+             onError={(error) => {
+               // Handle CSRF token validation error
+               showError = true;
+               errorMessage = "Session expired. Please refresh the page.";
+             }}
+           />
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (1)

Line range hint 169-270: Architecture review: Security configuration maintains robust protection

The security configuration:

  1. Preserves essential security features (rate limiting, authentication chains)
  2. Maintains secure cookie configuration
  3. Successfully integrates Spring's CSRF protection

Consider documenting the CSRF token requirements in the API documentation for client implementations.

app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java (1)

81-83: Clarify the comment for accurate context

The comment may be misleading. Since SAFE_READ_ONLY_METHODS includes GET and HEAD, which are methods supported by HTML forms, consider updating the comment to reflect that CSRF checks are not required for safe, read-only methods.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 5e89aa0 and 7d044af.

📒 Files selected for processing (9)
  • app/client/cypress/support/commands.js (2 hunks)
  • app/client/src/pages/UserAuth/CsrfTokenInput.tsx (1 hunks)
  • app/client/src/pages/UserAuth/Login.tsx (2 hunks)
  • app/client/src/pages/UserAuth/SignUp.tsx (2 hunks)
  • app/client/src/pages/setup/DetailsForm.tsx (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java (1 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (2 hunks)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java (0 hunks)
  • app/server/appsmith-server/src/test/java/com/appsmith/server/configurations/CsrfTest.java (1 hunks)
💤 Files with no reviewable changes (1)
  • app/server/appsmith-server/src/main/java/com/appsmith/server/filters/CSRFFilter.java
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/commands.js (1)

Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:

  • Follow best practices for Cypress code and e2e automation.
  • Avoid using cy.wait in code.
  • Avoid using cy.pause in code.
  • Avoid using agHelper.sleep().
  • Use locator variables for locators and do not use plain strings.
  • Use data-* attributes for selectors.
  • Avoid Xpaths, Attributes and CSS path.
  • Avoid selectors like .btn.submit or button[type=submit].
  • Perform logins via API with LoginFromAPI.
  • Perform logout via API with LogOutviaAPI.
  • Perform signup via API with SignupFromAPI.
  • Avoid using it.only.
  • Avoid using after and aftereach in test cases.
  • Use multiple assertions for expect statements.
  • Avoid using strings for assertions.
  • Do not use duplicate filenames even with different paths.
  • Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (10)
app/client/src/pages/UserAuth/CsrfTokenInput.tsx (1)

1-8: LGTM! The component correctly implements CSRF token handling.

The component successfully extracts and provides the CSRF token in the expected format for Spring Security.

Let's verify the CSRF token usage in forms:

app/client/src/pages/setup/DetailsForm.tsx (2)

27-27: LGTM: Clean import of CsrfTokenInput component.


109-109: Verify CSRF token handling in form submission.

The CsrfTokenInput is correctly placed at the start of the form. However, let's verify that the form submission handler properly includes this token.

✅ Verification successful

CSRF token handling is properly implemented

The implementation is secure because:

  • The CsrfTokenInput component correctly extracts the CSRF token from cookies
  • The global axios instance in Api.ts is configured with withCredentials: true
  • The request interceptor adds X-Requested-By: Appsmith header
  • All API requests are automatically handled through this configured axios instance

The form submission will include the CSRF token as it's properly set up as a hidden form input, and the API layer handles the rest of the security measures.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for form submission handling code
rg -t typescript -t javascript "handleSubmit|onSubmit.*DetailsForm" 

# Look for any existing CSRF token validation
ast-grep --pattern 'fetch($_, {
  $$$
  headers: {
    $$$
    "X-CSRF-Token": $_,
    $$$
  }
  $$$
})'

Length of output: 211


Script:

#!/bin/bash
# Search for form submission handling code with correct file extension
rg -t ts -t tsx -t js -t jsx "handleSubmit|onSubmit.*DetailsForm"

# Look for CSRF token related code in the DetailsForm
rg -t ts -t tsx -t js -t jsx "CsrfTokenInput|csrf|CSRF" "app/client/src/pages/setup/DetailsForm.tsx" -A 5 -B 5

# Look for axios or fetch calls in DetailsForm
rg -t ts -t tsx "axios\.|fetch\(" "app/client/src/pages/setup/DetailsForm.tsx" -A 5

# Check CsrfTokenInput implementation
fd "CsrfTokenInput" --type f --exec cat {}

Length of output: 626


Script:

#!/bin/bash
# Search for form submission handling with ripgrep using file content matching
rg -g "*.{ts,tsx,js,jsx}" "handleSubmit|onSubmit.*DetailsForm"

# Look for form submission or API calls in DetailsForm using file content matching
rg -g "*.{ts,tsx,js,jsx}" "axios|fetch|submit" "app/client/src/pages/setup/DetailsForm.tsx"

# Look for the form element and its props in DetailsForm
rg -g "*.{ts,tsx,js,jsx}" "<form.*>|onSubmit={" "app/client/src/pages/setup/DetailsForm.tsx" -A 5 -B 5

Length of output: 3839


Script:

#!/bin/bash
# Look for form element and its props in DetailsForm with fixed regex
rg -g "*.{ts,tsx,js,jsx}" "<form|onSubmit" "app/client/src/pages/setup/DetailsForm.tsx" -A 5 -B 5

# Look for any HTTP client setup or configuration files
fd -e ts -e tsx -e js "api|http|client|axios" --type f --exec rg -l "csrf|CSRF"

# Check if there's any global HTTP client configuration
rg -g "*.{ts,tsx,js,jsx}" "axios\.defaults|fetch.*headers"

Length of output: 2204


Script:

#!/bin/bash
# Check the API request interceptors implementation
cat app/client/src/api/__tests__/apiRequestInterceptors.ts

# Check the main API configuration
cat app/client/src/api/Api.ts

# Look for form submission in DetailsForm with broader context
rg -g "*.{ts,tsx}" "function.*DetailsForm|const.*DetailsForm" app/client/src/pages/setup/DetailsForm.tsx -A 20

Length of output: 9102

app/client/src/pages/UserAuth/Login.tsx (2)

56-57: LGTM: Clean import of CSRF component

The import is appropriately placed with other component imports.


189-189: Verify CSRF token handling in form submission

The CSRF token input is correctly placed at the start of the form. However, we should verify that:

  1. The form's POST action to /api/v1/login properly includes the CSRF token
  2. The Spring backend is configured to validate this token
✅ Verification successful

CSRF protection is properly implemented

The implementation is secure with:

  • Frontend: CsrfTokenInput component correctly extracts token from cookie and adds it as hidden form input
  • Backend: Comprehensive CSRF configuration in CsrfConfig.java with:
    • Token validation for form submissions
    • Proper cookie handling with CookieServerCsrfTokenRepository
    • Security exceptions for safe methods (GET) and API requests
    • Token regeneration on logout
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the login endpoint is properly configured in Spring Security
rg -g '*.java' -A 5 'configure.*HttpSecurity.*login'

# Verify CSRF configuration
rg -g '*.java' -A 5 'csrf\(\).*disable|csrf\(\).*enable'

Length of output: 113


Script:

#!/bin/bash
# Let's check the CsrfTokenInput component implementation
fd -e tsx -e ts "CsrfTokenInput" --exec cat {}

# Check form submission handling in Login component
rg -g "Login.tsx" "onSubmit|handleSubmit" -A 5

# Look for CSRF configuration in security config files
fd -e java "Security" --exec cat {}

# Check if there are any CSRF related utilities or services
rg -g "*.{ts,tsx,java}" "csrf" -i

Length of output: 29683

app/client/src/pages/UserAuth/SignUp.tsx (2)

62-62: LGTM: Clean import addition

The CsrfTokenInput import is properly organized with other component imports.


247-247: Verify CSRF token handling in form submission

The CsrfTokenInput is correctly placed at the start of the form. However, since we're using direct form submission via formElement.submit(), let's verify that the CSRF token is properly included in the request.

✅ Verification successful

CSRF token implementation is correctly handled

The implementation is secure:

  • CsrfTokenInput correctly extracts the XSRF token from cookies
  • Token is properly included as a hidden form field named "_csrf"
  • API interceptors validate CSRF with X-Requested-By header
  • Direct form submission will include the CSRF token automatically
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any CSRF-related tests in the codebase
rg -g '*.{spec,test}.{ts,tsx,js,jsx}' -l 'csrf|CSRF' 

# Check if there are any CSRF-related Cypress commands or tests
rg -g 'cypress/**/*.{js,ts}' -l 'X-Requested-By.*Appsmith'

Length of output: 264


Script:

#!/bin/bash
# Check CSRF token implementation and usage
rg -g '*.{ts,tsx,js,jsx}' 'CsrfTokenInput'

# Check form submission handling in SignUp.tsx
rg -g 'SignUp.tsx' -A 5 'formElement.submit'

# Check if there's any CSRF middleware or configuration
rg -g '*.{ts,js}' 'csrf|CSRF' --type-add 'config:*.config.*' --type config

Length of output: 1654


Script:

#!/bin/bash
# Check CsrfTokenInput implementation
cat app/client/src/pages/UserAuth/CsrfTokenInput.tsx

# Check API interceptors for CSRF handling
cat app/client/src/api/__tests__/apiRequestInterceptors.ts

# Check form element attributes in SignUp.tsx
rg -g 'SignUp.tsx' '<form.*>' -A 2

Length of output: 4262

app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java (2)

114-116: LGTM: Clean dependency injection of CsrfConfig

The autowiring of CsrfConfig aligns with Spring's dependency injection patterns and the PR's objective to use Spring's native CSRF protection.


169-170: Verify CSRF token handling in authentication flows

The transition to Spring's native CSRF protection is good, but we should ensure proper CSRF token handling in:

  1. Login form submissions
  2. OAuth2 authentication flows
  3. API endpoints that were previously CSRF-exempt

Let's verify the CSRF configuration:

✅ Verification successful

CSRF protection is properly configured for authentication flows

The verification shows:

  • Spring's CSRF protection is correctly applied via CsrfConfig before any security filters
  • OAuth2 endpoints (/oauth2/**) are properly CSRF-exempt via permitAll
  • Login endpoint is protected with rate limiting and proper authentication handling
  • No legacy CSRF filters remain in the codebase, confirming complete migration to Spring's native CSRF protection
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for CSRF token handling in authentication-related files
echo "Checking CSRF token handling in authentication flows..."
rg -l "csrf|CSRF" --type java --glob "**/*Authentication*.java"

# Look for any remaining custom CSRF implementations
echo "Checking for any remaining custom CSRF implementations..."
rg -l "CSRFFilter|CsrfFilter" --type java

Length of output: 355


Script:

#!/bin/bash
# Let's check SecurityConfig for CSRF configuration
echo "Checking SecurityConfig for CSRF configuration..."
rg -A 10 -B 10 "csrf" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java

# Check for OAuth2 related configurations
echo "Checking OAuth2 configurations..."
rg -l "oauth2|OAuth2" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/

# Check for authentication endpoints and their security configuration
echo "Checking authentication endpoints..."
rg -A 5 "antMatchers.*auth|permitAll|authenticated" app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/SecurityConfig.java

# Check for any custom security filters
echo "Checking custom security filters..."
fd -e java --exec rg -l "extends.*Filter|implements.*Filter" {} \;

Length of output: 6085

app/client/cypress/support/commands.js (1)

943-943: LGTM: Added CSRF protection header.

The addition of the X-Requested-By header in the signup API call is consistent with the CSRF protection implementation.

app/client/cypress/support/commands.js Show resolved Hide resolved
Comment on lines 71 to 73
// TODO: This shouldn't be necessary. This is weaker than the default and recommended option,
// `XorServerCsrfTokenRequestAttributeHandler`. Figure out a way to switch to the default.
.csrfTokenRequestHandler(new ServerCsrfTokenRequestAttributeHandler());
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 9, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Address the TODO: Switch to the recommended CSRF token request handler

The current usage of ServerCsrfTokenRequestAttributeHandler is weaker than the default XorServerCsrfTokenRequestAttributeHandler. To enhance security, consider switching to the default handler as suggested in the TODO comment.

Would you like assistance in updating the code to use XorServerCsrfTokenRequestAttributeHandler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes please.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sharat87 Sure! You can switch to the recommended XorServerCsrfTokenRequestAttributeHandler by removing the line that sets the custom handler. Here's the updated code:

@Override
public void customize(@NonNull ServerHttpSecurity.CsrfSpec spec) {
    spec.requireCsrfProtectionMatcher(this)
        .csrfTokenRepository(new Repository());
}

By omitting .csrfTokenRequestHandler(new ServerCsrfTokenRequestAttributeHandler()), Spring Security will use the default XorServerCsrfTokenRequestAttributeHandler, which enhances security by masking the CSRF token on each request.

Let me know if you need any further assistance!

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I've already tried that, it doesn't work. All requests that need CSRF verification fail. The token creation is still being done by the ServerCsrfTokenRequestAttributeHandler class, and so verification by XorServerCsrfTokenRequestAttributeHandler fails.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sharat87 Understood. The issue occurs because the XorServerCsrfTokenRequestAttributeHandler masks the CSRF token, but your token generation is still using the default handler, leading to a mismatch.

To resolve this, you need to explicitly set the XorServerCsrfTokenRequestAttributeHandler for both token generation and validation. Here's how you can modify your code:

@Override
public void customize(@NonNull ServerHttpSecurity.CsrfSpec spec) {
    XorServerCsrfTokenRequestAttributeHandler csrfTokenHandler = new XorServerCsrfTokenRequestAttributeHandler();
    csrfTokenHandler.setCsrfRequestAttributeName("_csrf"); // Ensure this matches the name used in your forms

    spec.requireCsrfProtectionMatcher(this)
        .csrfTokenRepository(new Repository())
        .csrfTokenRequestHandler(csrfTokenHandler);
}

Make sure that your CsrfTokenRepository (Repository class) is compatible with the XorServerCsrfTokenRequestAttributeHandler. If necessary, adjust your repository or consider using the default CookieServerCsrfTokenRepository without customization.

Additionally, ensure that your forms include the masked CSRF token. If you're using a hidden input field, the XorServerCsrfTokenRequestAttributeHandler will automatically handle the masking. Verify that your client-side code submits the CSRF token correctly.

Let me know if this helps!

Copy link
Member Author

Choose a reason for hiding this comment

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

How do I get the client code to mask the token?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sharat87 When using XorServerCsrfTokenRequestAttributeHandler, the CSRF token is masked, and you need to use the masked token in your client code. Instead of reading the token from the XSRF-TOKEN cookie, you should retrieve it from the X-CSRF-TOKEN response header that the server provides. Here's how you can modify your CsrfTokenInput component:

import React, { useEffect, useState } from "react";

export default function CsrfTokenInput() {
  const [csrfToken, setCsrfToken] = useState("");

  useEffect(() => {
    fetch(window.location.href, {
      method: "HEAD",
      credentials: "include", // Include cookies in the request
    })
      .then((response) => {
        const token = response.headers.get("X-CSRF-TOKEN") || "";
        setCsrfToken(token);
      })
      .catch((error) => {
        console.error("Error fetching CSRF token:", error);
      });
  }, []);

  return <input type="hidden" name="_csrf" value={csrfToken} />;
}

Notes:

  • Fetch the Masked Token: The XorServerCsrfTokenRequestAttributeHandler provides the masked CSRF token in the X-CSRF-TOKEN header. Sending a HEAD request to your server allows you to retrieve this header without fetching the entire page.
  • Include Credentials: Ensure you include { credentials: "include" } in your fetch options so that cookies are sent with the request, allowing the server to recognize the session.
  • Use in Forms and Requests: Include the csrfToken value in your form submissions or as a header in your API requests. For form submissions, the hidden input field will automatically include it.

By updating your client code to use the masked token from the X-CSRF-TOKEN header, the server will correctly validate the token when handling requests.

Let me know if you need further assistance!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, doesn't work. I removed the call to .csrfTokenRequestHandler, so that is uses the default XorServerCsrfTokenRequestAttributeHandler, but I don't see any X-XSRF-TOKEN header in any response from the server.

I think I'll pass on this for this PR. I've sunk several weeks over this already, and I'd rather have the protection the current setup gives until we figure that out. 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

@sharat87 Understood. Thanks for your efforts on this. It makes sense to proceed with your current setup for now. Would you like me to open a GitHub issue to track this problem for future investigation?


✏️ Learnings added
Learnt from: sharat87
PR: appsmithorg/appsmith#37292
File: app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CsrfConfig.java:0-0
Timestamp: 2024-11-09T15:23:36.012Z
Learning: Removing the call to `.csrfTokenRequestHandler` to use the default `XorServerCsrfTokenRequestAttributeHandler` results in no `X-XSRF-TOKEN` header being sent in responses from the server, causing CSRF verification to fail.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

@sharat87 sharat87 added ok-to-test Required label for CI and removed ok-to-test Required label for CI labels Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working ok-to-test Required label for CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant