fix(http-security-headers): support array responses

[nfantone]

What does this implement/fix? Explain your changes.

Add support for handlers returning arrays as responses in @middy/http-security-response-headers. Typically, API Gateway response shape and serialization are handled by other middlewares down the chain. In its current form, http-security-headers restricts flexibility by expecting an object as handler.response, producing malformed responses if it's not.

Any other comments?

• Unrelated to this PR, but I noticed that, sadly, all exported functions within middy are anonymous. I was really tempted to change the module.exports for http-security-headers to a proper named function - but didn't to keep changes to the point. I could start a new PR to address this issue for all @middy/core and @middy/* individual middlewares if there's any interest for it.
• Would be nice to have some sort of coding standard (besides a manual linter run) in place. middy could greatly benefit from having an eslint, prettier and .editorconfig config files.
• The "husky" config is calling git add manually, which is no longer needed and raises a warning.

⚠ Some of your tasks use `git add` command. Please remove it from the config since all modifications made by tasks will be automatically added to the git commit index.

• The test:packages:typings script was failing for me, which causes npm test to also fail. Maybe I was doing something wrong or missing a step?

Where has this been tested?

Node.js Versions: v14.15.4

Middy Versions: Any version.

AWS SDK Versions: Not relevant.

Todo list

• Feature/Fix fully implemented
• Added tests
• Updated relevant documentation
• Updated relevant examples

Did not notice any "relevant documentation" or "examples" with regards to this. If you point me to them, I'll be more than happy to amend them.

[willfarrell]

Thanks for opening a PR (FYI you're committing to an unreleased branch).

Applying the middleware that modify headers earlier so that they are executed after response modifiers will resolve this issue. For a better developer experience, we should handler this in (http-cors,http-error-handler,http-security-headers). I'm thinking the check logic should live in the util package so it can be shared. I'll merge this in, then update the others.

Thank you for your additional comments.

• anonymous exports: This was done on purpose to reduce the level of effort needed for this major update. I'd be interested to hear why you feel they're bad and what you think a good solution is to fix them cc @lmammino
• coding standard: Adding in prettier is something I'm thinking about. We did have an .editorconfig, must have gotten lost while cleaning up after the v2 rewrite. I'll add that back in.
• husky: change will be in the next push.
• typescript test: This is known, updating our typescript support and the testing for them is something we want in place before this branch become the primary.

[nfantone]

Thank you very much for merging this, @willfarrell.

(FYI you're committing to an unreleased branch).

I'm aware 👍 . Thanks for clarifying, though. Thought this was the right way to go since the public alpha had already been released?

Applying the middleware that modify headers earlier so that they are executed after response modifiers will resolve this issue.

I agree - but this introduces a level of dependency which makes individual middlewares require others to be declared in a certain order for them to function properly. Making middlewares resilient to changes and generally isolated from others is key, IMHO.

I'm thinking the check logic should live in the util package so it can be shared.

Not a bad idea. Up until now, I've been always adding my own "response normalizer" middleware, containing a variation of the following:

/**
 * Ensures that the given `response` payload conforms to AWS APIGateway proxy
 * spec, returning at least a `body` property.
 *
 * @example
 *  normalizeResponse('foo'); // { body: 'foo' };
 *  normalizeResponse({ statusCode: 404 }); // { statusCode: 404 };
 *
 * @function
 * @param {*} response The response to normalize.
 * @returns {import('aws-lambda').APIGatewayProxyStructuredResult} The normalized response.
 */
const normalizeResponse = ifElse(
  anyPass([has('body'), has('headers'), has('statusCode')]),
  applySpec({
    body: either(prop('body'), omit(['headers', 'statusCode'])),
    headers: prop('headers'),
    statusCode: prop('statusCode')
  }),
  objOf('body')
);

The snippet uses ramda - but I think it reads well enough. If handler.response contains any of body, headers or statusCode properties, then their values are used to conform to the AG spec; if it doesn't, it wraps whatever value is passed in with { body }. It also supports returning something like { message: 'user not found', statusCode: 404 }, which will be interpreted as { body: { message: 'user not found' }, statusCode: 404 }. In that sense, body, headers and statusCode become "reserved keywords" and everything else becomes the body.

anonymous exports: [...] I'd be interested to hear why you feel they're bad and what you think a good solution is to fix them

Not really bad per sé, more like a (really) nice thing to have when debugging and tracing errors. Right now, a stacktrace for a thrown error within a middy wrapped handler looks like this:

Uncaught TypeError: cannot read property 'filter' of undefined
    at module.exports

Whereas, if it had properly named functions, it'd look something like:

Uncaught TypeError: cannot read property 'filter' of undefined
    at httpCors 

All I did was modify @middy/http-cors and replace module.exports = () => ... with module.exports = function httpCors() { ... }

Adding in prettier is something I'm thinking about.

You'll see nothing but gains. Go ahead and try it out.

[willfarrell]

Thought this was the right way to go since the public alpha had already been released?
The main branch most exists for image paths in the docs right now. We're developing on release/2.x, that's my bad on the confusion. But, yes we do want people contributing to the v2 going forward.

I've already pushed the changes for the http normalization. I kept it simple. The case with an object without body is not handled specifically to not introduce reserved words.

Not really bad per sé, more like a (really) nice thing to have when debugging and tracing errors.

That is a good reason in my books, I'll push an update shortly, but will use a different pattern that more consistent with the code base.

You'll see nothing but gains. Go ahead and try it out.
I use prettier else where, but with a few flags. It's just deciding to fully use without these preferences or not.

Thanks again for the feedback, we really appreciate you taking the time to share it with us.

[nfantone]

@willfarrell

The case with an object without body is not handled specifically to not introduce reserved words.

Well, but there already are reserved keywords, aren't there? How would you return a non 200 status code from a handler otherwise? Or a set of custom headers?

You'd need to be able to do something like this from your function:

function handleEvent(event, context) {
  // ...
  return {
    statusCode: 404,
    headers: { 'Set-Cookie': '__uid=76g-Xz9883==' }
  };
}

Evidently, that can't be serialized and interpreted as being the body (which should be empty in the example above). So, it follows from there that statusCode and headers properties cannot be part of the response payload (presumably a JSON object) as-is.

For statusCode you could leverage throwing HttpErrors as a workaround, if that suits your needs (although you'd have to install an entirely new module just for that). But headers would still be a problem.