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

CHI-2311: POC - Do not merge - persist redux locally #1936

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

stephenhand
Copy link
Collaborator

Description

  • Save redux state to local storage when updated (debounced by 1 second)
  • Exclude configuration from persisted state
  • Replace JS Set objects with serialization friendly alternative

Checklist

  • Corresponding issue has been opened
  • New tests added
  • Feature flags added
  • N/A Strings are localized
  • Tested for chat contacts
  • Tested for call contacts

Related Issues

CHI-2311

Verification steps

  • Enable the feature flag and verify that partially completed forms and routing locations are persisted both over refreshes and closing / reopening

… load on initialise. Remove use of Set types from redux state
@robert-bo-davis
Copy link
Collaborator

robert-bo-davis commented Dec 13, 2023

In my experience, persisting certain parts of Redux state to local storage can be beneficial in specific scenarios, such as storing user preferences that rarely change and are not ideal for database storage. However, it's important to approach this method with caution. Persisting complex state to local storage leads to challenges like client-side application lockups that aren't resolved by a simple page refresh, potential security risks on shared computers and around local storage in general, unexpected behavior when opening multiple tabs, and the complexity of managing cache invalidation.

If you're considering this approach, here are some strategies that may help mitigate some of the potential issues:

  1. Invalidate state on logout and login to improve data security and relevancy.
  2. Implement TTL for your local storage cache to avoid permanent lockups, allowing the application to reset eventually without waiting for the next deployment or for the flex session to expire and require login.
  3. Regularly clean up local storage to prevent excessive data buildup. Local storage limits aren't enormous.
  4. Use a whitelist approach for persisting state slices to minimize the stored data and the impact of issues that will arise.
  5. Carefully weigh the risks and benefits before deciding to persist any part of state to local storage and use it as a last resort.

@robert-bo-davis
Copy link
Collaborator

Oh...

  1. Consider session storage instead of local storage because it helps with some of the potential issues, but shares the security issues and shares a similar, though slightly different set of weird behaviors when new tabs are opened.

@stephenhand
Copy link
Collaborator Author

In my experience, persisting certain parts of Redux state to local storage can be beneficial in specific scenarios, such as storing user preferences that rarely change and are not ideal for database storage. However, it's important to approach this method with caution. Persisting complex state to local storage leads to challenges like client-side application lockups that aren't resolved by a simple page refresh, potential security risks on shared computers and around local storage in general, unexpected behavior when opening multiple tabs, and the complexity of managing cache invalidation.

If you're considering this approach, here are some strategies that may help mitigate some of the potential issues:

  1. Invalidate state on logout and login to improve data security and relevancy.
  2. Implement TTL for your local storage cache to avoid permanent lockups, allowing the application to reset eventually without waiting for the next deployment or for the flex session to expire and require login.
  3. Regularly clean up local storage to prevent excessive data buildup. Local storage limits aren't enormous.
  4. Use a whitelist approach for persisting state slices to minimize the stored data and the impact of issues that will arise.
  5. Carefully weigh the risks and benefits before deciding to persist any part of state to local storage and use it as a last resort.

Thanks for this - whilst I had mitigations in mind for the other things you mentioned, I hadn't considered the security implications.

One thing we could do is only persist the routing & the drafts / metadata of case & contacts - which would generally only hold the stuff that's rendered to the screen anyway, so could be scraped by an XSS

And sorry for the naïve question - but why is session / locally stored data any more vulnerable to being read via an XSS attack than the redux state already held in memory? The persisted state would never hold more than what's already loaded

As for the wierd multi tab behaviour -I deliberately only load state at startup, then only save after that - which means you could never be sure which tab's state you would 'resume' if you closed them both at the same time, but at least you wouldn't see weird random updates in multiple tabs. Also - the multi tab experience in Flex is already far from great :-)

@robert-bo-davis
Copy link
Collaborator

robert-bo-davis commented Dec 13, 2023

And sorry for the naïve question - but why is session / locally stored data any more vulnerable to being read via an XSS attack than the redux state already held in memory?

