Skip to content

Conversation

@ozelalisen
Copy link
Member

Issue # (if applicable)

Closes #35050.

Reason for this change

AWS CDK S3 deployment was causing MD5 hash mismatches between local files and deployed files, even when no markers were present for replacement. This broke integrity validation for customers relying on MD5 checksums to verify file content hasn't been tampered with during deployment.

Description of changes

Core Fix:
• Modified replace_markers() function to track whether any actual marker replacements occur during line-by-line processing
• Only replace the original file if changes were made; otherwise preserve the original file untouched
• This maintains the performance benefits of line-by-line processing from PR #34020 while fixing the file integrity issue

Key Changes:
• Added replacements_made boolean tracking in the marker replacement loop
• Compare each line before and after replacement to detect actual changes
• Conditional file replacement: only overwrite original if modifications occurred
• Remove temporary file and keep original when no changes are made

Describe any new or updated permissions being added

No new or updated IAM permissions are required. This is a bug fix that only modifies the internal file processing logic without changing any AWS API calls or resource access patterns.

Description of how you validated changes

• Added unit tests
• Verified core functionality with Issue Reproduction Steps on Issue #35050.
• Confirmed MD5 hash preservation

Checklist

• [x] My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

@aws-cdk-automation aws-cdk-automation requested a review from a team July 25, 2025 12:33
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p0 labels Jul 25, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 25, 2025
Copy link
Collaborator

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

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 dismissed their stale review July 25, 2025 15:40

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 78c98d8
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 25, 2025
@Abogical Abogical self-assigned this Jul 28, 2025
Copy link
Member

@Abogical Abogical left a comment

Choose a reason for hiding this comment

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

I don't think the unit test (at packages/@aws-cdk/custom-resource-handlers/test/aws-s3-deployment/bucket-deployment-handler/test.py) are correct. They seem to pass even before any changes to packages/@aws-cdk/custom-resource-handlers/lib/aws-s3-deployment/bucket-deployment-handler/index.py are made. Try the following:

$ git checkout main -- packages/@aws-cdk/custom-resource-handlers/lib/aws-s3-deployment/bucket-deployment-handler/index.py
$ ./packages/@aws-cdk/custom-resource-handlers/test/aws-s3-deployment/bucket-deployment-handler/test.sh
# ^ This should fail but it doesn't.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 28, 2025
@Abogical Abogical added the pr/do-not-merge This PR should not be merged at this time. label Jul 28, 2025
@Abogical
Copy link
Member

The original issue probably isn't a regression. This PR may not be needed. Adding a do-not-merge label.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2025

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort p0 pr/do-not-merge This PR should not be merged at this time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws-s3-deployment: unexpected file tampering where a multiline JSON file gets manipulated to a single line after being uploaded to S3

3 participants