Skip to content
Merged
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
@@ -1,5 +1,6 @@
import SecurityCheck from './check.js'
import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
import {deterministicFindingsDocumentSchema} from '../../../services/app-security-engine/results/schema.js'
import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js'
import {Config} from '@oclif/core'
import {fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
Expand Down Expand Up @@ -125,6 +126,48 @@ describe('app security check command boundary', () => {
})
})

test('scans an extension directory that the TOML adds outside the app directory and records it in coverage', async () => {
await inTemporaryDirectory(async (directory) => {
const appRoot = joinPath(directory, 'app')
await createApp(appRoot)
await writeFile(
joinPath(appRoot, 'shopify.app.toml'),
`extension_directories = ["../shared/*"]\n${validAppConfiguration()}`,
)
await mkdir(joinPath(directory, 'shared', 'theme', 'blocks'), {recursive: true})
await writeFile(
joinPath(directory, 'shared', 'theme', 'shopify.extension.toml'),
'name = "theme"\ntype = "theme"\n',
)
await writeFile(
joinPath(directory, 'shared', 'theme', 'blocks', 'banner.liquid'),
'<p>{{ block.settings.text }}</p>\n',
)
const appDirectory = await fileRealPath(appRoot)
const sharedTheme = await fileRealPath(joinPath(directory, 'shared', 'theme'))

const result = await runCommand(['--path', appRoot, '--json', '--skip-instructions'])

expect(result.exitCode).toBe(0)
const output = JSON.parse(result.stdout)
expect(output.selection.scan_directories).toEqual([
{directory: appDirectory, origin: 'app_directory'},
{directory: sharedTheme, origin: 'app_config_directory'},
])
const deterministicFindings = await readJson(
appSecurityArtifactPaths(appDirectory, 'shopify.app').deterministicFindingsPath,
)
// `review` reads the results back through this schema.
expect(deterministicFindingsDocumentSchema.parse(deterministicFindings).coverage).toMatchObject({
scope: {include_dirs: [], excludes: [], no_git_ignore: false},
scan_directories: [
{directory: '.', origin: 'app_directory'},
{directory: '../shared/theme', origin: 'app_config_directory'},
],
})
})
})

