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

feat: multibyte characters in reason result in false diffs #999

Closed
1 of 2 tasks
KouKinyo opened this issue Sep 1, 2022 · 0 comments · Fixed by #1000
Closed
1 of 2 tasks

feat: multibyte characters in reason result in false diffs #999

KouKinyo opened this issue Sep 1, 2022 · 0 comments · Fixed by #1000
Labels
feature-request A feature should be added or improved.

Comments

@KouKinyo
Copy link

KouKinyo commented Sep 1, 2022

Description

When you use multibyte characters in reason, false diffs are shown since CloudFormation does not accept them.

For example, you create a bucket and add a suppression with multibyte characters in reason:

const myBucket = new s3.Bucket(this, "MyBucket", {
  blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL,
  encryption: s3.BucketEncryption.S3_MANAGED,
  enforceSSL: true,
});

NagSuppressions.addResourceSuppressions(myBucket, [{ id: "AwsSolutions-S1", reason: "あいうえおかきくけこ" }]);

After deploying it, cdk diff shows a false diff even though you did not change anything:

Resources
[~] AWS::S3::Bucket MyBucket MyBucketF68F3FF0 
 └─ [~] Metadata
     └─ [~] .cdk_nag:
         └─ [~] .rules_to_suppress:
             └─ @@ -1,6 +1,6 @@
                [ ] [
                [ ]   {
                [ ]     "id": "AwsSolutions-S1",
                [-]     "reason": "??????????"
                [+]     "reason": "あいうえおかきくけこ"
                [ ]   }
                [ ] ]

This makes it hard to understand the true differences of your stacks.

Use Case

In some regions using multibyte character languages, allowing multibyte characters will be useful to get a quick grasp of suppression reasons.

Proposed Solution

  • Check if reason string contains multibyte characters.
  • If it does, base64 encode it before writing into Cfn metadata.
  • If it does not, write it into Cfn metadata as it is.
  • Either way, reasons in CSV report should not be encoded for audit.

Other information

Checking if a string contains multibyte characters:
https://stackoverflow.com/a/73307152

Acknowledge

  • I may be able to implement this feature request
  • This feature might incur a breaking change
@KouKinyo KouKinyo added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Sep 1, 2022
@dontirun dontirun removed the needs-triage This issue or PR still needs to be triaged. label Sep 1, 2022
@mergify mergify bot closed this as completed in 6f095f1 Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants