Skip to content

fix(lambda-python): escape Docker bundling command arguments - #38583

Open
kumsmrit wants to merge 2 commits into
mainfrom
fix-python-lambda-bundling
Open

fix(lambda-python): escape Docker bundling command arguments#38583
kumsmrit wants to merge 2 commits into
mainfrom
fix-python-lambda-bundling

Conversation

@kumsmrit

Copy link
Copy Markdown
Contributor

Issue # (if applicable)

Closes #.

Reason for this change

  • The Python Docker bundling path builds a shell command from generated copy and install commands. Some generated arguments were not escaped consistently with the rest of the Docker bundling command path.

Description of changes

  • Standardize shell-argument escaping for Python Docker bundling commands, including copied asset paths, output paths, dependency install targets, and rsync exclude patterns.
  • Validate the bundled output path suffix so generated output remains under the bundling output directory.

Describe any new or updated permissions being added

Description of how you validated changes

  • Added regression coverage for escaped exclude patterns, escaped output path suffixes, and output path suffixes.
  • Updated existing aws-lambda-python-alpha test expectations to reflect standardized argument escaping.
  • aws-lambda-python-alpha unit tests pass and aws-cdk-lib builds successfully

Checklist


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

@kumsmrit
kumsmrit deployed to automation August 17, 2026 11:47 — with GitHub Actions Active
@kumsmrit
kumsmrit deployed to automation August 17, 2026 11:47 — with GitHub Actions Active
@github-actions github-actions Bot added the p2 label Aug 17, 2026
@aws-cdk-automation
aws-cdk-automation requested a review from a team August 17, 2026 11:47
@kumsmrit
kumsmrit deployed to automation August 17, 2026 11:47 — with GitHub Actions Active
@mergify mergify Bot added the contribution/core This is a PR that came from AWS. label Aug 17, 2026
@mergify
mergify Bot deployed to automation August 17, 2026 11:48 Active
@mergify
mergify Bot deployed to automation August 17, 2026 11:48 Active
@github-actions

Copy link
Copy Markdown
Contributor

👋 It looks like your PR description follows the template but is missing a valid issue number in the first section.

PRs without a linked issue will receive lower priority for review and merging. Please update the description to include a reference like Closes #123. If no existing issue matches your change, create one first.

@aws-cdk-automation aws-cdk-automation left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This review is outdated)

@aws-cdk-automation aws-cdk-automation added the pr/needs-further-review PR requires additional review from our team specialists due to the scope or complexity of changes. label Aug 17, 2026
@kumsmrit
kumsmrit marked this pull request as ready for review August 17, 2026 12:58
@kumsmrit
kumsmrit deployed to automation August 17, 2026 12:58 — with GitHub Actions Active
}

function posixShellEscape(arg: string): string {
return "'" + arg.replace(/'/g, "'\\''") + "'";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it's already escaped? We should also probably skip the transformation in case arg is unresolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For "already escaped": this helper is intentionally a literal-value escaper; callers pass the raw argument value, and the function emits shell syntax that preserves it as one shell word. It is being applied at the point where these values become shell syntax, so there should not be any prior escaping to collide with.

For unresolved tokens: the values here are used for local synth-time bundling and not deploy-time. inputDir is the framework /asset-input constant; outputDir is based on /asset-output; and user-supplied inputs like assetExcludes / outputPathSuffix are plain string inputs consumed by the local bundling command. Even if someone forced an encoded token string into one of those fields, skipping escaping would not make it usable since the Docker/local bundling command runs before token resolution.

@kumsmrit kumsmrit added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 17, 2026
@kumsmrit
kumsmrit deployed to automation August 17, 2026 16:59 — with GitHub Actions Active
@kumsmrit
kumsmrit deployed to automation August 17, 2026 16:59 — with GitHub Actions Active
@aws-cdk-automation
aws-cdk-automation dismissed their stale review August 17, 2026 17:00

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 17, 2026
@kumsmrit kumsmrit removed the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 18, 2026
@mergify

mergify Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

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

@kumsmrit
kumsmrit deployed to automation August 18, 2026 16:02 — with GitHub Actions Active
@mergify

mergify Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-08-18 16:02 UTC · Rule: default-squash · triggered by rule automatic merge
  • 🟠 Checks running · in-place
  • 🚫 Left the queue2026-08-18 16:04 UTC · at 3ca7dc49399cfb0a2b4d52eb3ad3685152735a04

This pull request spent 1 minute 50 seconds in the queue, with no time running CI.

Waiting for
  • any of: [🛡 GitHub branch protection]
    • check-neutral = validate-pr
    • check-skipped = validate-pr
    • check-success = validate-pr
  • any of: [🛡 GitHub branch protection]
    • check-neutral = build
    • check-skipped = build
    • check-success = build
All conditions

Reason

Pull request #38583 has been dequeued

Queue conditions are not satisfied:

  • #changes-requested-reviews-by=0
  • check-success=build
  • check-success=validate-pr

Hint

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.
If you do update this pull request, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio queue comment.

Tick the box to put this pull request back in the merge queue (same as @mergifyio queue).

  • Requeue this pull request

@mergify
mergify Bot deployed to automation August 18, 2026 16:03 Active
@mergify
mergify Bot deployed to automation August 18, 2026 16:03 Active

@aws-cdk-automation aws-cdk-automation left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

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.

@kumsmrit kumsmrit added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 18, 2026
@kumsmrit
kumsmrit deployed to automation August 18, 2026 16:07 — with GitHub Actions Active
@kumsmrit
kumsmrit deployed to automation August 18, 2026 16:08 — with GitHub Actions Active
@kumsmrit kumsmrit removed the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 18, 2026
@kumsmrit
kumsmrit deployed to automation August 18, 2026 16:09 — with GitHub Actions Active

@mrgrain mrgrain left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code for this should be centralized and aligned with NodeJS bundling.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution/core This is a PR that came from AWS. p2 pr/needs-further-review PR requires additional review from our team specialists due to the scope or complexity of changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants