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

[BUG] Response content-type changed in 2.11 for some endpoints #3718

Closed
cwperks opened this issue Nov 15, 2023 · 1 comment · Fixed by #3717
Closed

[BUG] Response content-type changed in 2.11 for some endpoints #3718

cwperks opened this issue Nov 15, 2023 · 1 comment · Fixed by #3717
Assignees
Labels
bug Something isn't working

Comments

@cwperks
Copy link
Member

cwperks commented Nov 15, 2023

In 2.11, there was a change to introduce a SecurityResponse wrapper around a BytesRestResponse. In the method to cast the SecurityResponse to a RestResponse, the method creates a BytesRestResponse using the constructor without contentType. (Here's the BytesResponseResponse constructor for context). Even if an instance of SecurityResponse is created with a Content-Type header it is being ignored since the constructor of BytesRestResponse will set it at the time the RestResponse is instantiated.

APIs impacted:

  • /_plugins/security/api/authtoken
  • Any Unauthed call now responds with application/json instead of text/plain
@cwperks cwperks added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Nov 15, 2023
@stephen-crawford stephen-crawford self-assigned this Nov 15, 2023
@peternied peternied removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Nov 15, 2023
@stephen-crawford
Copy link
Collaborator

stephen-crawford commented Nov 15, 2023

The change to the SecurityResponse class causes all of the methods hit with asRestResponse() to be converted.

This means that the impacted APIs can be found by moving up a level to where the asRestResponse() method is called inside of the SecurityRestFilter . Then we can look at each of the scenarios inside the wrap method to determine which calls are impacted.

Notably, the wrap method is called on all rest requests so it is more about the actual cases and whether they trigger the if-statements that lead to the asRestResponse than anything else.

The first asRestResponse is called when we try to determine whether the response has been cached. This is then based in the SecurityHttpServerTransport logic. Looking at this, it seems that we try to signify early responses for handling of decompression by identifying request attributes which are set in the Netty4HttpRequestHeaderVerifier.

The second asRestResponse happens when we check whether the request is authenticated and skip any that are not. This covers the scenarios that Craig mentioned above with his second bullet about unauthed calls.

The third asRestResponse happens when a request is denied. This is what it sounds like, we check whether a request is not allowlisted (if allowlisting is enabled). We return false if it is not allowlisted and then set this optional which will cause the asRestResponse call.

The final asRestResponse happens when a response is queued for delivery. This occurs when a response is placed in the response queue for a channel based on the SecurityRequestChannel interface. If the channel has a queued response we send a response by grabbing the queued response from the channel and calling asRestResponse on it.


APIs impacted appear to be any calls falling under the above scenarios and then specifically these classes:

AuthTokenProcessorHandler -- explicitly requires JSON response.

HTTPSpnegoAuthenticator adds a SecurityResponse.CONTENT_TYPE_APP_JSON header.

AbstractHTTPJWTAuthenticator -- constructs a new response with the <String, String> map.

...

This list is a work in progress. Additional usage can be found through searching for Optional<SecurityResponse>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants