redwoodjs / redwood

The App Framework for Startups
https://redwoodjs.com
MIT License
17.33k stars 994 forks source link

multiValueHeaders are not handled by awsLambda request handler #2065

Closed bhumilsarvaiya closed 2 years ago

bhumilsarvaiya commented 3 years ago

The awsLambda requestHandler defined in dev-server and api-server package doesn't handle multi-value headers. For example when multiple cookies are set in response. Example code:

let express = require("express");
let serverless = require("serverless-http");

let app = express();

app.use((req, res) => {
    res.cookie('x', 'y');
    res.cookie('a', 'b');
    res.json({});
});

export const handler = serverless(app);

Saving the above code in file api/functions/temp.js and then calling api http://localhost:8911/temp, it can be confirmed that the cookies are not sent back in the response. It is because in function expressResponseForLambdaResult, lambdaResult returns multiValueHeaders which is completely ignored.

thedavidprice commented 3 years ago

Thank you @bhumilsarvaiya Looping in @peterp for discussion.

bhumilsarvaiya commented 3 years ago

@thedavidprice I made this change in dev-server awsLambda request handler and got it to work for testing in dev env. Thought might help so sharing the code snippet here.

const expressResponseForLambdaResult = (
  expressResFn: Response,
  lambdaResult: APIGatewayProxyResult
) => {
  // Old code: const { statusCode = 200, headers, body = '' } = lambdaResult
  const { statusCode = 200, headers, body = '', multiValueHeaders } = lambdaResult

  if (headers) {
    Object.keys(headers).forEach((headerName) => {
      const headerValue: any = headers[headerName]
      expressResFn.setHeader(headerName, headerValue)
    })
  }
  // added new block to handle multiValueHeaders
  if (multiValueHeaders) {
    Object.keys(multiValueHeaders).forEach((headerName) => {
      const headerValue: Array<any> = multiValueHeaders[headerName]
      expressResFn.setHeader(headerName, headerValue)
    })
  }

  ...
thedavidprice commented 3 years ago

Very nice! @peterp is asleep now (in South Africa) but I'll keep this on his radar for some thoughts + direction. This week we're focused on getting v0.28 released, FYI.

Depending on your availability and the amount of work required, opening a draft PR does help move the discussion forward. We might not move forward and merge it, but we always welcome taking that step. Keep me posted if you're interested and/or have questions.

bhumilsarvaiya commented 3 years ago

Sure, will do that.

jtoar commented 2 years ago

@dthyresson @dac09 Could I assign this to one of you guys?

dthyresson commented 2 years ago

@dthyresson @dac09 Could I assign this to one of you guys?

We'll have to adapt this connect to Fastify -- and check it still ins an issue since this was raised when the api-server was still Express.

Can assign to me. I seem to have lost my assignment permissions.

dthyresson commented 2 years ago

I chatted with SuperTokens the morning and they are implementing multiValueHeaders as part of their SuperTokens JWT support.

Will revisit this issue and attach to their PR when submitted.

dthyresson commented 2 years ago

This issue will be resolved by https://github.com/redwoodjs/redwood/pull/3981 when merged in.