fix too restrictive regex on pathpart #769

helloworlddan · 2018-09-24T14:32:49Z

The RegEx was too restrictive when being used with proxy variables. The
+ in {proxy+} is valid and should not throw an exception.

Signed-off-by: Daniel Stamer daniel.stamer@cloudreach.com

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

The RegEx was too restrictive when being used with proxy variables. The \+ in {proxy+} is valid and should not throw an exception. Signed-off-by: Daniel Stamer <daniel.stamer@cloudreach.com>

eladb

Would you be able to add a unit test?

eladb · 2018-09-25T07:07:42Z

packages/@aws-cdk/aws-apigateway/lib/resource.ts

@@ -140,7 +140,7 @@ function validateResourcePathPart(part: string) {
        part = part.substr(1, part.length - 2);
    }

-    if (!/^[a-zA-Z0-9\.\_\-]+$/.test(part)) {
+    if (!/^[a-zA-Z0-9\.\_\-]+[\+]?$/.test(part)) {
        throw new Error(`Resource's path part only allow a-zA-Z0-9._- and curly braces at the beginning and the end: ${part}`);


Can you please update the message?

@eladb I updated the message to account for the optional trailing '+'.

RomainMuller · 2018-09-25T07:09:48Z

Thanks for the contribution!

I feel that it would be best if we can make sure the + is only allowed within a path parameter fragment, before the closing }. It otherwise makes the validation permit many invalid uses... Would you be able to change your code to this effect?

eladb · 2018-09-25T11:27:34Z

@RomainMuller just to make sure I understand, so we just want to allow either } or +} as suffixes, correct instead of generally allowing + as a character within the path part. Correct?

RomainMuller · 2018-09-25T15:35:44Z

@eladb Correct.

eladb · 2018-09-27T20:37:50Z

Hi @daniceman, when you are ready to merge this, let me know. I will take care of getting it aligned with "master". We've converted our codebase from 4 to 2 spaces, so there's a bit of juggling to do to help it land

Signed-off-by: Daniel Stamer <daniel.stamer@cloudreach.com>

helloworlddan · 2018-10-01T09:04:01Z

@RomainMuller

Thanks for the contribution!

I feel that it would be best if we can make sure the + is only allowed within a path parameter fragment, before the closing }. It otherwise makes the validation permit many invalid uses... Would you be able to change your code to this effect?

The regex checks for an optional trailing '+', so I think all matches should be valid. Am I understanding this incorrectly?

helloworlddan · 2018-10-01T09:07:20Z

@eladb

Hi @daniceman, when you are ready to merge this, let me know. I will take care of getting it aligned with "master". We've converted our codebase from 4 to 2 spaces, so there's a bit of juggling to do to help it land

I re-tabed the source to 2 spaces.

Rukenshia · 2018-10-01T13:05:40Z

@daniceman the trailing + is only allowed inside of parameters as far as I know, not in normal paths

…' into fix/apigateway-resource-pathpart-regex-fix

Signed-off-by: Daniel Stamer <daniel.stamer@cloudreach.com>

helloworlddan · 2018-10-01T14:28:11Z

@Rukenshia

@daniceman the trailing + is only allowed inside of parameters as far as I know, not in normal paths

I merged in your code & tests. Now the optional '+' is explicitly stripped before the path is RegEx matched.

@RomainMuller Could you quickly check if @Rukenshia fixed your concerns?

helloworlddan · 2018-10-01T14:51:51Z

@eladb I think this PR is ready to merge now.

eladb · 2018-10-01T15:11:53Z

CHANGELOG.md

+
+### Bug Fixes
+
+* **aws-apigateway:** fix validation of resources failing for proxy resource paths (`{proxy+}`)


We haven’t updated our CONTIBUTING guide yet, but this is not needed anymore. Instead just make sure your squashed commit to master adheres to conventionalcommits. Sorry for the trouble.

@eladb I am terribly sorry, but I am unable to follow your line of thought... Is there something else you'd like me to do in order to get this merged?

