Skip to content

fix: override user-set x-amzn-{lambda,request}-context headers#286

Merged
bnusunny merged 1 commit intoaws:mainfrom
fluxth:context-headers-prune
Sep 30, 2023
Merged

fix: override user-set x-amzn-{lambda,request}-context headers#286
bnusunny merged 1 commit intoaws:mainfrom
fluxth:context-headers-prune

Conversation

@fluxth
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

fluxth commented Sep 30, 2023

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

Copy link
Copy Markdown
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
Copy Markdown
Contributor

@fluxth Thanks for the contribution!

@bnusunny bnusunny merged commit 4b59b10 into aws:main Sep 30, 2023
@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