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

LogoutConfigurer forces POST even if CSRF is disabled for /logout #14913

Open
erizzo opened this issue Apr 15, 2024 · 0 comments
Open

LogoutConfigurer forces POST even if CSRF is disabled for /logout #14913

erizzo opened this issue Apr 15, 2024 · 0 comments
Labels
status: waiting-for-triage An issue we've not yet triaged type: bug A general bug

Comments

@erizzo
Copy link

erizzo commented Apr 15, 2024

With a configuration that includes CSRF protection (even customized), LogoutConfigurer assumes that /logout will only match POST requests (code in question here as of the time of this writing). That's not a problem in general, but if /logout has been configured to NOT match CSRF protection, it causes confusion and violates the principal of least surprise.

For example, with a custom CsrfConfigurer like csrf.ignoringRequestMatchers("/logout"); it's very likely that the developer intends to allow GET requests for /logout (what other reason would there be for ignoring /logout in CSRF?). That was certainly my expectation in configuring it that way.

However the logic in LogoutConfigurer.createLogoutRequestMatcher(H) requires POST if any CsrfConfigurer is present, ignoring the fact that CSRF is ignored for /logout.

private RequestMatcher createLogoutRequestMatcher(H http) {
	RequestMatcher post = createLogoutRequestMatcher("POST");
	if (http.getConfigurer(CsrfConfigurer.class) != null) {
		return post;
	}
	...
}

The workaround is to specify a request matcher for the LogoutConfigurer, but that's not obvious (I spent a couple of hours debugging through the Spring Security code to find what was happening and where it came from - a situation made more painful by the fact that you have to know which log categories to set to TRACE to get any info about why /logout isn't "available").

I'm not sure if this is considered a bug or if it was an intentional choice (and thus an enhancement request), but either way it seems like it would be easy to adjust the logic to look in the CSRF config to see if /logout (or whatever the configured logout URL is) is ignored and, if so, allow other HTTP methods for it. Alternatively, add something to the Logout DSL to make it simple to accomplish.

@erizzo erizzo added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged type: bug A general bug
Projects
None yet
Development

No branches or pull requests

1 participant