CSRF: exception thrown when authentication falied #30594
Comments
|
I'm getting the same 500 for all 404 endpoints as well. This is because we throw the |
|
At least in the case of the 404, I think it's correct to not invoke the (post-matching) request filters because no endpoint is matched. As for authentication, not sure. But in any case, the CSRF filter doesn't appear to work in conditions outside of lab testing :( |
|
Hi Steph @FroMage So what happens is, the authentication failure is thrown before the JAX-RS chain is even invoked, therefore the CSRF request filter was skipped, but because it was mapped by the JAX-RS exception mapper, the response is routed through the JAX-RS response chain (which I think is correct from the JAX-RS point of view) and the CSRF response filter is seeing the illegal state since the CSRF request filter was not even invoked. I think it shows that mapping the proactive-authentication exceptions is risky and I don't think it is a CSRF filters problem as such. IMHO the proper solution is to make sure that the mapped authentication failure responses are not routed through the JAX-RS response chain. |
|
If I understand it correctly, and change all occurrences of |
|
Well, all sorts of exceptions can lead to the response chain being invoked while the request chain has been aborted. I gave you 404 as an example, but all sorts of other reasons can cause this, including mismatching content types, other request filters aborting or throwing… So, IMO it's your response filter that should handle creating the CSRF cookie bytes if the request chain wasn't called. Simple ;) |
|
@FroMage But the request filter creates the actual token value which will need to be injected in the response HTML form to match this cookie. |
|
The request filter is useful. Keep it. But it doesn't make sense for the response filter to throw if the request filter hasn't been invoked. If the request filter hasn't been invoked, then the user's endpoint also hasn't been invoked, so nothing will use that CSRF value you generated. So you can safely generate it in the response filter, and set the cookie and move on to another bug ;) |
Not really, no point in generating this cookie if someone was doing GET expecting a form and this form would not be coming due to 401. This cookie should only be created if the response filter knows the token was created by the request filter, otherwise it would be just us adding unnecessary cookies to the client. If we create a new cookie and it was for example a Multipart Form request with the embedded token which failed due to Basic Auth failure, then when the user retries and authenticates correctly, the CSRF request filter will fail the request because now the cookie is new but the form token is old Just doing nothing is safe and simple IMHO - if it was POST form, the same matching cookie and form token would be returned once the authentication succeeds, if it is GET, then no new cookie is required either . Does it sound good to you ? I'll open a PR if we agree |
|
Are these tokens invalidated by submitting a form? |
|
If the verification is required then the token will be expected and matched against the cookie, if it is GET then it will be created for the response form to have it injected. |
|
What I mean is: when are these tokens invalidated? After each form submission? If not, and they're per-session, and set by a cookie, then I don't see the point of avoiding to set it even from responses that didn't go via the request filter chain. The cookie will be set, and sent back on any subsequent GET/POST request and you won't have to recreate it. |
|
@FroMage the filter does not invalidate the tokens. It requires them be present in the form payload if the verification is required. If the verification is required:
So how this cookie will be used ? |
|
Well it will be used by whatever client code needs to extract the token, and it won't have to be re-created at the next request. |
|
@FroMage The client is the browser showing the form to the user. OK, let me try various scenarios:
I'm not sure what we can't agree upon, is the request filter did not run, just don't try to compensate by running the response filter in any case, this is the right approach IMHO |
|
Steph, this should be as simple as https://github.com/sberyozkin/quarkus/tree/csrf_authentication_failure, I'm yet to add a test confirming the exception mapper was used in case of the failed authentication |
|
You seem to be under the impression that a POST will follow a GET immediately, but it won't. You can have applications with mostly GET requests, and some POST some times. The first GET will grant you a cookie and it will remain for the duration of the exchanges. So sure, you can delay the first cookie to the first successful GET, as you propose. But I don't see the point. The idea is to get that cookie to the user, and then forget about it until we need it for a POST. I mean, what you propose isn't wrong or worse than what I'm proposing. But mine seems easier and at least we guarantee that the cookie comes from the first GET request, successful or not. It's simpler to explain and describe and rely on. One thing for sure, the response filter should not throw exceptions and alter the original response. |
|
@FroMage But Steph, did you check the sequence 1 and 2 above. Let me reiterate what will happen if we create a cookie without the request filter running:
Looks like we have some misunderstanding here, my PR is only about not creating a cookie for the first failed GET, because in this case this cookie can not be used anywhere, I honestly can't see how. If you can describe exactly how a cookie created with the first failed GET can be used then I'll be happy to change the PR
Sounds good :-) |
|
@FroMage You are right it won't harm if the cookie is created with the first failed GET, but if we don't see the actual use of such a cookie then it is a cost, even if minor, creating it for every failed GET in Quarkus, that can be avoided, the PR is ready now |
|
There's no cost, it's going to be saved by the client, and sent back for the next request and we won't re-create it for future calls. |
|
Of course cookies sent via a non-200 request are still used by the browser. Why would you think not? |
|
@FroMage Looks like we are going in circles, Can you please type a sequence here, 1., 2., 3. etc, proving creating a cookie in the CSRF response filter without the CSRF request filter running will be useful to the CSRF feature ? |
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [flow-bin](https://github.com/flowtype/flow-bin) ([changelog](https://github.com/facebook/flow/blob/master/Changelog.md)) | devDependencies | minor | [`^0.199.0` -> `^0.200.0`](https://renovatebot.com/diffs/npm/flow-bin/0.199.0/0.200.0) | | [com.rometools:rome](http://rometools.com) ([source](https://github.com/rometools/rome)) | compile | minor | `1.18.0` -> `1.19.0` | | [org.postgresql:postgresql](https://jdbc.postgresql.org) ([source](https://github.com/pgjdbc/pgjdbc)) | build | patch | `42.5.3` -> `42.5.4` | | [org.jsoup:jsoup](https://jsoup.org/) ([source](https://github.com/jhy/jsoup)) | compile | patch | `1.15.3` -> `1.15.4` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.16.2.Final` -> `2.16.3.Final` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.16.2.Final` -> `2.16.3.Final` | --- ### Release Notes <details> <summary>flowtype/flow-bin</summary> ### [`v0.200.0`](flow/flow-bin@9618443...b6c1eb0) [Compare Source](flow/flow-bin@9618443...b6c1eb0) ### [`v0.199.1`](flow/flow-bin@05bb4e3...9618443) [Compare Source](flow/flow-bin@05bb4e3...9618443) </details> <details> <summary>rometools/rome</summary> ### [`v1.19.0`](https://github.com/rometools/rome/releases/tag/1.19.0) [Compare Source](rometools/rome@1.18.0...1.19.0) <!-- Release notes generated using configuration in .github/release.yml at 1.19.0 --> #### What's Changed ##### 🔨 Dependency Upgrades - Bump flatten-maven-plugin from 1.2.7 to 1.3.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#565 - Bump maven-bundle-plugin from 5.1.5 to 5.1.8 by [@​dependabot](https://github.com/dependabot) in rometools/rome#563 - Bump maven-dependency-plugin from 3.3.0 to 3.5.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#602 - Bump maven-deploy-plugin from 2.8.2 to 3.1.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#607 - Bump maven-jar-plugin from 3.2.2 to 3.3.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#574 - Bump maven-javadoc-plugin from 3.3.1 to 3.5.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#609 - Bump maven-scm-plugin from 1.12.2 to 1.13.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#554 - Bump assertj-core from 3.22.0 to 3.24.2 by [@​dependabot](https://github.com/dependabot) in rometools/rome#603 - Bump slf4j-api from 1.7.36 to 2.0.6 by [@​dependabot](https://github.com/dependabot) in rometools/rome#596 ##### Other Changes - Bump actions/setup-java from 3.3.0 to 3.10.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#606 - Bump logback-classic from 1.2.10 to 1.3.5 by [@​PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#611 **Full Changelog**: rometools/rome@1.18.0...1.19.0 </details> <details> <summary>pgjdbc/pgjdbc</summary> ### [`v42.5.4`](https://github.com/pgjdbc/pgjdbc/blob/HEAD/CHANGELOG.md#​4254-2023-02-15-102104--0500) ##### Fixed fix: fix testGetSQLTypeQueryCache by searching for xid type. We used to search for box type but it is now cached. xid is not cached, this nuance is required for the test. fix OidValueCorrectnessTest BOX_ARRAY OID, by adding BOX_ARRAY to the oidTypeName map \[MR [#​2810](https://github.com/pgjdbc/pgjdbc/issues/2810)]\((https://github.com/pgjdbc/pgjdbc/pull/2810). fixes [Issue #​2804](pgjdbc/pgjdbc#2804). fix: Make sure that github CI runs tests on all [MRs #​2809](\(https://github.com/pgjdbc/pgjdbc/pull/2809\)). </details> <details> <summary>quarkusio/quarkus</summary> ### [`v2.16.3.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.3.Final) [Compare Source](quarkusio/quarkus@2.16.2.Final...2.16.3.Final) ##### Major changes - [#​29756](quarkusio/quarkus#29756) - Support custom Flyway credentials/URL ##### Complete changelog - [#​31141](quarkusio/quarkus#31141) - Resolve roles allowed configuration expression after config setup is completed - [#​31129](quarkusio/quarkus#31129) - Fix stuck HTTP2 request when sent challenge has resumed request - [#​31125](quarkusio/quarkus#31125) - Add "keep-alive-enabled" parameter to REST client reactive - [#​31112](quarkusio/quarkus#31112) - Qute - fix assignability check when validating expressions - [#​31099](quarkusio/quarkus#31099) - Use the effective Maven project build config when initializing sources and classes paths for dev mode - [#​31092](quarkusio/quarkus#31092) - Make sure quarkus:go-offline properly supports test scoped dependencies - [#​31077](quarkusio/quarkus#31077) - Qute: regression in template extension method with byte array - [#​31076](quarkusio/quarkus#31076) - Quarkiverse: Install instead of verify - [#​31074](quarkusio/quarkus#31074) - Added quarkus-jms-spi to quarkus bom - [#​31059](quarkusio/quarkus#31059) - Path lookup must also consider interfaces - [#​31046](quarkusio/quarkus#31046) - Simplify Quarkiverse release.yml workflow - [#​31038](quarkusio/quarkus#31038) - Update Instrumentation Processor check logic to match comment - [#​31036](quarkusio/quarkus#31036) - Use CDI when accessing UserTransaction/TransactionManager in QuarkusTransaction - [#​31028](quarkusio/quarkus#31028) - Fix typo in snapstart enable config - [#​31016](quarkusio/quarkus#31016) - Re-initialize platform dependent netty classes/values at runtime - [#​30988](quarkusio/quarkus#30988) - Race condition in SmallRye Config property expansion for [@​RolesAllowed](https://github.com/RolesAllowed) value? - [#​30964](quarkusio/quarkus#30964) - Add ConfigMappings from a builder class to support full hot reload - [#​30961](quarkusio/quarkus#30961) - Error of quarkus:dev when the project.build.directory is overridden by a profile - [#​30960](quarkusio/quarkus#30960) - Register CDI Bean when ConfigMapping is marked as Unremovable - [#​30922](quarkusio/quarkus#30922) - Fix dependency parsing in JBangBuilderImpl - [#​30885](quarkusio/quarkus#30885) - Add concurrency configuration to the GitHub Action workflows - [#​30843](quarkusio/quarkus#30843) - Micrometer-Extension writes wrong URI-Tag when Path-Variables defined at Interface-Level - [#​30672](quarkusio/quarkus#30672) - Avoid creating CSRF cookie if no CSRF token was created - [#​30648](quarkusio/quarkus#30648) - Support passing filename to multipart form data output - [#​30594](quarkusio/quarkus#30594) - CSRF: exception thrown when authentication falied - [#​30570](quarkusio/quarkus#30570) - Set filename for PartItems in MultipartFormDataOutput - [#​30455](quarkusio/quarkus#30455) - Introduce `quarkus.datasource.devservices.init-script-path` - [#​29756](quarkusio/quarkus#29756) - Support custom Flyway credentials/URL - [#​29631](quarkusio/quarkus#29631) - [@​Unremovable](https://github.com/Unremovable) ConfigMapping is still removed - [#​29630](quarkusio/quarkus#29630) - Changes to configmappings not being applied during hot reload - [#​28709](quarkusio/quarkus#28709) - QuarkusTransaction does not fire [@​Initialized](https://github.com/Initialized)(TransactionScoped.class) - [#​24639](quarkusio/quarkus#24639) - configure dedicated db user for database migrations: DML-only user for datasource, but DDL user for migration - [#​23360](quarkusio/quarkus#23360) - "Request has already been read" using vertx + auth - [#​17839](quarkusio/quarkus#17839) - Invalid memory configuration for netty maxDirectMemory in native image </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v2.16.3.Final`](quarkusio/quarkus-platform@2.16.2.Final...2.16.3.Final) [Compare Source](quarkusio/quarkus-platform@2.16.2.Final...2.16.3.Final) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Describe the bug
With the CSRF extension, I have some tests that require authentication and check that I should get a 401 when not authenticated, that now throw a 500 error:
Looking at the stack trace, this looks like we got an authentication failure, went to an exception mapper (
io.quarkiverse.renarde.util.AuthenticationFailedExceptionMapper.myFilter) which led us to the CSRF response filter, without perhaps going through the request filter first?Not sure if this is a bug in the CSRF response filter, or in RESTEasy Reactive where we go through response filters without going through request filters first.
Expected behavior
No response
Actual behavior
No response
How to Reproduce?
No response
Output of
uname -aorverNo response
Output of
java -versionNo response
GraalVM version (if different from Java)
No response
Quarkus version or git rev
No response
Build tool (ie. output of
mvnw --versionorgradlew --version)No response
Additional information
No response
The text was updated successfully, but these errors were encountered: