From 000b4bc4950969f10861fb8d89ed38ccf73ec619 Mon Sep 17 00:00:00 2001 From: Leonid Rozenblyum Date: Tue, 14 Feb 2023 15:48:55 +0200 Subject: [PATCH] Fix NPE in HttpSecurity#addFilterBefore, HttpSecurity#addFilterAfter Before the fix, these methods would throw a NPE in case when the filter class passed as the second parameter, is not registered yet. In particular, this exception can occur when mixing standard and custom DSL to register filters. The fix doesn't change the situation that standard DSL for registration of filters cannot refer to filters that are registered via custom DSL even though those calls were done earlier. It just provides more user-friendly error handling for this and most likely other scenarios of calls of HttpSecurity#addFilterBefore, HttpSecurity#addFilterAfter. The error handling is implemented similarly to HttpSecurity#addFilter. Closes gh-12637 --- .../annotation/web/builders/HttpSecurity.java | 9 +++- .../builders/HttpSecurityAddFilterTest.java | 48 ++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java index 4d4c33cf0a..0566f82463 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/HttpSecurity.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -3254,7 +3254,12 @@ public final class HttpSecurity extends AbstractConfiguredSecurityBuilder registeredFilter) { - int order = this.filterOrders.getOrder(registeredFilter) + offset; + Integer registeredFilterOrder = this.filterOrders.getOrder(registeredFilter); + if (registeredFilterOrder == null) { + throw new IllegalArgumentException( + "The Filter class " + registeredFilter.getName() + " does not have a registered order"); + } + int order = registeredFilterOrder + offset; this.filters.add(new OrderedFilter(filter, order)); this.filterOrders.put(filter.getClass(), order); return this; diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/builders/HttpSecurityAddFilterTest.java b/config/src/test/java/org/springframework/security/config/annotation/web/builders/HttpSecurityAddFilterTest.java index d3b5044c42..cbe51d9aa7 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/builders/HttpSecurityAddFilterTest.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/builders/HttpSecurityAddFilterTest.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2021 the original author or authors. + * Copyright 2002-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,6 +30,7 @@ import org.assertj.core.api.ListAssert; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.beans.factory.UnsatisfiedDependencyException; import org.springframework.context.annotation.Bean; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; @@ -45,12 +46,29 @@ import org.springframework.security.web.context.request.async.WebAsyncManagerInt import org.springframework.security.web.header.HeaderWriterFilter; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; @ExtendWith(SpringTestContextExtension.class) public class HttpSecurityAddFilterTest { public final SpringTestContext spring = new SpringTestContext(this); + @Test + public void addFilterAfterFilterNotRegisteredYetThenThrowIllegalArgument() { + assertThatExceptionOfType(UnsatisfiedDependencyException.class) + .isThrownBy( + () -> this.spring.register(MyOtherFilterAfterMyFilterNotRegisteredYetConfig.class).autowire()) + .havingRootCause().isInstanceOf(IllegalArgumentException.class); + } + + @Test + public void addFilterBeforeFilterNotRegisteredYetThenThrowIllegalArgument() { + assertThatExceptionOfType(UnsatisfiedDependencyException.class) + .isThrownBy( + () -> this.spring.register(MyOtherFilterBeforeMyFilterNotRegisteredYetConfig.class).autowire()) + .havingRootCause().isInstanceOf(IllegalArgumentException.class); + } + @Test public void addFilterAfterWhenSameFilterDifferentPlacesThenOrderCorrect() { this.spring.register(MyFilterMultipleAfterConfig.class).autowire(); @@ -209,6 +227,34 @@ public class HttpSecurityAddFilterTest { } + @EnableWebSecurity + static class MyOtherFilterAfterMyFilterNotRegisteredYetConfig { + + @Bean + SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { + // @formatter:off + http + .addFilterAfter(new MyOtherFilter(), MyFilter.class); + // @formatter:on + return http.build(); + } + + } + + @EnableWebSecurity + static class MyOtherFilterBeforeMyFilterNotRegisteredYetConfig { + + @Bean + SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception { + // @formatter:off + http + .addFilterBefore(new MyOtherFilter(), MyFilter.class); + // @formatter:on + return http.build(); + } + + } + @EnableWebSecurity static class MyOtherFilterRelativeToMyFilterBeforeConfig {