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

Spring Security AuthenticationException message inconsistency #26357

Closed
rubensa opened this issue May 4, 2021 · 7 comments
Closed

Spring Security AuthenticationException message inconsistency #26357

rubensa opened this issue May 4, 2021 · 7 comments
Labels
status: superseded An issue that has been superseded by another

Comments

@rubensa
Copy link

rubensa commented May 4, 2021

In general, for any exception, the default behavior if you tell Spring Boot to include the error message via server.error.include-message=always is to include the Exception message in the JSON message attribute.
Looks like with any exception whose ancestor is org.springframework.security.core.AuthenticationException (like AccessDeniedException, AccountExpiredException, BadCredentialsException,...), the behavior differs and the message value contains the same value as error attribute.

Sample pom file:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>
  <parent>
    <groupId>org.springframework.boot</groupId>
    <artifactId>spring-boot-starter-parent</artifactId>
    <version>2.4.5</version>
    <relativePath /> <!-- lookup parent from repository -->
  </parent>
  <groupId>org.eu.rubensa.springboot.error</groupId>
  <artifactId>springboot-error-test</artifactId>
  <version>0.0.1-SNAPSHOT</version>
  <name>springboot-error-test</name>
  <description>Project for testing Spring Boot error handling</description>
  <properties>
    <java.version>8</java.version>
  </properties>
  <dependencies>
    <dependency>
      <groupId>org.springframework.boot</groupId>
      <artifactId>spring-boot-starter-web</artifactId>
    </dependency>
    <dependency>
      <groupId>org.springframework.boot</groupId>
      <artifactId>spring-boot-starter-security</artifactId>
    </dependency>

    <dependency>
      <groupId>org.springframework.boot</groupId>
      <artifactId>spring-boot-starter-test</artifactId>
      <scope>test</scope>
    </dependency>
    <dependency>
      <groupId>org.springframework.security</groupId>
      <artifactId>spring-security-test</artifactId>
      <scope>test</scope>
    </dependency>
  </dependencies>
</project>

Sample test class:

package org.eu.rubensa.springboot.error;

import com.fasterxml.jackson.databind.JsonNode;

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
import org.springframework.boot.test.web.client.TestRestTemplate;
import org.springframework.context.annotation.Configuration;
import org.springframework.http.HttpMethod;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.security.core.AuthenticationException;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RestController;

/**
 * The @SpringBootTest annotation will load the fully ApplicationContext. This
 * will not use slicing and scan for all the stereotype annotations
 * (@Component, @Service, @Respository and @Controller / @RestController) and
 * loads the full application context.
 */
@SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT,
    // From Spring 2.3.0 "server.error.include-message" and
    // "server.error.include-binding-errors" is set to "never"
    properties = { "server.error.include-message=always",
        /**
         * When you add the Security starter without custom security configurations,
         * Spring Boot endpoints will be secured using HTTP basic authentication with a
         * default user and generated password. To override that, you can configure
         * credentials in application.properties as follows
         */
        "spring.security.user.name=username", "spring.security.user.password=password" })
public class AuthenticationExceptionMessageInconsistencyTest {
  @Autowired
  private TestRestTemplate testRestTemplate;

  @Test
  public void testExceptionMessage() throws Exception {
    String exceptionParam = "custom";

    final ResponseEntity<JsonNode> response = testRestTemplate.withBasicAuth("username", "password")
        .exchange("/exception/{exception_id}", HttpMethod.GET, null, JsonNode.class, exceptionParam);
    Assertions.assertThat(response.getStatusCode()).isEqualTo(HttpStatus.INTERNAL_SERVER_ERROR);
    JsonNode jsonResponse = response.getBody();
    Assertions.assertThat(jsonResponse.findValue("status").asInt()).isEqualTo(500);
    Assertions.assertThat(jsonResponse.findValue("error").asText()).isEqualTo("Internal Server Error");
    // This is the exception message
    Assertions.assertThat(jsonResponse.findValue("message").asText()).isEqualTo("Custom exception");
    Assertions.assertThat(jsonResponse.findValue("path").asText()).isEqualTo("/exception/custom");
  }

