rehydratedState is not recalculated on UPDATE_ACTION

[dimitriy-k]

rehydratedState is defined only on INIT_ACTION. When localStorage is updated and then lazy-module is loaded, UPDATE_ACTION called and old state (rehydratedState from INIT) is used.

if ((action.type === INIT_ACTION || action.type === UPDATE_ACTION) && rehydratedState) { nextState = merge({}, nextState, rehydratedState); }

Use case:

1. Login => token is saved in localstorage
2. Reload the page => rehydratedState created with token
3. Login with different user => new token stored in localstorage (but not synced with rehydratedState!)
4. Go to lazy loaded page => UPDATE_ACTION, token is overridden with old token from step 2 :(((

[dimitriy-k]

can you move https://github.com/btroncone/ngrx-store-localstorage/blob/master/src/index.ts#L233
to line 252, so rehydratedState always recalculated

or change the order to
nextState = merge({}, rehydratedState, nextState);
in that case some tests will fail

[BovineEnthusiast]

Having the exact same use case and issue.

[dimitriy-k]

Hello, anyone?.... is that project dead?

[bufke]

It's not dead but no one is actively working on it that I know of. btroncone is still doing releases when pinged. If you can make a Pull Request that clearly demonstrates the problem in a test and a fix that makes the test pass I could review it when I find some time.

[dimitriy-k]

@bufke I have tried to create pull request, but I do not have permissions for this repo

[btroncone]

@bufke Is correct, we are not actively adding new features but we are trying to keep the project stable as best as possible. @dimitriy-k you shouldn't need permissions to create a pull request, only to merge PR's. Let me know when you have this available, thanks!

[dimitriy-k]

@btroncone I am not able to push new branch, how can I create pull request?

[BovineEnthusiast]

@dimitriy-k Have you forked the project? I don't see it in your GitHub profile. Make sure you're not cloning this project but rather made a fork and cloned that.

[btroncone]

Hello @dimitriy-k , you would create a fork of this repo, make your updates, then create a pull request. Please see this explanation for additional details, thanks! https://help.github.com/en/articles/creating-a-pull-request-from-a-fork

[dimitriy-k]

looks like UPDATE_ACTION is not needed at all

#122

[dimitriy-k]

@bufke can you review PR?

[CleverCoder]

I spent almost all day trying to track down what's causing old state to appear out of nowhere. This should be considered a high priority security issue, if multiple people share a browser. It's possible for an affected application to clear out state, even have it synched to storage, only to have the original hydrated state be merged in.
Thank you!!

[CleverCoder]

Well, I discovered today that due to the comments mentioned in the original report (INIT not being called for lazy loaded module metareducers), I needed to re-add the check for UPDATE. A better solution just seems to be to clear out the captured value of hydratedState once it's used. That seems to fix the original issue, but also prevent any accidental re-applying of the restored hydrated state (which could live on, long after ngrx state has been cleared)

[bufke]

This issue confuses me and I'm not that familiar with the original code base. @CleverCoder are you saying you have an alternative proposed fix different from #122

Is it easy to reproduce these issues? I'm guessing I never have been affected by this because I explicitly clear localstorage on logout. I also always handle server side auth errors by ensuring the user is fully logged out, including clearing localstorage.

[CleverCoder]

Yeah.. in my testing I found that the INIT_ACTION action wasn't being called for the lazy loaded modules. I wanted to understand the expected lifecycle, but couldn't find any NgRx reference that could help. What I did observe repeatedly was that for the lazy loaded modules, the first action being passed to the metareducer would be UPDATE_ACTION, and not INIT_ACTION. The current behavior of the code was to merge in the rehydrated state for either action. The problem I originally saw that led me here was that the rehydrated variable was not being cleared. I'm still not sure what triggered it, but the UPDATE action was disptched again, which caused the rehydrated state to be re-applied.

The fix I'm proposing is simply to leave the original code, where it applies the rehydrated state for either action, but once it's been applied clear out the rehydratedState variable to prevent it from accidentally being re-applied again. If we could rely 100% that the INIT action would be called for all module types, only once, then the fix proposed in #122 could work.

I'll try and put together a test case, but it's a pretty busy week.