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: override user-set x-amzn-{lambda,request}-context headers #286

Merged
merged 1 commit into from
Sep 30, 2023

Conversation

fluxth
Copy link
Contributor

@fluxth fluxth commented Sep 29, 2023

Description of changes:

Currently, when a client sets their own x-amzn-{lambda,request}-context headers, they will be included in the request to the app server.

This request to the lambda with the adapter installed:

$ curl -H "x-amzn-request-context: boom" http://localhost:8000

Will result in this request from the adapter to the app server:

GET / HTTP/1.1
[...]
x-amzn-request-context: boom
x-amzn-request-context: [json context from adapter]
x-amzn-lambda-context: [json context from adapter]
[...]

Many implementations of web frameworks will take the first header when you access a header key.

For example, Starlette/FastAPI does this:
https://github.com/encode/starlette/blob/ad02ee6336faadadf97e0c79dd3a91759f1f32a7/starlette/datastructures.py#L562-L567

This patch overrides every user-set x-amzn-{request,lambda}-context headers from the request by JSON context headers from the adapter.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@fluxth fluxth force-pushed the context-headers-prune branch 3 times, most recently from c625a3a to b4bf219 Compare September 29, 2023 09:49
@bnusunny
Copy link
Contributor

Thanks for this PR. This is a validate concern.

However, I would recommend a different implementation:

change req_headers.append() to req_headers.insert(). This will replace previous values if exists.

Currently, when a client sets their own
`x-amzn-{lambda,request}-context` headers, they will be included in the
request to the app server.

This request to the lambda with the adapter installed:

```
$ curl -H "x-amzn-request-context: boom" http://localhost:8000
```

Will result in this request from the adapter to the app server:

```
GET / HTTP/1.1
[...]
x-amzn-request-context: boom
x-amzn-request-context: [json context from adapter]
x-amzn-lambda-context: [json context from adapter]
[...]
```

Many implementations of web frameworks will take the *first* header when
you access a header key.

For example, Starlette/FastAPI does this:
https://github.com/encode/starlette/blob/ad02ee6336faadadf97e0c79dd3a91759f1f32a7/starlette/datastructures.py#L562-L567

This patch overrides every user-set `x-amzn-{request,lambda}-context`
headers from the request by JSON context headers from the adapter.

Signed-off-by: Thitat Auareesuksakul <thitat@flux.ci>
@fluxth fluxth force-pushed the context-headers-prune branch from b4bf219 to ce3c909 Compare September 30, 2023 07:57
@fluxth fluxth changed the title fix: remove user-set x-amzn-*-context headers fix: override user-set x-amzn-{lambda,request}-context headers Sep 30, 2023
@fluxth
Copy link
Contributor Author

fluxth commented Sep 30, 2023

@bnusunny Thank you for the recommendation!
I've updated the code in commit ce3c909.

Copy link
Contributor

@bnusunny bnusunny left a comment

Choose a reason for hiding this comment

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

LGTM

@bnusunny
Copy link
Contributor

@fluxth Thanks for the contribution!

@bnusunny bnusunny merged commit 4b59b10 into awslabs:main Sep 30, 2023
22 checks passed
@fluxth fluxth deleted the context-headers-prune branch September 30, 2023 10:41
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