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

Incorrect A-z range in smithy.api#pattern validations #2803

Open
2 of 3 tasks
gdavison opened this issue Sep 25, 2024 · 5 comments
Open
2 of 3 tasks

Incorrect A-z range in smithy.api#pattern validations #2803

gdavison opened this issue Sep 25, 2024 · 5 comments
Assignees
Labels
bug This issue is a bug. p3 This is a minor priority issue service-api This issue is due to a problem in a service API, not the SDK implementation.

Comments

@gdavison
Copy link
Contributor

gdavison commented Sep 25, 2024

Acknowledgements

Describe the bug

A number of smithy.api#pattern validations in the codegen models have an incorrect A-z character range

Some examples:

"com.amazonaws.fsx#ClientRequestToken": {
"type": "string",
"traits": {
"smithy.api#documentation": "<p>(Optional) An idempotency token for resource creation, in a string of up to 63\n ASCII characters. This token is automatically filled on your behalf when you use the\n Command Line Interface (CLI) or an Amazon Web Services SDK.</p>",
"smithy.api#length": {
"min": 1,
"max": 63
},
"smithy.api#pattern": "^[A-za-z0-9_.-]{0,63}$"
}
},

"com.amazonaws.fsx#VolumePath": {
"type": "string",
"traits": {
"smithy.api#length": {
"min": 1,
"max": 2048
},
"smithy.api#pattern": "^[A-za-z0-9\\_\\.\\:\\-\\/]*$"
}
},

"com.amazonaws.groundstation#JsonString": {
"type": "string",
"traits": {
"smithy.api#length": {
"min": 2,
"max": 8192
},
"smithy.api#pattern": "^[{}\\[\\]:.,\"0-9A-z\\-_\\s]{2,8192}$"
}
},

Regression Issue

  • Select this option if this issue appears to be a regression.

Expected Behavior

The range should be A-Z

Current Behavior

The range is A-z, which includes [, \, ], ^, _, and ` in addition to the upper- and lower-case letters

Reproduction Steps

Do a case-insensitive search for A-z in the repo

Possible Solution

No response

Additional Information/Context

No response

AWS Go SDK V2 Module Versions Used

Release 2024-09-24

Compiler and Version used

N/A

Operating System and version

N/A

@gdavison gdavison added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2024
@RanVaknin
Copy link
Contributor

Hi @gdavison ,

Thanks for reaching out. Can you explain a little more about why you think the regex should be changed? I'm not too familiar with fsx, but at least for jsonString, special characters like [,],{ etc are expected. In other words, I need to understand why you think this regex is wrong in order to efficiently raise this to the service team on your behalf.

If I understand correctly, you are saying the regex is too "loose", meaning you are not blocked in any way, but rather trying to improve the API?

Thanks again,
Ran~

@RanVaknin RanVaknin self-assigned this Sep 25, 2024
@RanVaknin RanVaknin added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p3 This is a minor priority issue service-api This issue is due to a problem in a service API, not the SDK implementation. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2024
@gdavison
Copy link
Contributor Author

gdavison commented Sep 26, 2024

In the examples from FSx, the regexes are clearly typos, since the A-z is immediately followed or preceded by a-z. This is also the case in most of the other regexes findable by doing a case-sensitive search.

For the Ground Station JsonString, the regex already includes the characters [, ], and _. The regex also excludes a large set of valid JSON characters which can be included in string values (the JSON spec says that a string can contain Unicode characters between '0020' (space) and '10FFFF'). I'm not familiar with Ground Station, so perhaps this doesn't affect it in practice. Regardless, if the intent is to use A-z to include both upper- and lower-case letters, it should be A-Za-z instead.

This isn't a blocker for us at the moment, though I have come across a number of cases where we have created validations based on API documentation which has included this incorrect format. Since we currently create the validations manually, we have been able to catch these errors.

We are doing some planning around automatically generating code using the Smithy specifications. We will have to add code to correct the regexes, rather than relying on the specifications to be accurate.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 27, 2024
@bhavya2109sharma
Copy link
Contributor

bhavya2109sharma commented Sep 30, 2024

Hello @gdavison

Thanks for the detailed explanation.
I have reached out to the FSx (#P158594413) and Ground Station (#P158601699) Service Teams with the respective internal tickets.

I will update it here once I will get follow up on the request.

Thanks,
Bhavya

@gdavison
Copy link
Contributor Author

gdavison commented Oct 1, 2024

Hi @bhavya2109sharma, FSx and Ground Station were only a few examples. Here are the others:

"smithy.api#pattern": "^[a-zA-Z0-9]{2,}(?:\\/[a-zA-z0-9-_+]+)*$"

Twice on the one line:

"smithy.api#pattern": "^(^\\$\\{Parameters\\.[a-zA-z]+([a-zA-z_0-9]*)}$)$"

"smithy.api#pattern": "^ami-[0-9A-z]+$"

"smithy.api#pattern": "^[A-za-z0-9_.-]{0,63}$"

Three times on the one line:

"smithy.api#pattern": "^arn:(aws|aws-us-gov|aws-cn):securitylake:[A-za-z0-9_/.\\-]{0,63}:[A-za-z0-9_/.\\-]{0,63}:[A-Za-z0-9][A-za-z0-9_/.\\-]{0,127}$"

"smithy.api#pattern": "^[a-zA-z][a-zA-Z0-9]*(-[a-zA-Z0-9]+)*$"

"smithy.api#pattern": "^[a-zA-z][a-zA-Z0-9]*(-[a-zA-Z0-9]+)*$"

Three times on the one line:

"smithy.api#pattern": "^arn:aws[a-z-]{0,7}:[A-Za-z0-9][A-za-z0-9_/.-]{0,62}:[A-za-z0-9_/.-]{0,63}:[A-za-z0-9_/.-]{0,63}:[A-Za-z0-9][A-Za-z0-9:_/+=,@.\\\\-]{0,1023}$"

Three times on the one line:

"smithy.api#pattern": "^arn:aws[a-z-]{0,7}:secretsmanager:[A-za-z0-9_/.-]{0,63}:[A-za-z0-9_/.-]{0,63}:secret:[A-Za-z0-9][A-za-z0-9_/.-]{8,519}$"

@bhavya2109sharma
Copy link
Contributor

bhavya2109sharma commented Oct 3, 2024

Hello @gdavison,

Thanks for pointing all these issues.

I have reached out to the following teams with respective tickets :

  • IotWinmaker (#P159392704) - Pending release
  • Imagebuilder (#P159382476) - Backlog
  • Resiliencehub (#P159400547) - Fixed
  • SecurityLake (#P159393954) - Fixed
  • Nimble (#P159397801) - Nimble is now officially deprecated
  • Timestream-influxdb (#P159394549) - Fixed
  • Workspaces (#P159398234) - Pending release

I will update it here once I will get follow up on the request.

Thanks,
Bhavya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p3 This is a minor priority issue service-api This issue is due to a problem in a service API, not the SDK implementation.
Projects
None yet
Development

No branches or pull requests

3 participants