From 6b3cd00cf9aa21dcb0d5483a943b21602bcf81af Mon Sep 17 00:00:00 2001 From: Donald Merand Date: Mon, 24 Aug 2026 12:06:39 -0400 Subject: [PATCH] Handle invalid remote contract schemas Assisted-By: devx/533b856c-5f30-42c9-8965-a066782729a4 --- .changeset/graceful-remote-contracts.md | 5 ++ .../fetch-extension-specifications.test.ts | 60 +++++++++++++- .../fetch-extension-specifications.ts | 14 +++- .../app/src/cli/utilities/json-schema.test.ts | 82 ++++++++++++++++++- packages/app/src/cli/utilities/json-schema.ts | 45 ++++++++-- 5 files changed, 193 insertions(+), 13 deletions(-) create mode 100644 .changeset/graceful-remote-contracts.md diff --git a/.changeset/graceful-remote-contracts.md b/.changeset/graceful-remote-contracts.md new file mode 100644 index 00000000000..f6136d1bb14 --- /dev/null +++ b/.changeset/graceful-remote-contracts.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Skip invalid remote contract schemas without aborting app commands. diff --git a/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts b/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts index 189242534d7..0f59e9a7325 100644 --- a/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts +++ b/packages/app/src/cli/services/generate/fetch-extension-specifications.test.ts @@ -1,8 +1,66 @@ import {fetchSpecifications} from './fetch-extension-specifications.js' import {testDeveloperPlatformClient, testOrganizationApp} from '../../models/app/app.test-data.js' -import {describe, expect, test} from 'vitest' +import {RemoteSpecification} from '../../api/graphql/extension_specifications.js' +import {afterEach, describe, expect, test} from 'vitest' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' describe('fetchExtensionSpecifications', () => { + afterEach(() => { + mockAndCaptureOutput().clear() + }) + + test('skips a remote-only specification with an invalid contract schema', async () => { + // Given + const invalidRemoteSpecification: RemoteSpecification = { + name: 'Invalid remote extension', + externalName: 'Invalid Remote Extension', + identifier: 'invalid_remote_extension', + externalIdentifier: 'invalid_remote_extension', + gated: false, + experience: 'extension', + managementExperience: 'cli', + registrationLimit: 1, + uidStrategy: 'uuid', + validationSchema: { + jsonSchema: '{"type":"object","properties":{"name":{"$ref":"#/definitions/missing"}}}', + }, + } + const validRemoteSpecification: RemoteSpecification = { + name: 'Valid remote extension', + externalName: 'Valid Remote Extension', + identifier: 'valid_remote_extension', + externalIdentifier: 'valid_remote_extension', + gated: false, + experience: 'extension', + managementExperience: 'cli', + registrationLimit: 1, + uidStrategy: 'uuid', + validationSchema: { + jsonSchema: '{"type":"object","properties":{"name":{"type":"string"}}}', + }, + } + const outputMock = mockAndCaptureOutput() + const developerPlatformClient = testDeveloperPlatformClient({ + specifications: () => Promise.resolve([invalidRemoteSpecification, validRemoteSpecification]), + }) + + // When + const specifications = await fetchSpecifications({ + developerPlatformClient, + app: testOrganizationApp(), + }) + + // Then + expect(specifications).not.toContainEqual( + expect.objectContaining({identifier: invalidRemoteSpecification.identifier}), + ) + expect(specifications).toContainEqual(expect.objectContaining({identifier: validRemoteSpecification.identifier})) + expect(outputMock.warn()).toContain( + `Remote contract validation for "${invalidRemoteSpecification.identifier}" is skipped`, + ) + expect(outputMock.warn()).toContain('Server-side validation remains the authority.') + }) + test('returns the filtered and mapped results including theme', async () => { // Given/When const got = await fetchSpecifications({ diff --git a/packages/app/src/cli/services/generate/fetch-extension-specifications.ts b/packages/app/src/cli/services/generate/fetch-extension-specifications.ts index a752d56b726..c14a400a0e7 100644 --- a/packages/app/src/cli/services/generate/fetch-extension-specifications.ts +++ b/packages/app/src/cli/services/generate/fetch-extension-specifications.ts @@ -7,7 +7,7 @@ import { } from '../../models/extensions/specification.js' import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js' import {MinimalAppIdentifiers} from '../../models/organization.js' -import {unifiedConfigurationParserFactory} from '../../utilities/json-schema.js' +import {unifiedConfigurationParserFactory, warnRemoteContractValidationSkipped} from '../../utilities/json-schema.js' import {getArrayRejectingUndefined} from '@shopify/cli-kit/common/array' import {outputDebug} from '@shopify/cli-kit/node/output' import {normaliseJsonSchema} from '@shopify/cli-kit/node/json-schema' @@ -137,8 +137,16 @@ function mergeLocalAndRemoteSpec( async function createRemoteOnlySpecification( remoteSpec: RemoteSpecification, validationSchema: {jsonSchema: string}, -): Promise { - const normalisedSchema = await normaliseJsonSchema(validationSchema.jsonSchema) +): Promise { + let normalisedSchema + try { + normalisedSchema = await normaliseJsonSchema(validationSchema.jsonSchema) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + warnRemoteContractValidationSkipped(remoteSpec.identifier) + return undefined + } + const hasLocalization = normalisedSchema.properties?.localization !== undefined const localSpec = createContractBasedModuleSpecification({ identifier: remoteSpec.identifier, diff --git a/packages/app/src/cli/utilities/json-schema.test.ts b/packages/app/src/cli/utilities/json-schema.test.ts index c139dc6e3de..5a486356721 100644 --- a/packages/app/src/cli/utilities/json-schema.test.ts +++ b/packages/app/src/cli/utilities/json-schema.test.ts @@ -1,6 +1,7 @@ import {unifiedConfigurationParserFactory} from './json-schema.js' -import {describe, test, expect} from 'vitest' +import {afterEach, describe, test, expect} from 'vitest' import {randomUUID} from '@shopify/cli-kit/node/crypto' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' describe('unifiedConfigurationParserFactory', () => { const mockParseConfigurationObject = (config: any) => { @@ -18,6 +19,10 @@ describe('unifiedConfigurationParserFactory', () => { } } + afterEach(() => { + mockAndCaptureOutput().clear() + }) + test('falls back to zod parser when no JSON schema is provided', async () => { // Given const merged = { @@ -60,6 +65,81 @@ describe('unifiedConfigurationParserFactory', () => { }) }) + test('falls back to zod parser when JSON schema has an invalid reference', async () => { + // Given + const merged = { + identifier: randomUUID(), + parseConfigurationObject: mockParseConfigurationObject, + validationSchema: { + jsonSchema: '{"type":"object","properties":{"name":{"$ref":"#/definitions/missing"}}}', + }, + } + const outputMock = mockAndCaptureOutput() + + // When + const parser = await unifiedConfigurationParserFactory(merged as any, merged.validationSchema) + const result = parser({type: 'product_subscription'}) + + // Then + expect(result).toEqual({ + state: 'ok', + data: {type: 'product_subscription'}, + errors: undefined, + }) + expect(outputMock.warn()).toContain(`Remote contract validation for "${merged.identifier}" is skipped`) + expect(outputMock.warn()).toContain('Server-side validation remains the authority.') + }) + + test('falls back to zod parser when JSON schema is invalid', async () => { + // Given + const merged = { + identifier: randomUUID(), + parseConfigurationObject: mockParseConfigurationObject, + validationSchema: { + jsonSchema: '{', + }, + } + const outputMock = mockAndCaptureOutput() + + // When + const parser = await unifiedConfigurationParserFactory(merged as any, merged.validationSchema) + const result = parser({type: 'product_subscription'}) + + // Then + expect(result).toEqual({ + state: 'ok', + data: {type: 'product_subscription'}, + errors: undefined, + }) + expect(outputMock.warn()).toContain(`Remote contract validation for "${merged.identifier}" is skipped`) + expect(outputMock.warn()).toContain('Server-side validation remains the authority.') + }) + + test('falls back to zod parser when JSON schema fails AJV compilation', async () => { + // Given + const merged = { + identifier: randomUUID(), + parseConfigurationObject: mockParseConfigurationObject, + validationSchema: { + jsonSchema: '{"type":"object","properties":{"name":{"type":"not-a-real-type"}}}', + }, + } + const outputMock = mockAndCaptureOutput() + const config = {type: 'product_subscription', localOnly: 'preserved'} + + // When + const parser = await unifiedConfigurationParserFactory(merged as any, merged.validationSchema) + const firstResult = parser(config) + const secondResult = parser(config) + + // Then + expect(firstResult).toEqual({state: 'ok', data: config, errors: undefined}) + expect(secondResult).toEqual({state: 'ok', data: config, errors: undefined}) + expect(outputMock.warn()).toBe( + `Remote contract validation for "${merged.identifier}" is skipped because its schema is invalid. Server-side validation remains the authority.`, + ) + }) + test('validates with both zod and JSON schema when both succeed', async () => { // Given const merged = { diff --git a/packages/app/src/cli/utilities/json-schema.ts b/packages/app/src/cli/utilities/json-schema.ts index 76ac3d5e411..6334ec92532 100644 --- a/packages/app/src/cli/utilities/json-schema.ts +++ b/packages/app/src/cli/utilities/json-schema.ts @@ -8,6 +8,7 @@ import { } from '@shopify/cli-kit/node/json-schema' import {isEmpty} from '@shopify/cli-kit/common/object' import {JsonMapType} from '@shopify/cli-kit/node/toml' +import {outputWarn} from '@shopify/cli-kit/node/output' /** * The base properties that are added to all JSON Schema contracts. @@ -36,27 +37,49 @@ export async function unifiedConfigurationParserFactory( handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties = 'strip', ) { const contractJsonSchema = validationSchema?.jsonSchema - if (contractJsonSchema === undefined || isEmpty(JSON.parse(contractJsonSchema))) { + if (contractJsonSchema === undefined) { return merged.parseConfigurationObject } - const contract = await normaliseJsonSchema(contractJsonSchema) + + let contract + try { + if (isEmpty(JSON.parse(contractJsonSchema))) { + return merged.parseConfigurationObject + } + contract = await normaliseJsonSchema(contractJsonSchema) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + warnRemoteContractValidationSkipped(merged.identifier) + return merged.parseConfigurationObject + } + contract.properties = {...JsonSchemaBaseProperties, ...contract.properties} const extensionIdentifier = merged.identifier + let remoteContractValidationEnabled = true const parseConfigurationObject = (config: object): ParseConfigurationResult => { // First we parse with zod. This may also change the format of the data. const zodParse = merged.parseConfigurationObject(config) + if (!remoteContractValidationEnabled) return zodParse // Then, even if this failed, we try to validate against the contract. const zodValidatedData = zodParse.state === 'ok' ? zodParse.data : undefined const subjectForAjv = zodValidatedData ?? (config as JsonMapType) - const jsonSchemaParse = jsonSchemaValidate( - subjectForAjv, - contract, - handleInvalidAdditionalProperties, - extensionIdentifier, - ) + let jsonSchemaParse: ReturnType + try { + jsonSchemaParse = jsonSchemaValidate( + subjectForAjv, + contract, + handleInvalidAdditionalProperties, + extensionIdentifier, + ) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + remoteContractValidationEnabled = false + warnRemoteContractValidationSkipped(extensionIdentifier) + return zodParse + } // Finally, we de-duplicate the error set from both validations -- identical messages for identical paths are removed let errors = zodParse.errors ?? [] @@ -88,3 +111,9 @@ export async function unifiedConfigurationParserFactory( } return parseConfigurationObject } + +export function warnRemoteContractValidationSkipped(identifier: string) { + outputWarn( + `Remote contract validation for "${identifier}" is skipped because its schema is invalid. Server-side validation remains the authority.`, + ) +}