Fix logging out users on upgrade / app recreate with same URL (fix #1176)#1179
Conversation
) This is a follow-up of #1162 (comment) PR #1162 introduced a new `generateRandomSuffix` helper function, used it for its needs (avoiding collisions of injected js/css). But it also replaced existing appname normalizing logic with it, introducing randomness in a place that used to be deterministic. As a result, starting with dd6e15f / v43.1.0, re-creating an app would cause the app to use a different appName, thus a different appData folder, thus losing user data including cookies. This PR leaves the `--inject` fixes of #1176, but reverts the appName logic to the pre-#1176 code.
| * so that an upgrade (same URL) of an existing app keeps using the same appData folder. | ||
| */ | ||
| function normalizeAppName(appName: string, url: string): string { | ||
| const hash = crypto.createHash('md5'); |
There was a problem hiding this comment.
I think I have a solution for this that is best of both worlds if you're interested in me applying it here.
There was a problem hiding this comment.
Change the generateRandomSuffix function to accept an optional seed. So here we could pass the url and get the same behavior, but for the JS code, just let it use a random one if it wasn't provided (as it is now).
There was a problem hiding this comment.
Change the
generateRandomSuffixfunction to accept an optional seed. So here we could pass the url and get the same behavior, but for the JS code, just let it use a random one if it wasn't provided (as it is now).
@TheCleric hmmmm we could, but I'd rather let normalizeAppName do its own thing.
Both functions do hashing, but have different needs, and there's a kind of backwards compatibility we help preserve by keeping normalizeAppName's hashing inlined. By making normalizeAppName use generateRandomSuffix, we'd be suggesting it can use a generic/reusable thing, which is not true: for backwards compat of appData folder we do want normalizeAppName to remain stable. So we don't want to extract this specific hashing further away in a function, where it'll be at the mercy of a future refactor that will fall into the same gotcha, we want to keep it close/inlined.
There was a problem hiding this comment.
@TheCleric actually, adding a test checking normalizeAppName stability
2e75b8a to
fa2640f
Compare
…tivefier#1176) (PR nativefier#1179) This is a follow-up of nativefier#1162 (comment) PR nativefier#1162 introduced a new `generateRandomSuffix` helper function, used it for its needs (avoiding collisions of injected js/css). But it also replaced existing appname normalizing logic with it, introducing randomness in a place that used to be deterministic. As a result, starting with 0e58f8e / v43.1.0, re-creating an app would cause the app to use a different appName, thus a different appData folder, thus losing user data including cookies. This PR leaves the `--inject` fixes of nativefier#1176, but reverts the appName logic to the pre-nativefier#1176 code.
This is a follow-up of #1162 (comment)
PR #1162 introduced a new
generateRandomSuffixhelper function,used it for its needs (avoiding collisions of injected js/css).
But it also replaced existing appname normalizing logic with it,
introducing randomness in a place that used to be deterministic.
As a result, starting with dd6e15f / v43.1.0, re-creating an app would cause
the app to use a different appName, thus a different appData folder, thus
losing user data including cookies.
This PR leaves the
--injectfixes of #1176, but reverts the appName logicto the pre-#1176 code.