Make memory and context views mobile-friendly (#813)

* Changes from fix/memory-and-context-mobile-friendly * fix: Improve file extension detection and add path traversal protection * refactor: Extract file extension utilities and add path traversal guards Code review improvements: - Extract isMarkdownFilename and isImageFilename to shared image-utils.ts - Remove duplicated code from context-view.tsx and memory-view.tsx - Add path traversal guard for context fixture utilities (matching memory) - Add 7 new tests for context fixture path traversal protection - Total 61 tests pass Addresses code review feedback from PR #813 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test: Add e2e tests for profiles crud and board background persistence * Update apps/ui/playwright.config.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix: Add robust test navigation handling and file filtering * fix: Format NODE_OPTIONS configuration on single line * test: Update profiles and board background persistence tests * test: Replace iPhone 13 Pro with Pixel 5 for mobile test consistency --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2026-03-18 10:23:07 +00:00 · 2026-02-26 03:31:40 -08:00
parent 6408f514a4
commit 583c3eb4a6
30 changed files with 3758 additions and 113 deletions
--- a/apps/ui/tests/utils/project/fixtures.spec.ts
+++ b/apps/ui/tests/utils/project/fixtures.spec.ts
@@ -0,0 +1,180 @@
+/**
+ * Tests for project fixture utilities
+ *
+ * Tests for path traversal guard and file operations in test fixtures
+ */
+
+import { test, expect } from '@playwright/test';
+import {
+  createMemoryFileOnDisk,
+  memoryFileExistsOnDisk,
+  resetMemoryDirectory,
+  createContextFileOnDisk,
+  contextFileExistsOnDisk,
+  resetContextDirectory,
+} from './fixtures';
+
+test.describe('Memory Fixture Utilities', () => {
+  test.beforeEach(() => {
+    resetMemoryDirectory();
+  });
+
+  test.afterEach(() => {
+    resetMemoryDirectory();
+  });
+
+  test('should create and detect a valid memory file', () => {
+    const filename = 'test-file.md';
+    const content = '# Test Content';
+
+    createMemoryFileOnDisk(filename, content);
+
+    expect(memoryFileExistsOnDisk(filename)).toBe(true);
+  });
+
+  test('should return false for non-existent file', () => {
+    expect(memoryFileExistsOnDisk('non-existent.md')).toBe(false);
+  });
+
+  test('should reject path traversal attempt with ../', () => {
+    const maliciousFilename = '../../../etc/passwd';
+
+    expect(() => {
+      createMemoryFileOnDisk(maliciousFilename, 'malicious content');
+    }).toThrow('Invalid memory filename');
+
+    expect(() => {
+      memoryFileExistsOnDisk(maliciousFilename);
+    }).toThrow('Invalid memory filename');
+  });
+
+  test('should handle Windows-style path traversal attempt ..\\ (platform-dependent)', () => {
+    const maliciousFilename = '..\\..\\..\\windows\\system32\\config';
+
+    // On Unix/macOS, backslash is treated as a literal character in filenames,
+    // not as a path separator, so path.resolve doesn't traverse directories.
+    // This test documents that behavior - the guard works for Unix paths,
+    // but Windows-style backslashes are handled differently per platform.
+    // On macOS/Linux: backslash is a valid filename character
+    // On Windows: would need additional normalization to prevent traversal
+    expect(() => {
+      memoryFileExistsOnDisk(maliciousFilename);
+    }).not.toThrow();
+
+    // The file gets created with backslashes in the name (which is valid on Unix)
+    // but won't escape the directory
+  });
+
+  test('should reject absolute path attempt', () => {
+    const maliciousFilename = '/etc/passwd';
+
+    expect(() => {
+      createMemoryFileOnDisk(maliciousFilename, 'malicious content');
+    }).toThrow('Invalid memory filename');
+
+    expect(() => {
+      memoryFileExistsOnDisk(maliciousFilename);
+    }).toThrow('Invalid memory filename');
+  });
+
+  test('should accept nested paths within memory directory', () => {
+    // Note: This tests the boundary - if subdirectories are supported,
+    // this should pass; if not, it should throw
+    const nestedFilename = 'subfolder/nested-file.md';
+
+    // Currently, the implementation doesn't create subdirectories,
+    // so this would fail when trying to write. But the path itself
+    // is valid (doesn't escape the memory directory)
+    expect(() => {
+      memoryFileExistsOnDisk(nestedFilename);
+    }).not.toThrow();
+  });
+
+  test('should handle filenames without extensions', () => {
+    const filename = 'README';
+
+    createMemoryFileOnDisk(filename, 'content without extension');
+
+    expect(memoryFileExistsOnDisk(filename)).toBe(true);
+  });
+
+  test('should handle filenames with multiple dots', () => {
+    const filename = 'my.file.name.md';
+
+    createMemoryFileOnDisk(filename, '# Multiple dots');
+
+    expect(memoryFileExistsOnDisk(filename)).toBe(true);
+  });
+});
+
+test.describe('Context Fixture Utilities', () => {
+  test.beforeEach(() => {
+    resetContextDirectory();
+  });
+
+  test.afterEach(() => {
+    resetContextDirectory();
+  });
+
+  test('should create and detect a valid context file', () => {
+    const filename = 'test-context.md';
+    const content = '# Test Context Content';
+
+    createContextFileOnDisk(filename, content);
+
+    expect(contextFileExistsOnDisk(filename)).toBe(true);
+  });
+
+  test('should return false for non-existent context file', () => {
+    expect(contextFileExistsOnDisk('non-existent.md')).toBe(false);
+  });
+
+  test('should reject path traversal attempt with ../ for context files', () => {
+    const maliciousFilename = '../../../etc/passwd';
+
+    expect(() => {
+      createContextFileOnDisk(maliciousFilename, 'malicious content');
+    }).toThrow('Invalid context filename');
+
+    expect(() => {
+      contextFileExistsOnDisk(maliciousFilename);
+    }).toThrow('Invalid context filename');
+  });
+
+  test('should reject absolute path attempt for context files', () => {
+    const maliciousFilename = '/etc/passwd';
+
+    expect(() => {
+      createContextFileOnDisk(maliciousFilename, 'malicious content');
+    }).toThrow('Invalid context filename');
+
+    expect(() => {
+      contextFileExistsOnDisk(maliciousFilename);
+    }).toThrow('Invalid context filename');
+  });
+
+  test('should accept nested paths within context directory', () => {
+    const nestedFilename = 'subfolder/nested-file.md';
+
+    // The path itself is valid (doesn't escape the context directory)
+    expect(() => {
+      contextFileExistsOnDisk(nestedFilename);
+    }).not.toThrow();
+  });
+
+  test('should handle filenames without extensions for context', () => {
+    const filename = 'README';
+
+    createContextFileOnDisk(filename, 'content without extension');
+
+    expect(contextFileExistsOnDisk(filename)).toBe(true);
+  });
+
+  test('should handle filenames with multiple dots for context', () => {
+    const filename = 'my.context.file.md';
+
+    createContextFileOnDisk(filename, '# Multiple dots');
+
+    expect(contextFileExistsOnDisk(filename)).toBe(true);
+  });
+});
--- a/apps/ui/tests/utils/project/fixtures.ts
+++ b/apps/ui/tests/utils/project/fixtures.ts
@@ -17,6 +17,7 @@ const WORKSPACE_ROOT = getWorkspaceRoot();
 const FIXTURE_PATH = path.join(WORKSPACE_ROOT, 'test/fixtures/projectA');
 const SPEC_FILE_PATH = path.join(FIXTURE_PATH, '.automaker/app_spec.txt');
 const CONTEXT_PATH = path.join(FIXTURE_PATH, '.automaker/context');
+const MEMORY_PATH = path.join(FIXTURE_PATH, '.automaker/memory');

 // Original spec content for resetting between tests
 const ORIGINAL_SPEC_CONTENT = `<app_spec>
@@ -50,11 +51,53 @@ export function resetContextDirectory(): void {
  fs.mkdirSync(CONTEXT_PATH, { recursive: true });
 }

+/**
+ * Reset the memory directory to empty state
+ */
+export function resetMemoryDirectory(): void {
+  if (fs.existsSync(MEMORY_PATH)) {
+    fs.rmSync(MEMORY_PATH, { recursive: true });
+  }
+  fs.mkdirSync(MEMORY_PATH, { recursive: true });
+}
+
+/**
+ * Resolve and validate a context fixture path to prevent path traversal
+ */
+function resolveContextFixturePath(filename: string): string {
+  const resolved = path.resolve(CONTEXT_PATH, filename);
+  const base = path.resolve(CONTEXT_PATH) + path.sep;
+  if (!resolved.startsWith(base)) {
+    throw new Error(`Invalid context filename: ${filename}`);
+  }
+  return resolved;
+}
+
 /**
 * Create a context file directly on disk (for test setup)
 */
 export function createContextFileOnDisk(filename: string, content: string): void {
-  const filePath = path.join(CONTEXT_PATH, filename);
+  const filePath = resolveContextFixturePath(filename);
+  fs.writeFileSync(filePath, content);
+}
+
+/**
+ * Resolve and validate a memory fixture path to prevent path traversal
+ */
+function resolveMemoryFixturePath(filename: string): string {
+  const resolved = path.resolve(MEMORY_PATH, filename);
+  const base = path.resolve(MEMORY_PATH) + path.sep;
+  if (!resolved.startsWith(base)) {
+    throw new Error(`Invalid memory filename: ${filename}`);
+  }
+  return resolved;
+}
+
+/**
+ * Create a memory file directly on disk (for test setup)
+ */
+export function createMemoryFileOnDisk(filename: string, content: string): void {
+  const filePath = resolveMemoryFixturePath(filename);
  fs.writeFileSync(filePath, content);
 }

@@ -62,7 +105,15 @@ export function createContextFileOnDisk(filename: string, content: string): void
 * Check if a context file exists on disk
 */
 export function contextFileExistsOnDisk(filename: string): boolean {
-  const filePath = path.join(CONTEXT_PATH, filename);
+  const filePath = resolveContextFixturePath(filename);
+  return fs.existsSync(filePath);
+}
+
+/**
+ * Check if a memory file exists on disk
+ */
+export function memoryFileExistsOnDisk(filename: string): boolean {
+  const filePath = resolveMemoryFixturePath(filename);
  return fs.existsSync(filePath);
 }

@@ -112,8 +163,29 @@ export async function setupProjectWithFixture(
    };
    localStorage.setItem('automaker-setup', JSON.stringify(setupState));

+    // Set settings cache so the fast-hydrate path uses our fixture project.
+    // Without this, a stale settings cache from a previous test can override
+    // the project we just set in automaker-storage.
+    const settingsCache = {
+      setupComplete: true,
+      isFirstRun: false,
+      projects: [
+        {
+          id: mockProject.id,
+          name: mockProject.name,
+          path: mockProject.path,
+          lastOpened: mockProject.lastOpened,
+        },
+      ],
+      currentProjectId: mockProject.id,
+      theme: 'dark',
+      sidebarOpen: true,
+      maxConcurrency: 3,
+    };
+    localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));
+
    // Disable splash screen in tests
-    sessionStorage.setItem('automaker-splash-shown', 'true');
+    localStorage.setItem('automaker-disable-splash', 'true');
  }, projectPath);
 }

@@ -123,3 +195,14 @@ export async function setupProjectWithFixture(
 export function getFixturePath(): string {
  return FIXTURE_PATH;
 }
+
+/**
+ * Set up a mock project with the fixture path (for profile/settings tests that need a project).
+ * Options such as customProfilesCount are reserved for future use (e.g. mocking server profile state).
+ */
+export async function setupMockProjectWithProfiles(
+  page: Page,
+  _options?: { customProfilesCount?: number }
+): Promise<void> {
+  await setupProjectWithFixture(page, FIXTURE_PATH);
+}
--- a/apps/ui/tests/utils/project/setup.ts
+++ b/apps/ui/tests/utils/project/setup.ts
@@ -84,6 +84,9 @@ export async function setupWelcomeView(
        setupComplete: true,
        isFirstRun: false,
        projects: opts?.recentProjects || [],
+        // Explicitly set currentProjectId to null so the fast-hydrate path
+        // does not restore a stale project from a previous test.
+        currentProjectId: null,
        theme: 'dark',
        sidebarOpen: true,
        maxConcurrency: 3,
@@ -103,7 +106,7 @@ export async function setupWelcomeView(
      }

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');

      // Set up a mechanism to keep currentProject null even after settings hydration
      // Settings API might restore a project, so we override it after hydration
@@ -226,7 +229,7 @@ export async function setupRealProject(
      localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');
    },
    { path: projectPath, name: projectName, opts: options, versions: STORE_VERSIONS }
  );
@@ -291,7 +294,7 @@ export async function setupMockProject(page: Page): Promise<void> {
    localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

    // Disable splash screen in tests
-    sessionStorage.setItem('automaker-splash-shown', 'true');
+    localStorage.setItem('automaker-disable-splash', 'true');
  }, STORE_VERSIONS);
 }

@@ -423,7 +426,7 @@ export async function setupMockProjectAtConcurrencyLimit(
      localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');
    },
    { maxConcurrency, runningTasks, versions: STORE_VERSIONS }
  );
@@ -505,7 +508,7 @@ export async function setupMockProjectWithFeatures(
      (window as { __mockFeatures?: unknown[] }).__mockFeatures = mockFeatures;

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');
    },
    { opts: options, versions: STORE_VERSIONS }
  );
@@ -577,7 +580,7 @@ export async function setupMockProjectWithContextFile(
      localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');

      // Set up mock file system with a context file for the feature
      // This will be used by the mock electron API
@@ -769,7 +772,7 @@ export async function setupEmptyLocalStorage(page: Page): Promise<void> {
    localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

    // Disable splash screen in tests
-    sessionStorage.setItem('automaker-splash-shown', 'true');
+    localStorage.setItem('automaker-disable-splash', 'true');
  }, STORE_VERSIONS);
 }

@@ -832,7 +835,7 @@ export async function setupMockProjectsWithoutCurrent(page: Page): Promise<void>
    localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

    // Disable splash screen in tests
-    sessionStorage.setItem('automaker-splash-shown', 'true');
+    localStorage.setItem('automaker-disable-splash', 'true');
  }, STORE_VERSIONS);
 }

@@ -910,7 +913,7 @@ export async function setupMockProjectWithSkipTestsFeatures(
      localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');
    },
    { opts: options, versions: STORE_VERSIONS }
  );
@@ -985,7 +988,7 @@ export async function setupMockMultipleProjects(
      localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');
    },
    { count: projectCount, versions: STORE_VERSIONS }
  );
@@ -1056,7 +1059,7 @@ export async function setupMockProjectWithAgentOutput(
      localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

      // Disable splash screen in tests
-      sessionStorage.setItem('automaker-splash-shown', 'true');
+      localStorage.setItem('automaker-disable-splash', 'true');

      // Set up mock file system with output content for the feature
      // Now uses features/{id}/agent-output.md path
@@ -1215,7 +1218,7 @@ export async function setupFirstRun(page: Page): Promise<void> {
    localStorage.setItem('automaker-settings-cache', JSON.stringify(settingsCache));

    // Disable splash screen in tests
-    sessionStorage.setItem('automaker-splash-shown', 'true');
+    localStorage.setItem('automaker-disable-splash', 'true');
  }, STORE_VERSIONS);
 }

@@ -1238,6 +1241,6 @@ export async function setupComplete(page: Page): Promise<void> {
    localStorage.setItem('automaker-setup', JSON.stringify(setupState));

    // Disable splash screen in tests
-    sessionStorage.setItem('automaker-splash-shown', 'true');
+    localStorage.setItem('automaker-disable-splash', 'true');
  }, STORE_VERSIONS);
 }