Skip to content

fix(event-handler): handle set-cookie header values with multiple attributes #4990

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.

Sign up for GitHub

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

Open
nateiler wants to merge 3 commits into
base: main
Choose a base branch
from
+205 −139
Open

fix(event-handler): handle set-cookie header values with multiple attributes #4990

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
969f0e1
fix: support multiple attributes in set-cookie headers
nateiler Feb 4, 2026
f0fe4ef
fix: multiple header values are comma-separated except set-cookie
nateiler Feb 4, 2026
1bbe3c1
style: removing comments within function
nateiler Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions packages/event-handler/src/http/converters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,26 @@ const webHeadersToApiGatewayV1Headers = (webHeaders: Headers) => {
const headers: Record<string, string> = {};
const multiValueHeaders: Record<string, Array<string>> = {};

const cookies = webHeaders.getSetCookie();
const allCookies: string[] = [];
for (const cookie of cookies) {
allCookies.push(...cookie.split(',').map((v) => v.trimStart()));
}

if (allCookies.length > 1) {
multiValueHeaders['set-cookie'] = allCookies;
} else if (allCookies.length === 1) {
headers['set-cookie'] = allCookies[0];
}

for (const [key, value] of webHeaders.entries()) {
if (key.toLowerCase() === 'set-cookie') {
continue;
}

const values = value.split(/[;,]/).map((v) => v.trimStart());
Copy link
Author

@nateiler nateiler Feb 5, 2026

Choose a reason for hiding this comment

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

Ultimately, splitting on a semicolon seems out of place and is the root of this entire issue. If a goal wasn't to keep backwards compatibility I would suggest removing it and using a comma only. Headers by default a separated by a comma.

const headers = new Headers({
  'accept': 'application/json'
})

headers.append('accept', 'text/html');

const acceptHeaders = headers.get('accept'); // Results in "application/json, text/html"

Copy link
Contributor

@svozza svozza Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

So the reason I wrote it this way was because I was under the impression that cookies were an exception in that they could be separated by a semicolon. I got this from the RFC 6265:

The user agent sends stored cookies to the origin server in the
Cookie header. If the server conforms to the requirements in
Section 4.1 (and the user agent conforms to the requirements in
Section 5), the user agent will send a Cookie header that conforms to
the following grammar:

cookie-header = "Cookie:" OWS cookie-string OWS
cookie-string = cookie-pair *( ";" SP cookie-pair )

It looks like I made a mistake and thought this referred to the Set-Cookie header rather than the Cookie header as indicated in the quote above.

Copy link
Contributor

@svozza svozza Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

What this means though is that to handle the Cookie header case correctly we do need to have special handling for the semicolon. Btw, regarding this:

If a goal wasn't to keep backwards compatibility

We don't need to preserve backward compatibility for what was a bug. If we change this so only Cookie gets this handling and that's fine.

Copy link
Author

@nateiler nateiler Feb 5, 2026

Choose a reason for hiding this comment

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

Thanks for the references to the RFC. In this particular circumstance the webHeadersToApiGatewayV1Headers function is converting from server to user agent (not also user agent to server) so we should be able to omit special handling of the Cookie header as per RFC6265 4.2 the Cookie header is a user agent to server header.

It seems like the semicolon was included to handle the special usage of the 'Cookie' header which we don't actually need here.

We don't need to preserve backward compatibility for what was a bug. If we change this so only Cookie gets this handling and that's fine.

I see your point here; however I think only set-cookie with a semicolon is a bug. Since the current implementation accepts any header with a semicolon separated value (while it may not be popular) removing it might break current implementations.

I'm unsure of the best path here. Ultimately it seems like dropping the semicolon would simplify and unify the usage across the board (all headers are treated the same).

Copy link
Contributor

@svozza svozza Feb 5, 2026

Choose a reason for hiding this comment

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

The issue here is larger though, splitting on ; causes worse problems. The accepted way to inidicate parameters in a header is with a semi colon, e.g., Accept: text/html; q=0.5, text/plain; q=0.2. With the current implementation, this ends up with a multivalue header value which is clearly wrong:

// ...
multiValueHeaders: {
  accept: [
    "text/html",
    "q=0.5",
    "text/plain",
    "q=0.2",
  ]
}
// ...

It's far more important that we handle this case correctly than headers that aren't Cookie erroneously delimited by ;.


if (headers[key]) {
multiValueHeaders[key] = [headers[key], ...values];
delete headers[key];
} else if (values.length > 1) {
if (values.length > 1) {
Comment on lines -234 to +250
Copy link
Contributor

@dreamorosi dreamorosi Feb 5, 2026

Choose a reason for hiding this comment

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

Why are we removing this logic?

Copy link
Author

@nateiler nateiler Feb 5, 2026

Choose a reason for hiding this comment

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

To my knowledge set-cookie is the only header that can have multiple keys. Other headers, when appended to, will append the value to the end via a comma. This logic would never be evaluated (which was verified through the code coverage)

multiValueHeaders[key] = values;
} else {
headers[key] = value;
Expand Down
242 changes: 127 additions & 115 deletions packages/event-handler/tests/unit/http/Router/basic-routing.test.ts
View file
Open in desktop
Copy link
Contributor

@dreamorosi dreamorosi Feb 5, 2026

Choose a reason for hiding this comment

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

There's a lot of diffs that seem to be unrelated to this PR - any chance you could revert?

Copy link
Author

@nateiler nateiler Feb 5, 2026

Choose a reason for hiding this comment

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

Ah, yeah. Looks like my own biome got the best of me.

Copy link
Author

@nateiler nateiler Feb 5, 2026

Choose a reason for hiding this comment

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

Actually, it looks like these changes were automatically made via lint-staged commit hook. After reverting them and attempting to commit, my revert gets stashed and I'm left with an empty commit which is prevented.

Original file line number Diff line number Diff line change
Expand Up @@ -29,73 +29,71 @@ describe.each([
['HEAD', 'head'],
['OPTIONS', 'options'],
];
it.each(httpMethods)(
'routes %s requests with object response',
async (method, verb) => {
// Prepare
const app = new Router();
(
app[verb as Lowercase<HttpMethod>] as (
path: string,
handler: RouteHandler
) => void
)('/test', async () => ({ result: `${verb}-test` }));

// Act
const actual = await app.resolve(createEvent('/test', method), context);

// Assess
expect(actual.statusCode).toBe(200);
expect(actual.body).toBe(JSON.stringify({ result: `${verb}-test` }));
expect(actual.headers?.['content-type']).toBe('application/json');
expect(actual.isBase64Encoded).toBe(false);
}
);

it.each(httpMethods)(
'routes %s requests with array response',
async (method, verb) => {
// Prepare
const app = new Router();
(
app[verb as Lowercase<HttpMethod>] as (
path: string,
handler: RouteHandler
) => void
)('/test', async () => [
it.each(
httpMethods
)('routes %s requests with object response', async (method, verb) => {
// Prepare
const app = new Router();
(
app[verb as Lowercase<HttpMethod>] as (
path: string,
handler: RouteHandler
) => void
)('/test', async () => ({ result: `${verb}-test` }));

// Act
const actual = await app.resolve(createEvent('/test', method), context);

// Assess
expect(actual.statusCode).toBe(200);
expect(actual.body).toBe(JSON.stringify({ result: `${verb}-test` }));
expect(actual.headers?.['content-type']).toBe('application/json');
expect(actual.isBase64Encoded).toBe(false);
});

it.each(
httpMethods
)('routes %s requests with array response', async (method, verb) => {
// Prepare
const app = new Router();
(
app[verb as Lowercase<HttpMethod>] as (
path: string,
handler: RouteHandler
) => void
)('/test', async () => [
{ id: 1, result: `${verb}-test-1` },
{ id: 2, result: `${verb}-test-2` },
]);

// Act
const actual = await app.resolve(createEvent('/test', method), context);

// Assess
expect(actual.statusCode).toBe(200);
expect(actual.body).toBe(
JSON.stringify([
{ id: 1, result: `${verb}-test-1` },
{ id: 2, result: `${verb}-test-2` },
]);

// Act
const actual = await app.resolve(createEvent('/test', method), context);

// Assess
expect(actual.statusCode).toBe(200);
expect(actual.body).toBe(
JSON.stringify([
{ id: 1, result: `${verb}-test-1` },
{ id: 2, result: `${verb}-test-2` },
])
);
expect(actual.headers?.['content-type']).toBe('application/json');
expect(actual.isBase64Encoded).toBe(false);
}
);
])
);
expect(actual.headers?.['content-type']).toBe('application/json');
expect(actual.isBase64Encoded).toBe(false);
});

it.each([['CONNECT'], ['TRACE']])(
'throws MethodNotAllowedError for %s requests',
async (method) => {
// Prepare
const app = new Router();
it.each([
['CONNECT'],
['TRACE'],
])('throws MethodNotAllowedError for %s requests', async (method) => {
// Prepare
const app = new Router();

// Act & Assess
const result = await app.resolve(createEvent('/test', method), context);
// Act & Assess
const result = await app.resolve(createEvent('/test', method), context);

expect(result.statusCode).toBe(HttpStatusCodes.METHOD_NOT_ALLOWED);
expect(result.body ?? '').toBe('');
}
);
expect(result.statusCode).toBe(HttpStatusCodes.METHOD_NOT_ALLOWED);
expect(result.body ?? '').toBe('');
});

it('accepts multiple HTTP methods', async () => {
// Act
Expand Down Expand Up @@ -273,7 +271,10 @@ describe('Class: Router - V1 Multivalue Headers Support', () => {
statusCode: 200,
body: JSON.stringify({ message: 'success' }),
headers: { 'content-type': 'application/json' },
multiValueHeaders: { 'set-cookie': ['session=abc123', 'theme=dark'] },
multiValueHeaders: {
'cache-control': ['no-cache', 'no-store'],
Copy link
Contributor

@svozza svozza Feb 5, 2026

Choose a reason for hiding this comment

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

Where are these cache-control headers coming from?

Copy link
Author

@nateiler nateiler Feb 5, 2026

Choose a reason for hiding this comment

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

I added them to illustrate a 'simple' multi-value header pair along with the set-cookie multi-value header which contains a semicolon.

'set-cookie': ['session=abc123; Max-Age=3600', 'theme=dark'],
},
}));

// Act
Expand All @@ -284,7 +285,10 @@ describe('Class: Router - V1 Multivalue Headers Support', () => {
statusCode: 200,
body: JSON.stringify({ message: 'success' }),
headers: { 'content-type': 'application/json' },
multiValueHeaders: { 'set-cookie': ['session=abc123', 'theme=dark'] },
multiValueHeaders: {
'cache-control': ['no-cache', 'no-store'],
'set-cookie': ['session=abc123; Max-Age=3600', 'theme=dark'],
},
isBase64Encoded: false,
});
});
Expand All @@ -298,7 +302,7 @@ describe('Class: Router - V2 Cookies Support', () => {
statusCode: 200,
body: JSON.stringify({ message: 'success' }),
headers: { 'content-type': 'application/json' },
cookies: ['session=abc123', 'theme=dark'],
cookies: ['session=abc123; Max-Age=3600', 'theme=dark'],
}));

// Act
Expand All @@ -312,7 +316,7 @@ describe('Class: Router - V2 Cookies Support', () => {
statusCode: 200,
body: JSON.stringify({ message: 'success' }),
headers: { 'content-type': 'application/json' },
cookies: ['session=abc123', 'theme=dark'],
cookies: ['session=abc123; Max-Age=3600', 'theme=dark'],
isBase64Encoded: false,
});
});
Expand Down Expand Up @@ -347,7 +351,10 @@ describe('Class: Router - ALB Support', () => {
statusCode: 200,
body: JSON.stringify({ message: 'success' }),
headers: { 'content-type': 'application/json' },
multiValueHeaders: { 'set-cookie': ['session=abc123', 'theme=dark'] },
multiValueHeaders: {
'cache-control': ['no-cache', 'no-store'],
'set-cookie': ['session=abc123; Max-Age=3600', 'theme=dark'],
},
}));

