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.

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

Jump to bottom

fix(adapter): handle multi value headers in AWS Lambda #2494

Merged
merged 8 commits into from
Apr 17, 2024
Merged

fix(adapter): handle multi value headers in AWS Lambda #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b36d9c0
Headers can be undefined on ALB
exoego Apr 11, 2024
0e5ec2c
Delegate headers from multiValueHeaders
exoego Apr 11, 2024
0ec2c09
Add test
exoego Apr 11, 2024
485d98c
Assert header values
exoego Apr 11, 2024
57967de
format
exoego Apr 11, 2024
72336fd
Make multiValueHeaders optional so it can be omitted
exoego Apr 11, 2024
389da2c
Avoid Object.entries when undefined
exoego Apr 11, 2024
d51f022
Write tests in runtime_tests
exoego Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 101 additions & 0 deletions runtime_tests/lambda/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,13 @@ describe('AWS Lambda Adapter for Hono', () => {
return c.text('Valid Cookies')
})

app.post('/headers', (c) => {
if (c.req.header('foo')?.includes('bar')) {
return c.json({ message: 'ok' })
}
return c.json({ message: 'fail' }, 400)
})

const handler = handle(app)

const testApiGatewayRequestContext = {
Expand Down Expand Up @@ -491,6 +498,100 @@ describe('AWS Lambda Adapter for Hono', () => {
])
})

describe('headers', () => {
describe('single-value headers', () => {
it('Should extract single-value headers and return 200 (ALBProxyEvent)', async () => {
const event = {
body: '{}',
httpMethod: 'POST',
isBase64Encoded: false,
path: '/headers',
headers: {
host: 'localhost',
foo: 'bar',
},
requestContext: testALBRequestContext,
}
const apiGatewayResponseV2 = await handler(event)
expect(apiGatewayResponseV2.statusCode).toBe(200)
})

it('Should extract single-value headers and return 200 (APIGatewayProxyEvent)', async () => {
const apigatewayProxyEvent = {
version: '1.0',
resource: '/headers',
httpMethod: 'POST',
headers: {
host: 'localhost',
foo: 'bar',
},
path: '/headers',
body: null,
isBase64Encoded: false,
requestContext: testApiGatewayRequestContext,
}
const apiGatewayResponseV2 = await handler(apigatewayProxyEvent)
expect(apiGatewayResponseV2.statusCode).toBe(200)
})

it('Should extract single-value headers and return 200 (APIGatewayProxyEventV2)', async () => {
const apigatewayProxyV2Event = {
version: '2.0',
routeKey: '$default',
headers: {
host: 'localhost',
foo: 'bar',
},
rawPath: '/headers',
rawQueryString: '',
requestContext: testApiGatewayRequestContextV2,
resource: '/headers',
body: null,
isBase64Encoded: false,
}
const apiGatewayResponseV2 = await handler(apigatewayProxyV2Event)
expect(apiGatewayResponseV2.statusCode).toBe(200)
})
})

describe('multi-value headers', () => {
it('Should extract multi-value headers and return 200 (ALBProxyEvent)', async () => {
const event = {
body: '{}',
httpMethod: 'POST',
isBase64Encoded: false,
path: '/headers',
multiValueHeaders: {
host: ['localhost'],
foo: ['bar'],
},
requestContext: testALBRequestContext,
}
const apiGatewayResponseV2 = await handler(event)
expect(apiGatewayResponseV2.statusCode).toBe(200)
})

it('Should extract multi-value headers and return 200 (APIGatewayProxyEvent)', async () => {
const apigatewayProxyEvent = {
version: '1.0',
resource: '/headers',
httpMethod: 'POST',
headers: {},
multiValueHeaders: {
host: ['localhost'],
foo: ['bar'],
},
path: '/headers',
body: null,
isBase64Encoded: false,
requestContext: testApiGatewayRequestContext,
}
const apiGatewayResponseV2 = await handler(apigatewayProxyEvent)
expect(apiGatewayResponseV2.statusCode).toBe(200)
})
})
})