The redux state is susceptible to XSS attack in the context of the memory of the currently running page/window. The session store and even more so local storage since it persists forever are susceptible to XSS attack from code that is served from anywhere in the current domain. Since we are twilio hosting flex and all of our code is served from https://flex.twilio.com/, any other twilio hosted flex application can access local storage. It can't even really be considered XSS vulnerability in our case since an attacker could spin up their own twilio hosted flex application, use phishing or other social engineering techniques to get users to open their app, and then use that app to harvest whatever they want out of local storage.

If we were self hosting flex at a domain that we control and were totally sure that there is no avenue for attack (are we using clickable user generated links anywhere? We are currently using dangerouslySetInnerHTML in one place, are we sure that no attacker could ever figure out a way to manipulate the data that is being displayed in that div?), then it would be slightly less worrisome.

@murilovmachado
Copy link
Contributor

One thing we could do is only persist the routing & the drafts / metadata of case & contacts - which would generally only hold the stuff that's rendered to the screen anyway, so could be scraped by an XSS

I like this idea a lot

@nickhurlburt
Copy link
Collaborator

Saving anything that might include PII or other sensitive data to local storage scares the hell out of me. Let's please not do that.

Is the problem that we're trying to solve that people can lose data across a refresh? I suppose I'd be a little worried that adding on local storage and the potential for "glitches" that could appear with that might be equal in pain to the current situation of losing context/routing on refresh. I've seen apps in previous companies get borked due to local storage issues and one of the web devs had to come over and manually fix them, and I would be wary about introducing something similar. Though I don't really have a good sense of the tradeoffs... it might be that capturing routing info/metadata in local/session storage might be worth the added complexity if it solves a lot of user pain.

@stephenhand
Copy link
Collaborator Author

The one thing we would have to do if we were to go down a partial loading route would be to move to a move to a more 'lazy load' type pattern for resources.

Currently there are places in the code that assume certain resources must be loaded up in redux for you to have gotten there. This holds true if we persist the whole state, but not if we only persist parts of it (or if we move to a standard react routing model where we can describe any location in the UI with a URL). In those cases we cannot assume any state is present in any part of the UI.

In that scenario, it makes more sense to have lazy load type actions, where the component just requests resources and the redux layer can serve them from the local store if present or request them from the backend if not, that way it can always load the data for a given part of the UI regardless of the state of the redux store.

It's a model I'd like to move to in the medium term now we have the redux store configured to support it (the resources state already uses this approach), but these sort of 'restore any state' enhancements would make it a necessity

@stephenhand
Copy link
Collaborator Author

There are also options like https://www.npmjs.com/package/redux-persist-transform-encrypt when we have the ability to customise our middlewares

@stephenhand stephenhand changed the title CHI-2311: persist redux locally CHI-2311: POC - Do not merge - persist redux locally Dec 14, 2023
@stephenhand
Copy link
Collaborator Author

@annaliseirby090 - the other thing to bear in mind is that we now have the reminder dialog when you try to close / refresh a browser window with unsaved data - might that make this feature less important?

@robert-bo-davis
Copy link
Collaborator

robert-bo-davis commented Dec 14, 2023

In that scenario, it makes more sense to have lazy load type actions, where the component just requests resources and the redux layer can serve them from the local store if present or request them from the backend if not, that way it can always load the data for a given part of the UI regardless of the state of the redux store.

Yes! I started down this path for profiles. This is generally the responsibility of a hook that can useEffect in one of the more common patterns in modern react. That hook can live in the state domain (like they do right now for profiles), but it is probably best not to try to create redux getter functions that trigger actions based on the current state. (I'm making some assumptions here about what you have planned, forgive me if I'm off-base).

The other side of the coin is that you need to start dealing with "partial loaded" resources when list response from the backend only contain part of the needed data, but they stick the partial data into the same resource entry in state that the detail view uses.

I would suggest that emulating the pattern that SWR and react-query both use using redux can be useful. The component that needs data shows the data that it currently has (if any partial data was cached during the list view or if the the resource has been loaded before) and regardless of the state it fires requests to the backend to update on mount and rehydrates based on the response. This makes navigation around the site feel fast (no giant loading spinners... the data fills in as it loads) and keeps the displayed data eventually consistent with what is stored on the backend. It does require keeping your components tightly coupled to the data that they display (see the division of the profile components for an example of how fine this divisions needs to be)

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