Fix #33853: [Bug]: Special characters in story controls break state in U#33894
Fix #33853: [Bug]: Special characters in story controls break state in U#33894danielalanbates wants to merge 1 commit intostorybookjs:nextfrom
Conversation
String arg values in URL were incorrectly rejected when they contained printable special characters (e.g. `!`, `@`, `.`, `'`). The validation was using `VALIDATION_REGEXP` (`[a-zA-Z0-9 _-]`) for both key names and string values, which was far too restrictive for user-provided text. - Change string value validation to allow any printable character (no control characters) via `PRINTABLE_STRING_REGEXP` - Keep `VALIDATION_REGEXP` for key name validation (unchanged) - Add `!str(...)` encoding in `encodeSpecialValues` for strings that start with `!`, to avoid collision with special value markers like `!true`, `!null`, `!date(...)`, etc. - Add corresponding `!str(...)` decoding in `valueDeserializer` - Update tests to reflect the new behavior Fixes storybookjs#33853
|
Related Documentation Checked 0 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
📝 WalkthroughWalkthroughA structured refactoring of the argument encoding/decoding pipeline shifts validation from rejecting non-alphanumeric characters to allowing printable special characters. Introduces PRINTABLE_STRING_REGEXP for consistent validation across modules and implements !str(...) escaping pattern to preserve strings starting with exclamation marks. Tests updated to verify new character handling and encoding behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/preview-api/modules/preview-web/parseArgsParam.test.ts (1)
234-241:⚠️ Potential issue | 🔴 CriticalTest expectations for invalid fractional numbers and unprefixed dates do not match the validation logic.
The relaxed
PRINTABLE_STRING_REGEXP(/^[^\x00-\x1F\x7F]*$/) accepts any printable characters. When values like"1.",".2","1.2.3", and"2001-02-03T04:05:06.789Z"failNUMBER_REGEXPinvalueDeserializer, they are returned as strings. ThenvalidateArgsaccepts these strings because they passPRINTABLE_STRING_REGEXP, resulting in{ key: value }being included in the output.However, the tests at lines 238–240 and 80–82 expect these to be rejected (returning
{}). The tests will fail because the validation logic accepts these printable strings while the test assertions expect rejection.To fix: Either update the tests to expect
{ key: "1." }etc., or add explicit validation invalidateArgsto reject strings that look like malformed numbers (e.g., values matching/^\d*\.?\d*$/ but not NUMBER_REGEXP).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/preview-api/modules/preview-web/parseArgsParam.test.ts` around lines 234 - 241, Tests expect malformed numeric-looking strings (e.g., "1.", ".2", "1.2.3") to be rejected but current logic (valueDeserializer + PRINTABLE_STRING_REGEXP) accepts them; update validateArgs to explicitly detect numeric-like strings that fail NUMBER_REGEXP and reject them instead of treating them as printable strings. Concretely, inside validateArgs add a check: if a value is a string and matches a loose numeric-like pattern (e.g., /^\d*\.?\d*$/ or similar) but does not match NUMBER_REGEXP, treat it as invalid (omit the key / return {} for that param); reference PRINTABLE_STRING_REGEXP, NUMBER_REGEXP, valueDeserializer and parseArgsParam when locating where to add this validation. Ensure tests are updated only if you prefer the alternate approach (but prefer code change to satisfy existing test expectations).
🧹 Nitpick comments (2)
code/core/src/preview-api/modules/preview-web/parseArgsParam.test.ts (1)
221-224:!str()decoding tests cover basic and nested cases, but consider adding the empty-content edge case.The test for
!str(str(nested))is a nice catch for the double-encoding scenario. Consider also testing the edge case where the original string is just"!"— i.e., the decoder receives!str()and should return"!".✏️ Suggested additional test case
expect(parseArgsParam('key:!str(important)')).toStrictEqual({ key: '!important' }); expect(parseArgsParam('key:!str(str(nested))')).toStrictEqual({ key: '!str(nested)' }); + expect(parseArgsParam('key:!str()')).toStrictEqual({ key: '!' }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/preview-api/modules/preview-web/parseArgsParam.test.ts` around lines 221 - 224, Add an edge-case unit test to parseArgsParam that asserts parsing 'key:!str()' returns { key: '!' } (i.e., the decoder should treat empty !str() as the single '!' character); update the existing test block that handles "!str(...)" (the it(...) containing cases for !str(important) and !str(str(nested))) by adding one more expect(parseArgsParam('key:!str()')).toStrictEqual({ key: '!' }) to cover this empty-content scenario.code/core/src/preview-api/modules/preview-web/parseArgsParam.ts (1)
10-11:PRINTABLE_STRING_REGEXPis duplicated acrossparseArgsParam.tsandrouter/utils.ts.Both files define identical
PRINTABLE_STRING_REGEXPconstants and carry "keep in sync" comments (Line 8 here, Line 76 inutils.ts). Consider extracting this regex (and possiblyVALIDATION_REGEXPand others) into a shared module to eliminate the sync burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/preview-api/modules/preview-web/parseArgsParam.ts` around lines 10 - 11, The PRINTABLE_STRING_REGEXP constant is duplicated between parseArgsParam.ts and router/utils.ts (and similarly VALIDATION_REGEXP/other validation constants), so extract these shared regex constants into a new central module (e.g., a validation or constants module) and replace the local definitions in parseArgsParam.ts and router/utils.ts with imports from that module; update references to PRINTABLE_STRING_REGEXP and any other duplicated symbols (VALIDATION_REGEXP) to import from the new shared module and remove the local definitions and "keep in sync" comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code/core/src/preview-api/modules/preview-web/parseArgsParam.test.ts`:
- Around line 234-241: Tests expect malformed numeric-looking strings (e.g.,
"1.", ".2", "1.2.3") to be rejected but current logic (valueDeserializer +
PRINTABLE_STRING_REGEXP) accepts them; update validateArgs to explicitly detect
numeric-like strings that fail NUMBER_REGEXP and reject them instead of treating
them as printable strings. Concretely, inside validateArgs add a check: if a
value is a string and matches a loose numeric-like pattern (e.g., /^\d*\.?\d*$/
or similar) but does not match NUMBER_REGEXP, treat it as invalid (omit the key
/ return {} for that param); reference PRINTABLE_STRING_REGEXP, NUMBER_REGEXP,
valueDeserializer and parseArgsParam when locating where to add this validation.
Ensure tests are updated only if you prefer the alternate approach (but prefer
code change to satisfy existing test expectations).
---
Nitpick comments:
In `@code/core/src/preview-api/modules/preview-web/parseArgsParam.test.ts`:
- Around line 221-224: Add an edge-case unit test to parseArgsParam that asserts
parsing 'key:!str()' returns { key: '!' } (i.e., the decoder should treat empty
!str() as the single '!' character); update the existing test block that handles
"!str(...)" (the it(...) containing cases for !str(important) and
!str(str(nested))) by adding one more
expect(parseArgsParam('key:!str()')).toStrictEqual({ key: '!' }) to cover this
empty-content scenario.
In `@code/core/src/preview-api/modules/preview-web/parseArgsParam.ts`:
- Around line 10-11: The PRINTABLE_STRING_REGEXP constant is duplicated between
parseArgsParam.ts and router/utils.ts (and similarly VALIDATION_REGEXP/other
validation constants), so extract these shared regex constants into a new
central module (e.g., a validation or constants module) and replace the local
definitions in parseArgsParam.ts and router/utils.ts with imports from that
module; update references to PRINTABLE_STRING_REGEXP and any other duplicated
symbols (VALIDATION_REGEXP) to import from the new shared module and remove the
local definitions and "keep in sync" comments.
1 participant
Fixes #33853
Summary
This PR fixes: [Bug]: Special characters in story controls break state in URL
Changes
Testing
Please review the changes carefully. The fix was verified against the existing test suite.
This PR was created with the assistance of Claude Sonnet 4.6 by Anthropic | effort: low. Happy to make any adjustments!
Summary by CodeRabbit
Bug Fixes
New Features
Tests