fix: url poisoning via user-controlled input (take 4) (#700)

rofe · web-flow · commit 31a3d9a690f4 · 2025-12-22T21:03:01.000+01:00
diff --git a/blocks/browse/da-browse/da-browse.js b/blocks/browse/da-browse/da-browse.js
@@ -1,7 +1,7 @@
 import { LitElement, html, nothing } from 'da-lit';
 import { DA_ORIGIN } from '../../shared/constants.js';
 import { daFetch, getFirstSheet } from '../../shared/utils.js';
-import { getNx } from '../../../scripts/utils.js';
+import { getNx, sanitizePathParts } from '../../../scripts/utils.js';
 
 // Components
 import '../da-breadcrumbs/da-breadcrumbs.js';
@@ -52,7 +52,7 @@ export default class DaBrowse extends LitElement {
     if ((e.metaKey || e.ctrlKey) && e.altKey && e.code === 'KeyT') {
       e.preventDefault();
       const { fullpath } = this.details;
-      const [, ...split] = fullpath.split('/');
+      const [...split] = sanitizePathParts(fullpath);
       if (split.length < 2) return;
 
       if (split[2] === '.trash') {
diff --git a/blocks/browse/da-list/da-list.js b/blocks/browse/da-list/da-list.js
@@ -1,6 +1,6 @@
 import { LitElement, html, repeat, nothing } from 'da-lit';
 import { DA_ORIGIN } from '../../shared/constants.js';
-import { getNx } from '../../../scripts/utils.js';
+import { getNx, sanitizePathParts } from '../../../scripts/utils.js';
 import { daFetch, aemAdmin } from '../../shared/utils.js';
 
 import '../da-list-item/da-list-item.js';
@@ -307,7 +307,7 @@ export default class DaList extends LitElement {
     this._itemsRemaining = this._selectedItems.length;
 
     const callback = async (item) => {
-      const [, org, site, ...rest] = item.path.split('/');
+      const [org, site, ...rest] = sanitizePathParts(item.path);
 
       // If already in trash or not in a site, its a direct delete
       const directDelete = item.path.includes('/.trash/') || rest.length === 0;
diff --git a/blocks/browse/da-list/helpers/utils.js b/blocks/browse/da-list/helpers/utils.js
@@ -1,4 +1,5 @@
 import { SUPPORTED_FILES, DA_ORIGIN } from '../../../shared/constants.js';
+import { sanitizePath, sanitizePathParts } from '../../../../scripts/utils.js';
 import { daFetch } from '../../../shared/utils.js';
 
 const MAX_DEPTH = 1000;
@@ -81,12 +82,6 @@ export async function getFullEntryList(entries) {
   return files.filter((file) => file);
 }
 
-export function sanitizePath(path) {
-  const pathArray = path.split('/');
-  const sanitizedArray = pathArray.map((element) => element.replaceAll(/[^a-zA-Z0-9.]/g, '-').toLowerCase());
-  return [...sanitizedArray].join('/');
-}
-
 export async function handleUpload(list, fullpath, file) {
   const { data, path } = file;
   const formData = new FormData();
@@ -122,8 +117,7 @@ export async function handleUpload(list, fullpath, file) {
 export function items2Clipboard(items) {
   const aemUrls = items.reduce((acc, item) => {
     if (item.ext) {
-      const path = item.path.replace('.html', '');
-      const [org, repo, ...pathParts] = path.substring(1).split('/');
+      const [org, repo, ...pathParts] = sanitizePathParts(item.path.replace('.html', ''));
       const pageName = pathParts.pop();
       pathParts.push(pageName === 'index' ? '' : pageName);
 
diff --git a/blocks/browse/da-sites/da-sites.js b/blocks/browse/da-sites/da-sites.js
@@ -1,5 +1,6 @@
 import { LitElement, html, nothing } from 'da-lit';
 import getSheet from '../../shared/sheet.js';
+import { sanitizeName } from '../../../scripts/utils.js';
 
 const sheet = await getSheet('/blocks/browse/da-sites/da-sites.css');
 
@@ -94,7 +95,7 @@ export default class DaSites extends LitElement {
       // eslint-disable-next-line no-unused-vars
       const [_, repo, org] = helixString.split('--');
       if (!repo || !org) return null;
-      return `#/${org}/${repo}`;
+      return `#/${sanitizeName(org, false)}/${sanitizeName(repo, false)}`;
     } catch (_) {
       return null;
     }
diff --git a/blocks/edit/da-library/da-library.js b/blocks/edit/da-library/da-library.js
@@ -9,7 +9,7 @@ import {
   ref,
   nothing,
 } from 'da-lit';
-import { getNx } from '../../../scripts/utils.js';
+import { getNx, sanitizePathParts } from '../../../scripts/utils.js';
 import { getBlocks, getBlockVariants } from './helpers/index.js';
 import getSheet from '../../shared/sheet.js';
 import inlinesvg from '../../shared/inlinesvg.js';
@@ -291,7 +291,7 @@ class DaLibrary extends LitElement {
 
   getParts() {
     const view = 'edit';
-    const [org, repo, ...path] = window.location.hash.replace('#/', '').split('/');
+    const [org, repo, ...path] = sanitizePathParts(window.location.hash.substring(1));
     return { view, org, repo, ref: 'main', path: `/${path.join('/')}` };
   }
 
diff --git a/blocks/edit/da-library/helpers/helpers.js b/blocks/edit/da-library/helpers/helpers.js
@@ -5,7 +5,7 @@ import getPathDetails from '../../../shared/pathDetails.js';
 import { daFetch, getFirstSheet } from '../../../shared/utils.js';
 import { getConfKey, openAssets } from '../../da-assets/da-assets.js';
 import { fetchKeyAutocompleteData } from '../../prose/plugins/slashMenu/keyAutocomplete.js';
-import { sanitiseRef } from '../../../../scripts/utils.js';
+import { sanitizeName } from '../../../../scripts/utils.js';
 
 const DA_ORIGIN = getDaAdmin();
 const REPLACE_CONTENT = '<content>';
@@ -18,7 +18,7 @@ const DA_PLUGINS = [
   'placeholders',
 ];
 
-const ref = sanitiseRef(new URLSearchParams(window.location.search).get('ref')) || 'main';
+const ref = sanitizeName(new URLSearchParams(window.location.search).get('ref'), false) || 'main';
 
 export function parseDom(dom) {
   const { schema } = window.view.state;
diff --git a/blocks/edit/utils/helpers.js b/blocks/edit/utils/helpers.js
@@ -1,4 +1,5 @@
 import { AEM_ORIGIN, DA_ORIGIN } from '../../shared/constants.js';
+import { sanitizePathParts } from '../../../../scripts/utils.js';
 import prose2aem from '../../shared/prose2aem.js';
 import { daFetch } from '../../shared/utils.js';
 
@@ -154,7 +155,7 @@ function parseAemError(xError) {
 }
 
 export async function getCdnConfig(path) {
-  const [org, site] = path.slice(1).toLowerCase().split('/');
+  const [org, site] = sanitizePathParts(path);
   const resp = await daFetch(`${AEM_ORIGIN}/config/${org}/sites/${site}.json`);
   if (!resp.ok) {
     // eslint-disable-next-line no-console
diff --git a/blocks/shared/pathDetails.js b/blocks/shared/pathDetails.js
@@ -1,4 +1,5 @@
 import { CON_ORIGIN, DA_ORIGIN } from './constants.js';
+import { sanitizePathParts } from '../../scripts/utils.js';
 
 let currpath;
 let currhash;
@@ -119,7 +120,7 @@ export default function getPathDetails(loc) {
   if (!fullpath || fullpath.startsWith('old_hash') || fullpath.startsWith('access_token')) return null;
 
   // Split everything up so it can be later used for both DA & AEM
-  const pathParts = fullpath.slice(1).toLowerCase().split('/');
+  const pathParts = sanitizePathParts(fullpath);
 
   // Redirect JSON files from edit view to sheet view
   if (editor === 'edit' && fullpath.endsWith('.json')) {
diff --git a/blocks/start/index.js b/blocks/start/index.js
@@ -1,4 +1,4 @@
-import { getNx } from '../../scripts/utils.js';
+import { getNx, sanitizePathParts } from '../../scripts/utils.js';
 import { DA_ORIGIN } from '../shared/constants.js';
 import { daFetch } from '../shared/utils.js';
 
@@ -32,7 +32,7 @@ async function getBlob(path) {
 
 async function bulkAemAdmin(org, site, files) {
   const paths = files.map((file) => {
-    const [, , ...parts] = file.path.slice(1).split('/');
+    const [, , ...parts] = sanitizePathParts(file.path);
     return `/${parts.join('/')}`.replace('.html', '');
   });
 
diff --git a/blocks/start/start.js b/blocks/start/start.js
@@ -3,6 +3,7 @@ import { getDaAdmin } from '../shared/constants.js';
 import getSheet from '../shared/sheet.js';
 import { daFetch } from '../shared/utils.js';
 import { copyConfig, copyContent, previewContent } from './index.js';
+import { sanitizePathParts } from '../../scripts/utils.js';
 
 const sheet = await getSheet('/blocks/start/start.css');
 
@@ -124,7 +125,7 @@ class DaStart extends LitElement {
     try {
       const { origin, pathname } = new URL(e.target.value);
       if (origin !== 'https://github.com') throw Error('Not github');
-      const [, org, site] = pathname.toLowerCase().trim().split('/');
+      const [org, site] = sanitizePathParts(pathname);
       if (!(org && site)) throw Error('No org or site');
       this.org = org;
       this.site = site;
diff --git a/scripts/utils.js b/scripts/utils.js
@@ -11,21 +11,36 @@
  */
 export const codeBase = `${import.meta.url.replace('/scripts/utils.js', '')}`;
 
-export function sanitiseRef(ref) {
-  if (!ref) return null;
+export function sanitizeName(name, preserveDots = true) {
+  if (!name) return null;
 
-  return ref.toLowerCase()
+  if (preserveDots && name.indexOf('.') !== -1) {
+    return name
+      .split('.')
+      .map((part) => sanitizeName(part))
+      .join('.');
+  }
+
+  return name
+    .toLowerCase()
     .normalize('NFD')
     .replace(/[\u0300-\u036f]/g, '')
-    .replace(/[^a-z0-9]+/g, '-')
-    .replace(/^-|-$/g, '');
+    .replace(/[^a-z0-9_]+/g, '-')
+    .replace(/-$/g, '');
 }
 
-export function sanitizeName() {}
-
-export function sanitizePathParts() {}
+export function sanitizePathParts(path) {
+  return path.slice(1)
+    .toLowerCase()
+    .split('/')
+    .map((name) => (name ? sanitizeName(name) : ''))
+    // remove path traversal parts, and empty strings unless at the end
+    .filter((name, i, parts) => !/^[.]{1,2}$/.test(name) && (name !== '' || i === parts.length - 1));
+}
 
-export function sanitizePath() {}
+export function sanitizePath(path) {
+  return `/${sanitizePathParts(path).join('/')}`;
+}
 
 export const [setNx, getNx] = (() => {
   let nx;
@@ -34,7 +49,7 @@ export const [setNx, getNx] = (() => {
       nx = (() => {
         const { hostname, search } = location || window.location;
         if (!(hostname.includes('.hlx.') || hostname.includes('.aem.') || hostname.includes('local'))) return nxBase;
-        const branch = sanitiseRef(new URLSearchParams(search).get('nx')) || 'main';
+        const branch = sanitizeName(new URLSearchParams(search).get('nx'), false) || 'main';
         if (branch === 'local') return 'http://localhost:6456/nx';
         return `https://${branch}--da-nx--adobe.aem.live/nx`;
       })();
diff --git a/test/e2e/tests/authenticated/collab.spec.js b/test/e2e/tests/authenticated/collab.spec.js
@@ -27,6 +27,7 @@ test('Collab cursors in multiple editors', async ({ browser, page }, workerInfo)
 
   await expect(page.locator('div.ProseMirror')).toBeVisible();
   await expect(page.locator('div.ProseMirror')).toHaveAttribute('contenteditable', 'true');
+  await page.waitForTimeout(1000);
   await page.locator('div.ProseMirror').fill('Entered by user 1');
 
   // Right now there should not be any collab indicators yet
diff --git a/test/e2e/tests/edit.spec.js b/test/e2e/tests/edit.spec.js
@@ -106,13 +106,14 @@ test('Change document by switching anchors', async ({ page }, workerInfo) => {
   await expect(page.locator('div.ProseMirror')).toBeVisible();
   await page.waitForTimeout(3000);
   await expect(page.locator('div.ProseMirror')).toHaveAttribute('contenteditable', 'true');
-
+  await page.waitForTimeout(1000);
   await page.locator('div.ProseMirror').fill('page B');
   await page.waitForTimeout(3000);
 
   await page.goto(urlA);
   await page.waitForTimeout(3000);
   await expect(page.locator('div.ProseMirror')).toBeVisible();
+  await page.waitForTimeout(1000);
   await expect(page.locator('div.ProseMirror')).toContainText('mytable');
   await page.waitForTimeout(2000);
   await expect(page.locator('div.ProseMirror')).toContainText('k 2');
@@ -121,6 +122,7 @@ test('Change document by switching anchors', async ({ page }, workerInfo) => {
   await page.goto(urlB);
   await page.waitForTimeout(3000);
   await expect(page.locator('div.ProseMirror')).toBeVisible();
+  await page.waitForTimeout(1000);
   await expect(page.locator('div.ProseMirror')).toContainText('page B');
 });
 
@@ -131,6 +133,7 @@ test('Add code mark', async ({ page }, workerInfo) => {
   await expect(page.locator('div.ProseMirror')).toBeVisible();
   await page.waitForTimeout(3000);
   await expect(page.locator('div.ProseMirror')).toHaveAttribute('contenteditable', 'true');
+  await page.waitForTimeout(1000);
   await page.locator('div.ProseMirror').fill('This is a line that will contain a code mark.');
 
   // Forward
diff --git a/test/unit/blocks/browse/helpers/helpers.test.js b/test/unit/blocks/browse/helpers/helpers.test.js
@@ -1,6 +1,6 @@
 import { expect } from '@esm-bundle/chai';
 import { stub } from 'sinon';
-import { getFullEntryList, handleUpload, sanitizePath } from '../../../../../blocks/browse/da-list/helpers/utils.js';
+import { getFullEntryList, handleUpload } from '../../../../../blocks/browse/da-list/helpers/utils.js';
 
 const goodEntry = {
   isDirectory: false,
@@ -81,10 +81,4 @@ describe('Upload and format', () => {
     const item = await handleUpload(list, fullpath, packagedFile);
     expect(item).to.exist;
   });
-
-  it('Returns sanitize file path', async () => {
-    const path = '/new folder/geo_metrixx.jpg';
-    const item = sanitizePath(path);
-    expect(item).to.equal('/new-folder/geo-metrixx.jpg');
-  });
 });
diff --git a/test/unit/scripts/utils.test.js b/test/unit/scripts/utils.test.js
