awslabs / aws-lambda-rust-runtime

A Rust runtime for AWS Lambda
Apache License 2.0
3.3k stars 335 forks source link

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

Closed jreppnow closed 5 months ago

jreppnow commented 5 months ago

Issue

This is the Rust equivalent to these changes made in the Go runtime library for lambda HTTP: https://github.com/awslabs/aws-lambda-go-api-proxy/pull/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

jreppnow commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

jreppnow commented 5 months ago

@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.