Make Content-Type exceptions thrown early by RESTEasy RFC compliant - #50229
Make Content-Type exceptions thrown early by RESTEasy RFC compliant#50229niels-van-nerum wants to merge 4 commits into
Conversation
…mpliant RESTEasy Reactive catches malformed Content-Type early in pre-validation, before our Exception handlers/mappers. This causes exceptions to be thrown in a non-compliant fashion. This commit adds a Vert.x handler that does the same check RESTEasy Reactive would, and maps it to an RFC 6749 compliant response. AI was used to understand the codebase, and wrote most of the code, while I did the debugging, and traversing paths for correctness. Closes keycloak#49964 Signed-off-by: Niels Van Nerum <vannerumniels@icloud.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure malformed Content-Type headers are rejected early (before RESTEasy Reactive initializes exception mappers), returning a consistent, RFC/OAuth-aligned 400 error response instead of leaking lower-level parsing behavior.
Changes:
- Added a parameterized integration test covering malformed
Content-Typevalues against the token endpoint. - Introduced a Vert.x
RoutingContextfilter to reject malformedContent-Typeheaders early with a JSON OAuth error payload. - Registered the new filter via a Quarkus build step so it runs in the HTTP handler chain.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/base/src/test/java/org/keycloak/tests/oauth/TokenInputValidationTest.java | Adds coverage for token endpoint behavior with malformed Content-Type headers. |
| quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/services/ContentTypeExceptionFilter.java | Adds an early Vert.x filter that rejects malformed Content-Type values with a JSON error. |
| quarkus/deployment/src/main/java/org/keycloak/quarkus/deployment/KeycloakProcessor.java | Registers the new filter into the Quarkus HTTP filter chain at build time. |
| String contentType = routingContext.request().getHeader("Content-Type"); | ||
|
|
||
| if (contentType != null && !contentType.isBlank() && !isParseable(contentType)) { |
| routingContext.response() | ||
| .setStatusCode(400) | ||
| .putHeader("Content-Type", MediaType.APPLICATION_JSON) | ||
| .end(MALFORMED_CONTENT_TYPE_RESPONSE); |
| import jakarta.ws.rs.core.MediaType; | ||
| import org.jboss.logging.Logger; | ||
|
|
||
| public class ContentTypeExceptionFilter implements Handler<RoutingContext> { |
Signed-off-by: Niels Van Nerum <vannerumniels@icloud.com>
Signed-off-by: Niels Van Nerum <vannerumniels@icloud.com>
|
I put in two more commits to make things a bit better, we can squash when the PR is signed off. |
| assertEquals(400, response.getStatusCode()); | ||
| assertEquals(OAuthErrorException.INVALID_REQUEST, response.getError()); | ||
| } |
| @@ -0,0 +1,39 @@ | |||
| package org.keycloak.quarkus.runtime.services; | |||
| import io.vertx.core.Handler; | ||
| import io.vertx.ext.web.RoutingContext; | ||
| import jakarta.ws.rs.core.MediaType; | ||
| import org.jboss.logging.Logger; | ||
|
|
||
| public class RejectMalformedContentTypeFilter implements Handler<RoutingContext> { | ||
| private static final Logger LOGGER = Logger.getLogger(RejectMalformedContentTypeFilter.class); | ||
| private static final String MALFORMED_CONTENT_TYPE_RESPONSE = | ||
| "{\"error\":\"invalid_request\",\"error_description\":\"Invalid Content-Type header\"}"; |
…ponse Signed-off-by: Niels Van Nerum <vannerumniels@icloud.com>
michalvavrik
left a comment
There was a problem hiding this comment.
Thank you for your PR. IMO we need maintainer to decide which way to get, there is no point having 2 PRs for the same issue, so let's wait with addressing the comment I left.
| } | ||
|
|
||
| private String errorResponse() { | ||
| OAuth2ErrorRepresentation error = new OAuth2ErrorRepresentation("invalid_request", ERROR_DESCRIPTION); |
There was a problem hiding this comment.
Is this correct error description for every Keycloak request with a wrong content type? Should admin API endpoints receive 415 instead?
|
Thank you for proposing this fix @niels-van-nerum but global filter is not desirable solution. We will go a different way. Discussion of how this issue should be handled belongs to the linked issue, not into this PR. I left there comments, if you have different proposal, please let us know there. Thanks again. |
|
@michalvavrik Thanks for the follow up! No worries in closing my PR. I realized I stepped into a bigger issue then I initially thought off, and let the more capable people do the work ;) |
|
thanks @niels-van-nerum |
I wrote a failing test first (TDD-style), then added ContentTypeExceptionFilter, modelled after the other filters in the same package.
RESTEasy Reactive validates the Content-Type header during dispatch, before path matching and before any JAX-RS exception mappers are initialized. So the IllegalArgumentException from MediaTypeHeaderDelegate.internalParse() escapes the JAX-RS pipeline entirely and lands in Quarkus' generic error handler as a 500.
The fix validates the header one layer lower, at the Vert.x routing layer, using the same public API (MediaType.valueOf()) that RESTEasy uses internally. Any value RESTEasy would accept passes through, any value it would reject is caught early and returned as RFC 6749 compliant 400.
Resolves #49964