  @Test
  public void testAuthenticationExceptionMessage() throws Exception {
    String exceptionParam = "custom-authentication";

    final ResponseEntity<JsonNode> response = testRestTemplate.withBasicAuth("username", "password")
        .exchange("/exception/{exception_id}", HttpMethod.GET, null, JsonNode.class, exceptionParam);
    Assertions.assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNAUTHORIZED);
    JsonNode jsonResponse = response.getBody();
    Assertions.assertThat(jsonResponse.findValue("status").asInt()).isEqualTo(401);
    Assertions.assertThat(jsonResponse.findValue("error").asText()).isEqualTo("Unauthorized");
    // This should be the exception message but is the same as error
    Assertions.assertThat(jsonResponse.findValue("message").asText()).isEqualTo("Unauthorized");
    Assertions.assertThat(jsonResponse.findValue("path").asText()).isEqualTo("/exception/custom-authentication");
  }

  /**
   * A nested @Configuration class wild be used instead of the application’s
   * primary configuration.
   * <p>
   * Unlike a nested @Configuration class, which would be used instead of your
   * application’s primary configuration, a nested @TestConfiguration class is
   * used in addition to your application’s primary configuration.
   */
  @Configuration
  /**
   * Tells Spring Boot to start adding beans based on classpath settings, other
   * beans, and various property settings.
   */

  @EnableAutoConfiguration
  /**
   * The @ComponentScan tells Spring to look for other components, configurations,
   * and services in the the TestWebConfig package, letting it find the
   * TestController class.
   * <p>
   * We only want to test the classes defined inside this test configuration
   */
  static class TestConfig {
    @RestController
    public class TestController {
      @GetMapping("/exception/{exception_id}")
      public void getSpecificException(@PathVariable("exception_id") String pException) {
        if ("custom".equals(pException)) {
          throw new CustomException("Custom exception");
        } else if ("custom-authentication".equals(pException)) {
          throw new MyAuthenticationException("Custom authentication exception");
        }
      }
    }

    public class CustomException extends RuntimeException {
      public CustomException(String message) {
        super(message);
      }
    }

    public class MyAuthenticationException extends AuthenticationException {
      public MyAuthenticationException(String message) {
        super(message);
      }
    }
  }
}

Spring Boot Version: 2.4.5

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 4, 2021
@philwebb
Copy link
Member

philwebb commented May 4, 2021

This is actually the expected behavior. The DefaultErrorAttributes class will use the RequestDispatcher#ERROR_MESSAGE attribute whenever it is set. In this case the AuthenticationException triggered BasicAuthenticationEntryPoint which calls response.sendError. You can provide your own DefaultErrorAttributes subclass as a bean if you want to override the behavior.

@philwebb philwebb closed this as completed May 4, 2021
@philwebb philwebb added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged labels May 4, 2021
@rubensa
Copy link
Author

rubensa commented May 4, 2021

Thanks for your answer, @philwebb

I have a couple of questions here, if you don't mind.

DefaultErrorAttributes uses the current HttpServeltRequest attribute javax.servlet.error.message, if exists. If not, the exception message is used, again if exists. Then a default "No message available" is used.

The DefaultErrorAttributes is used by the BasicErrorController to "dispatch" the exceptions to the client (as JSON or HTML as requested).

This is all right for me, and looks like this behavior work for "most" exceptions thrown at any point in the application.

But, looks like the Spring Security exception handling mechanism uses a different approach, directly managing the HttpServletResponse object, and not letting Spring Boot default exception handling mechanism to do its work.

Why are the AuthenticationException exceptions working differently? Is there any reason as why this exception handlers behaves differently? Shouldn't this be "overrided" by Spring Boot so it's behavior is "consistent" with the rest of the Exception handling?

@rubensa
Copy link
Author

rubensa commented May 5, 2021

