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

Update CSRF filter to support multipart/form-data payloads #28383

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Oct 4, 2022

Fixes #28379

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member Author

I can probably add another test template to produce a multipart/form-data

@sberyozkin sberyozkin force-pushed the csrf_multipart_media_type branch from a6ecc98 to 26999b1 Compare October 7, 2022 18:55
@sberyozkin sberyozkin marked this pull request as draft October 7, 2022 18:55
@sberyozkin
Copy link
Member Author

@FroMage CSRF feature can only partially help when dealing with multipart/form-data - it will help with generating a token and getting it injected into a user template. This is because, as opposed to a form urlencoded payload, reading the multipart payload in the filter and then restoring is not realistic, as most likely it will have file(s) submitted.

But indeed even with large forms reading and restoring can be of concern, so we already recommend in docs in such cases to do the verification in the actual endpoint code - check the form token param against the cookie, but it looks like it does not work yet with RestEasy reactive, so I'll open another issue

@quarkus-bot

This comment has been minimized.

@sberyozkin sberyozkin force-pushed the csrf_multipart_media_type branch from 26999b1 to 1113312 Compare October 7, 2022 21:51
@sberyozkin
Copy link
Member Author

Note by default the filter will cover the whole application URI space for POST payloads, and enforce that the forms are url encoded, so a few properties exist to specify which path has to be protected (I made it a Set in this PR), and whether, when the filter verification is enabled, some other payloads should be allowed which is useful in a test where a token verification is required for one method, but for the other one it is delayed till the endpoint is invoked

@sberyozkin sberyozkin force-pushed the csrf_multipart_media_type branch from 1113312 to 25c7b05 Compare October 10, 2022 10:52
@sberyozkin
Copy link
Member Author

Tests look good now after the fix from Georgios

@FroMage
Copy link
Member

FroMage commented Oct 10, 2022

From your code, I don't see where you verify the token automatically for anything but url-encoded forms. It doesn't look like you added support for multipart forms.

As for restoring the forms, I thought you didn't need to do that, since Vert.x was responsible for reading and caching them.

@sberyozkin
Copy link
Member Author

@FroMage Hi,

From your code, I don't see where you verify the token automatically for anything but url-encoded forms. It doesn't look like you added support for multipart forms.

I agree, I was just about to rename the PR, which currently is to verify that the CSRF filter can only help with generating the tokens and having them injected in the template. But...

As for restoring the forms, I thought you didn't need to do that, since Vert.x was responsible for reading and caching them.

changes everything :-), if that is the case then I'll be happy to get rid of the code which restores the forms and that will also help with auto-verification in case of the multipart/form-data, let me try

@sberyozkin sberyozkin force-pushed the csrf_multipart_media_type branch from 25c7b05 to 6a8176a Compare October 10, 2022 16:53
@sberyozkin sberyozkin marked this pull request as ready for review October 10, 2022 16:54
@sberyozkin
Copy link
Member Author

sberyozkin commented Oct 10, 2022

@FroMage This is very good, thanks, I was able to get rid of the code which restores the form stream, glad you've pointed it out :-)

The tokens in multipart/form-data can also be auto-verified, I've updated the test to check that the flag which is set by the filter in case of the successful verification is available on the context and also kept the code verifying the token in the application code if someone prefers it

@quarkus-bot

This comment has been minimized.

@sberyozkin sberyozkin force-pushed the csrf_multipart_media_type branch from 6a8176a to d63a8c9 Compare October 11, 2022 16:17
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 11, 2022

Failing Jobs - Building d63a8c9

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18
Quickstarts Compilation - JDK 17 Compile Quickstarts ⚠️ Check → Logs Raw logs

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@sberyozkin sberyozkin merged commit 3b2e75e into quarkusio:main Oct 14, 2022
@quarkus-bot quarkus-bot bot added this to the 2.14 - main milestone Oct 14, 2022
@sberyozkin sberyozkin deleted the csrf_multipart_media_type branch October 14, 2022 12:11
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Oct 14, 2022
@gsmet gsmet modified the milestones: 2.14 - main, 2.13.3.Final Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSRF: support multipart forms as well
3 participants