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

fix: do not duplicate headers (single val, multi val) on return #852

Merged
merged 3 commits into from
Apr 1, 2024

Conversation

jreppnow
Copy link
Contributor

@jreppnow jreppnow commented Apr 1, 2024

Issue

This is the Rust equivalent to these changes made in the Go runtime library for lambda HTTP: awslabs/aws-lambda-go-api-proxy#73

The issue here is that the API Gateway does not properly merge single-value headers and multi-value returned by the Lambda and instead returns both to the client. Since this library just puts the same headers into both, this means that every header is returned twice (more or less, in the case of actual multi-value headers, only the first element is duplicated). In the case of CORS, this is fatal, as browsers generally reject duplicate CORS headers. Unless this change can go through, we would be forced to turn off application-level CORS handling and rely on the API gateway instead (which we would really like to avoid..).

Description of changes:

Adapts the code to return all headers as multi-value headers only, returning an empty HeaderMap for the single-value headers.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

@jreppnow
Copy link
Contributor Author

jreppnow commented Apr 1, 2024

If required, I could also provide logic that actually splits the headers into single-value and multi-value headers and returns them as they should be returned.

Edit: Actually, this seems non-trivial and this current simpler solution is most likely preferable.

@calavera
Copy link
Contributor

calavera commented Apr 1, 2024

According to the docs, this change is not necessary:

If the same key-value pair is specified in both, only the values from multiValueHeaders will appear in the merged list.

It'd be necessary if you constructed the APIGW response event with different keys in the headers.

Am I missing anything?

@jreppnow
Copy link
Contributor Author

jreppnow commented Apr 1, 2024

Yup, that's the specification, but the real implementation does not behave that way unfortunately. See the linked PR above as well. Obviously, this is a workaround, although the "correctness" of the current implementation is also questionable, as there is no need to specify the same headers twice either.

We have a web app that is supposed to be deployed as a Lambda with an API gateway in front, and we can't call its requests from Javascript, as the Browser treats duplicate CORS headers as a CORS error and blocks the requests.

We are using https://github.com/lazear/axum-aws-lambda, which in turn calls this library. Seeing that the issue has not been fixed on AWS side for the at least 4 years that it has been known, this workaround seems like the most practical solution for the present time, although I can understand the hesitation considering the clear wording of the specification stating that the current implementation is fine.

@jreppnow
Copy link
Contributor Author

jreppnow commented Apr 1, 2024

In fact, you can inspect the issue in action if you navigate to this endpoint, which is served via the Lambda in question:

https://compass.api.labbase.jp/health_check

You will note how a lot of the response header (including the CORS-relevant allow-credentials) are duplicated. The ones not duplicated are the ones added or updated by CloudFront, which is sitting in-between, or the API Gateway itself.

HTTP/3 200 
content-type: text/plain; charset=utf-8
content-length: 3
alt-svc: h3=":443"; ma=86400
date: Mon, 01 Apr 2024 15:31:28 GMT
access-control-allow-credentials: true
access-control-allow-credentials: true
vary: origin, access-control-request-method, access-control-request-headers
vary: origin, access-control-request-method, access-control-request-headers
apigw-requestid: VjX0jhuftjMEJcg=
x-cache: Miss from cloudfront
via: 1.1 2992eaea59550bad6012c4c656826fac.cloudfront.net (CloudFront)
x-amz-cf-pop: NRT20-C3
x-amz-cf-id: 54S0EZPOSaVsnOT8tCi6hBRXNbwQeQbTeg-UmPIUA-VJnujRywq_Zw==

@calavera
Copy link
Contributor

calavera commented Apr 1, 2024

Well, that's unfortunate. The change looks safe.

Copy link
Contributor

@calavera calavera left a comment

Choose a reason for hiding this comment

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

LGTM

@calavera calavera merged commit 5b1de1d into awslabs:main Apr 1, 2024
4 checks passed
@jreppnow jreppnow deleted the feat/reppnj/avoid-duplicate-headers branch April 2, 2024 04:18
@jreppnow
Copy link
Contributor Author

jreppnow commented Apr 2, 2024

@calavera Thanks a lot for the fast response! We have re-built our application with a git dependency on this crate (and the intermittent one) and deployed it. You can confirm that the header duplication has indeed been resolved by visiting the above link again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants