Core: Sync args state to URL #13803

ghengeveld · 2021-02-03T14:23:07Z

Issue: #12291

What I did

This synchronizes args state with the URL.

How to test

Is this testable with Jest or Chromatic screenshots? No, but a Cypress test could.
Does this need a new example in the kitchen sink apps? No
Does this need an update to the documentation? Maybe

If your answer is yes to any of these, please make sure to include it in your PR.

tmeasday

I'm unsure about this approach.

We need to also parse args from the URL if you browse directly to the /iframe.html (this is what folks want to do in tests after all)

Following through from that, we need to have the arg parsing logic in the preview. Possibly we also should set the iframe URL from the preview too (this part is up for debate).

I sort of feel like the manager should perhaps just pass the arg query parameters straight through to the preview by whacking them on the iframe URL (similar to how it does the story id, IIRC) and let the preview map them to actual arg values and ultimately emit the args, to be later captured by the manager.

OTOH I'm not sure who should then serialize said args to the URL.

addons/controls/src/ControlsPanel.tsx

lib/api/src/modules/url.ts

j3rem1e · 2021-02-04T11:44:02Z

Imho, this changes will add security issue like XSS.

Some components (if not all) doesn't sanitize its properties, because there are used internally. When using args, the user has to interact with Storybook.

However, if the properties are automatically inject through the url to the component, it's possible to craft an url to abuse components without the user knowledge.

ghengeveld · 2021-02-07T13:25:54Z

I'm unsure about this approach.

We need to also parse args from the URL if you browse directly to the /iframe.html (this is what folks want to do in tests after all)

Following through from that, we need to have the arg parsing logic in the preview. Possibly we also should set the iframe URL from the preview too (this part is up for debate).

@tmeasday You're right. My first draft didn't include any handling for the iframe (i.e. support "ejecting"). I've added that now, and that indeed involved parsing the args parameter in the preview instead of in the manager. The preview will now set the initial args based on its URL, rather than the manager. The manager will pick up that args change and update accordingly.

I sort of feel like the manager should perhaps just pass the arg query parameters straight through to the preview by whacking them on the iframe URL (similar to how it does the story id, IIRC) and let the preview map them to actual arg values and ultimately emit the args, to be later captured by the manager.

OTOH I'm not sure who should then serialize said args to the URL.

I don't think we need to do this. When ejecting, the relevant args are added to the iframe URL, so it will parse and use them. When loaded from the manager, the iframe URL already inherits the params from the manager URL, so it will initialize in the same way and send a signal to the manager to update the args based on its URL. When navigating to a different story, the iframe URL will not be updated, but the args are updated by other means (from the manager). The URL is only relevant for the initial page load, not when switching between stories. Of course this means that "open frame in new tab" will not work as expected, but I think this is a reasonable limitation if it means we can avoid additional URL syncing complexity. People should just use the eject (or copy) feature for this purpose.

ghengeveld · 2021-02-07T13:47:11Z

Imho, this changes will add security issue like XSS.

Some components (if not all) doesn't sanitize its properties, because there are used internally. When using args, the user has to interact with Storybook.

However, if the properties are automatically inject through the url to the component, it's possible to craft an url to abuse components without the user knowledge.

@j3rem1e That's correct. We are not doing any kind of sanitization, and args are often passed directly to a component, which may be vulnerable to XSS (that depends on the details of how those props are used).

Although I think the risk involved is very small (Storybooks typically run on localhost or a unique subdomain, so stealing a cookie isn't really interesting), we should at least offer some way to secure against this. For one, I think we should offer a way to opt out of this feature. Another thing we could do is include a signed hash in the URL, and verify it before applying the URL args. We could also attempt to sanitize the args, but it's very tricky to get that right. It would be the least impactful thing to do though. What do you think?

For some added context: The knobs addon is also vulnerable to this. While it doesn't sync to the URL, it does parse from the URL.

shilman · 2021-02-07T14:25:52Z

@j3rem1e can you provide an XSS example, even if it's contrived, to help me understand?

j3rem1e · 2021-02-07T14:47:46Z

@ghengeveld I work for a company focused on security, so my first reflex is that default should be secure :-)

Let's take a hypothetical example:

I have a Timeline component. This component shows a discussion and each message is generated from markdown in my backend. my backend is secured, the generated html is sanitized on the backend, the canal is https, my Timeline doesn't sanitize a 2nd time the messages. It's an internal component and it's not possible to use it without the backend.

