Skip to content

Conversation

@takeshixx
Copy link

Set passthroughBehavior behavior to NEVER to prevent errors for CORS preflight requests with content-types other than application/json.

Issue # (if applicable)

This was reported as #18297, but it was closed without a fix.

Reason for this change

Using addCorsPreflight() will add a mock integration for OPTIONS requests and maps them to content-type application/json. OPTIONS requests with a content-type header other than application/json lead to HTTP 500 Internal Server Errors.

Description of changes

Setting the passthroughBehavior to NEVER returns a mime type error instead of a internal server error, which is the appropriate response.

It should be noted that this setting was proposed in the initial implementation of addCorsPreflight() in #906 already. However, it looks like it didn't make it into the CDK. Instead the default configuration is use, which sets it to WHEN_NO_MATCH.

Description of how you validated changes

I tested the change by manually overriding the passthroughBehavior on the LambdaRestApi resource:

lambdaRestApi.methods
      .filter((m) => m.httpMethod === 'OPTIONS')
      .forEach((om) => {
        om.node.children.forEach((c) => {
          const mm = c as apigw.CfnMethod;
          mm.addOverride('Properties.Integration.PassthroughBehavior', apigw.PassthroughBehavior.NEVER);
        });
      });

Checklist


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

Set `passthroughBehavior` behavior to `NEVER` to prevent errors for CORS preflight requests with content-types other than `application/json`.
@github-actions github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jul 31, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team July 31, 2024 08:27
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@takeshixx
Copy link
Author

Clarification Request if test cases are required. No "code changes", just changed a mock integration setting.

@aws-cdk-automation aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Jul 31, 2024
This was referenced Aug 1, 2024
@paulhcsun
Copy link
Contributor

Hi @takeshixx, apologies for the late response. As discussed, please add a unit test case for this change and run the integ tests to update any snapshots.

@paulhcsun paulhcsun removed the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Aug 12, 2024
@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3710c7d
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@takeshixx
Copy link
Author

I have added a unit test that calls resource.addCorsPreflight() and ensures PassthroughBehavior is set to NEVER. I ran:

cd packages/aws-cdk-list
yarn build
yarn test

And the test ran without errors. Is there anything else that is missing?

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Aug 30, 2024
@github-actions
Copy link
Contributor

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 30, 2024
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants