Skip to content

Commit 926db8b

Browse files
isaacroldanisaacroldan
committed
Guard CLI bundle upload against oversized and out-of-app paths
1 parent 0494413 commit 926db8b

14 files changed

Lines changed: 201 additions & 15 deletions

‎.changeset/guard-bundle-upload-size-and-paths.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/app': patch
3+
---
4+
5+
Guard app bundle uploads against oversized bundles and asset paths resolving outside the app directory

‎packages/app/src/cli/services/build/steps/include-assets-step.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ describe('executeIncludeAssetsStep', () => {
2525
options: {
2626
stdout: mockStdout,
2727
stderr: mockStderr,
28-
app: {} as any,
28+
app: {directory: '/test'} as any,
2929
environment: 'production',
3030
},
3131
stepResults: new Map(),

‎packages/app/src/cli/services/build/steps/include-assets-step.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ export async function executeIncludeAssetsStep(
125125
const config = IncludeAssetsConfigSchema.parse(step.config)
126126
const {extension, options} = context
127127
const outputDir = resolveOutputDir(extension.outputPath)
128+
const appDirectory = options.app.directory
128129

129130
const aggregatedPathMap = new Map<string, string | string[]>()
130131
// Track basenames written across all configKey entries in this build. In
@@ -147,6 +148,7 @@ export async function executeIncludeAssetsStep(
147148
baseDir: extension.directory,
148149
outputDir,
149150
context,
151+
appDirectory,
150152
destination: sanitizedDest,
151153
usedBasenames,
152154
preserveFilePaths: entry.preserveFilePaths,
@@ -172,6 +174,8 @@ export async function executeIncludeAssetsStep(
172174
outputDir: destinationDir,
173175
patterns: entry.include,
174176
ignore: entry.ignore ?? [],
177+
appDirectory,
178+
sourceDirConfigValue: entry.baseDir ?? '.',
175179
},
176180
options,
177181
)
@@ -189,6 +193,7 @@ export async function executeIncludeAssetsStep(
189193
destination: sanitizedDest,
190194
baseDir: extension.directory,
191195
outputDir,
196+
appDirectory,
192197
},
193198
options,
194199
)

‎packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import {assertPathWithinAppDir} from './assert-path-within-app.js'
2+
import {AbortError} from '@shopify/cli-kit/node/error'
3+
import {describe, expect, test} from 'vitest'
4+
5+
describe('assertPathWithinAppDir', () => {
6+
test('allows a path inside the app directory', () => {
7+
expect(() =>
8+
assertPathWithinAppDir('/app/extensions/ext-a/icon.png', '/app', 'extensions/ext-a/icon.png'),
9+
).not.toThrow()
10+
})
11+
12+
test('allows the app directory itself', () => {
13+
expect(() => assertPathWithinAppDir('/app', '/app', '.')).not.toThrow()
14+
})
15+
16+
test('rejects a relative path that escapes the app directory via ..', () => {
17+
expect(() => assertPathWithinAppDir('/other/secret.env', '/app', '../other/secret.env')).toThrow(AbortError)
18+
expect(() => assertPathWithinAppDir('/other/secret.env', '/app', '../other/secret.env')).toThrow(
19+
/resolves outside the app directory/,
20+
)
21+
})
22+
23+
test('rejects an absolute path that points outside the app directory', () => {
24+
expect(() => assertPathWithinAppDir('/Users/me', '/app', '/Users/me')).toThrow(AbortError)
25+
})
26+
27+
test('includes the original config value in the error message for debuggability', () => {
28+
expect(() => assertPathWithinAppDir('/other', '/app', '~/anywhere')).toThrow(/Asset path '~\/anywhere'/)
29+
})
30+
})

‎packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import {relativePath, isAbsolutePath} from '@shopify/cli-kit/node/path'
2+
import {AbortError} from '@shopify/cli-kit/node/error'
3+
4+
/**
5+
* Throws if `resolvedPath` is not inside `appDirectory`.
6+
*
7+
* Guards against accidental misuse where an extension config points an asset
8+
* source outside the app folder (e.g. `source = "../../"` or an absolute home
9+
* directory path). Adversarial bypass via symlinks is out of scope — server-side
10+
* size enforcement is the real boundary; this check is a fast-fail for DX.
11+
*
12+
* `configValue` is the raw value from configuration used in the error message
13+
* so the user can locate the offending entry.
14+
*/
15+
export function assertPathWithinAppDir(resolvedPath: string, appDirectory: string, configValue: string): void {
16+
const relative = relativePath(appDirectory, resolvedPath)
17+
if (relative.startsWith('..') || isAbsolutePath(relative)) {
18+
throw new AbortError(
19+
`Asset path '${configValue}' resolves outside the app directory.`,
20+
`Asset sources must be inside the app folder. Resolved to: ${resolvedPath}`,
21+
)
22+
}
23+
}

‎packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ describe('copyByPattern', () => {
2424
outputDir: '/out',
2525
patterns: ['**/*.ts', '**/*.tsx'],
2626
ignore: [],
27+
appDirectory: '/src',
28+
sourceDirConfigValue: '.',
2729
},
2830
{stdout: mockStdout},
2931
)
@@ -47,6 +49,8 @@ describe('copyByPattern', () => {
4749
outputDir: '/out',
4850
patterns: ['**/*.png'],
4951
ignore: [],
52+
appDirectory: '/src',
53+
sourceDirConfigValue: '.',
5054
},
5155
{stdout: mockStdout},
5256
)
@@ -72,6 +76,8 @@ describe('copyByPattern', () => {
7276
outputDir: '/out/sub',
7377
patterns: ['**/*'],
7478
ignore: [],
79+
appDirectory: '/out',
80+
sourceDirConfigValue: 'sub',
7581
},
7682
{stdout: mockStdout},
7783
)
@@ -96,6 +102,8 @@ describe('copyByPattern', () => {
96102
outputDir: '/out',
97103
patterns: ['*.png'],
98104
ignore: [],
105+
appDirectory: '/out',
106+
sourceDirConfigValue: '.',
99107
},
100108
{stdout: mockStdout},
101109
)
@@ -120,6 +128,8 @@ describe('copyByPattern', () => {
120128
outputDir: '/out/dist',
121129
patterns: ['*.js'],
122130
ignore: [],
131+
appDirectory: '/src',
132+
sourceDirConfigValue: '.',
123133
},
124134
{stdout: mockStdout},
125135
)
@@ -139,6 +149,8 @@ describe('copyByPattern', () => {
139149
outputDir: '/out',
140150
patterns: ['**/*'],
141151
ignore: ['**/*.test.ts', 'node_modules/**'],
152+
appDirectory: '/src',
153+
sourceDirConfigValue: '.',
142154
},
143155
{stdout: mockStdout},
144156
)

‎packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import {assertPathWithinAppDir} from './assert-path-within-app.js'
12
import {joinPath, dirname, relativePath} from '@shopify/cli-kit/node/path'
23
import {glob, copyFile, mkdir} from '@shopify/cli-kit/node/fs'
34

@@ -10,10 +11,13 @@ export async function copyByPattern(
1011
outputDir: string
1112
patterns: string[]
1213
ignore: string[]
14+
appDirectory: string
15+
sourceDirConfigValue: string
1316
},
1417
options: {stdout: NodeJS.WritableStream},
1518
): Promise<{filesCopied: number; outputPaths: string[]}> {
16-
const {sourceDir, outputDir, patterns, ignore} = config
19+
const {sourceDir, outputDir, patterns, ignore, appDirectory, sourceDirConfigValue} = config
20+
assertPathWithinAppDir(sourceDir, appDirectory, sourceDirConfigValue)
1721
const files = await glob(patterns, {
1822
absolute: true,
1923
cwd: sourceDir,

‎packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.test.ts‎

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,13 @@ describe('copyConfigKeyEntry', () => {
2828
await mkdir(outDir)
2929

3030
const context = makeContext({static_root: 'public'}, mockStdout)
31-
const result = await copyConfigKeyEntry({key: 'static_root', baseDir: tmpDir, outputDir: outDir, context})
31+
const result = await copyConfigKeyEntry({
32+
key: 'static_root',
33+
baseDir: tmpDir,
34+
outputDir: outDir,
35+
context,
36+
appDirectory: tmpDir,
37+
})
3238

3339
expect(result.filesCopied).toBe(2)
3440
await expect(fileExists(joinPath(outDir, 'index.html'))).resolves.toBe(true)
@@ -48,7 +54,13 @@ describe('copyConfigKeyEntry', () => {
4854
await mkdir(outDir)
4955

5056
const context = makeContext({schema_path: 'src/schema.json'}, mockStdout)
51-
const result = await copyConfigKeyEntry({key: 'schema_path', baseDir: tmpDir, outputDir: outDir, context})
57+
const result = await copyConfigKeyEntry({
58+
key: 'schema_path',
59+
baseDir: tmpDir,
60+
outputDir: outDir,
61+
context,
62+
appDirectory: tmpDir,
63+
})
5264

5365
expect(result.filesCopied).toBe(1)
5466
await expect(fileExists(joinPath(outDir, 'schema.json'))).resolves.toBe(true)
@@ -72,7 +84,13 @@ describe('copyConfigKeyEntry', () => {
7284
await mkdir(outDir)
7385

7486
const context = makeContext({files: ['a/schema.json', 'b/schema.json']}, mockStdout)
75-
const result = await copyConfigKeyEntry({key: 'files', baseDir: tmpDir, outputDir: outDir, context})
87+
const result = await copyConfigKeyEntry({
88+
key: 'files',
89+
baseDir: tmpDir,
90+
outputDir: outDir,
91+
context,
92+
appDirectory: tmpDir,
93+
})
7694

7795
expect(result.filesCopied).toBe(2)
7896
// Both have basename schema.json — second one gets renamed
@@ -87,7 +105,13 @@ describe('copyConfigKeyEntry', () => {
87105
await mkdir(outDir)
88106

89107
const context = makeContext({}, mockStdout)
90-
const result = await copyConfigKeyEntry({key: 'static_root', baseDir: tmpDir, outputDir: outDir, context})
108+
const result = await copyConfigKeyEntry({
109+
key: 'static_root',
110+
baseDir: tmpDir,
111+
outputDir: outDir,
112+
context,
113+
appDirectory: tmpDir,
114+
})
91115

92116
expect(result.filesCopied).toBe(0)
93117
expect(result.pathMap.size).toBe(0)
@@ -101,7 +125,7 @@ describe('copyConfigKeyEntry', () => {
101125

102126
const context = makeContext({assets_dir: 'nonexistent'}, mockStdout)
103127
await expect(
104-
copyConfigKeyEntry({key: 'assets_dir', baseDir: tmpDir, outputDir: outDir, context}),
128+
copyConfigKeyEntry({key: 'assets_dir', baseDir: tmpDir, outputDir: outDir, context, appDirectory: tmpDir}),
105129
).rejects.toThrow(`Couldn't find`)
106130
})
107131
})
@@ -121,7 +145,13 @@ describe('copyConfigKeyEntry', () => {
121145
await mkdir(outDir)
122146

123147
const context = makeContext({roots: ['public', 'assets']}, mockStdout)
124-
const result = await copyConfigKeyEntry({key: 'roots', baseDir: tmpDir, outputDir: outDir, context})
148+
const result = await copyConfigKeyEntry({
149+
key: 'roots',
150+
baseDir: tmpDir,
151+
outputDir: outDir,
152+
context,
153+
appDirectory: tmpDir,
154+
})
125155

126156
// Promise.all runs copies sequentially; glob on the shared outDir may see files
127157
// from the other copy, so the total count is at least 3 (one per real file).
@@ -147,6 +177,7 @@ describe('copyConfigKeyEntry', () => {
147177
baseDir: tmpDir,
148178
outputDir: outDir,
149179
context,
180+
appDirectory: tmpDir,
150181
destination: 'static/icons',
151182
})
152183

@@ -174,6 +205,7 @@ describe('copyConfigKeyEntry', () => {
174205
baseDir: tmpDir,
175206
outputDir: outDir,
176207
context,
208+
appDirectory: tmpDir,
177209
})
178210

179211
expect(result.filesCopied).toBe(3)
@@ -200,6 +232,7 @@ describe('copyConfigKeyEntry', () => {
200232
baseDir: tmpDir,
201233
outputDir: outDir,
202234
context,
235+
appDirectory: tmpDir,
203236
})
204237

205238
expect(result.filesCopied).toBe(0)
@@ -227,6 +260,7 @@ describe('copyConfigKeyEntry', () => {
227260
baseDir: tmpDir,
228261
outputDir: outDir,
229262
context: contextA,
263+
appDirectory: tmpDir,
230264
usedBasenames,
231265
preserveFilePaths: true,
232266
})
@@ -237,6 +271,7 @@ describe('copyConfigKeyEntry', () => {
237271
baseDir: tmpDir,
238272
outputDir: outDir,
239273
context: contextB,
274+
appDirectory: tmpDir,
240275
usedBasenames,
241276
preserveFilePaths: true,
242277
})
@@ -263,6 +298,7 @@ describe('copyConfigKeyEntry', () => {
263298
baseDir: tmpDir,
264299
outputDir: outDir,
265300
context,
301+
appDirectory: tmpDir,
266302
preserveFilePaths: true,
267303
})
268304
await expect(promise).rejects.toThrow(AbortError)
@@ -285,6 +321,7 @@ describe('copyConfigKeyEntry', () => {
285321
baseDir: tmpDir,
286322
outputDir: outDir,
287323
context,
324+
appDirectory: tmpDir,
288325
preserveFilePaths: true,
289326
})
290327

@@ -310,6 +347,7 @@ describe('copyConfigKeyEntry', () => {
310347
baseDir: tmpDir,
311348
outputDir: outDir,
312349
context,
350+
appDirectory: tmpDir,
313351
})
314352

315353
expect(result.filesCopied).toBe(1)

‎packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import {assertPathWithinAppDir} from './assert-path-within-app.js'
12
import {joinPath, basename, relativePath, extname} from '@shopify/cli-kit/node/path'
23
import {glob, copyFile, copyDirectoryContents, fileExists, mkdir, isDirectory} from '@shopify/cli-kit/node/fs'
34
import {outputContent, outputDebug, outputToken} from '@shopify/cli-kit/node/output'
@@ -24,6 +25,7 @@ export async function copyConfigKeyEntry(config: {
2425
baseDir: string
2526
outputDir: string
2627
context: BuildContext
28+
appDirectory: string
2729
destination?: string
2830
usedBasenames?: Set<string>
2931
preserveFilePaths?: boolean
@@ -33,6 +35,7 @@ export async function copyConfigKeyEntry(config: {
3335
baseDir,
3436
outputDir,
3537
context,
38+
appDirectory,
3639
destination,
3740
usedBasenames = new Set<string>(),
3841
preserveFilePaths = false,
@@ -66,6 +69,7 @@ export async function copyConfigKeyEntry(config: {
6669
/* eslint-disable no-await-in-loop */
6770
for (const sourcePath of uniquePaths) {
6871
const fullPath = joinPath(baseDir, sourcePath)
72+
assertPathWithinAppDir(fullPath, appDirectory, sourcePath)
6973
const exists = await fileExists(fullPath)
7074
if (!exists) {
7175
throw new Error(

0 commit comments

Comments
 (0)