it('Should handle a POST request and return a 200 response if cookies match (APIGatewayProxyEvent V1 and V2)', async () => {
const apiGatewayEvent = {
version: '1.0',
Expand Down
21 changes: 16 additions & 5 deletions src/adapter/aws-lambda/handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface APIGatewayProxyEventV2 {
version: string
routeKey: string
headers: Record<string, string | undefined>
multiValueHeaders?: undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

What is your rationale for this? The documentation does not appear to include multiValueHeaders in ver 2.0. https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html

Copy link
Contributor Author

@exoego exoego Apr 11, 2024 •
edited

Choose a reason for hiding this comment

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

It simplifies code in my opinion.

It allows

event.multiValueHeaders?.['host']?.[0]

instead of

'multiValueHeaders' in event
      ? event.multiValueHeaders.['host']?.[0]
      : undefined

and it also allows

} else if (event.multiValueHeaders) {
  for (const [k, values] of
    Object.entries(event.multiValueHeaders)) {

instead of

 } else if ('multiValueHeaders' in event) {
  for (const [k, values] of
    Object.entries(event.multiValueHeaders || {})) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. The implementation is clean, and I think this is good.

Note.

Below is a note for when the need arises to completely remove 'multiValueHeaders'.

const createRequest = (event: LambdaEvent) => {
  const queryString = extractQueryString(event)
  const domainName =
    event.requestContext && 'domainName' in event.requestContext
      ? event.requestContext.domainName
      : getHostFromEvent(event)

  const path = isProxyEventV2(event) ? event.rawPath : event.path
  const urlPath = `https://${domainName}${path}`
  const url = queryString ? `${urlPath}?${queryString}` : urlPath

  const headers = new Headers()
  getCookies(event, headers)
  if (event.headers) {
    for (const [k, v] of Object.entries(event.headers)) {
      if (v) {
        headers.set(k, v)
      }
    }
  }
  if (isAPIGatewayProxyEvent(event) && event.multiValueHeaders) {
    for (const [k, values] of Object.entries(event.multiValueHeaders)) {
      if (values) {
        values.forEach((v) => headers.append(k, v))
      }
    }
  }

  const method = isProxyEventV2(event) ? event.requestContext.http.method : event.httpMethod
  const requestInit: RequestInit = {
    headers,
    method,
  }

  if (event.body) {
    requestInit.body = event.isBase64Encoded ? Buffer.from(event.body, 'base64') : event.body
  }

  return new Request(url, requestInit)
}

const getHostFromEvent = (event: LambdaEvent): string => {
  if (!event.headers) {
    return 'localhost'
  }

  if (isAPIGatewayProxyEvent(event)) {
    return event.headers['host'] ?? event.multiValueHeaders?.['host']?.[0] ?? 'localhost'
  } else {
    return event.headers['host'] ?? 'localhost'
  }
}
const isAPIGatewayProxyEvent = (event: LambdaEvent): event is APIGatewayProxyEvent => {
  return 'multiValueHeaders' in event
}

cookies?: string[]
rawPath: string
rawQueryString: string
Expand Down Expand Up @@ -63,7 +64,8 @@ export interface APIGatewayProxyEvent {
// When calling Lambda through an Application Load Balancer
export interface ALBProxyEvent {
httpMethod: string
headers: Record<string, string | undefined>
headers?: Record<string, string | undefined>
multiValueHeaders?: Record<string, string[] | undefined>
Copy link
Contributor

Choose a reason for hiding this comment

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

Note:
https://docs.aws.amazon.com/elasticloadbalancing/latest/application/lambda-functions.html#multi-value-headers

path: string
body: string | null
isBase64Encoded: boolean
Expand Down Expand Up @@ -198,16 +200,25 @@ const createRequest = (event: LambdaEvent) => {
const domainName =
event.requestContext && 'domainName' in event.requestContext
? event.requestContext.domainName
: event.headers['host']
: event.headers?.['host'] ?? event.multiValueHeaders?.['host']?.[0]
const path = isProxyEventV2(event) ? event.rawPath : event.path
const urlPath = `https://${domainName}${path}`
const url = queryString ? `${urlPath}?${queryString}` : urlPath

const headers = new Headers()
getCookies(event, headers)
for (const [k, v] of Object.entries(event.headers)) {
if (v) {
headers.set(k, v)
if (event.headers) {
for (const [k, v] of Object.entries(event.headers)) {
if (v) {
headers.set(k, v)
}
}
}
if (event.multiValueHeaders) {
Comment on lines +216 to +217
Copy link
Contributor Author

@exoego exoego Apr 17, 2024 •
edited

Choose a reason for hiding this comment

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

Note) This is not } else if because headers are not optional, at least in type definition in this repo.
It means that we need to take care of headers and multiValueHeaders in tests.

for (const [k, values] of Object.entries(event.multiValueHeaders)) {
if (values) {
values.forEach((v) => headers.append(k, v))
}
}
}

Expand Down