fix: AwsSolutions-APIG4 triggers for CORS preflight endpoints

[georeeve]

Fixes #1815.

Currently this PR just marks all OPTIONS methods as compliant. I'm not sure the best way to narrow this down to specifically CORS preflight endpoints. We could try and match the template that CDK generates through its addCorsPreflight method, but that won't include any custom preflight endpoints which people may be using, and any changes to the CDK template may break our checks here.

I took a look at the MDN docs for the OPTIONS method, and it seems like it has limited use cases outside of CORS. Perhaps we can mark them all as compliant?

Here is the CloudFormation CfnMethod template which CDK generates when using the following code:

new apigateway.RestApi(this, 'TestRestApi', {
    restApiName: 'Test',
    defaultCorsPreflightOptions: {
        allowOrigins: apigateway.Cors.ALL_ORIGINS,
    },
});

{
  "apiKeyRequired": false,
  "authorizationType": "NONE",
  "httpMethod": "OPTIONS",
  "integration": {
    "type": "MOCK",
    "requestTemplates": {
      "application/json": "{ statusCode: 200 }"
    },
    "integrationResponses": [
      {
        "statusCode": "204",
        "responseParameters": {
          "method.response.header.Access-Control-Allow-Headers": "'Content-Type,X-Amz-Date,Authorization,X-Api-Key,X-Amz-Security-Token,X-Amz-User-Agent'",
          "method.response.header.Access-Control-Allow-Origin": "'*'",
          "method.response.header.Access-Control-Allow-Methods": "'OPTIONS,GET,PUT,POST,DELETE,PATCH,HEAD'"
        }
      }
    ]
  },
  "methodResponses": [
    {
      "statusCode": "204",
      "responseParameters": {
        "method.response.header.Access-Control-Allow-Headers": true,
        "method.response.header.Access-Control-Allow-Origin": true,
        "method.response.header.Access-Control-Allow-Methods": true
      }
    }
  ],
  "resourceId": {
    "Fn::GetAtt": ["Test", "RootResourceId"]
  },
  "restApiId": {
    "Ref": "Test"
  }
}

[dontirun]

I thinks it's better to keep the current implementation of the rule and add additional information on the description that it may not apply to pre-flight endpoints Instead of marking all OPTIONSas compliant.

While not ideal I'd rather users suppress false positives instead of opening up the rule for false negatives

[georeeve]

I'm alright with updating the description, but I would rather update the check as it is affecting a built-in feature of CDK. If all we are concerned about is false negatives, then could we take the other approach of matching the built-in CORS feature by matching against the MOCK integration outlined above? Again, we might have to change it if CDK changes their implementation, but I think it's better than simply updating the rule description and having users craft their own exemption config for what is a built-in feature to CDK.

[georeeve]

Perhaps we could be a bit more generic and check the above methodResponses? I don't imagine they're likely to change, and I can't see why you would return that on any other OPTIONS endpoint which is not dedicated to responding to CORS preflight requests.

[georeeve]

I've just updated the check to match the methodResponses responses above. I think this is a better approach than just updating the description, but please do have a look and let me know.

If you still think it's better to leave it then I'm happy to revert and just change the description. However, I did try and come up with an exemption config which only exempts OPTIONS methods, but I couldn't get it to work for some reason. Do you have any examples that I could use if so?

[dontirun]

However, I did try and come up with an exemption config which only exempts OPTIONS methods, but I couldn't get it to work for some reason. Do you have any examples that I could use if so?

You can mark that specific case as NagRuleCompliance.NOT_APPLICABLE

[dontirun]

Please remove the r prefix from the resource names. That's a legacy naming convention that I'd like to remove from the code base

[georeeve]

Done, I've just removed it from the tests within the describe block, let me know if you want it removed for the whole file.

[dontirun]

Thanks for contributing! 🎉