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

JaxRS Security wrong content-type #5998

Closed
SlyngDK opened this issue Dec 6, 2019 · 11 comments · Fixed by #10545
Closed

JaxRS Security wrong content-type #5998

SlyngDK opened this issue Dec 6, 2019 · 11 comments · Fixed by #10545
Labels
area/security kind/bug Something isn't working
Milestone

Comments

@SlyngDK
Copy link
Contributor

SlyngDK commented Dec 6, 2019

Describe the bug
When security is enabled, and you are calling a JaxRS endpoint, without any matching roles.
The "Forbidden" is return as plain text, but content-type is "application/json".

Expected behavior
Returned with content-type matching content.

Actual behavior
The "Forbidden" is return as plain text, but content-type is "application/json".

To Reproduce
Reproducer can be found in this repo: https://github.com/SlyngDK/quarkus-jaxrs-security-wrong-content-type

Configuration

See reproducer ^^^

Environment (please complete the following information):

  • Output of java -version: openjdk version "1.8.0_232"
  • Quarkus version or git rev: 1.0.1.Final

Additional context
This there changed some thing about default returned Content-Type???
Maybe this is also affecting other areas.
I have seen same problem with a ContainerRequestFilter calling requestContext.abortWith(), which not setting the MediaType on the response, changed with the new versions.

@SlyngDK SlyngDK added the kind/bug Something isn't working label Dec 6, 2019
@geoand
Copy link
Contributor

geoand commented Dec 7, 2019

@sberyozkin @stuartwdouglas I assume this is a bug right? I am not sure what the proper behavior should be in this case.

@sberyozkin
Copy link
Member

@geoand Hi, not sure yet
@SlyngDK what do you mean Forbidden is returned as Plain text ?
Do you see something being returned in the body, beyond the 403 HTTP status ?
Content-Type is probably just redundant...(should not be returned at all)

@geoand
Copy link
Contributor

geoand commented Dec 8, 2019

Thanks for checking @sberyozkin !

@SlyngDK
Copy link
Contributor Author

SlyngDK commented Dec 8, 2019

@sberyozkin Yes the body returned is "Forbidden", which don't match the Content-Type of application/json.
Take a look at the test in the reproducer.
https://github.com/SlyngDK/quarkus-jaxrs-security-wrong-content-type/blob/master/src/test/java/org/acme/quickstart/GreetingResourceTest.java

@Test
    public void testForbiddenEndpoint() {
        given()
                .when()
                .header("Authorization", "Basic " + Base64.getEncoder().encodeToString("user:passwd".getBytes()))
                .get("/hello/forbidden")
                .then()
                .statusCode(403)
                .header("Content-Type", "text/plain")
                .body(is("Forbidden"));
    }

@sberyozkin
Copy link
Member

@SlyngDK Thanks...
Well, at least AFAIK, 403 should not return anything in the body at all, because if it does it raises the issues like the one you have opened. For example, what if the Content-Type is application/xml ?
IMHO rather than going the path of formatting the Forbidden value, it would be better just not return it in the body at all :-)

@sberyozkin
Copy link
Member

I'm nearly 100% sure that the Content-Type was about the content negotiation on the normal, non-exceptional path, example, the client sets Accept: application/json, or as in your case it is a wildcard, JAX-RS figures out from the resource method (in your case it is forbidden producing JSON) that the response ContentType is application/json and sets it on the response HTTP context, and now the security checks kicks in, and ForbiddenException is thrown but we are now on the exceptional path and as such the Content-Type should just be erased and the body should be empty.
@stuartwdouglas do you agree ?

@sberyozkin
Copy link
Member

Alternatively. @SlyngDK, you may be able to intercept it with ExceptionMapper<ForbiddenException> and provide a format you prefer.
But IMHO we should not be starting trying to format Forbidden per the content type set by the client on the normal path

@sberyozkin
Copy link
Member

That said it looks like all you are suggesting is that Content-Type is set to text/plain :-). This can work but note that having Forbidden in the body does not add anything to the information which 403 already provides :-), i.e 403 == Forbidden

@SlyngDK
Copy link
Contributor Author

SlyngDK commented Dec 8, 2019

I created this issue because behavior have changed, to something wrong.
I am just pointing out that, content returned needs to match the Content-Type.

@sberyozkin
Copy link
Member

@SlyngDK I agree that because it is already the way it works, i.e, some extra text is returned in the body, this issue is relevant, thank you. (I'd fix it by not returning anything at all :-) but I realize not everyone may like this solution :-) )

Thanks

@stuartwdouglas
Copy link
Member

I think we could vary this based on the accept header. If the client is expecting a HTML page then we can return 'Forbidden', otherwise we should leave it blank, and ideally provide some way for the user to customise it.

stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Jul 8, 2020
In most cases the client is not a browser.

Fixes quarkusio#5998
stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Jul 8, 2020
In most cases the client is not a browser.

Fixes quarkusio#5998
@sberyozkin sberyozkin added this to the 1.7.0 - master milestone Jul 8, 2020
@gsmet gsmet modified the milestones: 1.7.0 - master, 1.6.1.Final Jul 16, 2020
gsmet pushed a commit to gsmet/quarkus that referenced this issue Jul 16, 2020
In most cases the client is not a browser.

Fixes quarkusio#5998
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants