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.

Sign up for GitHub

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

Jump to bottom

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):

aws-cdk/packages/aws-cdk-lib/core/lib/cfn-element.ts

Lines 85 to 92 in 36fd79d

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
@nmussy nmussy mentioned this issue Apr 3, 2024
Merged
1 task
@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
@nmussy nmussy mentioned this issue Apr 3, 2024
Merged
1 task
@github-actions github-actions bot mentioned this issue Apr 8, 2024
Closed
mergify bot pushed a commit that referenced this issue Apr 8, 2024
@nmussy
fix(integ-tests): httpApiCall.expect with resolved URL (#29705)
49b4aa1 
### 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*
@github-actions github-actions bot mentioned this issue May 1, 2024
Closed
This was referenced May 23, 2024
Open
Open
@rwlxxvii rwlxxvii mentioned this issue May 27, 2024
Merged
@NOUIY NOUIY mentioned this issue May 30, 2024
Open
@NOUIY NOUIY mentioned this issue Jun 7, 2024
Open
@niravparikh05 niravparikh05 mentioned this issue Jun 13, 2024
Open
@NOUIY NOUIY mentioned this issue Jun 14, 2024
Open
@mergify mergify bot closed this as completed in #29708 Jun 17, 2024
@mergify mergify bot closed this as completed in b196b13 Jun 17, 2024
@github-actions GitHub Actions
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
@github-actions GitHub Actions
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.

@NOUIY NOUIY mentioned this issue Jun 20, 2024
Open
scorbiere pushed a commit to scorbiere/aws-cdk that referenced this issue Jun 20, 2024
@nmussy @scorbiere
fix(core): overrideLogicalId validation (aws#29708)
d0d05dd 
### 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
@nmussy
fix(core): overrideLogicalId validation (aws#29708)
85f07bb 
### 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
@nmussy @mazyu36
fix(core): overrideLogicalId validation (aws#29708)
f7316aa 
### 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*
This was referenced Jul 12, 2024
Open
Open
This was referenced Jul 19, 2024
Open
Open
@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.
Assignees
No one assigned
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
Projects
None yet
Milestone
No milestone
3 participants
@nmussy @aws-cdk-automation @tim-finnigan