Skip to content

fix(codegen-ui-react): emit non-identifier theme keys as string literals#1202

Merged
bobbor merged 3 commits into
aws-amplify:mainfrom
osama-rizk:harden-theme-key-rendering
Jul 14, 2026
Merged

fix(codegen-ui-react): emit non-identifier theme keys as string literals#1202
bobbor merged 3 commits into
aws-amplify:mainfrom
osama-rizk:harden-theme-key-rendering

Conversation

@osama-rizk

Copy link
Copy Markdown
Contributor

Description

Theme keys are passed directly to factory.createIdentifier(key) in the theme renderer. factory.createIdentifier() emits its argument verbatim as source code, so a theme key that isn't a valid JavaScript identifier can produce malformed generated output.

This change routes theme keys through a small helper that:

  • emits valid JS identifiers as identifiers (unchanged behavior), and
  • emits any other key as a properly escaped string-literal key, keeping the generated object literal well-formed.

Changes

  • react-theme-studio-template-renderer.ts: add buildThemePropertyName() helper; use it at the four theme-key emission sites (splitAndBuildThemeValues, buildThemeValues, buildThemeBreakpointValues).
  • Added a unit test covering a non-identifier theme key.

Testing

  • react-theme-studio-template-renderer suite: 7/7 pass, all 6 existing snapshots unchanged (no behavior change for valid themes).
  • @aws-amplify/codegen-ui-react builds clean (tsc).

Amplify Gen 1 (including Amplify Studio and this UI component generator)
is in maintenance mode. As of May 1, 2026 it receives only critical bug
fixes and security patches; end of life is May 1, 2027.

Mirrors the official announcement (aws-amplify/amplify-cli#14881) and
links the Gen 1 to Gen 2 migration guide.
Theme keys were passed directly to factory.createIdentifier(), which emits
its argument verbatim as source. Keys that are not valid JS identifiers
could therefore produce malformed generated output. Route theme keys through
a helper that emits valid identifiers as identifiers and any other key as a
properly escaped string-literal key, keeping the generated object literal
well-formed. No change for valid themes (existing snapshots unchanged).
@osama-rizk
osama-rizk requested a review from a team as a code owner July 9, 2026 10:23
@codecov-commenter

codecov-commenter commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.92%. Comparing base (24c87ab) to head (6d9614c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1202      +/-   ##
==========================================
+ Coverage   93.90%   93.92%   +0.02%     
==========================================
  Files         150      151       +1     
  Lines        6137     6142       +5     
  Branches     1841     1842       +1     
==========================================
+ Hits         5763     5769       +6     
+ Misses        356      355       -1     
  Partials       18       18              
Files with missing lines Coverage Δ
...-react/lib/react-theme-studio-template-renderer.ts 92.50% <100.00%> (+1.59%) ⬆️
packages/codegen-ui-react/lib/utils/identifiers.ts 100.00% <100.00%> (ø)
packages/codegen-ui-react/lib/workflow/events.ts 97.14% <100.00%> (+0.08%) ⬆️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24c87ab...6d9614c. 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.

Comment thread packages/codegen-ui-react/lib/react-theme-studio-template-renderer.ts Outdated
Comment thread packages/codegen-ui-react/lib/react-theme-studio-template-renderer.ts Outdated
Comment thread packages/codegen-ui-react/lib/react-theme-studio-template-renderer.ts Outdated
…ey tests

- Extract SIMPLE_JS_IDENTIFIER_RE to lib/utils/identifiers.ts; use it in the
  theme renderer and events.ts (removes duplicate literals).
- Export buildThemePropertyName and add a direct unit block covering the
  identifier/non-identifier boundary.
- Add explicit attack-payload tests (object-literal breakout, indirect eval,
  string-concat) and per-sink tests for buildThemeValues and
  buildThemeBreakpointValues.
- Split test payload into a legitimate CSS-key case and labelled injection
  cases; add SECURITY rationale comment and document the string-literal fallback.
@osama-rizk

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review, @soberm — all six comments addressed in 6d9614c:

  • [blocker] Replaced the malformed-string test with explicit CVE-class attack payloads (object-literal breakout, indirect eval, string-concat), each asserting quoted-key-only / never-bare emission. Added per-sink tests for buildThemeValues and buildThemeBreakpointValues.
  • [major] regex duplication — extracted SIMPLE_JS_IDENTIFIER_RE to lib/utils/identifiers.ts, now imported by the theme renderer and workflow/events.ts.
  • [major] untestable helper — exported buildThemePropertyName; added a direct describe block enumerating the identifier boundary.
  • [suggestion] security comment — added the CVE-2025-4318 rationale in-code.
  • [suggestion] fallback divergence — documented the intentional string-literal fallback vs the '' fallback in the component path.
  • [nitpick] — split the payload into a labelled legitimate CSS-key case and injection cases.

Verification: react-theme-studio-template-renderer 24/24 pass, events 21/21 pass, tsc build clean.

@bobbor
bobbor merged commit 062c3a5 into aws-amplify:main Jul 14, 2026
9 of 16 checks passed
@bobbor bobbor mentioned this pull request Jul 14, 2026
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.

4 participants