mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
fix(frontend): tighten legacy-migration key matching
The legacy migration was consuming any localStorage key starting with `app-`, `flow-`, or `rawapp-`, with no constraint on what followed and no shape check on the decoded payload. Two failure modes called out in review: 1. A future feature (or third-party extension) picking a name like `app-recent` would silently lose data on first migration run. 2. A stray key that happened to base64-decode to valid JSON but wasn't a real legacy draft would still get promoted to the new format, surfacing later as a phantom "Restored from local storage" toast on the next edit. Two guards: - `LEGACY_PATH_SHAPE = /^[uf]\/[^/]+\/.+$/`: after a `<prefix>-` match, the remainder must look like a Windmill item path (`u/owner/name` or `f/folder/name`, possibly with deeper segments). Bare-prefix empty-path entries (`app` / `flow` / `rawapp` for `/add` autosaves) still match the exact branch and don't go through the shape gate. - `isPlausibleLegacyValue`: after decode, require the payload to carry the field the legacy writers actually produced (`flow.flow` for flows, any of `summary|value|policy|path` for apps, any of `files|runnables|data` for raw apps). Both are belt-and-suspenders: nothing else currently uses these key prefixes, but enforcing the shape locally keeps the migration safe against future namespace collisions.
This commit is contained in:
@@ -124,6 +124,40 @@ describe('migrateLegacyUserDrafts', () => {
|
||||
expect(localStorage.getItem('app-u/me/garbled')).toBe('not-base64!!!')
|
||||
})
|
||||
|
||||
it('leaves keys whose path does not match the legacy `u|f/owner/name` shape alone', () => {
|
||||
// A future feature or neighbouring code might pick a key like
|
||||
// `app-recent` for its own purposes. The path doesn't look like a
|
||||
// Windmill item path, so the migration must skip it.
|
||||
localStorage.setItem('app-recent', 'whatever')
|
||||
localStorage.setItem('app-some_other_app', 'whatever')
|
||||
// `flow-u/me/foo` matches the shape and would be migrated, but the
|
||||
// payload also needs to look like a Windmill draft (asserted below).
|
||||
localStorage.setItem('flow-u/me/foo', encodeLegacy({ flow: { value: { modules: [] } } }))
|
||||
|
||||
migrateLegacyUserDrafts('main')
|
||||
|
||||
expect(localStorage.getItem('app-recent')).toBe('whatever')
|
||||
expect(localStorage.getItem('app-some_other_app')).toBe('whatever')
|
||||
expect(localStorage.getItem('userdraft/w/main/flow/u/me/foo')).not.toBeNull()
|
||||
})
|
||||
|
||||
it('skips legacy-shaped keys whose payload does not look like a Windmill draft', () => {
|
||||
// `app-u/me/dash` matches LEGACY_PATH_SHAPE and decodes to valid JSON,
|
||||
// but none of the App-shape fields (summary/value/policy/path) are
|
||||
// present. Treat it as unrelated and leave it untouched.
|
||||
const unrelated = encodeLegacy({ random: 'data', count: 7 })
|
||||
localStorage.setItem('app-u/me/dash', unrelated)
|
||||
const unrelatedFlow = encodeLegacy({ stepsState: {} })
|
||||
localStorage.setItem('flow-u/me/bar', unrelatedFlow)
|
||||
|
||||
migrateLegacyUserDrafts('main')
|
||||
|
||||
expect(localStorage.getItem('app-u/me/dash')).toBe(unrelated)
|
||||
expect(localStorage.getItem('userdraft/w/main/app/u/me/dash')).toBeNull()
|
||||
expect(localStorage.getItem('flow-u/me/bar')).toBe(unrelatedFlow)
|
||||
expect(localStorage.getItem('userdraft/w/main/flow/u/me/bar')).toBeNull()
|
||||
})
|
||||
|
||||
it('migrates multiple legacy entries in a single invocation', () => {
|
||||
localStorage.setItem(
|
||||
'app-u/me/a',
|
||||
|
||||
@@ -36,13 +36,24 @@ const LEGACY_PREFIXES: ReadonlyArray<{ prefix: string; newKind: LegacyKind }> =
|
||||
{ prefix: 'app', newKind: 'app' }
|
||||
]
|
||||
|
||||
/**
|
||||
* A Windmill item path: `u/<owner>/<name…>` or `f/<folder>/<name…>`. The
|
||||
* `<name…>` segment may itself contain slashes, so we don't constrain it
|
||||
* past requiring at least one character. Used to reject incidentally-named
|
||||
* localStorage keys (e.g. `app-recent` from a future feature, or a
|
||||
* neighbouring app's data) before treating them as Windmill drafts.
|
||||
*/
|
||||
const LEGACY_PATH_SHAPE = /^[uf]\/[^/]+\/.+$/
|
||||
|
||||
function matchLegacyKey(
|
||||
key: string
|
||||
): { prefix: string; newKind: LegacyKind; path: string } | undefined {
|
||||
for (const { prefix, newKind } of LEGACY_PREFIXES) {
|
||||
if (key === prefix) return { prefix, newKind, path: '' }
|
||||
if (key.startsWith(prefix + '-')) {
|
||||
return { prefix, newKind, path: key.slice(prefix.length + 1) }
|
||||
const path = key.slice(prefix.length + 1)
|
||||
if (!LEGACY_PATH_SHAPE.test(path)) return undefined
|
||||
return { prefix, newKind, path }
|
||||
}
|
||||
}
|
||||
return undefined
|
||||
@@ -56,6 +67,33 @@ function decodeLegacyState(raw: string): unknown {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Per-kind shape gate. The legacy keys (`app-foo`, `flow-foo`, ...) are
|
||||
* unusual enough that nothing else in the codebase has used them, but
|
||||
* matching `LEGACY_PATH_SHAPE` doesn't prove the payload is actually a
|
||||
* Windmill draft (any base64-of-JSON could pass). Promoting a stray payload
|
||||
* would silently surface as a phantom "Restored from local storage" toast
|
||||
* on the next edit, so we reject anything that doesn't carry the fields the
|
||||
* legacy writers actually produced.
|
||||
*/
|
||||
function isPlausibleLegacyValue(kind: LegacyKind, decoded: unknown): boolean {
|
||||
if (decoded == null || typeof decoded !== 'object') return false
|
||||
const obj = decoded as Record<string, unknown>
|
||||
switch (kind) {
|
||||
case 'flow':
|
||||
// Legacy FlowBuilder wrote { flow, path, selectedId, draft_triggers, ... }.
|
||||
return obj.flow != null && typeof obj.flow === 'object'
|
||||
case 'app':
|
||||
// Legacy AppEditor wrote the App object directly. It carries summary,
|
||||
// value, policy, and path among other fields — any one of those is a
|
||||
// strong signal it's actually a Windmill app payload.
|
||||
return 'summary' in obj || 'value' in obj || 'policy' in obj || 'path' in obj
|
||||
case 'raw_app':
|
||||
// Legacy RawAppEditor wrote { files, runnables, data }.
|
||||
return 'files' in obj || 'runnables' in obj || 'data' in obj
|
||||
}
|
||||
}
|
||||
|
||||
function transformLegacyValue(kind: LegacyKind, decoded: unknown): unknown {
|
||||
const obj = decoded as Record<string, unknown>
|
||||
switch (kind) {
|
||||
@@ -113,7 +151,7 @@ export function migrateLegacyUserDrafts(workspace: string): void {
|
||||
|
||||
try {
|
||||
const decoded = decodeLegacyState(raw)
|
||||
if (decoded == null || typeof decoded !== 'object') continue
|
||||
if (!isPlausibleLegacyValue(match.newKind, decoded)) continue
|
||||
const value = transformLegacyValue(match.newKind, decoded)
|
||||
const target = newKey(workspace, match.newKind, match.path)
|
||||
if (value !== undefined && localStorage.getItem(target) == null) {
|
||||
|
||||
Reference in New Issue
Block a user