Skip to content

fix(codegen-ui-react): add sanitization to binding expression #1174

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

Merged
merged 3 commits into from
Apr 10, 2025

Conversation

ioanabrooks
Copy link
Contributor

@ioanabrooks ioanabrooks commented Apr 10, 2025

Problem

When Amplify Studio processes components with collection types, it recursively renders children and builds expression bindings without properly sanitizing them.

Solution

Updated property binding process:

  • Added property value escaping during the component rendering pipeline
  • Modified buildBindingExpression to safely handle property values
  • Implemented custom validation instead of traditional HTML sanitization because:
  • We're dealing with JSX expressions, not HTML
  • Properties are intentionally rendered as expressions
  • Standard HTML sanitizers wouldn't address the issue

credit @iCodeForBananas

Additional Notes

Links

Ticket

GitHub issue:

Other links

Verification

Manual tests

Automated tests

  • Unit tests added/updated
  • E2E tests added/updated
  • N/A - (provide a reason)
  • deferred - (provide GitHub issue for tracking)

Housekeeping

  • No non-essential console logs
  • All new files contain license notice

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ioanabrooks ioanabrooks requested a review from a team as a code owner April 10, 2025 19:14
@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.87%. Comparing base (bbc3435) to head (a0d01e4).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1174      +/-   ##
==========================================
+ Coverage   93.86%   93.87%   +0.01%     
==========================================
  Files         150      150              
  Lines        6114     6127      +13     
  Branches     1838     1838              
==========================================
+ Hits         5739     5752      +13     
  Misses        357      357              
  Partials       18       18              
Files with missing lines Coverage Δ
...egen-ui-react/lib/react-component-render-helper.ts 92.14% <100.00%> (+0.26%) ⬆️
packages/codegen-ui-react/lib/utils/constants.ts 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbc3435...a0d01e4. Read the comment docs.

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

mhyjasmi
mhyjasmi previously approved these changes Apr 10, 2025
Copy link
Contributor

@cwoolum cwoolum left a comment

Choose a reason for hiding this comment

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

buildAuthExpression may run into the same issue. Wouldn't this be a problem anywhere

@ioanabrooks ioanabrooks merged commit ca98c38 into main Apr 10, 2025
8 checks passed
@ioanabrooks ioanabrooks deleted the update-component-renderer branch April 10, 2025 22:44
Copy link

@blakedunson blakedunson left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants