fix(apigateway): CORS OPTIONS method should not require auth by corymhall · Pull Request #22402 · aws/aws-cdk

corymhall · 2022-10-06T18:53:49Z

When you create a RestApi and you provide defaultCorsPreflightOptions we automatically create a CORS OPTIONS method for each method. If you also provide defaultMethodOptions then those default options get passed through to the CORS OPTION method as well. In the case of authentication options this should not be the case.

This PR explicitly sets the authentication related options to NONE values which overrides whatever is provided in defaultMethodOptions.

I've updated an integration tests to assert that an OPTIONS call is successful (I also tested before the fix to assert that it failed).

fixes #8615

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

When you create a RestApi and you provide `defaultCorsPreflightOptions` we automatically create a CORS OPTIONS method for each method. If you also provide `defaultMethodOptions` then those default options get passed through to the CORS OPTION method as well. In the case of authentication options this should not be the case. This PR explicitly sets the authentication related options to NONE values which overrides whatever is provided in `defaultMethodOptions`. I've updated an integration tests to assert that an OPTIONS call is successful (I also tested before the fix to assert that it failed). fixes #8615

gitpod-io · 2022-10-06T18:53:57Z

Naumel

added now

Naumel · 2022-10-12T10:00:20Z

packages/@aws-cdk/aws-apigateway/README.md


 A full working example is shown below.

-[Full token authorizer example](test/authorizers/integ.token-authorizer.lit.ts).


I did not mind the link, since code is better represented as such, instead of READMEs, but I appreciate the lack of clicking around.

Naumel · 2022-10-12T10:19:22Z

packages/@aws-cdk/aws-apigateway/lib/method.ts

    const defaultMethodOptions = props.resource.defaultMethodOptions || {};
    const authorizer = options.authorizer || defaultMethodOptions.authorizer;
-    const authorizerId = authorizer?.authorizerId;
+    const authorizerId = authorizer?.authorizerId ? authorizer.authorizerId : undefined;


Pardon my lack of coffee, but what's the difference?

Answer: authorizerId being "" would still lead to an undefined.

Follow-up: is that needed?

Naumel · 2022-10-12T10:27:28Z

