fix(documents): decode display filename + guard path decode against malformed %

__tests__/hardening/batch3-documentAttach.test.ts

@@ -27,7 +27,7 @@ jest.mock('../../src/services/pdfExtractor', () => ({
   pdfExtractor: { isAvailable: jest.fn(() => false), extractText: jest.fn() },
 }));
 
-import { documentService } from '../../src/services/documentService';
+import { documentService, sanitizePathSegment } from '../../src/services/documentService';
 
 const rnfs = RNFS as jest.Mocked<typeof RNFS>;
 
@@ -109,35 +109,50 @@ describe('Batch3 · document attach validation (real documentService)', () => {
 
   // ── #17: URL-encoded filename should display decoded ────────────────────────
   //
-  // BUG-FOUND: documentService.processDocumentFromPath returns `fileName` VERBATIM
-  // for display (src/services/documentService.ts:156,190). It decodeURIComponent()s
-  // only the file PATH inside resolveContentUri, never the display name. So a name
-  // like 'my%20notes.txt' is surfaced to the attachment chip un-decoded, contrary
-  // to Provit case #17 ("shows the human-readable decoded filename, not the raw
-  // encoded string"). On device the picker usually hands back an already-decoded
-  // name, which is why the E2E may still pass — but the service seam does not
-  // guarantee it. Fixing this belongs in src (decode the display name once in the
-  // service); per hardening rules we do NOT edit src, so the correct-behavior
-  // assertion is skipped and the actual (buggy) behavior is pinned below.
+  // FIXED (#17): the display filename is now decoded once in the service
+  // (documentService.decodeDisplayName), so a percent-encoded name shows human-readable
+  // in the chip/preview. Fails-before (returned 'my%20notes.txt') / passes-after.
   describe('URL-encoded filename display decode (#17)', () => {
-    it.skip('BUG-FOUND: display fileName should be decoded (my%20notes.txt -> "my notes.txt")', async () => {
+    it('decodes a percent-encoded display fileName (my%20notes.txt -> "my notes.txt")', async () => {
       stubReadableFile('body');
       const att = await documentService.processDocumentFromPath(
         '/docs/my%20notes.txt',
         'my%20notes.txt',
       );
-      // Desired behavior per Provit #17: the chip shows the decoded, human-readable name.
       expect(att!.fileName).toBe('my notes.txt');
     });
 
-    it.skip('pins ACTUAL behavior: the display fileName is returned un-decoded (documents the bug) — SKIP: do not enshrine the bug as passing; see the desired-behavior skip above', async () => {
+    it('leaves a malformed percent-sequence name untouched instead of throwing', async () => {
+      // decodeURIComponent('100%.txt') throws — the guard must fall back to the raw name.
+      stubReadableFile('body');
+      const att = await documentService.processDocumentFromPath('/docs/100%.txt', '100%.txt');
+      expect(att!.fileName).toBe('100%.txt');
+    });
+
+    it('does NOT let a percent-encoded traversal name escape the attachments dir (security)', async () => {
+      // '%2E%2E%2Fescape.txt' decodes to '../escape.txt'. The DISPLAY name may show it
+      // decoded, but the filesystem destination must be sanitized — no '/' or '..' in the
+      // path segment handed to copyFile, so the write can't leave ATTACHMENTS_DIR.
       stubReadableFile('body');
       const att = await documentService.processDocumentFromPath(
-        '/docs/my%20notes.txt',
-        'my%20notes.txt',
+        '/docs/%2E%2E%2Fescape.txt',
+        '%2E%2E%2Fescape.txt',
       );
-      // NOT decoded today — this is the current, incorrect behavior we are pinning.
-      expect(att!.fileName).toBe('my%20notes.txt');
+      expect(att).not.toBeNull();
+      // The persistent copy destination (2nd arg to RNFS.copyFile) must contain no
+      // separator/traversal after the id_ prefix.
+      const destPaths = rnfs.copyFile.mock.calls.map(c => String(c[1]));
+      const persistent = destPaths.find(p => p.includes('attachments'));
+      expect(persistent).toBeDefined();
+      expect(persistent).not.toContain('../');
+      expect(persistent!.split('attachments/')[1]).not.toContain('/'); // basename only, no nested dirs
+    });
+
+    it('sanitizePathSegment neutralizes separators + traversal but keeps a normal name', () => {
+      expect(sanitizePathSegment('my notes.txt')).toBe('my notes.txt');
+      expect(sanitizePathSegment('a/b/c.txt')).toBe('a_b_c.txt');
+      expect(sanitizePathSegment('%2E%2E%2Fescape.txt')).not.toContain('/');
+      expect(sanitizePathSegment('%2E%2E%2Fescape.txt')).not.toContain('..');
     });
 
     it('resolves the file even when the PATH is URL-encoded (path decode works)', async () => {

src/services/documentService.ts

@@ -22,6 +22,50 @@ const MAX_FILE_SIZE = 5 * 1024 * 1024;
 // Persistent directory for attached documents
 const ATTACHMENTS_DIR = `${RNFS.DocumentDirectoryPath}/attachments`;
 
+/**
+ * decodeURIComponent that never throws: a stray/malformed '%' (e.g. '100%.txt' or a
+ * bad %-sequence) makes the native decodeURIComponent throw 'URI malformed'. Falling
+ * back to the raw string keeps callers (path resolution AND display-name decode) from
+ * crashing on odd-but-valid filenames. Defined once; both call sites use it.
+ */
+function safeDecodeURIComponent(s: string): string {
+  try {
+    return decodeURIComponent(s);
+  } catch (e) {
+    // Surface the fallback so a malformed-encoding case is diagnosable rather than silent.
+    console.warn(`[DocumentService] decodeURIComponent failed for "${s}", using raw value:`, e instanceof Error ? e.message : e);
+    return s;
+  }
+}
+
+/**
+ * Decode a percent-encoded display filename for the chip/preview (e.g. 'my%20notes.txt'
+ * → 'my notes.txt'). Only touches names that actually contain a '%'. FOR DISPLAY ONLY —
+ * never use the result as a filesystem path segment (see sanitizePathSegment).
+ */
+export function decodeDisplayName(name: string): string {
+  return name.includes('%') ? safeDecodeURIComponent(name) : name;
+}
+
+/**
+ * A filesystem-safe basename for path interpolation. The display name may decode to
+ * contain real separators or traversal (a percent-encoded '%2F' / '%2E%2E%2F' becomes
+ * '/' / '../'), which would let a copy escape the cache/attachments dir. Strip path
+ * separators, collapse '..' segments, and remove control chars — the result is only ever
+ * used to build a destination filename, never shown to the user.
+ */
+export function sanitizePathSegment(name: string): string {
+  const decoded = decodeDisplayName(name);
+  const flattened = decoded
+    .replace(/[/\\]/g, '_') // path separators -> underscore
+    // eslint-disable-next-line no-control-regex -- deliberately stripping control chars
+    .replace(/[\u0000-\u001f]/g, '') // strip control chars
+    .replace(/\.{2,}/g, '_')             // collapse any '..'(..+) traversal
+    .replace(/^\.+/, '')                 // strip leading dots (hidden/relative)
+    .trim();
+  return flattened.length > 0 ? flattened : 'document';
+}
+
 class DocumentService {
   /**
    * Ensure the persistent attachments directory exists
@@ -57,9 +101,11 @@ class DocumentService {
     // RNFS.DocumentDirectoryPath is the app's Documents directory (without file://)
     const documentsPath = RNFS.DocumentDirectoryPath;
 
-    // Decode URL-encoded characters (like %20 for spaces) and strip file:// prefix
-    // This is critical because RNFS.exists() needs decoded paths, not URL-encoded
-    const decodedUri = decodeURIComponent(uri);
+    // Decode URL-encoded characters (like %20 for spaces) and strip file:// prefix.
+    // Critical because RNFS.exists() needs decoded paths, not URL-encoded. Guarded:
+    // a raw decodeURIComponent throws 'URI malformed' on a stray '%' (e.g. '100%.txt'),
+    // which would crash the whole attach — fall back to the raw uri in that case.
+    const decodedUri = safeDecodeURIComponent(uri);
     const cleanUri = decodedUri.replace(/^file:\/\//, '');
     console.log(`[DocumentService] Decoded and cleaned path: ${cleanUri}`);
     console.log(`[DocumentService] Documents path: ${documentsPath}`);
@@ -153,13 +199,25 @@ class DocumentService {
   async processDocumentFromPath(filePath: string, fileName?: string, maxCharsOverride?: number): Promise<MediaAttachment | null> {
     try {
       console.log(`[DocumentService] Processing document - filePath: ${filePath}, fileName: ${fileName}`);
-      const name = fileName || filePath.split('/').pop() || 'document';
-      const extension = `.${name.split('.').pop()?.toLowerCase()}`;
+      // Decode a percent-encoded display name (e.g. 'my%20notes.txt' → 'my notes.txt').
+      // A content:// / file:// URI's last path segment (used when no fileName is given)
+      // is URL-encoded; without this the chip shows the raw encoded string. Guarded:
+      // decodeURIComponent throws on a malformed %-sequence, so fall back to the raw name.
+      const rawName = fileName || filePath.split('/').pop() || 'document';
+      // Two distinct derivations from the raw name:
+      //  - displayName: decoded, human-readable — shown in the chip/preview + errors.
+      //  - safeFsName:  sanitized — the ONLY value interpolated into a filesystem path
+      //    (temp copy + persistent copy). Keeping these separate stops a decoded
+      //    separator/traversal ('%2F'/'%2E%2E%2F' → '/'/'..') from escaping the cache/
+      //    attachments dir.
+      const displayName = decodeDisplayName(rawName);
+      const safeFsName = sanitizePathSegment(rawName);
+      const extension = `.${displayName.split('.').pop()?.toLowerCase()}`;
       const isPdf = extension === PDF_EXTENSION;
       console.log(`[DocumentService] Detected extension: ${extension}, isPdf: ${isPdf}`);
       this.validateFileType(extension, isPdf);
 
-      const resolvedPath = await this.resolveContentUri(filePath, name);
+      const resolvedPath = await this.resolveContentUri(filePath, safeFsName);
       console.log(`[DocumentService] Resolved path: ${resolvedPath}`);
 
       // Verify the file exists and is accessible
@@ -174,7 +232,7 @@ class DocumentService {
       }
 
       if (!fileExists) {
-        throw new Error(`File not found: ${name}`);
+        throw new Error(`File not found: ${displayName}`);
       }
 
       const stat = await RNFS.stat(resolvedPath);
@@ -185,9 +243,9 @@ class DocumentService {
 
       const maxChars = maxCharsOverride ?? Math.floor((useAppStore.getState().settings.contextLength || APP_CONFIG.maxContextLength) * 4 * 0.5);
       const textContent = await this.readContent(resolvedPath, isPdf, maxChars);
-      const { id, uri } = await this.savePersistentCopy(resolvedPath, filePath, name);
+      const { id, uri } = await this.savePersistentCopy(resolvedPath, filePath, safeFsName);
 
-      return { id, type: 'document', uri, fileName: name, textContent, fileSize: stat.size };
+      return { id, type: 'document', uri, fileName: displayName, textContent, fileSize: stat.size };
     } catch (error: any) {
       throw error;
     }