Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ export async function executeIncludeAssetsStep(
let configKeyCount = 0
for (const entry of config.inclusions) {
if (entry.type !== 'configKey') continue

const warn = (msg: string) => options.stdout.write(msg)
const rawDest = entry.destination ? sanitizeRelativePath(entry.destination, warn) : undefined
const sanitizedDest = rawDest === '' ? undefined : rawDest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -316,4 +316,132 @@ describe('copyConfigKeyEntry', () => {
await expect(fileExists(joinPath(outDir, 'tools.json'))).resolves.toBe(true)
})
})

describe('value guard', () => {
test('throws when value is an empty string', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: ''})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' can't be empty.`,
)
})
})

test('throws when value is whitespace-only', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: ' '})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' can't be empty.`,
)
})
})

test(`throws when value is '.'`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: '.'})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' is not a valid path: '.'`,
)
})
})

test(`throws when value is './'`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: './'})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' is not a valid path: './'`,
)
})
})

test(`throws when value is './' with surrounding whitespace`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: ' ./ '})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' is not a valid path: ' ./ '`,
)
})
})

test(`throws when one path in an array is invalid`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({roots: ['./public', '.']})
await expect(copyConfigKeyEntry({key: 'roots', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'roots' is not a valid path: '.'`,
)
})
})

test(`throws when value is '../foo' (escapes via single parent)`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: '../foo'})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' is not a valid path: '../foo'`,
)
})
})

test(`throws when value is '../../../foo' (escapes via deep parent chain)`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: '../../../foo'})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' is not a valid path: '../../../foo'`,
)
})
})

test(`throws when value escapes via interior '..' segments`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({assets: 'public/../../bad'})
await expect(copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})).rejects.toThrow(
`'assets' is not a valid path: 'public/../../bad'`,
)
})
})

test(`does not throw when interior '..' resolves back inside`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const pubDir = joinPath(tmpDir, 'public')
await mkdir(pubDir)
await writeFile(joinPath(pubDir, 'index.html'), '<html/>')
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)

const context = makeContext({assets: 'public/../public'})
const result = await copyConfigKeyEntry({key: 'assets', baseDir: tmpDir, outputDir: outDir, context})

expect(result.filesCopied).toBe(1)
await expect(fileExists(joinPath(outDir, 'index.html'))).resolves.toBe(true)
})
})

test(`uses leaf segment of dotted configKey for the error message`, async () => {
await inTemporaryDirectory(async (tmpDir) => {
const outDir = joinPath(tmpDir, 'out')
await mkdir(outDir)
const context = makeContext({extension_points: [{assets: ''}]})
await expect(
copyConfigKeyEntry({key: 'extension_points[].assets', baseDir: tmpDir, outputDir: outDir, context}),
).rejects.toThrow(`'assets' can't be empty.`)
})
})
})
})
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {joinPath, basename, relativePath, extname} from '@shopify/cli-kit/node/path'
import {joinPath, basename, relativePath, extname, resolvePath, isSubpath} from '@shopify/cli-kit/node/path'
import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs'
import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output'
import {AbortError} from '@shopify/cli-kit/node/error'
Expand Down Expand Up @@ -59,6 +59,22 @@
// should only be copied once; the pathMap entry is reused for all references.
const uniquePaths = [...new Set(paths)]

// Validate up-front so a bad value fails fast before any copying happens.
// Use the leaf segment of `key` as the user-facing field name (e.g.
// `extension_points[].assets` → `assets`) so error messages match the
// developer's TOML rather than internal config paths.
const fieldName = key.split('.').pop()?.replace(/\[\]$/, '') ?? key
for (const sourcePath of uniquePaths) {
const trimmed = sourcePath.trim()
if (trimmed === '') {
throw new Error(`'${fieldName}' can't be empty.`)
}
const resolved = resolvePath(baseDir, trimmed)
if (resolved === baseDir || !isSubpath(baseDir, resolved)) {
throw new Error(`'${fieldName}' is not a valid path: '${sourcePath}'`)

Check failure on line 74 in packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts

View workflow job for this annotation

GitHub Actions / Unit tests with Node 26.1.0 in macos-latest

src/cli/services/dev/extension/server/middlewares.test.ts > getExtensionAssetMiddleware() > serves a ../tools.json source after include_assets flattens it into the output directory

Error: 'tools' is not a valid path: '../tools.json' ❯ Module.copyConfigKeyEntry src/cli/services/build/steps/include-assets/copy-config-key-entry.ts:74:13 ❯ src/cli/services/dev/extension/server/middlewares.test.ts:403:33 ❯ Module.inTemporaryDirectory ../cli-kit/src/public/node/fs.ts:81:12 ❯ src/cli/services/dev/extension/server/middlewares.test.ts:378:5

Check failure on line 74 in packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts

View workflow job for this annotation

GitHub Actions / Unit tests with Node 26.1.0 in ubuntu-latest

src/cli/services/dev/extension/server/middlewares.test.ts > getExtensionAssetMiddleware() > serves a ../tools.json source after include_assets flattens it into the output directory

Error: 'tools' is not a valid path: '../tools.json' ❯ Module.copyConfigKeyEntry src/cli/services/build/steps/include-assets/copy-config-key-entry.ts:74:13 ❯ src/cli/services/dev/extension/server/middlewares.test.ts:403:33 ❯ Module.inTemporaryDirectory ../cli-kit/src/public/node/fs.ts:81:12 ❯ src/cli/services/dev/extension/server/middlewares.test.ts:378:5
}
Comment on lines +68 to +75
}

// Process sequentially to avoid filesystem race conditions on shared output paths.
const pathMap = new Map<string, string | string[]>()
let filesCopied = 0
Expand Down
Loading