// Act
Expand All @@ -362,7 +369,10 @@ describe('Class: Router - ALB Support', () => {
statusDescription: '200 OK',
body: JSON.stringify({ message: 'success' }),
headers: { 'content-type': 'application/json' },
multiValueHeaders: { 'set-cookie': ['session=abc123', 'theme=dark'] },
multiValueHeaders: {
'cache-control': ['no-cache', 'no-store'],
'set-cookie': ['session=abc123; Max-Age=3600', 'theme=dark'],
},
isBase64Encoded: false,
});
});
Expand Down Expand Up @@ -391,31 +401,31 @@ describe('Class: Router - ALB Support', () => {
statusCode: Number(code),
expectedDescription: `${code} ${text}`,
}))
)(
'returns statusDescription "$expectedDescription" for status code $statusCode',
async ({ statusCode, expectedDescription }) => {
// Prepare
const app = new Router();
const noBodyStatuses = new Set([201, 204, 205, 304]);
const body = noBodyStatuses.has(statusCode)
? null
: JSON.stringify({ status: statusCode });
app.get('/test', () => new Response(body, { status: statusCode }));

// Act
const result = await app.resolve(
createTestALBEvent('/test', 'GET'),
context
);

// Assess
expect(result.statusCode).toBe(statusCode);
expect(result).toHaveProperty('statusDescription', expectedDescription);
if (!noBodyStatuses.has(statusCode)) {
expect(result.body).toBe(JSON.stringify({ status: statusCode }));
}
)('returns statusDescription "$expectedDescription" for status code $statusCode', async ({
statusCode,
expectedDescription,
}) => {
// Prepare
const app = new Router();
const noBodyStatuses = new Set([201, 204, 205, 304]);
const body = noBodyStatuses.has(statusCode)
? null
: JSON.stringify({ status: statusCode });
app.get('/test', () => new Response(body, { status: statusCode }));

// Act
const result = await app.resolve(
createTestALBEvent('/test', 'GET'),
context
);

// Assess
expect(result.statusCode).toBe(statusCode);
expect(result).toHaveProperty('statusDescription', expectedDescription);
if (!noBodyStatuses.has(statusCode)) {
expect(result.body).toBe(JSON.stringify({ status: statusCode }));
}
);
});
});

