fix(#8868): don't forward all headers for get requests #8924
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Added an e2e test to demonstrate fixed behavior, and an opposite e2e test here: https://github.com/medic/cht-core/compare/test-login?expand=1 that shows the 400 request: https://github.com/medic/cht-core/actions/runs/8274351579/job/22639815092#step:17:286 |
|
Hi @latin-panda Would you mind reviewing this? Thanks a lot! |
latin-panda
approved these changes
Mar 14, 2024
dianabarsan
added a commit
that referenced
this pull request
Mar 14, 2024
Node 19 adds keep-alive header to requests by default. This makes the content-length header very significant and important because it enables the server to know when one request has been fully received and another one is sent through the same connection. When authenticating a user, we take all headers from the original user's request and pass them to a GET /_session request, presumably so we don't have to filter out all the ways through which the user can authenticate (cookie, authorization etc). This meant that if the original request was a POST, and it had a content-length header, this header was also forwarded. This meant that when the connection would later be reused, the server (haproxy in this case) would truncate the previous's request content-length number of characters from the next request, believing the previous request was not finished. Obviously, this new truncated request was invalid and generated a 400 status code. This only seems to happen when accessing API directly or through the AWS load balancer, and never when running requests through nginx. To avoid next request truncation, I am no longer forwarding content-length headers in the session request. #8868 (cherry picked from commit 831bd6e)
dianabarsan
added a commit
that referenced
this pull request
Mar 15, 2024
Node 19 adds keep-alive header to requests by default. This makes the content-length header very significant and important because it enables the server to know when one request has been fully received and another one is sent through the same connection. When authenticating a user, we take all headers from the original user's request and pass them to a GET /_session request, presumably so we don't have to filter out all the ways through which the user can authenticate (cookie, authorization etc). This meant that if the original request was a POST, and it had a content-length header, this header was also forwarded. This meant that when the connection would later be reused, the server (haproxy in this case) would truncate the previous's request content-length number of characters from the next request, believing the previous request was not finished. Obviously, this new truncated request was invalid and generated a 400 status code. This only seems to happen when accessing API directly or through the AWS load balancer, and never when running requests through nginx. To avoid next request truncation, I am no longer forwarding content-length headers in the session request. #8868 (cherry picked from commit 831bd6e)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Node 19 adds keep-alive header to requests by default. This makes the
content-lengthheader very significant and important because it enables the server to know when one request has been fully received and another one is sent through the same connection.When authenticating a user, we take all headers from the original user's request and pass them to a GET /_session request, presumably so we don't have to filter out all the ways through which the user can authenticate (cookie, authorization etc). This meant that if the original request was a POST, and it had a
content-lengthheader, this header was also forwarded. This meant that when the connection would later be reused, the server (haproxy in this case) would truncate the previous's requestcontent-lengthnumber of characters from the next request, believing the previous request was not finished. Obviously, this new truncated request was invalid and generated a 400 status code.This only seems to happen when accessing API directly or through the AWS load balancer, and never when running requests through nginx.
To avoid next request truncation, I am no longer forwarding
content-lengthheaders in the session request.#8868
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.