test('writes the results under the --client-id results key and creates .shopify/.gitignore', async () => {
await inTemporaryDirectory(async (directory) => {
await createApp(directory)
Expand Down
5 changes: 3 additions & 2 deletions packages/app/src/cli/services/app-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,14 @@ async function logMetadata(app: {apiKey: string}, organization: Organization, re
interface LocalAppContextOutput {
app: AppInterface
project: Project
activeConfig: ActiveConfig
}

/**
* This function loads an app locally without making any network calls.
* It uses local specifications and doesn't require the app to be linked.
*
* @returns The local app and project instances.
* @returns The local app and project instances, and the selected app configuration.
*/
export async function localAppContext({
directory,
Expand All @@ -205,5 +206,5 @@ export async function localAppContext({
throw new AbortError(styledConfigurationError(app.errors.getErrors()[0]!))
}

return {app, project}
return {app, project, activeConfig}
}
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ export const coverageSchema = zod.object({
),
scope: createScopeSchema(),
scan_directories: zod.array(
zod.object({directory: zod.string(), origin: zod.enum(['app_directory', 'include_dir'])}),
zod.object({directory: zod.string(), origin: zod.enum(['app_directory', 'include_dir', 'app_config_directory'])}),
),
})

Expand Down
19 changes: 16 additions & 3 deletions packages/app/src/cli/services/app-security-engine/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,27 @@ export function getAgentInstructions(): string {
return EMBEDDED_APP_SECURITY_INSTRUCTIONS
}

/** A scan directory equal to the app directory is the app directory itself, however it was requested. */
function coverageScanDirectories({appDirectory, scanDirectories}: ScanInput): CoverageScanDirectory[] {
function coverageScanDirectories({
appDirectory,
scanDirectories,
appConfigDirectories = [],
}: ScanInput): CoverageScanDirectory[] {
return scanDirectories.map((directory) => ({
directory: normalizePath(relativePath(appDirectory, directory)) || '.',
origin: directory === appDirectory ? 'app_directory' : 'include_dir',
origin: coverageOrigin(directory, appDirectory, appConfigDirectories),
}))
}

/** A scan directory equal to the app directory is the app directory itself, however it was requested. */
function coverageOrigin(
directory: string,
appDirectory: string,
appConfigDirectories: ReadonlyArray<string>,
): CoverageScanDirectory['origin'] {
if (directory === appDirectory) return 'app_directory'
return appConfigDirectories.includes(directory) ? 'app_config_directory' : 'include_dir'
}

export async function scanApp(input: ScanInput, options: ScanOptions = {}): Promise<AppSecurityScan> {
const {ignoredScanDirectories, otherAppDirectories, ...result} = await scan(input, options)
const engineVersion = getEngineVersion()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,8 @@ function groupSourcePathsByExtensionDirectory(
* `extension_directories`. The app security check still scans every `shopify.extension.toml`
* inside the repository boundary, including unconfigured extensions, because
* those files can still contain secrets, XSS, and other security evidence.
* Extensions in a nested app or an `--include-dir` directory count as this app's.
* Extensions in a nested app, an `--include-dir` directory, or a directory that the selected app configuration
* file's `extension_directories` or `web_directories` add count as this app's.
*/
export function findExtensions(appRoot: string, repositoryFiles: ReadonlyArray<string>): ExtensionInfo[] {
const extensionTomls = repositoryFiles.filter((path) => basename(path) === 'shopify.extension.toml')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,11 @@ interface CheckFlags {
json?: boolean
}

function toml(path: string, clientId = 'client-app'): FileSpec {
/** `topLevelKeys` come first, since top-level keys must precede tables. */
function toml(path: string, clientId = 'client-app', topLevelKeys = ''): FileSpec {
return [
path,
`name = "test-app"
`${topLevelKeys}name = "test-app"
client_id = "${clientId}"
application_url = "https://example.com"
embedded = true
Expand Down Expand Up @@ -943,4 +944,43 @@ describe('layout catalogue: check --list-files', () => {
})
})
})

test('28. extension-and-web-directories-outside-the-app', async () => {
const layout: Layout = {
repositories: ['monorepo'],
files: [
toml(
'monorepo/app/shopify.app.toml',
'client-app',
'extension_directories = ["../shared-extensions/*"]\nweb_directories = ["../backend"]\n',
),
'monorepo/app/app/routes/index.tsx',
['monorepo/shared-extensions/theme/shopify.extension.toml', 'name = "theme"\ntype = "theme"\n'],
'monorepo/shared-extensions/theme/blocks/banner.liquid',
'monorepo/shared-extensions/README.md',
['monorepo/backend/shopify.web.toml', 'name = "backend"\nroles = ["backend"]\n\n[commands]\ndev = "dev"\n'],
'monorepo/backend/src/server.ts',
],
}
await inLayout(layout, async (root) => {
const {resolution, stdout, stderr} = await checkListFiles(root, 'monorepo/app')

expectConfigSelection(resolution, {
appDirectory: join(root, 'monorepo/app'),
tomlFileName: 'shopify.app.toml',
resultsKey: 'shopify.app',
})
// Each matched TOML's directory is scanned, not every directory the glob starts from.
expect(listedPaths(stdout)).toEqual([
'../backend/shopify.web.toml',
'../backend/src/server.ts',
'../shared-extensions/theme/blocks/banner.liquid',
'../shared-extensions/theme/shopify.extension.toml',
'app/routes/index.tsx',
'shopify.app.toml',
])
expect(warningText(stderr)).not.toContain("another app's configuration")
expect(resolution.commands.scan.args).toEqual(checkArgs())
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ export interface ScanInput {
requestedScanDirectories: ReadonlyArray<string>
/** Absolute path of the selected app configuration file. Absent when scanning without app configuration. */
appConfigFilePath?: string
/** The scan directories that the selected app configuration file's `extension_directories` and `web_directories` add. */
appConfigDirectories?: ReadonlyArray<string>
clientId?: string
}

Expand All @@ -95,7 +97,7 @@ export interface AppSecurityScope {
/** A scan directory as recorded in coverage: relative to the app directory, `.` for the app directory itself. */
export interface CoverageScanDirectory {
directory: string
origin: 'app_directory' | 'include_dir'
origin: 'app_directory' | 'include_dir' | 'app_config_directory'
}

export interface ScanOptions {
Expand Down
152 changes: 151 additions & 1 deletion packages/app/src/cli/services/app-security-selection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {fetchOrCreateOrganizationApp} from './context.js'
import use from './app/config/use.js'
import {AbortError} from '@shopify/cli-kit/node/error'
import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
import {joinPath} from '@shopify/cli-kit/node/path'
import {basename, joinPath} from '@shopify/cli-kit/node/path'
import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui'
import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest'
import {symlink} from 'node:fs/promises'
Expand Down Expand Up @@ -77,6 +77,7 @@ describe('resolveAppSecuritySelection with an app configuration', () => {
kind: 'config',
appDirectory,
appConfigFilePath: joinPath(appDirectory, 'shopify.app.toml'),
appConfigDirectories: [],
configClientId: 'default-client-id',
clientIdOverride: undefined,
})
Expand Down Expand Up @@ -326,6 +327,124 @@ describe('resolveAppSecuritySelection with several TOMLs and none selected', ()
})
})

/** A TOML whose `extension_directories` and `web_directories` come first, since top-level keys must precede tables. */
async function writeConfigurationWithDirectories(
appDirectory: string,
directories: {extension_directories?: string[]; web_directories?: string[]},
name = 'shopify.app.toml',
): Promise<void> {
const keys = Object.entries(directories).map(([key, entries]) => `${key} = ${JSON.stringify(entries)}\n`)
await mkdir(appDirectory)
await writeFile(joinPath(appDirectory, name), `${keys.join('')}${validAppConfiguration()}`)
}

/** A theme extension named after its directory, since extension handles must be unique. */
async function writeThemeExtension(directory: string): Promise<void> {
await mkdir(directory)
await writeFile(joinPath(directory, 'shopify.extension.toml'), `name = "${basename(directory)}"\ntype = "theme"\n`)
}

async function writeBackendWeb(directory: string): Promise<void> {
await mkdir(directory)
await writeFile(
joinPath(directory, 'shopify.web.toml'),
'name = "backend"\nroles = ["backend"]\n\n[commands]\ndev = "dev"\n',
)
}

describe('resolveAppSecuritySelection app configuration directories', () => {
test('are the directories of the extension and web TOMLs that relative entries match outside the app directory', async () => {
await inTemporaryDirectory(async (directory) => {
const appDirectory = joinPath(directory, 'app')
await writeConfigurationWithDirectories(appDirectory, {
extension_directories: ['extensions/*', '../shared/*', '../no-extensions/*'],
web_directories: ['../backend'],
})
await writeThemeExtension(joinPath(appDirectory, 'extensions', 'inside'))
await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
await writeFile(joinPath(directory, 'shared', 'README.md'), 'Shared extensions\n')
await mkdir(joinPath(directory, 'no-extensions', 'empty'))
await writeBackendWeb(joinPath(directory, 'backend'))

const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})

expect(selection).toMatchObject({
appConfigDirectories: [
await fileRealPath(joinPath(directory, 'backend')),
await fileRealPath(joinPath(directory, 'shared', 'theme')),
],
})
})
})

test('ignore absolute entries, the way the CLI does', async () => {
await inTemporaryDirectory(async (directory) => {
const appDirectory = joinPath(directory, 'app')
await writeConfigurationWithDirectories(appDirectory, {
extension_directories: [joinPath(directory, 'shared', '*')],
web_directories: [joinPath(directory, 'backend')],
})
await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
await writeBackendWeb(joinPath(directory, 'backend'))

const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})

expect(selection).toMatchObject({appConfigDirectories: []})
})
})

test('leave out a directory reached through a symbolic link outside the app directory', async () => {
await inTemporaryDirectory(async (directory) => {
const appDirectory = joinPath(directory, 'app')
await writeConfigurationWithDirectories(appDirectory, {extension_directories: ['../linked-shared/*']})
await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
await symlink(joinPath(directory, 'shared'), joinPath(directory, 'linked-shared'), 'dir')

const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})

expect(selection).toMatchObject({appConfigDirectories: []})
})
})

test('leave out an extension directory inside the app directory that links outside it', async () => {
await inTemporaryDirectory(async (directory) => {
const appDirectory = joinPath(directory, 'app')
await writeConfigurationWithDirectories(appDirectory, {extension_directories: ['extensions/*']})
await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
await mkdir(joinPath(appDirectory, 'extensions'))
await symlink(joinPath(directory, 'shared', 'theme'), joinPath(appDirectory, 'extensions', 'theme'), 'dir')

const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})

expect(selection).toMatchObject({appConfigDirectories: []})
})
})

test('come from the selected TOML only', async () => {
await inTemporaryDirectory(async (directory) => {
const appDirectory = joinPath(directory, 'app')
await writeConfigurationWithDirectories(appDirectory, {})
await writeFile(
joinPath(appDirectory, 'shopify.app.staging.toml'),
`extension_directories = ["../shared/*"]\n${validAppConfiguration('staging-client-id')}`,
)
await writeThemeExtension(joinPath(directory, 'shared', 'theme'))

const defaultSelection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})
const stagingSelection = await resolveAppSecuritySelection({
path: appDirectory,
config: 'staging',
allowPrompts: false,
})

expect(defaultSelection).toMatchObject({appConfigDirectories: []})
expect(stagingSelection).toMatchObject({
appConfigDirectories: [await fileRealPath(joinPath(directory, 'shared', 'theme'))],
})
})
})
})

describe('resolveAppSecuritySelection with --without-app-config', () => {
test('aborts without --client-id', async () => {
await inTemporaryDirectory(async (directory) => {
Expand Down Expand Up @@ -773,4 +892,35 @@ describe('mergeScanDirectories', () => {
test('accepts an include directory elsewhere on the same Windows drive', () => {
expect(mergeScanDirectories('C:/work/app', ['C:/backend']).scanDirectories).toHaveLength(2)
})

test('lists each app configuration directory after the include directories', () => {
expect(mergeScanDirectories('/work/app', ['/work/backend'], ['/work/shared/theme'])).toEqual({
scanDirectories: [
{directory: '/work/app', origin: 'app_directory'},
{directory: '/work/backend', origin: 'include_dir'},
{directory: '/work/shared/theme', origin: 'app_config_directory'},
],
requestedScanDirectories: ['/work/app', '/work/backend', '/work/shared/theme'],
})
})

test('counts an app configuration directory that is also an include directory as the include directory', () => {
expect(mergeScanDirectories('/work/app', ['/work/shared/theme'], ['/work/shared/theme']).scanDirectories).toEqual([
{directory: '/work/app', origin: 'app_directory'},
{directory: '/work/shared/theme', origin: 'include_dir'},
])
})

test('drops an app configuration directory inside an include directory', () => {
expect(mergeScanDirectories('/work/app', ['/work/shared'], ['/work/shared/theme']).scanDirectories).toEqual([
{directory: '/work/app', origin: 'app_directory'},
{directory: '/work/shared', origin: 'include_dir'},
])
})

test('rejects an app configuration directory on another Windows drive', () => {
expect(() => mergeScanDirectories('C:/work/app', [], ['D:/shared/theme'])).toThrowError(
new AbortError('Extension or web directory D:/shared/theme: must be on the same drive as the app directory.'),
)
})
})
Loading
Loading