snok / asgi-correlation-id

Request ID propagation for ASGI apps
MIT License
369 stars 29 forks source link

The headers with same key are overwritten #22

Closed lakshaythareja closed 2 years ago

lakshaythareja commented 2 years ago

Starlette responses adds cookies as headers with the same key. self.raw_headers.append((b"set-cookie", cookie_val.encode("latin-1")))

In your middleware handle outgoing request over writes the same key with only the last value.

        async def handle_outgoing_request(message: Message) -> None:
            if message['type'] == 'http.response.start':
                headers = {k.decode(): v.decode() for (k, v) in message['headers']}
                headers[self.header_name] = correlation_id.get()
                headers['Access-Control-Expose-Headers'] = self.header_name
                response_headers = Headers(headers=headers)
                message['headers'] = response_headers.raw
            await send(message)
sondrelg commented 2 years ago

If I recall correctly, the intended behaviour was for headers = {k.decode(): v.decode() for (k, v) in message['headers']} to capture existing headers, and to add to that before writing back to the message headers. In other words, it should add, but not change unrelated header values.

I'm not sure I understand the issue 100%, can you elaborate on what exactly the issue is?

lakshaythareja commented 2 years ago

Lets say my header list contains a list with keys as set cookies and values as access_token, refresh_token(diff entries). Then it just keeps the last refresh token and doesn't keep the first value of access token.

sondrelg commented 2 years ago

But this middleware does not set a set-cookie header. Are you referring to another header being overwritten?

lakshaythareja commented 2 years ago

Yes it overrides other headers.

On Sat, 12 Mar, 2022, 9:51 pm Sondre Lillebø Gundersen, < @.***> wrote:

But this middleware does not set a set-cookie header. Are you referring to another header being overwritten?

— Reply to this email directly, view it on GitHub https://github.com/snok/asgi-correlation-id/issues/22#issuecomment-1065911580, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMH5VS3NHXELFSYXETYH5ZLU7S77VANCNFSM5PWM4WRQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

sondrelg commented 2 years ago

Could you open a PR with a failing test to illustrate what is wrong? I'm not able to replicate the issue on my own

lakshaythareja commented 2 years ago

You can check this image, here in message there are 4 headers with key as set-cookie and different values: image

headers = {k.decode(): v.decode() for (k, v) in message['headers']} replaces the same key with different values with just 1 key and 1 value.

sondrelg commented 2 years ago

I see. That seems worth fixing! Would you be interested in creating a PR with a fix? 🙂

lakshaythareja commented 2 years ago

https://github.com/snok/asgi-correlation-id/pull/25

lakshaythareja commented 2 years ago

@sondrelg can you please merge the PR and release a new version I will then start using it.

sondrelg commented 2 years ago

Yes I'll review and release it early tomorrow morning (~15 hours from now) 👍 Thanks for the effort @lakshaythareja

sondrelg commented 2 years ago

v1.1.3 should be out within the next minute, so will consider this closed 👍