fix(export): sanitize URIs before using them as directory paths (#1471)

erunion · web-flow · commit ce2568c36682 · 2026-06-05T17:35:41.000-07:00
## 🧰 Changes

Addresses a bug in the new `export docs` and `export reference` commands
where if the `parent` or `category` URI we get back from our API call
contains directory separators we wouldn't do any sanitization on them,
potentially leading to destructive file mutations. To prevent this this
adds in some checks to ensure that these URIs are valid URIs, containing
valid slugs, and do not ever traverse outside of the directory the user
supplies to the export command to export their files to.
diff --git a/src/lib/exportDocs.ts b/src/lib/exportDocs.ts
@@ -13,6 +13,7 @@ import { isRecord } from '../utils.js';
 
 import { parse as parseFrontmatter } from './frontmatter.js';
 import { oraOptions } from './logger.js';
+import { decodeURILastSegment, isSafePathSegment, resolvePathWithinRoot } from './safePath.js';
 
 type ChangelogsRequestSchema = PageRequestSchema<'changelogs'>;
 type CustomPagesRequestSchema = PageRequestSchema<'custom_pages'>;
@@ -47,7 +48,7 @@ interface FileMapEntry {
  * Scrub out unnecessary data from the API and simplify fields.
  *
  */
-function scrub(data: GeneralRequestSchema): Record<string, unknown> | undefined {
+function scrub(data: GeneralRequestSchema): Record<string, unknown> | null | undefined {
   const denylist = new Set(['api', 'href', 'links', 'project', 'renderable', 'updated_at', 'uri']);
 
   /** API defaults (to ensure we omit these when they're unchanged) */
@@ -79,10 +80,14 @@ function scrub(data: GeneralRequestSchema): Record<string, unknown> | undefined
 
       scrubbed[key] = filtered;
     } else if (key === 'category' && isRecord(value) && typeof value.uri === 'string') {
-      const name = decodeURIComponent(value.uri.split('/').pop() || '');
+      const name = decodeURILastSegment(value.uri);
+      if (name === null) return null;
+
       scrubbed[key] = { uri: name };
     } else if (key === 'parent' && isRecord(value) && typeof value.uri === 'string') {
-      const slugPart = decodeURIComponent(value.uri.split('/').pop() || '');
+      const slugPart = decodeURILastSegment(value.uri);
+      if (slugPart === null) return null;
+
       scrubbed[key] = { uri: slugPart };
     } else if (!(key in DEFAULTS && value === DEFAULTS[key as keyof typeof DEFAULTS])) {
       scrubbed[key] = value;
@@ -121,7 +126,7 @@ function buildFileMap(this: APIv2PageExportCommands, files: string[]): Map<strin
       }
     } catch (err) {
       const message = err instanceof Error ? err.message : String(err);
-      this.error(`Error parsing frontmatter in ${filePath}: ${message}`);
+      throw new Error(`Error parsing frontmatter in ${filePath}: ${message}`, { cause: err });
     }
 
     if (frontmatter) {
@@ -159,14 +164,13 @@ function buildPath(
   visited: Set<string> = new Set(),
 ): string | null {
   if (visited.has(slug)) {
-    this.error(`Circular reference detected for slug: ${slug}`);
+    throw new Error(`Circular reference detected for slug: ${slug}`);
   }
   visited.add(slug);
 
   const fileInfo = fileMap.get(slug);
   if (!fileInfo) {
-    this.error(`File not found for slug: ${slug}`);
-    return null;
+    throw new Error(`File not found for slug: ${slug}`);
   }
 
   const parts: string[] = [];
@@ -230,7 +234,11 @@ function restructureFiles(this: APIv2PageExportCommands, tempFolder: string, fin
   this.debug('Copying files to final structure:');
 
   for (const r of results) {
-    const dest = path.join(finalFolder, r.newPath);
+    const dest = resolvePathWithinRoot(finalFolder, r.newPath);
+    if (!dest) {
+      throw new Error(`Refusing to write outside export directory: ${r.newPath}`);
+    }
+
     fs.mkdirSync(path.dirname(dest), { recursive: true });
     fs.copyFileSync(r.oldPath, dest);
   }
@@ -329,12 +337,35 @@ export async function exportDocs(this: APIv2PageExportCommands): Promise<FullExp
                   parentCounts[parentKey] += 1;
                 }
 
-                const frontmatter = scrub(output);
-                const yamlFront = dumpYAML(frontmatter, { sortKeys: true });
-                const md = `---\n${yamlFront}---\n${body}`;
-
-                fs.writeFileSync(path.join(tempFolder, `${output.slug}.md`), md, { encoding: 'utf-8' });
-                downloads.completed.push(output);
+                const pageSlug = output.slug;
+                if (!pageSlug || !isSafePathSegment(pageSlug)) {
+                  this.warn(`Skipping page "${String(pageSlug)}": slug is invalid.`);
+                  this.debug(
+                    `"${String(pageSlug)}" contains directory separators and is not safe to use within the filesystem.`,
+                  );
+                  downloads.failed.push(String(pageSlug));
+                } else {
+                  const frontmatter = scrub(output);
+                  if (!frontmatter) {
+                    this.warn(`Skipping page "${pageSlug}": category or parent URI in API response is invalid.`);
+                    this.debug(
+                      `"${String(pageSlug)}" is invalid because its category or parent URI contains directory separators and is not safe to use within the filesystem.`,
+                    );
+                    downloads.failed.push(pageSlug);
+                  } else {
+                    const yamlFront = dumpYAML(frontmatter, { sortKeys: true });
+                    const md = `---\n${yamlFront}---\n${body}`;
+                    const tempPath = resolvePathWithinRoot(tempFolder, `${pageSlug}.md`);
+
+                    if (!tempPath) {
+                      this.warn(`Skipping page "${pageSlug}": refused to write outside the temporary export folder.`);
+                      downloads.failed.push(pageSlug);
+                    } else {
+                      fs.writeFileSync(tempPath, md, { encoding: 'utf-8' });
+                      downloads.completed.push(output);
+                    }
+                  }
+                }
               }
             }
           } finally {
diff --git a/src/lib/safePath.ts b/src/lib/safePath.ts
@@ -0,0 +1,51 @@
+import path from 'node:path';
+
+/**
+ * Determine if a URL path segment is safe to use as a filesystem path component, preventing us
+ * from treating a path segment as a potential directory traversal attack.
+ *
+ */
+export function isSafePathSegment(segment: unknown): boolean {
+  if (!segment || typeof segment !== 'string') return false;
+  if (segment === '.' || segment === '..') return false;
+
+  // oxlint-disable-next-line no-control-regex
+  if (/[\0/\\]/.test(segment)) return false;
+
+  if (segment.includes('..')) return false;
+
+  return true;
+}
+
+/**
+ * Decode the last path segment of a URI and validate it as a safe path component.
+ *
+ */
+export function decodeURILastSegment(uri: string): string | null {
+  const encoded = uri.split('/').pop() || '';
+  let decoded: string;
+
+  try {
+    decoded = decodeURIComponent(encoded);
+  } catch {
+    return null;
+  }
+
+  return isSafePathSegment(decoded) ? decoded : null;
+}
+
+/**
+ * Resolve the segments of a supplied path within a root directory and only return the path if it
+ * is contained within the supplied root.
+ *
+ */
+export function resolvePathWithinRoot(root: string, ...segments: string[]): string | null {
+  const resolvedRoot = path.resolve(root);
+  const dest = path.resolve(resolvedRoot, ...segments);
+
+  if (dest === resolvedRoot || dest.startsWith(`${resolvedRoot}${path.sep}`)) {
+    return dest;
+  }
+
+  return null;
+}
diff --git a/test/commands/pages/__snapshots__/export.test.ts.snap b/test/commands/pages/__snapshots__/export.test.ts.snap
@@ -0,0 +1,31 @@
+// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html
+
+exports[`rdme docs upload > should not write outside the export directory when category URI contains path traversal 1`] = `
+"- 📩 Exporting files from ReadMe...
+ ›   Warning: Skipping page "rdme-cat-poc": category or parent URI in API 
+ ›   response is invalid.
+✖ 📩 Exporting files from ReadMe... 1 file(s) failed.
+"
+`;
+
+exports[`rdme docs upload > should not write outside the export directory when slug contains path traversal 1`] = `
+"- 📩 Exporting files from ReadMe...
+ ›   Warning: Skipping page "../../evil": slug is invalid.
+✖ 📩 Exporting files from ReadMe... 1 file(s) failed.
+"
+`;
+
+exports[`rdme reference upload > should not write outside the export directory when category URI contains path traversal 1`] = `
+"- 📩 Exporting files from ReadMe...
+ ›   Warning: Skipping page "rdme-cat-poc": category or parent URI in API 
+ ›   response is invalid.
+✖ 📩 Exporting files from ReadMe... 1 file(s) failed.
+"
+`;
+
+exports[`rdme reference upload > should not write outside the export directory when slug contains path traversal 1`] = `
+"- 📩 Exporting files from ReadMe...
+ ›   Warning: Skipping page "../../evil": slug is invalid.
+✖ 📩 Exporting files from ReadMe... 1 file(s) failed.
+"
+`;
diff --git a/test/commands/pages/export.test.ts b/test/commands/pages/export.test.ts
@@ -1,5 +1,6 @@
 import type { OclifOutput } from '../../helpers/oclif.js';
 
+import { randomUUID } from 'node:crypto';
 import fs from 'node:fs';
 import os from 'node:os';
 import path from 'node:path';
@@ -171,4 +172,85 @@ Child body`),
       fs.rmSync(tmpDir, { recursive: true, force: true });
     }
   });
+
+  it('should not write outside the export directory when category URI contains path traversal', async () => {
+    const tmpDir = tempExportDir();
+    const parentDir = path.dirname(tmpDir);
+    const escapeDir = path.join(parentDir, `escape-${randomUUID()}`);
+
+    try {
+      const evilCategory = '%2e%2e%2fescape';
+      const mock = getAPIv2Mock({ authorization })
+        .get(`/branches/stable/categories/${route}`)
+        .reply(200, { data: [{ title: evilCategory }] })
+        .get(`/branches/stable/categories/${route}/${encodeURIComponent(evilCategory)}/pages`)
+        .reply(200, { data: [{ slug: 'rdme-cat-poc' }] })
+        .get(`/branches/stable/${route}/rdme-cat-poc`)
+        .reply(200, {
+          data: {
+            slug: 'rdme-cat-poc',
+            title: 'PoC',
+            type: 'basic',
+            content: { body: 'TRAVERSAL POC FILE.' },
+            category: { uri: `/branches/stable/categories/${route}/${evilCategory}` },
+          },
+        });
+
+      const output = await run([tmpDir, '--key', key]);
+
+      expect(fs.writeFileSync).not.toHaveBeenCalled();
+      expect(fs.copyFileSync).not.toHaveBeenCalled();
+      expect(fs.existsSync(escapeDir)).toBe(false);
+
+      expect(output.stderr).toMatchSnapshot();
+      expect(output.result).toMatchObject({ failed: ['rdme-cat-poc'] });
+
+      mock.done();
+    } finally {
+      if (fs.existsSync(escapeDir)) {
+        fs.rmSync(escapeDir, { recursive: true, force: true });
+      }
+      fs.rmSync(tmpDir, { recursive: true, force: true });
+    }
+  });
+
+  it('should not write outside the export directory when slug contains path traversal', async () => {
+    const tmpDir = tempExportDir();
+    const parentDir = path.dirname(tmpDir);
+    const evilDir = path.join(parentDir, `evil-${randomUUID()}`);
+
+    try {
+      const mock = getAPIv2Mock({ authorization })
+        .get(`/branches/stable/categories/${route}`)
+        .reply(200, { data: [{ title: 'Main' }] })
+        .get(`/branches/stable/categories/${route}/Main/pages`)
+        .reply(200, { data: [{ slug: '../../evil' }] })
+        .get(`/branches/stable/${route}/${encodeURIComponent('../../evil')}`)
+        .reply(200, {
+          data: {
+            slug: '../../evil',
+            title: 'Evil',
+            type: 'basic',
+            content: { body: 'malicious body' },
+            category: { uri: `https://api.readme.com/v2/branches/stable/categories/${route}/main` },
+          },
+        });
+
+      const output = await run([tmpDir, '--key', key]);
+
+      expect(fs.writeFileSync).not.toHaveBeenCalled();
+      expect(fs.copyFileSync).not.toHaveBeenCalled();
+      expect(fs.existsSync(evilDir)).toBe(false);
+
+      expect(output.stderr).toMatchSnapshot();
+      expect(output.result).toMatchObject({ failed: ['../../evil'] });
+
+      mock.done();
+    } finally {
+      if (fs.existsSync(evilDir)) {
+        fs.rmSync(evilDir, { recursive: true, force: true });
+      }
+      fs.rmSync(tmpDir, { recursive: true, force: true });
+    }
+  });
 });
