Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: missing logical ID override validation #29701

Closed
nmussy opened this issue Apr 3, 2024 · 4 comments · Fixed by #29708 · 4 remaining pull requests
Closed

core: missing logical ID override validation #29701

nmussy opened this issue Apr 3, 2024 · 4 comments · Fixed by #29708 · 4 remaining pull requests
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@nmussy
Copy link
Contributor

nmussy commented Apr 3, 2024

Describe the bug

There is currently no validation made when a logical ID is overridden, despite it requiring to be alphanumeric (docs):

public overrideLogicalId(newLogicalId: string) {
if (this._logicalIdLocked) {
throw new Error(`The logicalId for resource at path ${Node.of(this).path} has been locked and cannot be overridden\n` +
'Make sure you are calling "overrideLogicalId" before Stack.exportValue');
} else {
this._logicalIdOverride = newLogicalId;
}
}

A RegExp match would have caught #29700

Expected Behavior

The logical ID should be matched against /^[A-Za-z0-9]+$/, and an error should be thrown if it doesn't

Current Behavior

No runtime check is made, and an error is thrown by CloudFormation at deploy time, such as:

Deployment failed: Error [ValidationError]: Template format error: Outputs name 'AssertionResultsHttpApiCallhttpbin.org/get0f06632dfa7261b35a1569da58f981ba' is non alphanumeric.

Reproduction Steps

import { App, CfnOutput, Stack } from "aws-cdk-lib";

const app = new App();
const stack = new Stack(app, 'TestStack', {});

new CfnOutput(stack, 'ValidName', { value: 'value' }).overrideLogicalId('Invalid/Name');
$ npm run cdk synth
Outputs:
  Invalid/Name:
    Value: value
$ npm run cdk deploy

 ❌  TestStack failed: Error [ValidationError]: Template format error: Outputs name 'Invalid/Name' is non alphanumeric.
Template format error: Outputs name 'Invalid/Name' is non alphanumeric.

Possible Solution

I think throwing an error here is the most sensible solution. Silently removing invalid characters for a manual override doesn't seem like a safe compromise, we need to warn the user (or CDK contributors) about this issue

Additional Information/Context

No response

CDK CLI Version

2.135.0

Framework Version

No response

Node.js Version

v20.11.1

OS

macOS 14.4.1

Language

TypeScript

Language Version

5.4.3

Other information

No response

@nmussy nmussy added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 3, 2024
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Apr 3, 2024
@tim-finnigan tim-finnigan self-assigned this Apr 3, 2024
@tim-finnigan tim-finnigan added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 3, 2024
@tim-finnigan
Copy link

Thanks for creating this issue! I think throwing an error for invalid input here makes sense as well.

@tim-finnigan tim-finnigan added p2 effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Apr 3, 2024
@tim-finnigan tim-finnigan removed their assignment Apr 3, 2024
mergify bot pushed a commit that referenced this issue Apr 8, 2024
### Issue # (if applicable)

Closes #29700

I've also opened #29701 to catch similar issues at synth time in the future

### Reason for this change

When running `httpApiCall('url').expect`, the `AssertionResults` output logical ID would be overridden with an invalid name, containing the URL slashes.

This issue was not noticed earlier because, as far as I can tell, assertions were only made with `Token`/`Ref` URLs, e.g. `apigw.DomainName`

### Description of changes

* Remove non-alphanumeric characters from the overridden `AssertionResults` output logical ID
* I've also added a bit of documentation to `ExpectedResult`, I noticed it was slightly lacking while creating the integration test

### Description of how you validated changes

I've added unit and integration tests. The integration tests include both tests with API Gateway, to cover unresolved URLs, and to https://httpbin.org/ to test this fix

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify mergify bot closed this as completed in #29708 Jun 17, 2024
@mergify mergify bot closed this as completed in b196b13 Jun 17, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

scorbiere pushed a commit to scorbiere/aws-cdk that referenced this issue Jun 20, 2024
### Issue # (if applicable)

Closes aws#29701

### Reason for this change

Calling `overrideLogicalId` on a `Construct` with an invalid logical ID ([docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html#resources-section-structure-logicalid)) would not throw an error at synthesis time. CloudFormation would 

### Description of changes

* Validate `overrideLogicalId` (must not be empty, must not be over 255 characters, must match `/^[A-Za-z0-9]+$/`
* Document exceptions with `@error` JSDoc tags

### Description of how you validated changes

I've added unit tests, integration tests should not be necessary

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
sarangarav pushed a commit to sarangarav/aws-cdk that referenced this issue Jun 21, 2024
### Issue # (if applicable)

Closes aws#29701

### Reason for this change

Calling `overrideLogicalId` on a `Construct` with an invalid logical ID ([docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html#resources-section-structure-logicalid)) would not throw an error at synthesis time. CloudFormation would 

### Description of changes

* Validate `overrideLogicalId` (must not be empty, must not be over 255 characters, must match `/^[A-Za-z0-9]+$/`
* Document exceptions with `@error` JSDoc tags

### Description of how you validated changes

I've added unit tests, integration tests should not be necessary

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mazyu36 pushed a commit to mazyu36/aws-cdk that referenced this issue Jun 22, 2024
### Issue # (if applicable)

Closes aws#29701

### Reason for this change

Calling `overrideLogicalId` on a `Construct` with an invalid logical ID ([docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/resources-section-structure.html#resources-section-structure-logicalid)) would not throw an error at synthesis time. CloudFormation would 

### Description of changes

* Validate `overrideLogicalId` (must not be empty, must not be over 255 characters, must match `/^[A-Za-z0-9]+$/`
* Document exceptions with `@error` JSDoc tags

### Description of how you validated changes

I've added unit tests, integration tests should not be necessary

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@aws-cdk-automation
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.