From 6feec59e2b035919c264029cabfd50fe26973577 Mon Sep 17 00:00:00 2001 From: Josh White Date: Fri, 8 May 2026 18:34:11 -0400 Subject: [PATCH] Validate extensions.targeting assets value in include-assets-step --- .../build/steps/include-assets-step.test.ts | 101 ++++++++++++++++++ .../build/steps/include-assets-step.ts | 26 ++++- 2 files changed, 126 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts index e9decd6b02d..fe0782fe354 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.test.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.test.ts @@ -551,6 +551,107 @@ describe('executeIncludeAssetsStep', () => { }) }) + describe(`extension_points[].assets value guard`, () => { + const buildStep = (): LifecycleStep => ({ + id: 'copy-assets', + name: 'Copy Assets', + type: 'include_assets', + config: { + inclusions: [{type: 'configKey', key: 'extension_points[].assets'}], + }, + }) + + const buildContext = (assetsValue: unknown) => + ({ + ...mockContext, + extension: { + ...mockExtension, + configuration: {extension_points: [{target: 'admin.app.home.render', assets: assetsValue}]}, + } as unknown as ExtensionInstance, + }) as BuildContext + + test('throws when value is an empty string', async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext(''))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' can't be empty.`, + ) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + expect(fs.glob).not.toHaveBeenCalled() + }) + + test('throws when value is whitespace-only', async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext(' '))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' can't be empty.`, + ) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + }) + + test(`throws when value is '.'`, async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext('.'))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' is not a valid path: '.'`, + ) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + }) + + test(`throws when value is './'`, async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext('./'))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' is not a valid path: './'`, + ) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + }) + + test(`throws when value is './' with surrounding whitespace`, async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext(' ./ '))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' is not a valid path: ' ./ '`, + ) + }) + + test(`throws when one of multiple extension_points has '.'`, async () => { + const context = { + ...mockContext, + extension: { + ...mockExtension, + configuration: { + extension_points: [ + {target: 'admin.app.home.render', assets: './public'}, + {target: 'admin.app.other.render', assets: '.'}, + ], + }, + } as unknown as ExtensionInstance, + } as BuildContext + await expect(executeIncludeAssetsStep(buildStep(), context)).rejects.toThrow( + `'assets' for target 'admin.app.other.render' is not a valid path: '.'`, + ) + }) + + test(`throws when value is '../foo' (escapes via single parent)`, async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext('../foo'))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' is not a valid path: '../foo'`, + ) + expect(fs.copyDirectoryContents).not.toHaveBeenCalled() + }) + + test(`throws when value is '../../../foo' (escapes via deep parent chain)`, async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext('../../../foo'))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' is not a valid path: '../../../foo'`, + ) + }) + + test(`throws when value escapes via interior '..' segments ('public/../../bad')`, async () => { + await expect(executeIncludeAssetsStep(buildStep(), buildContext('public/../../bad'))).rejects.toThrow( + `'assets' for target 'admin.app.home.render' is not a valid path: 'public/../../bad'`, + ) + }) + + test(`does not throw when interior '..' resolves back inside ('public/../public')`, async () => { + vi.mocked(fs.fileExists).mockResolvedValue(true) + vi.mocked(fs.isDirectory).mockResolvedValue(true) + vi.mocked(fs.copyDirectoryContents).mockResolvedValue() + vi.mocked(fs.glob).mockResolvedValue(['file.json']) + + await expect(executeIncludeAssetsStep(buildStep(), buildContext('public/../public'))).resolves.toBeDefined() + }) + }) + describe('pattern entries', () => { test('copies files matching include patterns', async () => { // Given diff --git a/packages/app/src/cli/services/build/steps/include-assets-step.ts b/packages/app/src/cli/services/build/steps/include-assets-step.ts index 1aba68d5d5d..05230ef7f50 100644 --- a/packages/app/src/cli/services/build/steps/include-assets-step.ts +++ b/packages/app/src/cli/services/build/steps/include-assets-step.ts @@ -2,7 +2,7 @@ import {generateManifestFile, resolveOutputDir} from './include-assets/generate- import {copyByPattern} from './include-assets/copy-by-pattern.js' import {copySourceEntry} from './include-assets/copy-source-entry.js' import {copyConfigKeyEntry} from './include-assets/copy-config-key-entry.js' -import {joinPath, sanitizeRelativePath} from '@shopify/cli-kit/node/path' +import {isSubpath, joinPath, resolvePath, sanitizeRelativePath} from '@shopify/cli-kit/node/path' import {z} from 'zod' import type {LifecycleStep, BuildContext} from '../client-steps.js' @@ -138,6 +138,11 @@ export async function executeIncludeAssetsStep( let configKeyCount = 0 for (const entry of config.inclusions) { if (entry.type !== 'configKey') continue + + if (entry.key === 'extension_points[].assets') { + validateAssetsValue(extension.configuration, extension.directory) + } + const warn = (msg: string) => options.stdout.write(msg) const rawDest = entry.destination ? sanitizeRelativePath(entry.destination, warn) : undefined const sanitizedDest = rawDest === '' ? undefined : rawDest @@ -207,3 +212,22 @@ export async function executeIncludeAssetsStep( return {filesCopied: counts.reduce((sum, count) => sum + (count ?? 0), 0)} } + +function validateAssetsValue(configuration: {[key: string]: unknown}, baseDir: string) { + const extensionPoints = configuration.extension_points + if (!Array.isArray(extensionPoints)) return + for (const ep of extensionPoints) { + if (typeof ep !== 'object' || ep === null) continue + const {target, assets} = ep as {target?: unknown; assets?: unknown} + if (typeof assets !== 'string') continue + const targetCtx = typeof target === 'string' ? ` for target '${target}'` : '' + const trimmed = assets.trim() + if (trimmed === '') { + throw new Error(`'assets'${targetCtx} can't be empty.`) + } + const resolved = resolvePath(baseDir, trimmed) + if (resolved === baseDir || !isSubpath(baseDir, resolved)) { + throw new Error(`'assets'${targetCtx} is not a valid path: '${assets}'`) + } + } +}