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

fix(s3-deployment): handle properly quoted strings in JSON files #33698

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

scorbiere
Copy link
Contributor

The S3 deployment custom resource now properly handles strings containing quotes when used in JSON files, while maintaining the existing behavior for other file types including YAML files.

Issue #22661

Closes #22661 .

Reason for this change

Investigation Findings

  1. At the Source.jsonData level, we only have markers, not actual values:
// User code
Source.jsonData('config.json', {
  secret_value: param.stringValue // Token containing "test"with"quotes"
})

// What actually gets written:
{
  "secret_value": "<<marker:0xbaba:0>>" // Marker, not the actual value
}
  1. The actual value substitution happens in the S3 deployment Lambda (custom resource), where the markers are replaced with actual values.

BEFORE Fix (Current Broken State)

When the Lambda replaces markers:

// Lambda receives:
{"secret_value":"<<marker:0xbaba:0>>"}
// And the mapping:
markers = {
  "<<marker:0xbaba:0>>": "test"with"quotes"
}

// Simple string replacement results in:
{"secret_value":"test"with"quotes"}  // Invalid JSON

AFTER (With Fix)

The Lambda now detects JSON files and handles them specially:

  1. Detects if the file is JSON
  2. If JSON:
    • Parses the JSON structure
    • Properly escapes strings during marker replacement
    • Re-serializes to valid JSON
  3. If not JSON (including YAML):
    • Uses simple string replacement (which works fine for YAML)
// Lambda receives same input:
{"secret_value":"<<marker:0xbaba:0>>"}
// But now properly escapes quotes in JSON context:
{"secret_value":"test\"with\"quotes"}  // Valid JSON

Key Insights

  1. The issue wasn't in the CDK token system but in the marker replacement in the Lambda
  2. YAML files work without special handling because YAML is more permissive with quotes
  3. The fix:
    • Maintains existing behavior for non-JSON files (YAML, text, etc.)
    • Properly handles JSON string escaping
    • Potential Breaking Change: This fix might cause double-escaping for customers who implemented workarounds (e.g., manually adding escape characters in marker values to handle JSON)

Description of changes

Changes:

  • Added JSON detection and special handling in the Lambda custom resource
  • Added integration tests for both JSON and YAML files with quoted values
  • Added bucket cleanup configuration to the test stack

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Test cases show that:

  • JSON files properly escape quotes: {"secret_value": "test\"with\"quotes"}
  • YAML files work as-is: secret_value: test"with"quotes

Checklist


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

The S3 deployment custom resource now properly handles strings containing quotes
when used in JSON files, while maintaining the existing behavior for other file
types including YAML files.

Changes:
- Added JSON detection and special handling in the Lambda custom resource
- Added integration tests for both JSON and YAML files with quoted values
- Added bucket cleanup configuration to the test stack

Test cases show that:
- JSON files properly escape quotes: {"secret_value": "test\"with\"quotes"}
- YAML files work as-is: secret_value: test"with"quotes
@aws-cdk-automation aws-cdk-automation requested a review from a team March 6, 2025 03:35
@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Mar 6, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 6, 2025
Copy link

codecov bot commented Mar 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.37%. Comparing base (d12854a) to head (a6658d3).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #33698      +/-   ##
==========================================
+ Coverage   82.24%   82.37%   +0.13%     
==========================================
  Files         119      120       +1     
  Lines        6875     6933      +58     
  Branches     1161     1169       +8     
==========================================
+ Hits         5654     5711      +57     
- Misses       1118     1119       +1     
  Partials      103      103              
Flag Coverage Δ
suite.unit 82.37% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
packages/aws-cdk ∅ <ø> (∅)
packages/aws-cdk-lib/core 82.37% <ø> (+0.13%) ⬆️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: a6658d3
  • 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 Mar 9, 2025
@gracelu0 gracelu0 self-assigned this Mar 10, 2025
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. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-s3-deployment: Source.jsonData does not escape quotes
3 participants