packages/@aws-cdk/aws-apigateway/lib/resource.ts

      ],
    }), {
+      authorizer: {
+        authorizerId: '',


As per comment above, this would still lead to an undefined, why specify it?

mergify · 2022-10-12T14:25:01Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ions

aws-cdk-automation · 2022-10-12T15:38:56Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 5541378
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-10-12T15:39:12Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

plumdog · 2022-11-03T20:48:54Z

@Naumel @corymhall I've found this because the diff for one of my projects showed the auth options being unset for the OPTIONS methods, which (wrongly?) concerned me.

Is it really the case that OPTIONS should always be unauthenticated? Is this a how-to-do-REST-properly thing?

I'm slightly surprised it's not an optional setting to enable this behaviour from some props, as I imagine this change could - theoretically, at least - expose something sensitive that was previously hidden.

markcarroll · 2022-11-23T17:37:06Z

@Naumel @corymhall this PR did not fix the issue in #8615 due to a bug in

aws-cdk/packages/@aws-cdk/aws-apigateway/lib/method.ts

Line 203 in 31d6e65

apiKeyRequired: options.apiKeyRequired || defaultMethodOptions.apiKeyRequired,

If the default is to have API key required, the fix added in #22402 will get overridden by the default setting - options.apiKeyRequired is false which leads the other side of the boolean operator to get evaluated which then returns whatever defaultMethodOptions.apiKeyRequired is set to.

taidv · 2023-11-08T10:05:49Z

Invalid Method authorization type specified. Authorization Scopes are only valid for COGNITO_USER_POOLS authorization type (Service: AmazonApiGateway; Status Code: 400; Error Code: BadRequestException;

For those who encounter the above error after upgrading CDK to version 2.46.0 or higher, it appears that the issue may be related to the configuration of 'authorizer' and 'authorizationScopes' in 'defaultMethodOptions.'

This error can occur because this PR sets the authorizer to NONE, but 'defaultMethodOptions' still includes 'authorizationScopes,' leading to the aforementioned error.

As a workaround, you can resolve this issue by removing 'authorizationScopes' from 'defaultMethodOptions' and setting it when using 'addMethod.' This ensures that the 'authorizationScopes' is applied correctly.

For more information on this topic, you can refer to the official AWS documentation on API Gateway methods: API Method Documentation.

lazylace37 · 2024-04-04T17:32:47Z

Unfortunately the workaround provided by @taidv cannot be applied to ProxyResources, because they rely on defaultMethodOptions.

To recap: if you are getting this error:
Invalid Method authorization type specified. Authorization Scopes are only valid for COGNITO_USER_POOLS authorization type (Service: AmazonApiGateway; Status Code: 400; Error Code: BadRequestException;
you are probably using defaultMethodOptions like this in your API or your ProxyResource:

const apis = new apigw.RestApi(this, "rest-api", {
  ...
  defaultMethodOptions: {
    authorizer: userPoolAuthorizer,
    authorizationType: apigw.AuthorizationType.COGNITO,
    authorizationScopes: ["api/myscope"],
  },
});

This PR resets authorizationType for OPTIONs, but not authorizationScopes. This is illegal, thus the error.
A dirty fix is this; just put it after your api.root.addProxy:

api.methods.forEach((method) => {
  if (method.httpMethod === "OPTIONS") {
    const methodCfn = method.node.defaultChild as apigw.CfnMethod;
    methodCfn.authorizationScopes = [];
  }
});

aws-cdk-automation requested a review from a team October 6, 2022 18:54

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Oct 6, 2022

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 6, 2022

switching from .lit because it doesn't work with integ-tests

d9a1dd1

Naumel approved these changes Oct 12, 2022

View reviewed changes

Merge branch 'main' into corymhall/apigateway/cors-default-method-opt…

5541378

…ions

mergify bot merged commit ef72089 into aws:main Oct 12, 2022

markcarroll mentioned this pull request Nov 23, 2022

[aws-apigateway] impossible to remove default method authorization #8827

Closed

markcarroll mentioned this pull request Nov 23, 2022

defaultMethodOptions should be selectively applied to CORS OPTIONS method #8615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apigateway): CORS OPTIONS method should not require auth#22402

fix(apigateway): CORS OPTIONS method should not require auth#22402
mergify[bot] merged 3 commits intoaws:mainfrom
corymhall:corymhall/apigateway/cors-default-method-options

corymhall commented Oct 6, 2022

Uh oh!

gitpod-io bot commented Oct 6, 2022

Uh oh!

Naumel left a comment

Uh oh!

Naumel Oct 12, 2022

Uh oh!

Naumel Oct 12, 2022

Uh oh!

Naumel Oct 12, 2022

Uh oh!

mergify bot commented Oct 12, 2022

Uh oh!

aws-cdk-automation commented Oct 12, 2022

Uh oh!

mergify bot commented Oct 12, 2022

Uh oh!

plumdog commented Nov 3, 2022 •

edited

Loading

Uh oh!

markcarroll commented Nov 23, 2022

Uh oh!

taidv commented Nov 8, 2023

Uh oh!

lazylace37 commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants


		A full working example is shown below.

		[Full token authorizer example](test/authorizers/integ.token-authorizer.lit.ts).

Conversation

corymhall commented Oct 6, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Oct 6, 2022

Uh oh!

Naumel left a comment

Choose a reason for hiding this comment

Uh oh!

Naumel Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

Naumel Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

Naumel Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Oct 12, 2022

Uh oh!

aws-cdk-automation commented Oct 12, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Oct 12, 2022

Uh oh!

plumdog commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markcarroll commented Nov 23, 2022

Uh oh!

taidv commented Nov 8, 2023

Uh oh!

lazylace37 commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

plumdog commented Nov 3, 2022 •

edited

Loading