fix(s3-assets): throw if path property is empty #29425

nmussy · 2024-03-10T06:19:21Z

Issue # (if applicable)

Reason for this change

It was reported that a Code.fromAsset('') was creating an infinite loop by including itself through cdk.out. This is caused by the following line:

aws-cdk/packages/aws-cdk-lib/aws-s3-assets/lib/asset.ts

Line 145 in 730fe63

sourcePath: path.resolve(props.path),

If an empty string is given to path.resolve(), the current working directory is returned.

Description of changes

I've added a check that verifies that the given path property is not empty.

Description of how you validated changes

I've added a test for both the aws-lambda package, where the issue was originally reported, and aws-s3-assets, where the fix was implemented

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

nmussy · 2024-03-10T06:19:43Z

packages/aws-cdk-lib/aws-s3-assets/lib/asset.ts

+    if (!props.path) {
+      throw new Error('Asset path cannot be empty');
+    }


It might also be a good idea to check that cdk.out is excluded from the asset source, as Code.fromAsset('.') would cause the same reported issue.

Good call out, I'm good with the changes in this PR so I'll approve for now but feel free to add that check in a separate PR.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

nmussy · 2024-03-10T06:26:18Z

Exemption Request: This is just an additional property constraint, a unit test should be sufficient

aws-cdk-automation · 2024-04-01T00:09:35Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

msambol · 2024-04-01T21:35:32Z

packages/aws-cdk-lib/aws-s3-assets/test/asset.test.ts

 test('fails if directory not found', () => {
  const stack = new cdk.Stack();
  expect(() => new Asset(stack, 'MyDirectory', {
    path: '/path/not/found/' + Math.random() * 999999,
-  })).toThrow();
+  })).toThrow(/Cannot find asset/);


this just throws inherently, correct? Possible to catch this in asset.ts and add the path: Cannot find asset at path: <path> ?

similar to

aws-cdk/packages/aws-cdk-lib/core/lib/asset-staging.ts

Lines 174 to 176 in e63c777

if (!fs.existsSync(this.sourcePath)) {

throw new Error(`Cannot find asset at ${this.sourcePath}`);

}

I just wanted to refine the existing test to help differentiate between the two errors, since I was adding a new one.

I see it now 👍🏼

msambol

LGTM. Thanks @nmussy

paulhcsun

Thanks for the contribution @nmussy! Apologies for the delay on reviewing this.

And appreciate the eyes on this @msambol.

paulhcsun · 2024-04-04T21:42:16Z

packages/aws-cdk-lib/aws-s3-assets/lib/asset.ts

+    if (!props.path) {
+      throw new Error('Asset path cannot be empty');
+    }


Good call out, I'm good with the changes in this PR so I'll approve for now but feel free to add that check in a separate PR.

paulhcsun · 2024-04-04T21:50:46Z

@Mergifyio update

mergify · 2024-04-04T21:50:51Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2024-04-04T22:18:52Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 74ecaa4
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2024-04-04T23:13:34Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

fix(s3-assets): throw if path property is empty

73cc174

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Mar 10, 2024

aws-cdk-automation requested a review from a team March 10, 2024 06:19

github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Mar 10, 2024

nmussy commented Mar 10, 2024

View reviewed changes

aws-cdk-automation previously requested changes Mar 10, 2024

View reviewed changes

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Mar 10, 2024

github-actions bot mentioned this pull request Apr 1, 2024

Monthly PR metrics report #29668

Closed

msambol reviewed Apr 1, 2024

View reviewed changes

msambol approved these changes Apr 2, 2024

View reviewed changes

aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Apr 2, 2024

paulhcsun self-assigned this Apr 4, 2024

paulhcsun approved these changes Apr 4, 2024

View reviewed changes

Merge branch 'main' into fix-s3-assets-throw-empty-path

74ecaa4

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 4, 2024

paulhcsun added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 4, 2024

mergify bot merged commit 2814011 into aws:main Apr 4, 2024
14 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(s3-assets): throw if path property is empty #29425

fix(s3-assets): throw if path property is empty #29425

nmussy commented Mar 10, 2024

nmussy Mar 10, 2024

msambol Apr 1, 2024

paulhcsun Apr 4, 2024

aws-cdk-automation left a comment

nmussy commented Mar 10, 2024

aws-cdk-automation commented Apr 1, 2024

msambol Apr 1, 2024

msambol Apr 1, 2024

nmussy Apr 2, 2024

msambol Apr 2, 2024

msambol left a comment

paulhcsun left a comment

paulhcsun Apr 4, 2024

paulhcsun commented Apr 4, 2024

mergify bot commented Apr 4, 2024

aws-cdk-automation commented Apr 4, 2024

mergify bot commented Apr 4, 2024

	if (!fs.existsSync(this.sourcePath)) {
	throw new Error(`Cannot find asset at ${this.sourcePath}`);
	}

fix(s3-assets): throw if path property is empty #29425

fix(s3-assets): throw if path property is empty #29425

Conversation

nmussy commented Mar 10, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

nmussy commented Mar 10, 2024

aws-cdk-automation commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msambol left a comment

Choose a reason for hiding this comment

paulhcsun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulhcsun commented Apr 4, 2024

mergify bot commented Apr 4, 2024

✅ Branch has been successfully updated

aws-cdk-automation commented Apr 4, 2024

AWS CodeBuild CI Report

mergify bot commented Apr 4, 2024