Skip to content

#185 ALB/ELB set cookies when multiValueHeaders is not set#186

Merged
jordaneremieff merged 1 commit intoKludex:mainfrom
nathanglover:185-fix-set-cookies
Apr 20, 2021
Merged

#185 ALB/ELB set cookies when multiValueHeaders is not set#186
jordaneremieff merged 1 commit intoKludex:mainfrom
nathanglover:185-fix-set-cookies

Conversation

@nathanglover
Copy link
Copy Markdown
Contributor

I believe this fixes #185 by adding back the code used in 0.11.0.

def handle_headers(
    self,
    response_headers: List[List[bytes]],
) -> Tuple[Dict[str, str], Dict[str, List[str]]]:
    headers, multi_value_headers = self._handle_multi_value_headers(
        response_headers
    )
    if "multiValueHeaders" not in self.trigger_event:
        # If there are multiple occurrences of headers, create case-mutated
        # variations: https://github.com/logandk/serverless-wsgi/issues/11
        for key, values in multi_value_headers.items():
            if len(values) > 1:
                for value, cased_key in zip(values, all_casings(key)):
                    headers[cased_key] = value

        multi_value_headers = {}

    return headers, multi_value_headers

Let me know if anything else should be added or removed.

Comment on lines +290 to +295
"headers": {
"content-type": "text/plain; charset=utf-8",
"set-cookie": "cookie1=cookie1; Secure",
"Set-cookie": "cookie2=cookie2; Secure",
},
"multiValueHeaders": {},
Copy link
Copy Markdown
Contributor

@jurasofish jurasofish Apr 18, 2021

Choose a reason for hiding this comment

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

Only one of headers and multiValueHeaders should be specified
"You must use multiValueHeaders if you have enabled multi-value headers and headers otherwise"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The documentation you linked makes it sound like all headers should be in multiValueHeaders, even those with a single value, if it is enabled.

I'm not sure if 0.11.0 did this or did what the current "main" branch does and only send the headers that have multiple values in multiValueHeaders.

I think using one or the other may be the way to go, it's a bit easier to understand what is going on, and is how the ALB documentation reads online. Let us know what AWS says if anything.

Comment on lines +46 to +47
if multi_value_headers:
event["multiValueHeaders"] = {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

again this specifies headers and multiValueHeaders

@jurasofish
Copy link
Copy Markdown
Contributor

The use of different case to send multiple headers makes me a little nervous.
I would say it's undefined - or at least unclear - how ALB handles that.
I've just opened a case with AWS support to ask about it.

@jordaneremieff
Copy link
Copy Markdown
Collaborator

Thanks @nathanglover for the PR and @jurasofish for the discussion.

Regarding the headers in the response, there was a previous PR (ultimately not merged) where this was discussed and the user reached the similar conclusion of this PR, so I think I will merge it but we can revisit later specifically if there are issues.

@jordaneremieff jordaneremieff merged commit 881568d into Kludex:main Apr 20, 2021
khamaileon pushed a commit to khamaileon/mangum that referenced this pull request Jan 13, 2024
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.

0.11.0 Regression - ALB/ELB - Properly handle multiple set cookies in output when multiValueHeaders is not set

4 participants