fix: return 400 instead of 500 for malformed Content-Type headers on token endpoint - #50200
Conversation
…token endpoint Malformed Content-Type headers (e.g. 'invalid/@@##', empty string, XSS payloads) sent to the OAuth2 token endpoint cause RESTEasy's MediaTypeHeaderDelegate to throw an IllegalArgumentException. The KeycloakErrorHandler was catching this but returning HTTP 500 with 'unknown_error'. Per RFC 6749 section 5.2, the token endpoint must return HTTP 400 with an 'invalid_request' error for malformed requests. Closes keycloak#49964 Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to make Keycloak’s OAuth 2.0 token endpoint return an RFC 6749 §5.2-compliant 400 Bad Request with an invalid_request error when the incoming Content-Type header is malformed (instead of bubbling up to a 500 Internal Server Error).
Changes:
- Update the global REST exception handler to map
IllegalArgumentExceptionto HTTP 400 andinvalid_request. - Add a new integration test class exercising malformed/empty/XSS-like
Content-Typeheaders against the token endpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
services/src/main/java/org/keycloak/services/error/KeycloakErrorHandler.java |
Maps IllegalArgumentException to 400 + invalid_request in the global exception mapper. |
tests/base/src/test/java/org/keycloak/tests/error/MalformedContentTypeTest.java |
Adds integration tests to ensure malformed Content-Type no longer produces a 500. |
| if (throwable instanceof IllegalArgumentException) { | ||
| return Response.Status.BAD_REQUEST; | ||
| } |
| if (throwable instanceof IllegalArgumentException) { | ||
| return OAuthErrorException.INVALID_REQUEST; | ||
| } |
| HttpPost post = new HttpPost(tokenUri()); | ||
| post.setHeader("Content-Type", "</>\"alert(1)"); | ||
| post.setEntity(new StringEntity("grant_type=password&client_id=admin-cli&username=admin&password=admin")); |
…ng errors Signed-off-by: Gautam Kumar <gautamkumarofficial@users.noreply.github.com>
|
Thanks Copilot for the review! Just to clarify — the concerns about mapping all The code now uses The XSS payload from the issue ( |
|
@GautamKumarOffical Thanks for the PR. I think the issue is not "protocol specific" as it probably applies to more Keycloak endpoints (not just to the OIDC token endpoint). Hence seems to me that cloud-native might be more appropriate team to handle this to eventually do something at RestEasy/quarkus layer instead of handling this at the level of token endpoint. |
|
Thanks for the feedback, @mposolda. You're right — this isn't specific to the token endpoint. The same malformed Content-Type issue could hit other RestEasy endpoints. That said, the token endpoint is where this surfaces most often in practice (OIDC clients sending form-urlencoded to the token endpoint is the classic case). The fix here is minimal — just a validation guard before the existing parsing logic. Happy to close this PR if you'd prefer to handle it at the RestEasy/quarkus layer instead. Or if you'd like, I can broaden the scope to cover other endpoints too. Let me know which direction you'd prefer. |
michalvavrik
left a comment
There was a problem hiding this comment.
Thank you for your PR. I think we need a decision from maintainers on what to do and how to deal with this issue. It doesn't make sense to have 2 opened PRs on the same issue, but they are 2 different approaches and I like this one better. Please wait with addressing my the comment I left until there is the decision in the linked issue or here.
| return "conflict"; | ||
| } | ||
|
|
||
| if (throwable instanceof IllegalArgumentException && isMediaTypeParsingError(throwable)) { |
There was a problem hiding this comment.
I wonder if this breaks when run with the current Quarkus main considering I fixed the logic upstream to get 415, now Quarkus REST do not throw IllegalArgumentException.
| } | ||
|
|
||
| if (throwable instanceof IllegalArgumentException && isMediaTypeParsingError(throwable)) { | ||
| return OAuthErrorException.INVALID_REQUEST; |
There was a problem hiding this comment.
Is it desirable to return the invalid request description for every Keycloak request with a malformed content type? Should admin API requests receive 415 instead?
Since I believe this would be treated at most as a hardening it is ok to wait to pick up the fix from the quarkus side - it appears to be targeted for backporting, so we should get it fairly soon. |
@shawkins @michalvavrik Thanks, so does it mean that we can close both related PRs #50200 and #50229 and mark the related issue #49964 with the label |
I don't think so. Pedro said that specs require 400 and the invalid request in the desc. What I did in Quarkus is 415 because that is what they already do for a top level resources. IMO it is an improvement to previous 500, but I couldn't argue there for 400. AFAICT you need an exception mapper explicitly for the token endpoint, but I didn't look into this more.... |
Right, and ideally it would be endpoint based as the KeycloakErrorHandler affects admin endpoints and a few other things beyond OAuth as well. We should probably put more thought into this as it relates to admin api v2 - because as it stands we'll have two different styles of error handling - oauth for uncaught exceptions, or the explicit handling we have in the admin layer. |
|
Hello @GautamKumarOffical, I looked into this today and we need the solution for only selected endpoints (token endpoint, device authorization endpoint and logout endpoints seems to be only affected ATM) which is possible. I agree with comments that we should wait for Quarkus 3.33.3. Tests can look like this 74f5751. My proposed way would be I propose to close this one and wait. However if you prefer to research more focused solutions, feel free to rework this one. I'll leave it up to you. |
|
I just spend whole day on this and this PR is just hiding actual bugs in Keycloak and Quarkus. Let's go with #51410 or re-discuss desired approach in the linked issue. Thank you for the time you invested into this PR. |
Problem
Malformed headers (e.g. , empty strings, XSS payloads) sent to the OAuth 2.0 token endpoint cause RESTEasy's to throw an . The catches this but maps it to HTTP 500 with an response body.
Per RFC 6749 section 5.2, the token endpoint MUST return HTTP 400 with an parameter (e.g. ) for malformed requests.
Fix
Added handling in to return HTTP 400 Bad Request with error code instead of HTTP 500 Internal Server Error with .
Changes:
How to Reproduce
Before: HTTP 500 with
After: HTTP 400 with
Closes #49964