This component is tested through Storybook. As my controls are auto-generated, I see a "message" property which I can enter anything (and html). but it's not important here, because it is entered by the tester and run in the context of the browser of the tester.

With this PR, I can craft an URL which show the timeline with arbitrary html/javascript. I can send this url to anybody, and the script will execute in the context of the target browser.

In my case, Storybook run under a subdomain of my production server (jenkins) next to a wiki which documents internals of the application. jenkins and the wiki are secured, of course. However, If I send the crafted URL to a user who has admin access to the jenkins, and this user is logged into jenkins, then I can probably steal his cookie/create an user/etc.

If I send a "localhost" url to a dev whose is running storybook, then I can execute scripts in the context of "localhost" which is a secure location by default. I can do a lot of things in this context.

tmeasday · 2021-02-07T23:04:00Z

Another thing we could do is include a signed hash in the URL, and verify it before applying the URL args. We could also attempt to sanitize the args, but it's very tricky to get that right. It would be the least impactful thing to do though. What do you think?

This means URLs are no-longer "hackable" which I think would be a pity. Maybe we could add this signing feature as an opt-in security measure though? Although if the attacker has access to the SB they could easily construct a signed URL containing arbitrary args too, so I'm not sure it achieves all that much.

Probably the right thing is to make the whole feature opt-out (or opt-in).

In my case, Storybook run under a subdomain of my production server (jenkins) next to a wiki which documents internals of the application. jenkins and the wiki are secured, of course. However, If I send the crafted URL to a user who has admin access to the jenkins, and this user is logged into jenkins, then I can probably steal his cookie/create an user/etc.

PS aren't your cookies domain scoped?

tmeasday

I'd like to see some tests of the initial arg getting set on the preview side:

A unit test in the store when you set the args in selection specifier, probably add here: https://github.com/storybookjs/storybook/blob/3fd8b6b39746ea54a5e43eb19333097aa3cb0dce/lib/client-api/src/story_store.test.ts#L761:L761
A unit test of the url parsing, here:

storybook/lib/core/src/client/preview/url.test.ts

Line 66 in 3fd8b6b

describe('getSelectionSpecifierFromPath', () => {
A fuller integration test that the wiring all works (in the preview at least, its a bit tricky to do preview/manager tests) if you can. We have some in here: https://github.com/storybookjs/storybook/blob/next/lib/core/src/client/preview/start.test.ts but it could be tricky.
Potentially a full end-to-end test that one of the example Storybooks
a) renders properly with the args specified in the URL
b) the URL changes when you change the arg.

I actually don't even know how to do 4 ( 😉 ) but I reckon this is the kind of feature that could regress really easily as it relies on stuff across the whole stack, so I think its worth the effort to get a test up.

tmeasday · 2021-02-07T23:13:01Z

lib/api/src/modules/url.ts

+    fullAPI.on(SET_STORIES, () => {
+      const data = fullAPI.getCurrentStoryData();
+      const storyArgs = isStory(data) ? data.args : undefined;
+      initialArgs[data.id] = storyArgs;


Doesn't SET_CURRENT_STORY get called for the first story too?

No it doesn't. It only triggers when switching between stories.

tmeasday · 2021-02-07T23:13:26Z

lib/api/src/modules/url.ts

+      const data = fullAPI.getCurrentStoryData();
+      const args = isStory(data) ? data.args : undefined;
+      initialArgs[data.id] = initialArgs[data.id] || args;
+      updateArgsParam({ storyId: data.id, args });


Is this line necessary? By definition all the args are "initial", right?

This restores any previously customized args when switching back and forth between stories. So no, they're not always initial.

Of course this assumes we want this behavior of restoring args to a previously customized state rather than resetting to the initial args. I think we do.

tmeasday · 2021-02-07T23:14:02Z

lib/api/src/modules/url.ts

+      const argsString = stringifyArgs(customizedArgs);
+      const argsParam = argsString.length ? `&args=${argsString}` : '';
+      queryNavigate(`${fullAPI.getUrlState().path}${argsParam}`, { replace: true });
+      api.setQueryParams({ args: argsString });


Should we checking something's changed before calling replaceState?

I don't think that's worthwhile. Probably the browser already optimizes for that.

tmeasday · 2021-02-07T23:15:20Z

lib/core/src/client/preview/url.ts

@@ -60,8 +60,16 @@ const deprecatedLegacyQuery = deprecate(
 Use \`id=$storyId\` instead. 
 See https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#new-url-structure`
 );
-export const getSelectionSpecifierFromPath: () => StoreSelectionSpecifier = () => {
+
+// Lifted from @storybook/router utils


We can't import? 🤔

No, we have no shared code between manager and preview right now (core-events being the sole exception, but thats just a bunch of constants). Norbert and Michael are working on something which will introduced a core-common package, but until then it's easier to copy/paste.

lib/core/src/client/preview/start.ts

tmeasday · 2021-02-07T23:32:55Z

I jumped the gun here again didn't I @ghengeveld? (Noticed this PR is still a draft). Still hopefully the comments above are helpeful.

Of course this means that "open frame in new tab" will not work as expected, but I think this is a reasonable limitation if it means we can avoid additional URL syncing complexity. People should just use the eject (or copy) feature for this purpose.

Yeah, I'm already not sure how this works. Do we even update the URL with the id when changing story anyway?

shilman

Wonderful!!! 😻

j3rem1e

Looks good to me, it's a great feature.

ghengeveld · 2021-02-13T10:59:59Z

We're not quite there yet. The recent change to initialize the story args based on the URL in the story store changed the sequence of steps and is causing the URL to get stripped of other args whenever you change one arg.

tmeasday

Code's looking good to me

The recent change to initialize the story args based on the URL in the story store changed the sequence of steps and is causing the URL to get stripped of other args whenever you change one arg.

Can we encode this problem in a test then? Would be good not to regress later.

I think the unit + integration tests are sufficient to cover all the logic, and if it breaks regardless it'll be pretty apparent during use.

If you are happy the manager-side tests guarantee args will make it to the iframe and the URL will update correctly (perhaps see above), then I think it's good.

tmeasday · 2021-02-13T11:44:38Z

The recent change to initialize the story args based on the URL in the story store changed the sequence of steps and is causing the URL to get stripped of other args whenever you change one arg.

Is the problem here that the manager has no way to tell that the args of the story aren't the original args? Perhaps we need to get the timing of things just right so that the SET_STORIES event is emitted with the true args, and then the args change, with a STORY_ARGS_UPDATED before the story renders the first time. WDYT?

Warn when attempting to serialize or unserialize args with disallowed characters.

ghengeveld · 2021-02-15T16:09:15Z

Did a bunch more work on this. Still need to apply throttling because we're going a lot of work to serialize args.

ghengeveld · 2021-02-17T11:04:09Z

There's two commits that can be reverted when the next version of qs is released (and our dependency is updated to use it):

Currently, qs does not support the allowSparse flag (it's merged but yet to be released). That means any array values will be synced to the URL as a whole, rather than just the particular values that deviate from their initial value. To keep the URL readable and as short as possible, it's better to use sparse arrays when we can.

shilman

Merging!

ghengeveld added 9 commits January 26, 2021 13:34

Simplify

3540b7f

Drop unused file and flatten directory structure

c4374d5

Update docs link

2ba2b2b

Optimize and cleanup

5141e1b

Merge branch 'next' into controls-1.0

ce90d73

Formatting

87db81f

Serialize args to URL

0214497

Fix wrong typings

6e16d6d

Don't use manager cache when working on official storybook

0a91510

ghengeveld requested review from ndelangen and tmeasday as code owners February 3, 2021 14:23

tmeasday requested changes Feb 4, 2021

View reviewed changes

addons/controls/src/ControlsPanel.tsx Outdated Show resolved Hide resolved

addons/controls/src/ControlsPanel.tsx Outdated Show resolved Hide resolved

lib/api/src/modules/url.ts Outdated Show resolved Hide resolved

lib/api/src/modules/url.ts Outdated Show resolved Hide resolved

ghengeveld marked this pull request as draft February 4, 2021 08:46

ghengeveld added 3 commits February 4, 2021 09:48

No need for useMemo

658772d

Move args parsing/serialization to router module

146d8ec

Prevent encoding of args in iframe url

f8f86df

Add support for args param in preview

e58ceb2

shilman added addon: controls args core feature request labels Feb 6, 2021

ghengeveld changed the title ~~Sync args state to URL~~ Core: Sync args state to URL Feb 7, 2021

tmeasday requested changes Feb 7, 2021

View reviewed changes

ghengeveld added 2 commits February 12, 2021 12:32

Use the same validation rules for keys and values

86d5c6f

Add docs for args URL syncing

309171a

This was referenced Feb 12, 2021

Addon-controls: Blob control/arg type #13887

Closed

Addon-controls: Improve select arg serialization #13888

Closed

shilman approved these changes Feb 12, 2021

View reviewed changes

j3rem1e approved these changes Feb 12, 2021

View reviewed changes

tmeasday approved these changes Feb 13, 2021

View reviewed changes

shilman mentioned this pull request Feb 15, 2021

Addon-controls: Add JSON tree editor for Object/Array Type args #12824

Merged

Fix initial args value and map args to their expected data type.

ce4b618

Warn when attempting to serialize or unserialize args with disallowed characters.

ghengeveld requested a review from dangreenisrael as a code owner February 15, 2021 13:55

ghengeveld added 3 commits February 15, 2021 15:09

Fix tests

8c32cfb

Omit nested unchanged properties and array values

0e849d7

Fix test

f131c37

ghengeveld added 4 commits February 15, 2021 21:46

Rename file

952c2e6

Don't use sparse arrays because they aren't currently supported in QS

4e6e554

Add debounce

964c41e

Disable tests for sparse arrays

3af6bc7

shilman approved these changes Feb 17, 2021

View reviewed changes

shilman merged commit 70ac914 into next Feb 17, 2021

shilman deleted the url-state-sync branch February 17, 2021 15:04

This was referenced Feb 17, 2021

Addon-controls: Copy as URL and load from URL #12291

Closed

feat(addon-controls): support query parameters #12728

Closed

ghengeveld mentioned this pull request Feb 26, 2021

Storybook Controls : Ability to set controls via query strings to match @storybook/addon-knobs behaviour #14064

Closed

shilman mentioned this pull request Mar 1, 2021

StorybookUI: Opening canvas in new tab should maintain custom properties (such as the background set for the canvas) #12430

Closed

shilman mentioned this pull request Mar 12, 2021

Ability to pass URL parameters to stories #6133

Closed

antoniosZ mentioned this pull request May 13, 2021

url params don't work for some args (protected names maybe?) #14508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Sync args state to URL #13803

Core: Sync args state to URL #13803

ghengeveld commented Feb 3, 2021

tmeasday left a comment •

edited

Loading

j3rem1e commented Feb 4, 2021

ghengeveld commented Feb 7, 2021

ghengeveld commented Feb 7, 2021 •

edited

Loading

shilman commented Feb 7, 2021

j3rem1e commented Feb 7, 2021

tmeasday commented Feb 7, 2021 •

edited

Loading

tmeasday left a comment

tmeasday Feb 7, 2021

ghengeveld Feb 11, 2021

tmeasday Feb 7, 2021

ghengeveld Feb 11, 2021

tmeasday Feb 7, 2021

ghengeveld Feb 17, 2021

tmeasday Feb 7, 2021

ghengeveld Feb 11, 2021

tmeasday commented Feb 7, 2021

shilman left a comment

j3rem1e left a comment

ghengeveld commented Feb 13, 2021 •

edited

Loading

tmeasday left a comment

tmeasday commented Feb 13, 2021

ghengeveld commented Feb 15, 2021

ghengeveld commented Feb 17, 2021

shilman left a comment

Core: Sync args state to URL #13803

Core: Sync args state to URL #13803

Conversation

ghengeveld commented Feb 3, 2021

What I did

How to test

tmeasday left a comment • edited Loading

Choose a reason for hiding this comment

j3rem1e commented Feb 4, 2021

ghengeveld commented Feb 7, 2021

ghengeveld commented Feb 7, 2021 • edited Loading

shilman commented Feb 7, 2021

j3rem1e commented Feb 7, 2021

tmeasday commented Feb 7, 2021 • edited Loading

tmeasday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday commented Feb 7, 2021

shilman left a comment

Choose a reason for hiding this comment

j3rem1e left a comment

Choose a reason for hiding this comment

ghengeveld commented Feb 13, 2021 • edited Loading

tmeasday left a comment

Choose a reason for hiding this comment

tmeasday commented Feb 13, 2021

ghengeveld commented Feb 15, 2021

ghengeveld commented Feb 17, 2021

shilman left a comment

Choose a reason for hiding this comment

tmeasday left a comment •

edited

Loading

ghengeveld commented Feb 7, 2021 •

edited

Loading

tmeasday commented Feb 7, 2021 •

edited

Loading

ghengeveld commented Feb 13, 2021 •

edited

Loading