Bug Fixes ========= * **aws-apigateway:** allow + in path parts ([#769](#769)) ([9aadcb6](9aadcb6)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([88b599d](88b599d)), closes [#787](#787) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([a09afc4](a09afc4)), closes [#721](#721) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([6c523f2](6c523f2)) * **aws-ec2:** fix typo in resource identifier ([#818](#818)) ([bebfef0](bebfef0)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([8caa28c](8caa28c)) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c21ebb5](c21ebb5)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([96767d7](96767d7)) * Emit valid YAML-1.1 ([#876](#876)) ([3cedc0c](3cedc0c)), closes [#875](#875) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([8824356](8824356)), closes [#830](#830) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([7e5738f](7e5738f)) Features ======== * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([aa76305](aa76305)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([86a55a9](86a55a9)) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([8a5299a](8a5299a)), closes [#795](#795) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([3d1095e](3d1095e)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([bba3602](bba3602)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([1561a4d](1561a4d)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([4980c97](4980c97)) * **aws-dynamodb:** tags support ([#814](#814)) ([644947a](644947a)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([a67b2d9](a67b2d9)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([8215389](8215389)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([d730ac6](d730ac6)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([56f0b4e](56f0b4e)) * Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([4e050c3](4e050c3)) * Resolve paths to nyc & nodeunit ([#887](#887)) ([66ff0a8](66ff0a8)) * upgrade to jsii v0.7.7 ([c231242](c231242))

BREAKING CHANGES ================ * The `cdk.App` initializer doesn't accept any arguments and the `app.run()` method does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); In order to interact with applications written using this version, the CDK Toolkit must also be update using: $ npm i -g aws-cdk * **aws-iam:** This change moves the `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **jsx:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. Features ======== * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([a733bd1](a733bd1)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([1ee729e](1ee729e)) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([d69b1d6](d69b1d6)), closes [#795](#795) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([dc0af46](dc0af46)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([8302541](8302541)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([f6c7760](f6c7760)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([737b481](737b481)) * **aws-dynamodb:** tags support ([#814](#814)) ([c76d8c1](c76d8c1)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([fdb4974](fdb4974)) * Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([5f2cb9f](5f2cb9f)) * Resolve paths to nyc & nodeunit ([#887](#887)) ([6d71a87](6d71a87)) * upgrade to jsii v0.7.7 ([43d2d9e](43d2d9e)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([958dce6](958dce6)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([6920b9c](6920b9c)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([50e0c41](50e0c41)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([a95f081](a95f081)) Bug Fixes ========= * **aws-apigateway:** allow + in path parts ([#769](#769)) ([6905b7e](6905b7e)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b7c244f](b7c244f)), closes [#787](#787) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([d279a1d](d279a1d)), closes [#721](#721) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([cf62e9d](cf62e9d)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([9a53069](9a53069)) * Emit valid YAML-1.1 ([#876](#876)) ([6c98b73](6c98b73)), closes [#875](#875) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c38c3e7](c38c3e7)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([be65a04](be65a04)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([2a979cc](2a979cc)), closes [#830](#830) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([1ec443e](1ec443e)) Code Refactoring ================ * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([6c58556](6c58556)), closes [#196](#196) * remove app boilerplate and improvements to cx protocol ([#868](#868)) ([7bb5a60](7bb5a60)), closes [#216](#216)

Bug Fixes --------- * **aws-apigateway:** allow + in path parts ([#769](#769)) ([0c50d27](0c50d27)), closes [#768](#768) * **aws-cdk:** continue after exceptions in stack monitor ([#791](#791)) ([b0f3298](b0f3298)), closes [#787](#787) * **aws-cloudfront:** check for undefined and determining of the defaultRootObject prop is set or not ([#801](#801)) ([32a74c6](32a74c6)) * **aws-cloudfront:** properly support loggingConfig ([#809](#809)) ([5512f70](5512f70)), closes [#721](#721) * **aws-codecommit:** typo in README ([#780](#780)) ([0e79c2d](0e79c2d)) * **aws-ec2:** Add Burstable Generation 3 Instances ([#812](#812)) ([d36ee6d](d36ee6d)) * **aws-ec2:** fix capitalization of "VPCEndpointType" to "VpcEndpointType" ([#789](#789)) ([7a8ee2c](7a8ee2c)), closes [#765](#765) * **aws-ec2:** fix typo in resource identifier ([#818](#818)) ([f529c80](f529c80)) * **aws-elbv2:** fix load balancer registration ([#890](#890)) ([8cc9abe](8cc9abe)) * **aws-s3:** properly export bucketDomainName ([#844](#844)) ([a65060d](a65060d)) * **aws-sqs:** Queue.import() doesn't return a value ([#885](#885)) ([c592b7f](c592b7f)), closes [#879](#879) * **cdk:** fix TagManager to evaluate to undefined if no tags are included ([#882](#882)) ([477c827](477c827)) * **cdk:** init templates were not upgraded to typescript ^3.0.0 ([#904](#904)) ([2cc7475](2cc7475)) * **cdk:** jsx support conflicts with React usage ([#884](#884)) ([76d8031](76d8031)), closes [#830](#830) * **cfn2ts:** expect Token instead of CloudFormationToken ([#896](#896)) ([6eee1d2](6eee1d2)) * **docs:** fix issue [#718](#718) (Aurora DB example) ([#783](#783)) ([016f3a8](016f3a8)) * **docs:** update supported languages in README ([#819](#819), [#450](#450)) ([#820](#820)) ([ffac98c](ffac98c)) * Correct heading level of CHANGELOG.md 0.10.0 ([40d9ef0](40d9ef0)) * Emit valid YAML-1.1 ([#876](#876)) ([ff857ea](ff857ea)), closes [#875](#875) * **toolkit:** improve error message for large templates ([#900](#900)) ([a41f48f](a41f48f)), closes [#34](#34) Code Refactoring ---------------- * **aws-iam:** move IAM classes cdk to aws-iam ([#866](#866)) ([d46a95b](d46a95b)), closes [#196](#196) * **util:** remove [@aws-cdk](https://github.com/aws-cdk)/util ([#745](#745)) ([10015cb](10015cb)), closes [#709](#709) * **framework:** remove app boilerplate and improvements to cx protocol ([#868](#868)) ([005beec](005beec)), closes [#216](#216) Features -------- * **aws-apigateway:** "LambdaRestApi" and "addProxy" routes ([#867](#867)) ([905a95d](905a95d)) * **aws-cdk:** add maven wrapper to java template ([#811](#811)) ([72aa872](72aa872)) * **aws-cloudformation:** rename the CFN CodePipeline Actions. ([#771](#771)) ([007e7b4](007e7b4)) * **aws-cloudformation:** update the ReadMe of the module to reflect the new Action names. ([#775](#775)) ([6c0e75b](6c0e75b)), closes [#771](#771) * **aws-cloudfront:** Support Security Policy ([#804](#804)) ([b39bf11](b39bf11)), closes [#795](#795) * **aws-codedeploy:** Add the auto-scaling groups property to ServerDeploymentGroup. ([#739](#739)) ([0b28886](0b28886)) * **aws-codedeploy:** Deployment Configuration Construct. ([#653](#653)) ([e6b67ad](e6b67ad)) * **aws-codedeploy:** support setting a load balancer on a Deployment Group. ([#786](#786)) ([e7af9f5](e7af9f5)) * **aws-codepipeline:** allow specifying the runOrder property when creating Actions. ([#776](#776)) ([d146c8d](d146c8d)) * **aws-codepipeline, aws-codecommit, aws-s3:** change the convention for naming the source Actions to XxxSourceAction. ([#753](#753)) ([9c3ce7f](9c3ce7f)) * **aws-dynamodb:** IAM grants support ([#870](#870)) ([c5a4200](c5a4200)) * **aws-dynamodb:** support Global Secondary Indexes ([#760](#760)) ([3601440](3601440)) * **aws-dynamodb:** tags support ([#814](#814)) ([924c84e](924c84e)) * **aws-dynamodB:** support Local Secondary Indexes ([#825](#825)) ([3175af3](3175af3)) * **aws-ec2:** add support for ICMP protocol's classification Types & Codes to SecurityGroupRule ([#893](#893)) ([85bd3c0](85bd3c0)) * **aws-ec2:** allow configuring subnets for NAT gateway ([#874](#874)) ([8ec761c](8ec761c)) * **aws-ec2:** support UDP port ranges in SecurityGroups ([#835](#835)) ([b42ef90](b42ef90)) * **aws-elasticloadbalancingv2:** support for ALB/NLB ([#750](#750)) ([bd9ee01](bd9ee01)) * **aws-s3:** support granting public access to objects ([#886](#886)) ([bdee191](bdee191)), closes [#877](#877) * **cdk:** Add support for UseOnlineResharding with UpdatePolicies ([#881](#881)) ([1f717e1](1f717e1)) * **cdk:** configurable default SSM context provider ([#889](#889)) ([353412b](353412b)) * **core:** resource overrides (escape hatch) ([#784](#784)) ([5054eef](5054eef)), closes [#606](#606) * **aws-codepipeline**: Manage IAM permissions for (some) CFN CodePipeline actions ([#843](#843)) ([4c69118](4c69118)) * **toolkit:** Stop creating 'empty' stacks ([#779](#779)) ([1dddd8a](1dddd8a)) * **aws-autoscaling, aws-ec2:** Tagging support for AutoScaling/SecurityGroup ([#766](#766)) ([3d48eb2](3d48eb2)) ### BREAKING CHANGES * **framework:** The `cdk.App` constructor doesn't accept any arguments, and `app.run()` does not return a `string` anymore. All AWS CDK apps in all languages would need to be modified to adhere to the new API of the `cdk.App` construct. Instead of: const app = new App(process.argv); // ERROR // add stacks process.stdout.write(app.run()); // ERROR The new usage is: const app = new App(); // add stacks app.run(); * **framework:** The CDK is no longer shipped with built-in support for JSX. You can still use JSX but you will have to manually configure it. * **aws-iam:** `PolicyDocument`, `PolicyStatement` and all `PolicyPrincipal` classes moved from the @aws-cdk/cdk module and into the @aws-cdk/aws-iam module. * **aws-codepipeline-api**: `Artifact.subartifact` method of the CodePipeline API was renamed to `Artifact.atPath`. * constructor signature of `TagManager` has changed. `initialTags` is now passed inside a props object. * **util:** @aws-cdk/util is no longer available * **aws-elasticloadbalancingv2:** Adds classes for modeling Application and Network Load Balancers. AutoScalingGroups now implement the interface that makes constructs a load balancing target. The breaking change is that Security Group rule identifiers have been changed in order to make adding rules more reliable. No code changes are necessary but existing deployments may experience unexpected changes. * **aws-cloudformation:** this renames all CloudFormation Actions for CodePipeline to bring them in line with Actions defined in other service packages. * **aws-codepipeline, aws-codecommit, aws-s3:** change the names of the source Actions from XxxSource to XxxSourceAction. This is to align them with the other Actions, like Build. Also, CodeBuild has the concept of Sources, so it makes sense to strongly differentiate between the two.

fix too restrictive regex on pathpart

d95fc5e

The RegEx was too restrictive when being used with proxy variables. The \+ in {proxy+} is valid and should not throw an exception. Signed-off-by: Daniel Stamer <daniel.stamer@cloudreach.com>

helloworlddan mentioned this pull request Sep 24, 2018

RegEx is too restrictive.Exception will be thrown if you try to set pathPart to '{proxy+}', which is valid! #768

Closed

eladb approved these changes Sep 25, 2018

View reviewed changes

Rukenshia and others added 4 commits September 29, 2018 10:16

fix(aws-apigateway): Fix proxy resources being rejected by validation

d39906f

Fix testcase for addResource

6ee1a78

Merge branch 'master' into fix/apigateway-resource-pathpart-regex-fix

b595f71

Signed-off-by: Daniel Stamer <daniel.stamer@cloudreach.com>

extend error message on pathpart regex check

b026bc2

Signed-off-by: Daniel Stamer <daniel.stamer@cloudreach.com>

Rukenshia mentioned this pull request Oct 1, 2018

fix(aws-apigateway): Fix proxy resources being rejected by validation #813

Closed

Daniel Stamer added 2 commits October 1, 2018 16:21

Merge remote-tracking branch 'rukenshia/fix-apigateway-proxy-resource…

ed6c079

…' into fix/apigateway-resource-pathpart-regex-fix

drop optional '+' from RegEx after striping it from input

5c5f805

Signed-off-by: Daniel Stamer <daniel.stamer@cloudreach.com>

eladb approved these changes Oct 1, 2018

View reviewed changes

Remove CHANGELOG update

f41cb8f

rix0rrr merged commit 9aadcb6 into aws:master Oct 2, 2018

rix0rrr mentioned this pull request Oct 9, 2018

The Regex at doesn't allow a trailing +, it only allows the a-zA-Z1-0 etc portion. #862

Closed

eladb mentioned this pull request Oct 10, 2018

v0.11.0 #888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix too restrictive regex on pathpart #769

fix too restrictive regex on pathpart #769

helloworlddan commented Sep 24, 2018

eladb left a comment

eladb Sep 25, 2018

helloworlddan Oct 1, 2018

RomainMuller commented Sep 25, 2018

eladb commented Sep 25, 2018

RomainMuller commented Sep 25, 2018

eladb commented Sep 27, 2018

helloworlddan commented Oct 1, 2018 •

edited

Loading

helloworlddan commented Oct 1, 2018

Rukenshia commented Oct 1, 2018

helloworlddan commented Oct 1, 2018

helloworlddan commented Oct 1, 2018

eladb Oct 1, 2018

helloworlddan Oct 1, 2018


		### Bug Fixes

		* aws-apigateway: fix validation of resources failing for proxy resource paths (`{proxy+}`)

fix too restrictive regex on pathpart #769

fix too restrictive regex on pathpart #769

Conversation

helloworlddan commented Sep 24, 2018

eladb left a comment

Choose a reason for hiding this comment

eladb Sep 25, 2018

Choose a reason for hiding this comment

helloworlddan Oct 1, 2018

Choose a reason for hiding this comment

RomainMuller commented Sep 25, 2018

eladb commented Sep 25, 2018

RomainMuller commented Sep 25, 2018

eladb commented Sep 27, 2018

helloworlddan commented Oct 1, 2018 • edited Loading

helloworlddan commented Oct 1, 2018

Rukenshia commented Oct 1, 2018

helloworlddan commented Oct 1, 2018

helloworlddan commented Oct 1, 2018

eladb Oct 1, 2018

Choose a reason for hiding this comment

helloworlddan Oct 1, 2018

Choose a reason for hiding this comment

helloworlddan commented Oct 1, 2018 •

edited

Loading