refactor: make content type header case-insensitive

[danadajian]

📝 Description

• Follow-up to fix: Content Type case sensitivity #134 to prevent the case of the content-type header from failing request forwarding.

[danadajian]

This key now fails the test because our validation schema expects the value to be exactly application/json or application/x-www-form-urlencoded. If this header is undefined I think we would want the proxy to throw an error

[alexholtz]

in TypeScript, setting a key to undefined using object spread syntax effectively removes the key from the resulting object.

So in this case this line is effectively deleting 'content-type' from the baseEvent object, not leaving it in place with a value of undefined. I think we do want to remove this key one way or another, otherwise baseEvent.headers in this test will have key/value pairs for both content-type and Content-Type.

[danadajian]

😨 I see why you did this now! I did not realize content-type was already in the baseEvent object

[danadajian]

And I now understand the delete thing you asked about. The problem with delete is that it would screw up other tests that use the object

[alexholtz]

I just tested this in a console and I'm not sure it's fully effective:

const originalObject = {
  key1: 'value1',
  key2: 'value2',
};

const modifiedObject = {
  ...originalObject,
  key1: undefined,
};

console.log(modifiedObject);

Console output:

{ key1: undefined, key2: 'value2' }

So it may be necessary to use some variation of delete headers['content-type'];

[alexholtz]

I found a another way. Better? Maybe.

const { 'content-type', ...headersWithoutContentType } = baseEvent.headers;

const event: APIGatewayProxyWithLambdaAuthorizerEvent<any> = {
  ...baseEvent,
  headers: {
    ...headersWithoutContentType,
    'Content-Type': 'application/json'
  },
  body: JSON.stringify(VALID_PUSH_PAYLOAD),
  pathParameters: {
    endpointId
  }
};

[danadajian]

The way you wrote the test is really the best way. The tests passed in my first attempt because I made the test wrong and implemented wrong. Now I implemented correctly and the test (with no change) passes!

[alexholtz]

I did a little more testing and I think the way you've rewritten the headersSchema function does require changes to the tests to get a truly valid result.

Right now the only reason Content-Type applicaton/json value is the final value in the modified object is it is accessed by the headersSchema method second, and thus its value overwrites the undefined value for the content-type key. I do not know if that order can always be relied upon.

Previously the way I was setting content-type to undefined works reliably because of this line:

const contentType = headersResult['content-type'] || headersResult['Content-Type'];

The content-type will resolve to undefined, so Content-Type is used instead, and this is a reliable comparison that won't change based on order the key/value pair is processed.

It's worth pointing out this will only ever be an edge case for our own tests because no request from GitHub will ever come over with multiple keys for content-type, so the order they are processed won't ever matter, since it will only be one.

[danadajian]

So to confirm I have the correct understanding, I ran just that one Content-Type test and logged the value of mapKeys(obj, (_, key) => key.toLowerCase()) inside the preprocess method, and got

{
  accept: '*/*',
  'content-type': 'application/json',
  host: 'some-api.us-west-2.amazonaws.com'
}

So it is naturally sanitizing the object of any keys with values of undefined. When zod performs the actual validation using

z.object({
  'content-type': z.nativeEnum(CONTENT_TYPES)
})

it will reject 'content-type': undefined, so if the test passes it means that it's not seeing that. With your change from #134, it may have allowed that since the schema was .optional()

In any case, I updated the test to be more clear so there's no ambiguity whatsoever, and so it mimics a real headers object that the proxy would actually receive.

[danadajian]

Let's actually include the error thrown in the response! 😄

[danadajian]

This effectively calls .toLowerCase() on all the header keys prior to validating against the schema. So if GitHub changed the header to CONTENT-TYPE it would still pass

[danadajian]

🎉 This PR is included in version 2.0.7 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