diff --git a/test/lib/safePath.test.ts b/test/lib/safePath.test.ts
@@ -0,0 +1,49 @@
+import path from 'node:path';
+
+import { describe, expect, it } from 'vitest';
+
+import { decodeURILastSegment, isSafePathSegment, resolvePathWithinRoot } from '../../src/lib/safePath.js';
+
+describe('#isSafePathSegment', () => {
+  it.each([
+    ['main', true],
+    ['my-category', true],
+    ['intro', true],
+    ['', false],
+    ['.', false],
+    ['..', false],
+    ['../escape', false],
+    ['..\\escape', false],
+    ['foo/../bar', false],
+    ['foo/bar', false],
+    ['foo\\bar', false],
+  ])('isSafePathSegment(%j) -> %s', (segment, expected) => {
+    expect(isSafePathSegment(segment)).toBe(expected);
+  });
+});
+
+describe('#decodeURILastSegment', () => {
+  it('decodes a normal category segment', () => {
+    expect(decodeURILastSegment('/branches/1.0/categories/guides/main')).toBe('main');
+  });
+
+  it('rejects encoded path traversal', () => {
+    expect(decodeURILastSegment('/branches/1.0/categories/guides/%2e%2e%2fescape')).toBeNull();
+  });
+});
+
+describe('#resolvePathWithinRoot', () => {
+  const root = path.resolve('/tmp/export');
+
+  it('resolves paths inside the root', () => {
+    expect(resolvePathWithinRoot(root, 'main', 'intro.md')).toBe(path.resolve(root, 'main', 'intro.md'));
+  });
+
+  it('returns null when traversal escapes the root', () => {
+    expect(resolvePathWithinRoot(root, '../outside', 'intro.md')).toBeNull();
+  });
+
+  it('allows the root directory itself', () => {
+    expect(resolvePathWithinRoot(root)).toBe(root);
+  });
+});