describe.each([
Expand Down Expand Up @@ -475,29 +485,31 @@ describe.each([
expect(result.isBase64Encoded).toBe(true);
});

it.each([['image/png'], ['image/jpeg'], ['audio/mpeg'], ['video/mp4']])(
'sets isBase64Encoded for %s content-type',
async (contentType) => {
// Prepare
const app = new Router();
app.get(
'/media',
() =>
new Response('binary data', {
headers: { 'content-type': contentType },
})
);

// Act
const result = await app.resolve(createEvent('/media', 'GET'), context);

// Assess
expect(result.statusCode).toBe(200);
expect(result.body).toBe(Buffer.from('binary data').toString('base64'));
expect(result.headers?.['content-type']).toBe(contentType);
expect(result.isBase64Encoded).toBe(true);
}
);
it.each([
['image/png'],
['image/jpeg'],
['audio/mpeg'],
['video/mp4'],
])('sets isBase64Encoded for %s content-type', async (contentType) => {
// Prepare
const app = new Router();
app.get(
'/media',
() =>
new Response('binary data', {
headers: { 'content-type': contentType },
})
);

// Act
const result = await app.resolve(createEvent('/media', 'GET'), context);

// Assess
expect(result.statusCode).toBe(200);
expect(result.body).toBe(Buffer.from('binary data').toString('base64'));
expect(result.headers?.['content-type']).toBe(contentType);
expect(result.isBase64Encoded).toBe(true);
});

it('does not set isBase64Encoded for text content-types', async () => {
// Prepare
Expand Down
Loading
Loading