Skip to content

Test that malformed HTTP request is not validated #95886

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

Merged

Conversation

albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented May 5, 2023

This PR tests that malformed HTTP requests that
fail at the decoding stage don't go through validation and
are further dispatched as bad requests.

Related: #95112

@albertzaharovits albertzaharovits added the >test Issues or PRs that are addressing/adding tests label May 5, 2023
@albertzaharovits albertzaharovits changed the title Test malformed request Test malformed HTTP request not validated May 5, 2023
@albertzaharovits albertzaharovits added >non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels May 5, 2023
@albertzaharovits albertzaharovits changed the title Test malformed HTTP request not validated Test that malformed HTTP request is not validated May 5, 2023
@albertzaharovits albertzaharovits marked this pull request as ready for review May 5, 2023 15:13
Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - However, I also don't have enough experience to advise on the quality or coverage of the tests. The comments help and to the best of my ability they appear solid but if you are looking for critical feedback on details of what/how to test you may want to seek an additional reviewer (but that is not necessary since these are just tests).

@albertzaharovits albertzaharovits merged commit fc2fa4d into elastic:main May 8, 2023
@albertzaharovits albertzaharovits deleted the test-malformed-request branch May 8, 2023 07:16
@albertzaharovits
Copy link
Contributor Author

@jakelandis

I also don't have enough experience to advise on the quality or coverage of the tests.

It's tricky to asses the coverage when interfacing so closely with the netty code, as the API surface is large and not explicitly defined (it's only defined by the object types, and their order, as they flow through the pipeline from request decoding through aggregation).

This PR only covers a single case/observation: when there's a HTTP decoding error, a new FullHttpRequest is generated rather than a HttpRequest, as is normally the case.

So, the tests in Netty4HttpHeaderValidatorTests make sure to cover that the header validator works correctly with FullHttpRequests which might or might not have a decoding error attached. Working correctly in this case means that FullHttpRequests are validated just as HttpRequests are, including the case where there's a decoding error attached, in which case validation/authentication is not performed (because validation might choke on an invalid request and the request will be rejected irrespective of the validation outcome anyway - the test in Netty4HttpPipeliningHandlerTests).

The tests in SecurityNetty4HttpServerTransportTests are a sort of bonus ITs (more end-to-end). I've tested a couple of invalid request cases that should fail during decoding and ensured that validation is not invoked for those, while them being dispatched as bad requests (will return an error rather than execute the associated endpoint handler).

albertzaharovits added a commit to albertzaharovits/elasticsearch that referenced this pull request Jun 19, 2023
This PR tests that malformed HTTP requests that
fail at the decoding stage don't go through validation
and are further dispatched as bad requests.

Related: elastic#95112
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >test Issues or PRs that are addressing/adding tests v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants