mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-02-02 20:43:36 +00:00
feat: enhance global settings update with data loss prevention
- Added safeguards to prevent overwriting non-empty arrays with empty arrays during global settings updates, specifically for the 'projects' field. - Implemented logging for updates to assist in diagnosing accidental wipes of critical settings. - Updated tests to verify that projects are preserved during logout transitions and that theme changes are ignored if a project wipe is attempted. - Enhanced the settings synchronization logic to ensure safe handling during authentication state changes.
This commit is contained in:
@@ -11,7 +11,7 @@
|
|||||||
import type { Request, Response } from 'express';
|
import type { Request, Response } from 'express';
|
||||||
import type { SettingsService } from '../../../services/settings-service.js';
|
import type { SettingsService } from '../../../services/settings-service.js';
|
||||||
import type { GlobalSettings } from '../../../types/settings.js';
|
import type { GlobalSettings } from '../../../types/settings.js';
|
||||||
import { getErrorMessage, logError } from '../common.js';
|
import { getErrorMessage, logError, logger } from '../common.js';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Create handler factory for PUT /api/settings/global
|
* Create handler factory for PUT /api/settings/global
|
||||||
@@ -32,6 +32,18 @@ export function createUpdateGlobalHandler(settingsService: SettingsService) {
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Minimal debug logging to help diagnose accidental wipes.
|
||||||
|
if ('projects' in updates || 'theme' in updates || 'localStorageMigrated' in updates) {
|
||||||
|
const projectsLen = Array.isArray((updates as any).projects)
|
||||||
|
? (updates as any).projects.length
|
||||||
|
: undefined;
|
||||||
|
logger.info(
|
||||||
|
`Update global settings request: projects=${projectsLen ?? 'n/a'}, theme=${
|
||||||
|
(updates as any).theme ?? 'n/a'
|
||||||
|
}, localStorageMigrated=${(updates as any).localStorageMigrated ?? 'n/a'}`
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
const settings = await settingsService.updateGlobalSettings(updates);
|
const settings = await settingsService.updateGlobalSettings(updates);
|
||||||
|
|
||||||
res.json({
|
res.json({
|
||||||
|
|||||||
@@ -266,25 +266,80 @@ export class SettingsService {
|
|||||||
const settingsPath = getGlobalSettingsPath(this.dataDir);
|
const settingsPath = getGlobalSettingsPath(this.dataDir);
|
||||||
|
|
||||||
const current = await this.getGlobalSettings();
|
const current = await this.getGlobalSettings();
|
||||||
|
|
||||||
|
// Guard against destructive "empty array/object" overwrites.
|
||||||
|
// During auth transitions, the UI can briefly have default/empty state and accidentally
|
||||||
|
// sync it, wiping persisted settings (especially `projects`).
|
||||||
|
const sanitizedUpdates: Partial<GlobalSettings> = { ...updates };
|
||||||
|
let attemptedProjectWipe = false;
|
||||||
|
|
||||||
|
const ignoreEmptyArrayOverwrite = <K extends keyof GlobalSettings>(key: K): void => {
|
||||||
|
const nextVal = sanitizedUpdates[key] as unknown;
|
||||||
|
const curVal = current[key] as unknown;
|
||||||
|
if (
|
||||||
|
Array.isArray(nextVal) &&
|
||||||
|
nextVal.length === 0 &&
|
||||||
|
Array.isArray(curVal) &&
|
||||||
|
curVal.length > 0
|
||||||
|
) {
|
||||||
|
delete sanitizedUpdates[key];
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
const currentProjectsLen = Array.isArray(current.projects) ? current.projects.length : 0;
|
||||||
|
if (
|
||||||
|
Array.isArray(sanitizedUpdates.projects) &&
|
||||||
|
sanitizedUpdates.projects.length === 0 &&
|
||||||
|
currentProjectsLen > 0
|
||||||
|
) {
|
||||||
|
attemptedProjectWipe = true;
|
||||||
|
delete sanitizedUpdates.projects;
|
||||||
|
}
|
||||||
|
|
||||||
|
ignoreEmptyArrayOverwrite('trashedProjects');
|
||||||
|
ignoreEmptyArrayOverwrite('projectHistory');
|
||||||
|
ignoreEmptyArrayOverwrite('recentFolders');
|
||||||
|
ignoreEmptyArrayOverwrite('aiProfiles');
|
||||||
|
ignoreEmptyArrayOverwrite('mcpServers');
|
||||||
|
ignoreEmptyArrayOverwrite('enabledCursorModels');
|
||||||
|
ignoreEmptyArrayOverwrite('enabledCodexModels');
|
||||||
|
|
||||||
|
// Empty object overwrite guard
|
||||||
|
if (
|
||||||
|
sanitizedUpdates.lastSelectedSessionByProject &&
|
||||||
|
typeof sanitizedUpdates.lastSelectedSessionByProject === 'object' &&
|
||||||
|
!Array.isArray(sanitizedUpdates.lastSelectedSessionByProject) &&
|
||||||
|
Object.keys(sanitizedUpdates.lastSelectedSessionByProject).length === 0 &&
|
||||||
|
current.lastSelectedSessionByProject &&
|
||||||
|
Object.keys(current.lastSelectedSessionByProject).length > 0
|
||||||
|
) {
|
||||||
|
delete sanitizedUpdates.lastSelectedSessionByProject;
|
||||||
|
}
|
||||||
|
|
||||||
|
// If a request attempted to wipe projects, also ignore theme changes in that same request.
|
||||||
|
if (attemptedProjectWipe) {
|
||||||
|
delete sanitizedUpdates.theme;
|
||||||
|
}
|
||||||
|
|
||||||
const updated: GlobalSettings = {
|
const updated: GlobalSettings = {
|
||||||
...current,
|
...current,
|
||||||
...updates,
|
...sanitizedUpdates,
|
||||||
version: SETTINGS_VERSION,
|
version: SETTINGS_VERSION,
|
||||||
};
|
};
|
||||||
|
|
||||||
// Deep merge keyboard shortcuts if provided
|
// Deep merge keyboard shortcuts if provided
|
||||||
if (updates.keyboardShortcuts) {
|
if (sanitizedUpdates.keyboardShortcuts) {
|
||||||
updated.keyboardShortcuts = {
|
updated.keyboardShortcuts = {
|
||||||
...current.keyboardShortcuts,
|
...current.keyboardShortcuts,
|
||||||
...updates.keyboardShortcuts,
|
...sanitizedUpdates.keyboardShortcuts,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
// Deep merge phaseModels if provided
|
// Deep merge phaseModels if provided
|
||||||
if (updates.phaseModels) {
|
if (sanitizedUpdates.phaseModels) {
|
||||||
updated.phaseModels = {
|
updated.phaseModels = {
|
||||||
...current.phaseModels,
|
...current.phaseModels,
|
||||||
...updates.phaseModels,
|
...sanitizedUpdates.phaseModels,
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -144,6 +144,33 @@ describe('settings-service.ts', () => {
|
|||||||
expect(updated.keyboardShortcuts.agent).toBe(DEFAULT_GLOBAL_SETTINGS.keyboardShortcuts.agent);
|
expect(updated.keyboardShortcuts.agent).toBe(DEFAULT_GLOBAL_SETTINGS.keyboardShortcuts.agent);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should not overwrite non-empty projects with an empty array (data loss guard)', async () => {
|
||||||
|
const initial: GlobalSettings = {
|
||||||
|
...DEFAULT_GLOBAL_SETTINGS,
|
||||||
|
theme: 'solarized' as GlobalSettings['theme'],
|
||||||
|
projects: [
|
||||||
|
{
|
||||||
|
id: 'proj1',
|
||||||
|
name: 'Project 1',
|
||||||
|
path: '/tmp/project-1',
|
||||||
|
lastOpened: new Date().toISOString(),
|
||||||
|
},
|
||||||
|
] as any,
|
||||||
|
};
|
||||||
|
const settingsPath = path.join(testDataDir, 'settings.json');
|
||||||
|
await fs.writeFile(settingsPath, JSON.stringify(initial, null, 2));
|
||||||
|
|
||||||
|
const updated = await settingsService.updateGlobalSettings({
|
||||||
|
projects: [],
|
||||||
|
theme: 'light',
|
||||||
|
} as any);
|
||||||
|
|
||||||
|
expect(updated.projects.length).toBe(1);
|
||||||
|
expect((updated.projects as any)[0]?.id).toBe('proj1');
|
||||||
|
// Theme should be preserved in the same request if it attempted to wipe projects
|
||||||
|
expect(updated.theme).toBe('solarized');
|
||||||
|
});
|
||||||
|
|
||||||
it('should create data directory if it does not exist', async () => {
|
it('should create data directory if it does not exist', async () => {
|
||||||
const newDataDir = path.join(os.tmpdir(), `new-data-dir-${Date.now()}`);
|
const newDataDir = path.join(os.tmpdir(), `new-data-dir-${Date.now()}`);
|
||||||
const newService = new SettingsService(newDataDir);
|
const newService = new SettingsService(newDataDir);
|
||||||
|
|||||||
@@ -94,6 +94,17 @@ export function waitForMigrationComplete(): Promise<void> {
|
|||||||
return migrationCompletePromise;
|
return migrationCompletePromise;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Reset migration state when auth is lost (logout/session expired).
|
||||||
|
* This ensures that on re-login, the sync hook properly waits for
|
||||||
|
* fresh settings hydration before starting to sync.
|
||||||
|
*/
|
||||||
|
export function resetMigrationState(): void {
|
||||||
|
migrationCompleted = false;
|
||||||
|
migrationCompletePromise = null;
|
||||||
|
migrationCompleteResolve = null;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Parse localStorage data into settings object
|
* Parse localStorage data into settings object
|
||||||
*/
|
*/
|
||||||
|
|||||||
@@ -18,7 +18,7 @@ import { setItem } from '@/lib/storage';
|
|||||||
import { useAppStore, type ThemeMode, THEME_STORAGE_KEY } from '@/store/app-store';
|
import { useAppStore, type ThemeMode, THEME_STORAGE_KEY } from '@/store/app-store';
|
||||||
import { useSetupStore } from '@/store/setup-store';
|
import { useSetupStore } from '@/store/setup-store';
|
||||||
import { useAuthStore } from '@/store/auth-store';
|
import { useAuthStore } from '@/store/auth-store';
|
||||||
import { waitForMigrationComplete } from './use-settings-migration';
|
import { waitForMigrationComplete, resetMigrationState } from './use-settings-migration';
|
||||||
import type { GlobalSettings } from '@automaker/types';
|
import type { GlobalSettings } from '@automaker/types';
|
||||||
|
|
||||||
const logger = createLogger('SettingsSync');
|
const logger = createLogger('SettingsSync');
|
||||||
@@ -98,9 +98,35 @@ export function useSettingsSync(): SettingsSyncState {
|
|||||||
const lastSyncedRef = useRef<string>('');
|
const lastSyncedRef = useRef<string>('');
|
||||||
const isInitializedRef = useRef(false);
|
const isInitializedRef = useRef(false);
|
||||||
|
|
||||||
|
// If auth is lost (logout / session expired), immediately stop syncing and
|
||||||
|
// reset initialization so we can safely re-init after the next login.
|
||||||
|
useEffect(() => {
|
||||||
|
if (!authChecked) return;
|
||||||
|
|
||||||
|
if (!isAuthenticated) {
|
||||||
|
if (syncTimeoutRef.current) {
|
||||||
|
clearTimeout(syncTimeoutRef.current);
|
||||||
|
syncTimeoutRef.current = null;
|
||||||
|
}
|
||||||
|
lastSyncedRef.current = '';
|
||||||
|
isInitializedRef.current = false;
|
||||||
|
|
||||||
|
// Reset migration state so next login properly waits for fresh hydration
|
||||||
|
resetMigrationState();
|
||||||
|
|
||||||
|
setState({ loaded: false, error: null, syncing: false });
|
||||||
|
}
|
||||||
|
}, [authChecked, isAuthenticated]);
|
||||||
|
|
||||||
// Debounced sync function
|
// Debounced sync function
|
||||||
const syncToServer = useCallback(async () => {
|
const syncToServer = useCallback(async () => {
|
||||||
try {
|
try {
|
||||||
|
// Never sync when not authenticated (prevents overwriting server settings during logout/login transitions)
|
||||||
|
const auth = useAuthStore.getState();
|
||||||
|
if (!auth.authChecked || !auth.isAuthenticated) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
setState((s) => ({ ...s, syncing: true }));
|
setState((s) => ({ ...s, syncing: true }));
|
||||||
const api = getHttpApiClient();
|
const api = getHttpApiClient();
|
||||||
const appState = useAppStore.getState();
|
const appState = useAppStore.getState();
|
||||||
@@ -215,7 +241,7 @@ export function useSettingsSync(): SettingsSyncState {
|
|||||||
|
|
||||||
// Subscribe to store changes and sync to server
|
// Subscribe to store changes and sync to server
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!state.loaded) return;
|
if (!state.loaded || !authChecked || !isAuthenticated) return;
|
||||||
|
|
||||||
// Subscribe to app store changes
|
// Subscribe to app store changes
|
||||||
const unsubscribeApp = useAppStore.subscribe((newState, prevState) => {
|
const unsubscribeApp = useAppStore.subscribe((newState, prevState) => {
|
||||||
@@ -272,11 +298,11 @@ export function useSettingsSync(): SettingsSyncState {
|
|||||||
clearTimeout(syncTimeoutRef.current);
|
clearTimeout(syncTimeoutRef.current);
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
}, [state.loaded, scheduleSyncToServer, syncNow]);
|
}, [state.loaded, authChecked, isAuthenticated, scheduleSyncToServer, syncNow]);
|
||||||
|
|
||||||
// Best-effort flush on tab close / backgrounding
|
// Best-effort flush on tab close / backgrounding
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!state.loaded) return;
|
if (!state.loaded || !authChecked || !isAuthenticated) return;
|
||||||
|
|
||||||
const handleBeforeUnload = () => {
|
const handleBeforeUnload = () => {
|
||||||
// Fire-and-forget; may not complete in all browsers, but helps in Electron/webview
|
// Fire-and-forget; may not complete in all browsers, but helps in Electron/webview
|
||||||
@@ -296,7 +322,7 @@ export function useSettingsSync(): SettingsSyncState {
|
|||||||
window.removeEventListener('beforeunload', handleBeforeUnload);
|
window.removeEventListener('beforeunload', handleBeforeUnload);
|
||||||
document.removeEventListener('visibilitychange', handleVisibilityChange);
|
document.removeEventListener('visibilitychange', handleVisibilityChange);
|
||||||
};
|
};
|
||||||
}, [state.loaded, syncNow]);
|
}, [state.loaded, authChecked, isAuthenticated, syncNow]);
|
||||||
|
|
||||||
return state;
|
return state;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -81,9 +81,23 @@ export const THEME_STORAGE_KEY = 'automaker:theme';
|
|||||||
*/
|
*/
|
||||||
export function getStoredTheme(): ThemeMode | null {
|
export function getStoredTheme(): ThemeMode | null {
|
||||||
const stored = getItem(THEME_STORAGE_KEY);
|
const stored = getItem(THEME_STORAGE_KEY);
|
||||||
if (stored) {
|
if (stored) return stored as ThemeMode;
|
||||||
return stored as ThemeMode;
|
|
||||||
|
// Backwards compatibility: older versions stored theme inside the Zustand persist blob.
|
||||||
|
// We intentionally keep reading it as a fallback so users don't get a "default theme flash"
|
||||||
|
// on login/logged-out pages if THEME_STORAGE_KEY hasn't been written yet.
|
||||||
|
try {
|
||||||
|
const legacy = getItem('automaker-storage');
|
||||||
|
if (!legacy) return null;
|
||||||
|
const parsed = JSON.parse(legacy) as { state?: { theme?: unknown } } | { theme?: unknown };
|
||||||
|
const theme = (parsed as any)?.state?.theme ?? (parsed as any)?.theme;
|
||||||
|
if (typeof theme === 'string' && theme.length > 0) {
|
||||||
|
return theme as ThemeMode;
|
||||||
|
}
|
||||||
|
} catch {
|
||||||
|
// Ignore legacy parse errors
|
||||||
}
|
}
|
||||||
|
|
||||||
return null;
|
return null;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -104,4 +104,36 @@ test.describe('Settings startup sync race', () => {
|
|||||||
expect(settingsAfterHydration.projects?.length).toBeGreaterThan(0);
|
expect(settingsAfterHydration.projects?.length).toBeGreaterThan(0);
|
||||||
expect(settingsAfterHydration.projects?.[0]?.path).toBe(FIXTURE_PROJECT_PATH);
|
expect(settingsAfterHydration.projects?.[0]?.path).toBe(FIXTURE_PROJECT_PATH);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('does not wipe projects during logout transition', async ({ page }) => {
|
||||||
|
// Ensure authenticated and app is loaded at least to welcome/board.
|
||||||
|
await authenticateForTests(page);
|
||||||
|
await page.goto('/');
|
||||||
|
await page
|
||||||
|
.locator('[data-testid="welcome-view"], [data-testid="board-view"]')
|
||||||
|
.first()
|
||||||
|
.waitFor({ state: 'visible', timeout: 30000 });
|
||||||
|
|
||||||
|
// Confirm settings.json currently has projects (precondition).
|
||||||
|
const beforeLogout = JSON.parse(fs.readFileSync(SETTINGS_PATH, 'utf-8')) as {
|
||||||
|
projects?: Array<unknown>;
|
||||||
|
};
|
||||||
|
expect(beforeLogout.projects?.length).toBeGreaterThan(0);
|
||||||
|
|
||||||
|
// Navigate to settings and click logout.
|
||||||
|
await page.goto('/settings');
|
||||||
|
await page.locator('[data-testid="logout-button"]').click();
|
||||||
|
|
||||||
|
// Ensure we landed on logged-out or login (either is acceptable).
|
||||||
|
await page
|
||||||
|
.locator('text=You’ve been logged out, text=Authentication Required')
|
||||||
|
.first()
|
||||||
|
.waitFor({ state: 'visible', timeout: 30000 });
|
||||||
|
|
||||||
|
// The server settings file should still have projects after logout.
|
||||||
|
const afterLogout = JSON.parse(fs.readFileSync(SETTINGS_PATH, 'utf-8')) as {
|
||||||
|
projects?: Array<unknown>;
|
||||||
|
};
|
||||||
|
expect(afterLogout.projects?.length).toBeGreaterThan(0);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user