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

CsrfRequestDataValueProcessor uses a different attribute name then the rest of the CSRF parts. #12443

Open
mdeinum opened this issue Dec 21, 2022 · 5 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: feedback-provided Feedback has been provided type: enhancement A general enhancement

Comments

@mdeinum
Copy link
Contributor

mdeinum commented Dec 21, 2022

When working with Spring Webflux and CSRF protection additional steps are needed to expose the CSRF Token to the frontend. The reference guide mentions an @ControllerAdvice (which will only work for controllers) if you also have RouterFunction materials you need to add an additional HandlerFilterFunction so it copies the thing around.

When combining different ways of configuring endpoiints, it all adds up with additional cruft to have in place for a proper working of the CSRF tokens. Wouldn't it be easier for the CsrfRequestDataValueProcessor to utilize the same name CsrfToken.class.getName() to obtain the actual CsrfToken and expose it as hidden fields?

I wonder why the discrepancy with the regular CSRF protection and the additional (complexer) setup. As the servlet variant of the CsrfRequestDataValueProcessor does actually use the "proper" attribute name.

@mdeinum mdeinum added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Dec 21, 2022
@sjohnr
Copy link
Member

sjohnr commented Jan 3, 2023

Hi @mdeinum!

Generally, I would agree with the suggestion you're proposing here as a) aligning servlet and reactive is a good goal, and b) making it easier is a great goal!

Wouldn't it be easier for the CsrfRequestDataValueProcessor to utilize the same name CsrfToken.class.getName() to obtain the actual CsrfToken and expose it as hidden fields?

In general, it would be. However, see below:

I wonder why the discrepancy with the regular CSRF protection and the additional (complexer) setup. As the servlet variant of the CsrfRequestDataValueProcessor does actually use the "proper" attribute name.

The challenge I see here is that the CsrfWebFilter works every so slightly differently. First, it only places a value in an attribute named CsrfToken.class.getName(). Second, and more importantly, this value is actually a Mono<CsrfToken>, whereas the servlet version is a normal CsrfToken that happens to defer lookup until a method on it is called.

I don't believe the reactive CsrfRequestDataValueProcessor should be changed to be aware of a Mono<CsrfToken>.

I wonder if it would be beneficial to instead change the CsrfWebFilter to also add a _csrf attribute that exposes a CsrfToken that can defer access to the real Mono<CsrfToken>?

Note that we could potentially also align the servlet CsrfRequestDataValueProcessor to access the same attribute name in 6.x (e.g. 6.1), since it is now the default.

@mdeinum
Copy link
Contributor Author

mdeinum commented Jan 4, 2023

It isn't so much the naming but the additional work needed to get CSRF protection to work in a reactive setup, especially with a combination of controllers and router functions. Basically it works out-of-the-box with a plain Servlet approach whilst additional work (which is easily forgotten) is needed to make it work for the Reactive stack.

Basically that is the biggest drawback I see (and that people will run into, it bit me whilst writing the security chapter for my upcoming book and took me too long to figure out why it wasn't working for a router function but was working for an @Controller!

@sjohnr
Copy link
Member

sjohnr commented Jan 4, 2023

@mdeinum

It isn't so much the naming but the additional work needed to get CSRF protection to work in a reactive setup

Gotcha. Perhaps I wasn't clear in my explanation, but I mention the name _csrf because that's the attribute name used by the reactive CsrfRequestDataValueProcessor. So if we were to do I as suggested...

I wonder if it would be beneficial to instead change the CsrfWebFilter to also add a _csrf attribute that exposes a CsrfToken that can defer access to the real Mono<CsrfToken>?

... I believe it would work "out of the box." What do you think?

@sjohnr
Copy link
Member

sjohnr commented Jan 13, 2023

@mdeinum do you have thoughts on my comment above?

@sjohnr sjohnr added in: web An issue in web modules (web, webmvc) status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 13, 2023
@mdeinum
Copy link
Contributor Author

mdeinum commented Jan 16, 2023

I suspect that would make it work yes, as the mismatch seems to come from the naming (and that is why the copying is needed).

Which would also mean I would have to rewrite (read delete some stuff) from my book :). (Although that is a good thing in this case as it makes things easier and just work (tm)).

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants