AWS API Gateway: Improve validation of CORS configuration

[justingrant]

Serverless CLI commands report a configuration warning about CORS configuration for AWS Lambda functions when using the expanded (non-boolean) format of CORS config. I copied this configuration from Serverless's own blog post about configuring CORS support for Lambda functions. I'm using the second configuration format in the post that allows origin, headers, and allowCredentials properties under the cors key. (The other way to configure CORS is simply to use a boolean value.)

This configuration format also matches the TS types for the cors key in @types/serverless: https://github.com/serverless/typescript/blob/b700b729a0ad15aac0a6ed9dc146047a2a7aa583/index.d.ts#L190-L199

I get this error:

Serverless: Configuration warning at 'functions.createUser.events[0].http.cors': unsupported configuration format

serverless.yml

service: repro

provider:
  name: aws
  runtime: nodejs14.x
  versionFunctions: false
  stage: prod
  region: us-east-1

functions:
  createUser:
    handler: src/createUser.main
    events:
      - http:
          path: user
          method: post
          cors:
            headers:
              - Content-Type
              - X-Amz-Date
              - Authorization
              - X-Api-Key
              - X-Amz-Security-Token
              - X-Amz-User-Agent
              - cognito-identity-id
          authorizer: aws_iam

SLS_DEBUG=* sls info output

justingrant@jgmac api % SLS_DEBUG=* sls info
Serverless: Running "serverless" installed locally (in service node_modules)
Serverless: To ensure safe major version upgrades ensure "frameworkVersion" setting in service configuration (recommended setup: "frameworkVersion: ^2.55.0")

Serverless: Load command interactiveCli
Serverless: Load command config
Serverless: Load command config:credentials
Serverless: Load command config:tabcompletion
Serverless: Load command config:tabcompletion:install
Serverless: Load command config:tabcompletion:uninstall
Serverless: Load command create
Serverless: Load command install
Serverless: Load command package
Serverless: Load command deploy
Serverless: Load command deploy:function
Serverless: Load command deploy:list
Serverless: Load command deploy:list:functions
Serverless: Load command invoke
Serverless: Load command invoke:local
Serverless: Load command info
Serverless: Load command logs
Serverless: Load command metrics
Serverless: Load command print
Serverless: Load command remove
Serverless: Load command rollback
Serverless: Load command rollback:function
Serverless: Load command slstats
Serverless: Load command plugin
Serverless: Load command plugin
Serverless: Load command plugin:install
Serverless: Load command plugin
Serverless: Load command plugin:uninstall
Serverless: Load command plugin
Serverless: Load command plugin:list
Serverless: Load command plugin
Serverless: Load command plugin:search
Serverless: Load command config
Serverless: Load command config:credentials
Serverless: Load command upgrade
Serverless: Load command uninstall
Serverless: Load command login
Serverless: Load command logout
Serverless: Load command generate-event
Serverless: Load command test
Serverless: Load command dashboard
Serverless: Load command output
Serverless: Load command output:get
Serverless: Load command output:list
Serverless: Load command param
Serverless: Load command param:get
Serverless: Load command param:list
Serverless: Load command studio
Serverless: Skipping variables resolution with old resolver (new resolver reported no more variables to resolve)
Serverless: Configuration warning at 'functions.createUser.events[0].http.cors': unsupported configuration format
Serverless:  
Serverless: Learn more about configuration validation here: http://slss.io/configuration-validation
Serverless:  
Serverless: Deprecation warning: Starting with next major, Serverless will throw on configuration errors by default. Adapt to this behavior now by adding "configValidationMode: error" to service configuration
            More Info: https://www.serverless.com/framework/docs/deprecations/#CONFIG_VALIDATION_MODE_DEFAULT
Serverless: Invoke info
Serverless: Invoke aws:info
Service Information
service: h3-api
stage: prod
region: us-east-1
stack: h3-api-prod
resources: 130
api keys:
  None
endpoints:
  POST - (redacted)
functions:
  createUser: h3-api-prod-createUser
layers:
  None

Installed version

Serverless: Running "serverless" installed locally (in service node_modules)
Framework Core: 2.55.0 (local)
Plugin: 5.4.3
SDK: 4.2.6
Components: 3.15.1

[medikoo]

@justingrant by schema either origin or origins are required properties on cors:

serverless/lib/plugins/aws/package/compile/events/apiGateway/index.js

Line 94 in ef5a8fa

oneOf: [{ required: ['origin'] }, { required: ['origins'] }],

[medikoo]

@pgrzesik I think that we probably can improve an UX of that. Remove that oneOf required fields handling from schema, and validate that manually, throwing eventually a meaningful error message. What do you think?

[pgrzesik]

Hey @justingrant as @medikoo mentioned, either origin or origins is required and if I'm not mistaked the example in blog post always uses one of these. I also agree that the message that we have right now is not super helpful and it would be great to implement what you're suggesting here @medikoo 👍

[medikoo]

As I checked, currently when none are provided, generated CORS configuration ends with Access-Control-Allow-Origin: 'undefined'. In light of that I believe it's fine if we unconditionally throw, and not introduce it behind a depreaction flag.

I've updated the issue title. PR that improves that validation is welcome! It's purely about:

• Remove oneOf line from schema
• In context of cors.js introduce manual validation for those properties

[justingrant]

Hey @justingrant as @medikoo mentioned, either origin or origins is required and if I'm not mistaked the example in blog post always uses one of these. I also agree that the message that we have right now is not super helpful and it would be great to implement what you're suggesting here @medikoo 👍

Oh! Thanks so much for the info and the pointer to the relevant sources. Given that the TS type in @types/serverless didn't indicate any required properties (nor did the beta types in serverless/typescript), I'd naively assumed the problem was broken validation. Glad to know the problem was real.

I filed serverless/typescript#49 for the bad TS types.

@medikoo @pgrzesik - is there an example of the type of exception and error message that is used elsewhere for config validation? Anyone fixing this will want to follow precedent.

[pgrzesik]

Hello @justingrant 👋 Are you asking about error that is thrown as a part of our schema validation based on schema definition or about errors for specific scenarios? We have general schema validation, but some rules are really hard to properly express in JSON Schema so sometimes we resort to custom validation where errors have specific codes. In all cases, the error will be of ServerlessError type (

serverless/lib/serverless-error.js

Line 3 in 7e9bfd6

class ServerlessError extends Error {

) and will have code assigned to it. For schema validation code will always be INVALID_NON_SCHEMA_COMPLIANT_CONFIGURATION.

[medikoo]

As @pgrzesik explained.

Still I'd keep INVALID_NON_SCHEMA_COMPLIANT_CONFIGURATION reserved for errors that are result of JSON schema validation.

Here we're going to introduce validation error but via different means and in such case we introduce more specific error code.
Good example for similar errors we already configure like that:

serverless/lib/plugins/aws/package/compile/events/cloudFront.js

Lines 264 to 276 in 7e9bfd6

	if (functionObj.memorySize && functionObj.memorySize > maxMemorySize) {
	throw new ServerlessError(
	`"${functionName}" memorySize is greater than ${maxMemorySize} which is not supported by Lambda@Edge functions of type "${eventType}"`,
	'LAMBDA_EDGE_UNSUPPORTED_MEMORY_SIZE'
	);
	}
	if (functionObj.timeout && functionObj.timeout > maxTimeout) {
	throw new ServerlessError(
	`"${functionName}" timeout is greater than ${maxTimeout} which is not supported by Lambda@Edge functions of type "${eventType}"`,
	'LAMBDA_EDGE_UNSUPPORTED_TIMEOUT_VALUE'
	);
	}
	});

[issea1015]

@medikoo @pgrzesik Hello, I would like to take a chance to handle this if it is available :)

[pgrzesik]

Hello @issea1015 👋 That would be very welcome, thank you 🙇

[medikoo]

@issea1015 that'll be great, we look forward the PR!

[issea1015]

I found some other perspectives in the code:

• If http.cors.origin is not defined, we set http.cors.origin to the default value (‘*’) (when the type of http.cors property is object)

  • serverless/lib/plugins/aws/package/compile/events/apiGateway/lib/validate.js

    Line 64 in ef5a8fa

    cors.origin = http.cors.origin || '*';

• So it ends up to "Access-Control-Allow-Origin”:”*”

We still can make it strict to require explicit origin(s). But we have it set to the default (‘*’) when http.cors is true. In the same sense, I think it's not really bad idea to keep setting it to the default even when http.cors is object type and it misses .origin.

• https://github.com/serverless/serverless/blame/ef5a8faf13a8fbf8564e7c0621e88d1ea5357ea5/docs/providers/aws/events/apigateway.md#L225-L236

@medikoo @pgrzesik Could you let me know what do you think? Thanks in advance 🙇

[medikoo]

@issea1015 indeed, very good find. I believe approach is to assume same default for origin no matter if http.cors is defined as true or as {} (Technically setting http.cors: {} should have equivalent effect to http.cors: true)

In light of that I think we should simply remove conditional requirement for origin/origins from schema, and ensure that when object configuration is used, and none of this properties is set, we simply assume origin: *

@pgrzesik what do you think?

[pgrzesik]

That's a good find @issea1015 - I think we should remove the requirement on origin/origins in schema in that case as @medikoo mentions and if it's not set we should always assume '*'. I don't think we should change the behavior in any other way and requiring explicit set to origin would be a breaking change

[issea1015]

@medikoo @pgrzesik Thanks for the reviews! I'll make an MR to remove the requirements after confirming it works like we intended.