This is the flow that Spring follows to handle exceptions thrown from controller:

  1. First Spring searches for an exception handler (a method annotated with @ExceptionHandler) within the @Controller or in @ControllerAdvice classes. See ExceptionHandlerExceptionResolver.
  2. Then it checks if the thrown exception is annotated with @ResponseStatus or derives from ResponseStatusException. See ResponseStatusExceptionResolver.
  3. Then it goes through Spring MVC exceptions' default handlers. See DefaultHandlerExceptionResolver.

spring-exception-handling-mechanism

  1. And at the end if nothing is found, the control is forwarded to the error page view with the global error handler behind it. This step is not executed if the exception comes from the error handler itself.
  2. If no error view is found (e.g. global error handler is disabled) or step 4 is skipped, then the exception is handled by the container.

What the DefaultHandlerExceptionResolver does is calling response.sendError (like BasicAuthenticationEntryPoint does).

This ends been handled by BasicErrorController, as is the /error handler that the underlaying container delegates the error.

At some point (I think that the DispatcherServlet calls the resolveException method of HandlerExceptionResolverComosite that calls DefaultErrorAttributes resolveException method, as is this class is also a HandlerExceptionResolver) the exception thrown in by the controller is made available in the org.springframework.boot.web.servlet.error.DefaultErrorAttributes.ERROR or javax.servlet.error.exception request attributes so the DefaultErrorAttributes has access to it, on "print" time, and can get the original Exception message.

In the case of the Spring Security exceptions, looks like no body is making the exception available in any of those attributes so the DefaultErrorAttributes, on "print" time, can only use the javax.servlet.error.message request attribute.

Am I right here? Could Spring Boot register some filter or anything that calls DefaultErrorAttributes resolveException method to make the whole thing consistent?

@rubensa
Copy link
Author

rubensa commented May 11, 2021

With some more debugging I found the reason:

  • The Srping Security FilterChainProxy process the request
  • A Security Exception is thrown cause the user is not logged or something
  • The ExceptionTranslationFilter catches the Exception and delegates to BasicAuthenticationEntryPoint
  • The BasicAuthenticationEntryPoint calls response.sendError(HttpStatus.UNAUTHORIZED.value(), HttpStatus.UNAUTHORIZED.getReasonPhrase()); (the second parameter is supposed to be the error message - it does not use the exception message but a generic reason phrase for the HttpStatus)
  • These response is then checked by the Servlet Container (Tomcat by default)
  • The Serlvet Container checks the response has an error
  • It looks then if an error page is registered for that particular error code
  • None is registered
  • It look if a global error page is registered
  • Spring Boot registers the BasicErrorController as a global error page
  • The Servlet Container saves the error message in the request under javax.servlet.error.message attribute.
  • The Servlet Container redirects the request to the BasicErrorController
  • If the BasicErrorController can be reached (since Srping Boot 2.0 the /error end-point is protected) the BasicErrorController uses the DefaultErrorAttributes to render the response.
  • The DefaultErrorAttributes find the error message in the request javax.servlet.error.message attribute so it uses it as it's message attribute (the "real" exception message is so lost).

Why is DefaultErrorAttributes using the request javax.servlet.error.message attribute instead of the Exception message, or why is the BasicAuthenticationEntryPoint setting a "generic" phrase instead of the Exception message, I don't know the reason.

@philwebb
Copy link
Member

@rubensa Just to let you know that we're still investigating this issue and #26356. Thanks for your patience.

@philwebb philwebb reopened this May 18, 2021
@philwebb philwebb added status: waiting-for-triage An issue we've not yet triaged and removed status: invalid An issue that we don't feel is valid labels May 18, 2021
@rubensa
Copy link
Author

rubensa commented May 18, 2021

Thanks @philwebb for your time and support.

@mbhave
Copy link
Contributor

mbhave commented Jun 29, 2022

I ran the tests from here with Spring Boot 2.7.1 and they are green. The fix for #26356 has taken care of this issue too.

@mbhave mbhave closed this as completed Jun 29, 2022
@mbhave mbhave added status: superseded An issue that has been superseded by another and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

4 participants