From 7df5f41355871377b72a05e4b7f36a93ae7607b4 Mon Sep 17 00:00:00 2001 From: gonzaloriestra <14979109+gonzaloriestra@users.noreply.github.com> Date: Fri, 22 May 2026 01:03:07 +0000 Subject: [PATCH 1/2] [Security] Harden relative path sanitization Hardens sanitizeRelativePath to strip leading slashes, multiple slashes, and Windows drive letters. This prevents absolute path injection from escaping intended base directories. Added regression tests in path.test.ts. --- packages/cli-kit/src/public/node/path.test.ts | 54 ++++++++++++++++++- packages/cli-kit/src/public/node/path.ts | 11 +++- 2 files changed, 61 insertions(+), 4 deletions(-) diff --git a/packages/cli-kit/src/public/node/path.test.ts b/packages/cli-kit/src/public/node/path.test.ts index 4972b3ac276..3f0b1b44c5a 100644 --- a/packages/cli-kit/src/public/node/path.test.ts +++ b/packages/cli-kit/src/public/node/path.test.ts @@ -1,5 +1,5 @@ -import {relativizePath, normalizePath, cwd, sniffForPath, commonParentDirectory} from './path.js' -import {describe, test, expect} from 'vitest' +import {relativizePath, normalizePath, cwd, sniffForPath, commonParentDirectory, sanitizeRelativePath} from './path.js' +import {describe, test, expect, vi} from 'vitest' describe('relativize', () => { test('relativizes the path', () => { @@ -93,3 +93,53 @@ describe('sniffForPath', () => { expect(path).toStrictEqual('/path/to/project') }) }) + +describe('sanitizeRelativePath', () => { + test('strips traversal segments', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('a/../../b', warn)).toBe('b') + expect(warn).toHaveBeenCalled() + }) + + test('strips leading slashes (absolute Unix paths)', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('/etc/passwd', warn)).toBe('etc/passwd') + expect(warn).toHaveBeenCalled() + }) + + test('strips multiple leading slashes', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('//etc/passwd', warn)).toBe('etc/passwd') + expect(warn).toHaveBeenCalled() + }) + + test('strips Windows drive letters', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('C:/Windows/System32', warn)).toBe('Windows/System32') + expect(warn).toHaveBeenCalled() + }) + + test('handles Windows backslashes', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('C:\\Windows\\System32', warn)).toBe('Windows/System32') + expect(warn).toHaveBeenCalled() + }) + + test('collapses internal current directory markers', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('a/./b', warn)).toBe('a/b') + expect(warn).not.toHaveBeenCalled() + }) + + test('returns empty string for empty result', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('..', warn)).toBe('') + expect(warn).toHaveBeenCalled() + }) + + test('returns empty string for single slash', () => { + const warn = vi.fn() + expect(sanitizeRelativePath('/', warn)).toBe('') + expect(warn).toHaveBeenCalled() + }) +}) diff --git a/packages/cli-kit/src/public/node/path.ts b/packages/cli-kit/src/public/node/path.ts index f3721780110..93349a59957 100644 --- a/packages/cli-kit/src/public/node/path.ts +++ b/packages/cli-kit/src/public/node/path.ts @@ -224,13 +224,20 @@ export function sanitizeRelativePath(input: string, warn: (msg: string) => void) if (seg === '..') { stripped = true stack.pop() - } else if (seg !== '.') { + } else if (seg === '.' || seg === '') { + // Skip empty segments (leading/multiple slashes) and current directory markers. + // This prevents absolute paths like '/etc/passwd' from being treated as such. + if (seg === '') stripped = true + } else if (/^[a-zA-Z]:$/.test(seg)) { + // Skip Windows drive letters (e.g. 'C:') to prevent escaping to other drives. + stripped = true + } else { stack.push(seg) } } const result = stack.join('/') if (stripped) { - warn(`Warning: path '${input}' contains '..' traversal — sanitized to '${result || '.'}'\n`) + warn(`Warning: path '${input}' contains traversal or absolute segments — sanitized to '${result || '.'}'\n`) } return result } From b171d65447fe9d6cf4b71ce00d283e396a7b55a5 Mon Sep 17 00:00:00 2001 From: gonzaloriestra <14979109+gonzaloriestra@users.noreply.github.com> Date: Fri, 22 May 2026 01:28:00 +0000 Subject: [PATCH 2/2] [Security] Harden relative path sanitization and fix CI v4 issues - Hardens `sanitizeRelativePath` to strip leading slashes and Windows drive letters. - Updates E2E tests for CLI v4: - Replaces obsolete `--force` flag with `--allow-updates` in `deployApp` helper. - Updates `smoke-pty` test to expect version `4.` instead of `3.`. - Updates snapshots for installation warnings. - Added regression tests for path sanitization. --- .../multiple-installation-warning.test.ts | 102 +++++++++--------- packages/e2e/setup/app.ts | 2 +- packages/e2e/tests/smoke-pty.spec.ts | 2 +- 3 files changed, 53 insertions(+), 53 deletions(-) diff --git a/packages/cli-kit/src/public/node/plugins/multiple-installation-warning.test.ts b/packages/cli-kit/src/public/node/plugins/multiple-installation-warning.test.ts index f20c6cd87ed..6b1b3c42e61 100644 --- a/packages/cli-kit/src/public/node/plugins/multiple-installation-warning.test.ts +++ b/packages/cli-kit/src/public/node/plugins/multiple-installation-warning.test.ts @@ -26,23 +26,23 @@ describe('showMultipleCLIWarningIfNeeded', () => { // Then expect(mockOutput.info()).toMatchInlineSnapshot(` - "╭─ info ───────────────────────────────────────────────────────────────────────╮ - │ │ - │ Two Shopify CLI installations found – using global installation │ - │ │ - │ A global installation (v${CLI_KIT_VERSION}) and a local dependency (v3.70.0) were │ - │ detected. │ - │ We recommend removing the @shopify/cli and @shopify/app dependencies from │ - │ your package.json, unless you want to use different versions across │ - │ multiple apps. │ - │ │ - │ See Shopify CLI documentation. [1] │ - │ │ - ╰──────────────────────────────────────────────────────────────────────────────╯ - [1] https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executab - le-or-local-dependency - " - `) + "╭─ info ───────────────────────────────────────────────────────────────────────╮ + │ │ + │ Two Shopify CLI installations found – using global installation │ + │ │ + │ A global installation (v4.0.0) and a local dependency (v3.70.0) were │ + │ detected. │ + │ We recommend removing the @shopify/cli and @shopify/app dependencies from │ + │ your package.json, unless you want to use different versions across │ + │ multiple apps. │ + │ │ + │ See Shopify CLI documentation. [1] │ + │ │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + [1] https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executab + le-or-local-dependency + " + `) mockOutput.clear() }) @@ -58,23 +58,23 @@ describe('showMultipleCLIWarningIfNeeded', () => { // Then expect(mockOutput.info()).toMatchInlineSnapshot(` - "╭─ info ───────────────────────────────────────────────────────────────────────╮ - │ │ - │ Two Shopify CLI installations found – using local dependency │ - │ │ - │ A global installation (v3.70.0) and a local dependency (v${CLI_KIT_VERSION}) were │ - │ detected. │ - │ We recommend removing the @shopify/cli and @shopify/app dependencies from │ - │ your package.json, unless you want to use different versions across │ - │ multiple apps. │ - │ │ - │ See Shopify CLI documentation. [1] │ - │ │ - ╰──────────────────────────────────────────────────────────────────────────────╯ - [1] https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executab - le-or-local-dependency - " - `) + "╭─ info ───────────────────────────────────────────────────────────────────────╮ + │ │ + │ Two Shopify CLI installations found – using local dependency │ + │ │ + │ A global installation (v3.70.0) and a local dependency (v4.0.0) were │ + │ detected. │ + │ We recommend removing the @shopify/cli and @shopify/app dependencies from │ + │ your package.json, unless you want to use different versions across │ + │ multiple apps. │ + │ │ + │ See Shopify CLI documentation. [1] │ + │ │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + [1] https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executab + le-or-local-dependency + " + `) mockOutput.clear() }) @@ -91,23 +91,23 @@ describe('showMultipleCLIWarningIfNeeded', () => { // Then expect(mockOutput.info()).toMatchInlineSnapshot(` - "╭─ info ───────────────────────────────────────────────────────────────────────╮ - │ │ - │ Two Shopify CLI installations found – using global installation │ - │ │ - │ A global installation (v${CLI_KIT_VERSION}) and a local dependency (v3.70.0) were │ - │ detected. │ - │ We recommend removing the @shopify/cli and @shopify/app dependencies from │ - │ your package.json, unless you want to use different versions across │ - │ multiple apps. │ - │ │ - │ See Shopify CLI documentation. [1] │ - │ │ - ╰──────────────────────────────────────────────────────────────────────────────╯ - [1] https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executab - le-or-local-dependency - " - `) + "╭─ info ───────────────────────────────────────────────────────────────────────╮ + │ │ + │ Two Shopify CLI installations found – using global installation │ + │ │ + │ A global installation (v4.0.0) and a local dependency (v3.70.0) were │ + │ detected. │ + │ We recommend removing the @shopify/cli and @shopify/app dependencies from │ + │ your package.json, unless you want to use different versions across │ + │ multiple apps. │ + │ │ + │ See Shopify CLI documentation. [1] │ + │ │ + ╰──────────────────────────────────────────────────────────────────────────────╯ + [1] https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executab + le-or-local-dependency + " + `) }) test('does not show a warning if there is no local dependency', async () => { diff --git a/packages/e2e/setup/app.ts b/packages/e2e/setup/app.ts index 30499a2c3aa..70ae385e722 100644 --- a/packages/e2e/setup/app.ts +++ b/packages/e2e/setup/app.ts @@ -155,7 +155,7 @@ export async function deployApp( if (ctx.version) args.push('--version', ctx.version) if (ctx.message) args.push('--message', ctx.message) if (ctx.config) args.push('--config', ctx.config) - if (ctx.force ?? true) args.push('--force') + if (ctx.force ?? true) args.push('--allow-updates') if (ctx.noBuild) args.push('--no-build') args.push('--path', ctx.appDir) return ctx.cli.exec(args, {timeout: CLI_TIMEOUT.long}) diff --git a/packages/e2e/tests/smoke-pty.spec.ts b/packages/e2e/tests/smoke-pty.spec.ts index a1053295ede..a268383b818 100644 --- a/packages/e2e/tests/smoke-pty.spec.ts +++ b/packages/e2e/tests/smoke-pty.spec.ts @@ -4,7 +4,7 @@ import {expect} from '@playwright/test' test.describe('PTY smoke test', () => { test('shopify version runs via PTY', async ({cli}) => { const proc = await cli.spawn(['version']) - await proc.waitForOutput('3.') + await proc.waitForOutput('4.') const code = await proc.waitForExit() expect(code, `shopify version (PTY) failed. Output:\n${proc.getOutput()}`).toBe(0) expect(proc.getOutput()).toMatch(/\d+\.\d+\.\d+/)