From 666a14c11b0aa0d5b8f251dbd62745c68ff8bc94 Mon Sep 17 00:00:00 2001 From: Josh Larson Date: Wed, 9 Sep 2026 15:40:29 -0500 Subject: [PATCH 1/2] Reuse Shopify CLI app identity and config schemas in App Doctor. Walk-up discovery and config-file names now share isValidFormatAppConfigurationFileName. Access, auth, and webhook parsing use the CLI schemas, with URI strings kept only so INSECURE_WEBHOOK_URL can still report http. This is not Project.load(): bounded reads, coverage gaps, and unconfigured extensions stay Doctor-specific. Co-authored-by: AI (Pi/Grok 4.6) --- .../cli/models/app/config-file-naming.test.ts | 2 + .../src/cli/models/app/config-file-naming.ts | 3 + .../app/src/cli/models/project/project.ts | 11 +-- packages/app/src/cli/prompts/config.ts | 5 +- .../app-doctor-engine/scanners/discover.ts | 91 ++++++++----------- .../tests/deterministic-rules.test.ts | 15 ++- .../tests/discovery-safety.test.ts | 7 ++ 7 files changed, 74 insertions(+), 60 deletions(-) diff --git a/packages/app/src/cli/models/app/config-file-naming.test.ts b/packages/app/src/cli/models/app/config-file-naming.test.ts index ac8a24be6ab..beaec112f92 100644 --- a/packages/app/src/cli/models/app/config-file-naming.test.ts +++ b/packages/app/src/cli/models/app/config-file-naming.test.ts @@ -52,6 +52,8 @@ describe('isValidFormatAppConfigurationFileName', () => { test('returns false for invalid filenames', () => { expect(isValidFormatAppConfigurationFileName('production')).toBe(false) expect(isValidFormatAppConfigurationFileName('shopify.web.toml')).toBe(false) + expect(isValidFormatAppConfigurationFileName('shopify.application.toml')).toBe(false) + expect(isValidFormatAppConfigurationFileName('shopify.app.foo.bar.toml')).toBe(false) expect(isValidFormatAppConfigurationFileName('shopify.app..toml')).toBe(false) expect(isValidFormatAppConfigurationFileName('')).toBe(false) }) diff --git a/packages/app/src/cli/models/app/config-file-naming.ts b/packages/app/src/cli/models/app/config-file-naming.ts index 8d8823631e0..14ef195141e 100644 --- a/packages/app/src/cli/models/app/config-file-naming.ts +++ b/packages/app/src/cli/models/app/config-file-naming.ts @@ -5,6 +5,9 @@ import {basename} from '@shopify/cli-kit/node/path' const appConfigurationFileNameRegex = /^shopify\.app(\.[-\w]+)?\.toml$/ export type AppConfigurationFileName = 'shopify.app.toml' | `shopify.app.${string}.toml` +/** Glob used to discover app configuration files. Always filter matches with `isValidFormatAppConfigurationFileName`. */ +export const APP_CONFIG_FILE_GLOB = 'shopify.app*.toml' + /** * Gets the name of the app configuration file (e.g. `shopify.app.production.toml`) based on a provided config name. * diff --git a/packages/app/src/cli/models/project/project.ts b/packages/app/src/cli/models/project/project.ts index f34ac61bedb..1b0c39c31b5 100644 --- a/packages/app/src/cli/models/project/project.ts +++ b/packages/app/src/cli/models/project/project.ts @@ -1,4 +1,5 @@ import {configurationFileNames} from '../../constants.js' +import {APP_CONFIG_FILE_GLOB, isValidFormatAppConfigurationFileName} from '../app/config-file-naming.js' import {TomlFile, TomlFileError} from '@shopify/cli-kit/node/toml/toml-file' import {readAndParseDotEnv, DotEnvFile} from '@shopify/cli-kit/node/dot-env' import {fileExists, glob, findPathUp, readFile} from '@shopify/cli-kit/node/fs' @@ -12,8 +13,6 @@ import {joinPath, basename} from '@shopify/cli-kit/node/path' import {AbortError} from '@shopify/cli-kit/node/error' import {JsonMapType} from '@shopify/cli-kit/node/toml' -const APP_CONFIG_GLOB = 'shopify.app*.toml' -const APP_CONFIG_REGEX = /^shopify\.app(\.[-\w]+)?\.toml$/ const EXTENSION_TOML = '*.extension.toml' const WEB_TOML = 'shopify.web.toml' const DEFAULT_EXTENSION_DIR = 'extensions/*' @@ -164,8 +163,8 @@ export class Project { async function findProjectRoot(startDirectory: string): Promise { const found = await findPathUp( async (directory) => { - const matches = await glob(joinPath(directory, APP_CONFIG_GLOB)) - if (matches.length > 0) return directory + const matches = await glob(joinPath(directory, APP_CONFIG_FILE_GLOB)) + if (matches.some((path) => isValidFormatAppConfigurationFileName(basename(path)))) return directory }, { cwd: startDirectory, @@ -181,9 +180,9 @@ async function findProjectRoot(startDirectory: string): Promise { } async function discoverAppConfigFiles(directory: string, errors: TomlFileError[]): Promise { - const pattern = joinPath(directory, APP_CONFIG_GLOB) + const pattern = joinPath(directory, APP_CONFIG_FILE_GLOB) const paths = await glob(pattern) - const validPaths = paths.filter((filePath) => APP_CONFIG_REGEX.test(basename(filePath))) + const validPaths = paths.filter((filePath) => isValidFormatAppConfigurationFileName(basename(filePath))) return readTomlFilesCollectingErrors(validPaths, errors) } diff --git a/packages/app/src/cli/prompts/config.ts b/packages/app/src/cli/prompts/config.ts index 4edced86db5..d888e6e1122 100644 --- a/packages/app/src/cli/prompts/config.ts +++ b/packages/app/src/cli/prompts/config.ts @@ -1,5 +1,5 @@ /* eslint-disable no-await-in-loop */ -import {AppConfigurationFileName} from '../models/app/loader.js' +import {AppConfigurationFileName, isValidFormatAppConfigurationFileName} from '../models/app/loader.js' import { RenderTextPromptOptions, renderConfirmationPrompt, @@ -43,7 +43,8 @@ function filenameFromName(name: string, highlight = false): AppConfigurationFile } export async function findConfigFiles(directory: string): Promise { - return glob(joinPath(directory, 'shopify.app*.toml')) + const files = await glob(joinPath(directory, 'shopify.app*.toml')) + return files.filter((path) => isValidFormatAppConfigurationFileName(basename(path))) } export async function selectConfigFile(directory: string): Promise> { diff --git a/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts b/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts index 99da85654cf..b4aabe92898 100644 --- a/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts +++ b/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts @@ -1,7 +1,9 @@ +import {APP_CONFIG_FILE_GLOB, isValidFormatAppConfigurationFileName} from '../../../models/app/config-file-naming.js' import {AppAccessScopesSchema, AppAuthSchema} from '../../../models/extensions/specifications/app_config_app_access.js' import {WebhookSubscriptionSchema} from '../../../models/extensions/specifications/app_config_webhook_schemas/webhook_subscription_schema.js' +import {removeTrailingSlash} from '../../../models/extensions/specifications/validation/common.js' import {fileExistsSync, fileSizeSync, globSync, readFileSync} from '@shopify/cli-kit/node/fs' -import {cwd, dirname, extname, joinPath, relativePath, resolvePath} from '@shopify/cli-kit/node/path' +import {basename, cwd, dirname, extname, joinPath, relativePath, resolvePath} from '@shopify/cli-kit/node/path' import {zod} from '@shopify/cli-kit/node/schema' import {decodeToml} from '@shopify/cli-kit/node/toml/codec' import {lstatSync} from 'node:fs' @@ -19,11 +21,12 @@ export class AppRootDiscoveryError extends Error { /** * Find the nearest app root without ever substituting CWD for a bad explicit path. * - * This is intentionally not `Project.load()`. That loader also reads package, - * environment, and hidden configuration, and it does not keep the bounded raw - * bytes App Doctor hashes and reports as coverage. App Doctor reuses the same - * `shopify.app*.toml` candidate shape, then reads those files through its own - * repository boundary. + * App identity matches the rest of Shopify CLI: walk up for + * `shopify.app.toml` / `shopify.app..toml` using + * `isValidFormatAppConfigurationFileName`. This is not `Project.load()` — that + * loader also reads package, environment, and hidden configuration, follows + * different symlink policy, and does not keep the bounded raw bytes App Doctor + * hashes and reports as coverage. */ export function findAppRoot(startPath?: string): string { const requestedPath = resolvePath(startPath ?? cwd()) @@ -33,7 +36,7 @@ export function findAppRoot(startPath?: string): string { let directory = requestedPath if (startPath && lstatSync(requestedPath).isFile()) { - if (!requestedPath.endsWith('.toml')) { + if (!isValidFormatAppConfigurationFileName(basename(requestedPath))) { throw new AppRootDiscoveryError(`App path is not a directory or TOML file: ${startPath}`) } return dirname(requestedPath) @@ -43,14 +46,7 @@ export function findAppRoot(startPath?: string): string { } while (true) { - const tomls = globSync('shopify.app*.toml', { - cwd: directory, - deep: 1, - dot: false, - onlyFiles: false, - followSymbolicLinks: false, - }) - if (tomls.length > 0) return directory + if (listAppConfigFiles(directory).length > 0) return directory const parent = dirname(directory) if (parent === directory) break @@ -60,22 +56,25 @@ export function findAppRoot(startPath?: string): string { throw new AppRootDiscoveryError(`Could not find a shopify.app*.toml from: ${startPath ?? cwd()}`) } -/** - * Find and parse all shopify.app.*.toml files in the app root. - * - * Candidate discovery matches `Project.load()`, but parsing stays on bounded - * raw reads so unreadable files become coverage gaps instead of loader errors. - */ -export function findAppTomls(appRoot: string): AppTomlContent[] { - const files = globSync('shopify.app*.toml', { - cwd: appRoot, +function listAppConfigFiles(directory: string): string[] { + return globSync(APP_CONFIG_FILE_GLOB, { + cwd: directory, deep: 1, dot: false, onlyFiles: false, followSymbolicLinks: false, - }) + }).filter((file) => isValidFormatAppConfigurationFileName(basename(file))) +} - return files.flatMap((file) => { +/** + * Find and parse all shopify.app.*.toml files in the app root. + * + * Candidate discovery uses the same config-file identity as `Project.load()`, + * but parsing stays on bounded raw reads so unreadable files become coverage + * gaps instead of loader errors. + */ +export function findAppTomls(appRoot: string): AppTomlContent[] { + return listAppConfigFiles(appRoot).flatMap((file) => { const path = joinPath(appRoot, file) const content = readRepositoryText(appRoot, path) if (content === undefined) return [] @@ -116,6 +115,7 @@ export function loadAppToml(tomlPath: string, appRoot = dirname(tomlPath)): AppT } } +/** CLI webhook section shape. URI fields stay strings so INSECURE_WEBHOOK_URL can report http. */ const WebhooksSectionSchema = zod.object({ api_version: zod.string().optional(), privacy_compliance: zod @@ -128,10 +128,11 @@ const WebhooksSectionSchema = zod.object({ subscriptions: zod.array(zod.unknown()).optional(), }) -const EvidenceWebhookSubscriptionSchema = zod.object({ - uri: zod.string(), - topics: zod.array(zod.string()).optional(), - compliance_topics: zod.array(zod.string()).optional(), +const SecurityWebhookSubscriptionSchema = WebhookSubscriptionSchema.extend({ + uri: zod.preprocess( + (arg) => removeTrailingSlash(arg as string), + zod.string({invalid_type_error: 'Value must be string'}), + ), }) export function parseAppToml( @@ -170,14 +171,11 @@ function projectAccessScopes(value: unknown, path: string, appRoot?: string): st function projectRedirectUrls(value: unknown, path: string, appRoot?: string): string[] { if (value === undefined) return [] const parsed = AppAuthSchema.safeParse(value) - if (parsed.success) return parsed.data.redirect_urls - - const fallback = zod.object({redirect_urls: zod.array(zod.string())}).safeParse(value) - if (!fallback.success) { + if (!parsed.success) { recordSectionGap(appRoot, path, 'auth section could not be parsed') return [] } - return fallback.data.redirect_urls + return parsed.data.redirect_urls } function projectWebhooks( @@ -207,22 +205,12 @@ function projectWebhooks( } function projectWebhookSubscription(value: unknown): WebhookSubscription[] { - const strict = WebhookSubscriptionSchema.safeParse(value) - if (strict.success) { - return [ - { - topics: [...(strict.data.topics ?? []), ...(strict.data.compliance_topics ?? [])], - uri: strict.data.uri, - }, - ] - } - - const evidence = EvidenceWebhookSubscriptionSchema.safeParse(value) - if (!evidence.success) return [] + const parsed = SecurityWebhookSubscriptionSchema.safeParse(value) + if (!parsed.success) return [] return [ { - topics: [...(evidence.data.topics ?? []), ...(evidence.data.compliance_topics ?? [])], - uri: evidence.data.uri, + topics: [...(parsed.data.topics ?? []), ...(parsed.data.compliance_topics ?? [])], + uri: parsed.data.uri, }, ] } @@ -274,7 +262,7 @@ function normalizePath(path: string): string { function findNestedAppDirectories(appRoot: string): string[] { return [ ...new Set( - globSync('**/shopify.app*.toml', { + globSync(`**/${APP_CONFIG_FILE_GLOB}`, { followSymbolicLinks: false, cwd: appRoot, ignore: IGNORED_DIRECTORIES, @@ -282,6 +270,7 @@ function findNestedAppDirectories(appRoot: string): string[] { dot: false, onlyFiles: false, }) + .filter((path) => isValidFormatAppConfigurationFileName(basename(path))) .map((path) => normalizePath(dirname(path))) .filter((path) => path !== '.' && path.length > 0), ), @@ -300,7 +289,7 @@ function discoveryIgnores(directory: string, projectRoot: string): string[] { * Find extension-like repository content under the app root. * * `Project.load()` only considers paths in each app configuration's - * `extension_directories`. App Doctor scans every `shopify.extension.toml` + * `extension_directories`. App Doctor 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. * Nested apps, generated output, and test trees remain excluded. diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index 5f2399594f7..37ddb633d79 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -107,7 +107,7 @@ describe('deterministic rules product contract', () => { expect(parsed.apiVersion).toBeUndefined() }) - test('keeps insecure webhook URIs that the CLI schema rejects', () => { + test('keeps insecure webhook URIs that the CLI URI validator rejects', () => { const parsed = parseAppToml( { webhooks: { @@ -119,6 +119,19 @@ describe('deterministic rules product contract', () => { ) expect(parsed.webhooks).toEqual([{topics: ['orders/create'], uri: 'http://insecure.example/webhooks'}]) }) + + test('drops webhook subscriptions that are not CLI-shaped', () => { + const parsed = parseAppToml( + { + webhooks: { + api_version: '2023-07', + subscriptions: [{not: 'a-subscription'}, {topics: ['orders/create']}], + }, + }, + '/app/shopify.app.toml', + ) + expect(parsed.webhooks).toEqual([]) + }) }) describe('JavaScript regex mode', () => { diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts index 3dd4654caed..39d0e57e5da 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts @@ -64,6 +64,13 @@ describe.sequential('app root discovery', () => { expect(() => findAppRoot(missing)).toThrow(AppRootDiscoveryError) expect(() => findAppRoot(missing)).toThrow(`App path does not exist: ${missing}`) }) + + test('ignores files that match the glob but are not CLI app configuration names', async () => { + const root = await makeDirectory() + await writeFile(join(root, 'shopify.application.toml'), appConfiguration) + await writeFile(join(root, 'shopify.app.foo.bar.toml'), appConfiguration) + expect(() => findAppRoot(root)).toThrow(AppRootDiscoveryError) + }) }) describe('repository discovery exclusions', () => { From 22b84842de07a3c2ac0b91f9b2b0e2aa3005454d Mon Sep 17 00:00:00 2001 From: Josh Larson Date: Thu, 10 Sep 2026 09:14:17 -0500 Subject: [PATCH 2/2] Address App Doctor config-identity review comments. Use the shared app-config glob in findConfigFiles, describe non-app TOML paths as configuration files, and keep an insecure webhook URI when another subscription field fails the CLI schema. Co-authored-by: AI (Pi/Grok 4.6) --- packages/app/src/cli/prompts/config.ts | 3 +- .../app-doctor-engine/scanners/discover.ts | 35 +++++++++++++------ .../tests/deterministic-rules.test.ts | 13 +++++++ .../tests/discovery-safety.test.ts | 10 ++++++ 4 files changed, 50 insertions(+), 11 deletions(-) diff --git a/packages/app/src/cli/prompts/config.ts b/packages/app/src/cli/prompts/config.ts index d888e6e1122..6ff2d5f7c23 100644 --- a/packages/app/src/cli/prompts/config.ts +++ b/packages/app/src/cli/prompts/config.ts @@ -1,4 +1,5 @@ /* eslint-disable no-await-in-loop */ +import {APP_CONFIG_FILE_GLOB} from '../models/app/config-file-naming.js' import {AppConfigurationFileName, isValidFormatAppConfigurationFileName} from '../models/app/loader.js' import { RenderTextPromptOptions, @@ -43,7 +44,7 @@ function filenameFromName(name: string, highlight = false): AppConfigurationFile } export async function findConfigFiles(directory: string): Promise { - const files = await glob(joinPath(directory, 'shopify.app*.toml')) + const files = await glob(joinPath(directory, APP_CONFIG_FILE_GLOB)) return files.filter((path) => isValidFormatAppConfigurationFileName(basename(path))) } diff --git a/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts b/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts index b4aabe92898..03b49b6c9e9 100644 --- a/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts +++ b/packages/app/src/cli/services/app-doctor-engine/scanners/discover.ts @@ -37,7 +37,7 @@ export function findAppRoot(startPath?: string): string { let directory = requestedPath if (startPath && lstatSync(requestedPath).isFile()) { if (!isValidFormatAppConfigurationFileName(basename(requestedPath))) { - throw new AppRootDiscoveryError(`App path is not a directory or TOML file: ${startPath}`) + throw new AppRootDiscoveryError(`App path is not a directory or Shopify app configuration file: ${startPath}`) } return dirname(requestedPath) } @@ -190,7 +190,9 @@ function projectWebhooks( return {subscriptions: []} } - const webhookSubscriptions = (parsed.data.subscriptions ?? []).flatMap(projectWebhookSubscription) + const webhookSubscriptions = (parsed.data.subscriptions ?? []).flatMap((subscription) => + projectWebhookSubscription(subscription, path, appRoot), + ) const privacyCompliance = parsed.data.privacy_compliance const privacyComplianceWebhooks = [ {topic: 'customers/redact', uri: privacyCompliance?.customer_deletion_url}, @@ -204,15 +206,28 @@ function projectWebhooks( } } -function projectWebhookSubscription(value: unknown): WebhookSubscription[] { +const WebhookUriOnlySchema = zod.object({ + uri: zod.preprocess( + (arg) => removeTrailingSlash(arg as string), + zod.string({invalid_type_error: 'Value must be string'}), + ), +}) + +function projectWebhookSubscription(value: unknown, path: string, appRoot?: string): WebhookSubscription[] { const parsed = SecurityWebhookSubscriptionSchema.safeParse(value) - if (!parsed.success) return [] - return [ - { - topics: [...(parsed.data.topics ?? []), ...(parsed.data.compliance_topics ?? [])], - uri: parsed.data.uri, - }, - ] + if (parsed.success) { + return [ + { + topics: [...(parsed.data.topics ?? []), ...(parsed.data.compliance_topics ?? [])], + uri: parsed.data.uri, + }, + ] + } + + recordSectionGap(appRoot, path, 'webhook subscription could not be parsed') + const uriOnly = WebhookUriOnlySchema.safeParse(value) + if (!uriOnly.success) return [] + return [{topics: [], uri: uriOnly.data.uri}] } function recordSectionGap(appRoot: string | undefined, path: string, detail: string): void { diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts index 37ddb633d79..339c4b32080 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/deterministic-rules.test.ts @@ -132,6 +132,19 @@ describe('deterministic rules product contract', () => { ) expect(parsed.webhooks).toEqual([]) }) + + test('keeps an insecure URI when another subscription field is invalid', () => { + const parsed = parseAppToml( + { + webhooks: { + api_version: '2023-07', + subscriptions: [{topics: ['orders/create'], uri: 'http://insecure.example/webhooks', filter: 42}], + }, + }, + '/app/shopify.app.toml', + ) + expect(parsed.webhooks).toEqual([{topics: [], uri: 'http://insecure.example/webhooks'}]) + }) }) describe('JavaScript regex mode', () => { diff --git a/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts b/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts index 39d0e57e5da..4044323e4d7 100644 --- a/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts +++ b/packages/app/src/cli/services/app-doctor-engine/tests/discovery-safety.test.ts @@ -71,6 +71,16 @@ describe.sequential('app root discovery', () => { await writeFile(join(root, 'shopify.app.foo.bar.toml'), appConfiguration) expect(() => findAppRoot(root)).toThrow(AppRootDiscoveryError) }) + + test('rejects an explicit non-app TOML path with a configuration-file error', async () => { + const root = await makeDirectory() + const webToml = join(root, 'shopify.web.toml') + await writeFile(webToml, 'type = "frontend"\n') + expect(() => findAppRoot(webToml)).toThrow(AppRootDiscoveryError) + expect(() => findAppRoot(webToml)).toThrow( + `App path is not a directory or Shopify app configuration file: ${webToml}`, + ) + }) }) describe('repository discovery exclusions', () => {