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

Java: update java/spring-disabled-csrf-protection QHelp #18692

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jcogs33
Copy link
Contributor

@jcogs33 jcogs33 commented Feb 5, 2025

Description

Updates the overview section of the java/spring-disabled-csrf-protection QHelp to align with other CSRF queries. Context: #18288 (comment)

Also updates the name of the OWASP website. Context: #18288 (comment)

Autofix validation: I quickly checked the effect on autofix for a couple alerts. Autofix still successfully removes the .disable() call, but it now also discusses handling CSRF tokens in the fix descriptions and includes code related to CSRF tokens in one of the fix suggestions. Let me know if this sounds like a reason to avoid this wording update?
(Note that I purposefully did not include any explicit mention of tokens in this update, but the related Python and Ruby QHelp files do mention tokens.)

@jcogs33 jcogs33 added the no-change-note-required This PR does not need a change note label Feb 5, 2025
Copy link
Contributor

github-actions bot commented Feb 5, 2025

QHelp previews:

java/ql/src/Security/CWE/CWE-352/SpringCSRFProtection.qhelp

Disabled Spring CSRF protection

Cross-site request forgery (CSRF) is a type of vulnerability in which an attacker is able to force a user to carry out an action that the user did not intend.

The attacker tricks an authenticated user into submitting a request to the web application. Typically, this request will result in a state change on the server, such as changing the user's password. The request can be initiated when the user visits a site controlled by the attacker. If the web application relies only on cookies for authentication, or on other credentials that are automatically included in the request, then this request will appear as legitimate to the server.

Recommendation

When you use Spring, Cross-Site Request Forgery (CSRF) protection is enabled by default. Spring's recommendation is to use CSRF protection for any request that could be processed by a browser client by normal users.

Example

The following example shows the Spring Java configuration with CSRF protection disabled. This type of configuration should only be used if you are creating a service that is used only by non-browser clients.

import org.springframework.context.annotation.Configuration;
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;

@EnableWebSecurity
@Configuration
public class WebSecurityConfig extends WebSecurityConfigurerAdapter {
  @Override
  protected void configure(HttpSecurity http) throws Exception {
    http
      .csrf(csrf ->
        // BAD - CSRF protection shouldn't be disabled
        csrf.disable() 
      );
  }
}

References

@jcogs33 jcogs33 marked this pull request as ready for review February 5, 2025 20:00
@Copilot Copilot bot review requested due to automatic review settings February 5, 2025 20:00
@jcogs33 jcogs33 requested a review from a team as a code owner February 5, 2025 20:00

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant