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

SecurityMockMvcRequestPostProcessors.csrf() doesn't work with XorCsrfTokenRequestAttributeHandler #14125

Closed
ch4mpy opened this issue Nov 9, 2023 · 6 comments
Assignees
Labels
in: test An issue in spring-security-test type: bug A general bug

Comments

@ch4mpy
Copy link
Contributor

ch4mpy commented Nov 9, 2023

Describe the bug
Using SecurityMockMvcRequestPostProcessors.csrf() gives invalid CSRF token when configuration contains csrf.csrfTokenRequestHandler(new XorCsrfTokenRequestAttributeHandler()::handle)

To Reproduce
Configure a servlet application with oauth2Login and CSRF security for a SPA

Expected behavior
Test security framework should provide with the tooling to mimic a request from a SPA with valid CSRF token.

Sample
https://github.com/ch4mpy/reproducer_spring-security_gh-14125

This repo contains a minimal reproducer (reproducer-bff-servlet module) with a failing test (ReproducerBffServletApplicationTests::givenCsrfTokenIsPresent_whenLogout_thenOk).

It also contains an equivalent reactive app (which is not affected by the bug) and an Angular SPA working with both Spring backends.

@ch4mpy ch4mpy added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Nov 9, 2023
@jzheaux jzheaux self-assigned this Nov 14, 2023
@jzheaux jzheaux added in: test An issue in spring-security-test and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 14, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Nov 15, 2023

Hey, @ch4mpy, thank you for the reproducer, it was very helpful.

I talked briefly with @sjohnr about this and this is the current recommendation for working with SPAs and BREACH CSRF support. When I update your sample to use that recommendation, the tests pass fine.

The migration guide should be updated to reflect this which I'm happy to keep this ticket open for.

And seeing this ticket, as well as your comment on #5766, I wonder if you might have an opinion on #14149, which is an effort to simplify the SPA-centric CSRF configuration.

@ch4mpy
Copy link
Contributor Author

ch4mpy commented Nov 15, 2023

@jzheaux I had missed this updated documentation. Thank you for pointing it. I couldn't find an equivalent for reactive applications. Here is what I plan to use, is it correct?

http.csrf(csrf -> csrf
    .csrfTokenRepository(CookieServerCsrfTokenRepository.withHttpOnlyFalse())
    .csrfTokenRequestHandler(new SpaServerCsrfTokenRequestHandler()));

with:

/**
 * Adapted from https://docs.spring.io/spring-security/reference/servlet/exploits/csrf.html#csrf-integration-javascript-spa
 */
static final class SpaServerCsrfTokenRequestHandler extends ServerCsrfTokenRequestAttributeHandler {
	private final ServerCsrfTokenRequestAttributeHandler delegate = new XorServerCsrfTokenRequestAttributeHandler();

	@Override
	public void handle(ServerWebExchange exchange, Mono<CsrfToken> csrfToken) {
		/*
		 * Always use XorCsrfTokenRequestAttributeHandler to provide BREACH protection of the CsrfToken when it is rendered in the response body.
		 */
		this.delegate.handle(exchange, csrfToken);
	}

	@Override
	public Mono<String> resolveCsrfTokenValue(ServerWebExchange exchange, CsrfToken csrfToken) {
		final var hasHeader = exchange.getRequest().getHeaders().get(csrfToken.getHeaderName()).stream().filter(StringUtils::hasText).count() > 0;
		return hasHeader ? super.resolveCsrfTokenValue(exchange, csrfToken) : this.delegate.resolveCsrfTokenValue(exchange, csrfToken);
	}
}

@Bean
WebFilter csrfCookieWebFilter() {
    return (exchange, chain) -> {
        exchange.getAttributeOrDefault(CsrfToken.class.getName(), Mono.empty()).subscribe();
        return chain.filter(exchange);
    };
}

Maybe could this SpaCsrfTokenRequestHandler and SpaServerCsrfTokenRequestHandler be exposed publicly by spring-security (so that we don't have to duplicate this code in each app / lib)?

Regarding #14149, yep, that would be great to have this done in one step.

I currently offer this feature in my Boot starter with an enum in properties. But having it as a predefined customizer in spring-security would be useful for those not using Boot (you could still add a boot property later on to apply this customizer).

ch4mpy added a commit to ch4mpy/spring-addons that referenced this issue Nov 15, 2023
@LHai-dev
Copy link

LHai-dev commented Nov 25, 2023

hello ch4mpy
i use api-gateway with 0auth2 when i want to @postMapping is always 403 (Forbidden)
i want Communicate api gateways to student service using @PostMapping it always 403 (Forbidden) (using with nextJs)
and sorry for my bad English writing
image

` @Configuration
@EnableWebFluxSecurity
@EnableReactiveMethodSecurity
public class GatewayConfig {

    @Bean
    public SecurityWebFilterChain springSecurityFilterChain(ServerHttpSecurity http,
                                                            ReactiveClientRegistrationRepository repository) {
        return http
                .authorizeExchange(auth -> auth
                        .pathMatchers("/health", "/actuator/prometheus", "/actuator/health/**", "/fallback/**", "/register/**", "/**").permitAll()
                        .anyExchange().permitAll())
                // Set post-login URI to Next.js app (login being successful or not)
                .oauth2Login(oAuth2LoginSpec -> {
                    oAuth2LoginSpec.authenticationSuccessHandler(new RedirectServerAuthenticationSuccessHandler("/"));
                    oAuth2LoginSpec.authenticationFailureHandler(new RedirectServerAuthenticationFailureHandler("/"));
                    oAuth2LoginSpec.authorizationRequestResolver(pkceResolver(repository));
                })
                .httpBasic(ServerHttpSecurity.HttpBasicSpec::disable)
                .formLogin(ServerHttpSecurity.FormLoginSpec::disable)

                .logout(logout -> {
                    logout.logoutSuccessHandler(new SpaLogoutSucessHandler(repository,"http://127.0.0.1:9090"));
                })
                // Note the csrfCookieWebFilter below which actually attaches the CSRF token cookie to responses
http.csrf(csrf -> csrf
    .csrfTokenRepository(CookieServerCsrfTokenRepository.withHttpOnlyFalse())
    .csrfTokenRequestHandler(new SpaServerCsrfTokenRequestHandler()));
//                }).addFilterBefore(csrfCookieWebFilter(), SecurityWebFiltersOrder.CSRF)
                .csrf(csrf -> {
                    csrf.csrfTokenRepository(CookieServerCsrfTokenRepository.withHttpOnlyFalse());
                    csrf.csrfTokenRequestHandler(new XorServerCsrfTokenRequestAttributeHandler()::handle);
                })
                .cors(ServerHttpSecurity.CorsSpec::disable)

                .build();
    }


    @Bean
    public ServerOAuth2AuthorizationRequestResolver pkceResolver(ReactiveClientRegistrationRepository repository) {
        var resolver = new DefaultServerOAuth2AuthorizationRequestResolver(repository);
        resolver.setAuthorizationRequestCustomizer(OAuth2AuthorizationRequestCustomizers.withPkce());
        return resolver;
    }

    @Bean
    WebFilter csrfCookieWebFilter() {
        return (exchange, chain) -> {
            Mono<CsrfToken> csrfToken = exchange.getAttributeOrDefault(CsrfToken.class.getName(), Mono.empty());

            csrfToken.doOnSuccess(token -> {
                System.out.println("CSRF Token: " + token.getToken());
                System.out.println("CSRF Header Name: " + token.getHeaderName());
            }).subscribe();
            exchange.getAttributeOrDefault(CsrfToken.class.getName(), Mono.empty()).subscribe();


            return chain.filter(exchange);
        };
    }



    static class SpaLogoutSucessHandler implements ServerLogoutSuccessHandler {
        private final OidcClientInitiatedServerLogoutSuccessHandler delegate;

        public SpaLogoutSucessHandler(ReactiveClientRegistrationRepository clientRegistrationRepository, String postLogoutRedirectUri) {
            this.delegate = new OidcClientInitiatedServerLogoutSuccessHandler(clientRegistrationRepository);
            this.delegate.setPostLogoutRedirectUri(postLogoutRedirectUri);
        }

        @Override
        public Mono<Void> onLogoutSuccess(WebFilterExchange exchange, Authentication authentication) {
            return delegate.onLogoutSuccess(exchange, authentication).then(Mono.fromRunnable(() -> {
                exchange.getExchange().getResponse().setStatusCode(HttpStatus.ACCEPTED);
            }));
        }

    }
}
/**
 * Adapted from https://docs.spring.io/spring-security/reference/servlet/exploits/csrf.html#csrf-integration-javascript-spa
 */
static final class SpaServerCsrfTokenRequestHandler extends ServerCsrfTokenRequestAttributeHandler {
	private final ServerCsrfTokenRequestAttributeHandler delegate = new XorServerCsrfTokenRequestAttributeHandler();

	@Override
	public void handle(ServerWebExchange exchange, Mono<CsrfToken> csrfToken) {
		/*
		 * Always use XorCsrfTokenRequestAttributeHandler to provide BREACH protection of the CsrfToken when it is rendered in the response body.
		 */
		this.delegate.handle(exchange, csrfToken);
	}

	@Override
	public Mono<String> resolveCsrfTokenValue(ServerWebExchange exchange, CsrfToken csrfToken) {
		final var hasHeader = exchange.getRequest().getHeaders().get(csrfToken.getHeaderName()).stream().filter(StringUtils::hasText).count() > 0;
		return hasHeader ? super.resolveCsrfTokenValue(exchange, csrfToken) : this.delegate.resolveCsrfTokenValue(exchange, csrfToken);
	}
}

@Bean
WebFilter csrfCookieWebFilter() {
    return (exchange, chain) -> {
        exchange.getAttributeOrDefault(CsrfToken.class.getName(), Mono.empty()).subscribe();
        return chain.filter(exchange);
    };
} `

@ch4mpy
Copy link
Contributor Author

ch4mpy commented Nov 26, 2023

@LHai-dev correct me if my wrong, but your comment has nothing to do with this issue (if I'm right, you should probably delete it). It looks like an issue at runtime (not during JUnit tests like this ticket is about), and it also seems that this issue is due to the fact that you copied some code from my tutorials before I updated it with the latest recommandations linked above by @jzheaux

@jzheaux
Copy link
Contributor

jzheaux commented Nov 28, 2023

@ch4mpy, yes that looks correct.

Regarding making it public, I've added my initial thoughts over on #14149.

@ch4mpy
Copy link
Contributor Author

ch4mpy commented Nov 28, 2023

Thank you @jzheaux . I think that we can close this issue as the other one gives a follow up on good CSRF practices for SPAs (and this practices solve the bug I reported here)

@ch4mpy ch4mpy closed this as completed Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test An issue in spring-security-test type: bug A general bug
Projects
Status: Done
Development

No branches or pull requests

3 participants