JSON schema parsing strips extra properties

Author: shauns

packages/app/src/cli/services/generate/fetch-extension-specifications.ts

@@ -10,7 +10,7 @@ import {MinimalAppIdentifiers} from '../../models/organization.js'
 import {unifiedConfigurationParserFactory} 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'
+import {HandleInvalidAdditionalProperties, normaliseJsonSchema} from '@shopify/cli-kit/node/json-schema'
 
 interface FetchSpecificationsOptions {
   developerPlatformClient: DeveloperPlatformClient
@@ -75,7 +75,21 @@ async function mergeLocalAndRemoteSpecs(
     const merged = {...localSpec, ...remoteSpec, loadedRemoteSpecs: true} as RemoteAwareExtensionSpecification &
       FlattenedRemoteSpecification
 
-    const parseConfigurationObject = await unifiedConfigurationParserFactory(merged)
+    // If configuration is inside an app.toml -- i.e. single UID mode -- we must be able to parse a partial slice.
+    let handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties
+    switch (merged.uidStrategy) {
+      case 'uuid':
+        handleInvalidAdditionalProperties = 'fail'
+        break
+      case 'single':
+        handleInvalidAdditionalProperties = 'strip'
+        break
+      case 'dynamic':
+        handleInvalidAdditionalProperties = 'fail'
+        break
+    }
+
+    const parseConfigurationObject = await unifiedConfigurationParserFactory(merged, handleInvalidAdditionalProperties)
 
     return {
       ...merged,

packages/app/src/cli/utilities/json-schema.ts

@@ -2,7 +2,11 @@ import {FlattenedRemoteSpecification} from '../api/graphql/extension_specificati
 import {BaseConfigType} from '../models/extensions/schemas.js'
 import {configWithoutFirstClassFields, RemoteAwareExtensionSpecification} from '../models/extensions/specification.js'
 import {ParseConfigurationResult} from '@shopify/cli-kit/node/schema'
-import {jsonSchemaValidate, normaliseJsonSchema} from '@shopify/cli-kit/node/json-schema'
+import {
+  HandleInvalidAdditionalProperties,
+  jsonSchemaValidate,
+  normaliseJsonSchema,
+} from '@shopify/cli-kit/node/json-schema'
 import {isEmpty} from '@shopify/cli-kit/common/object'
 import {JsonMapType} from '@shopify/cli-kit/node/toml'
 
@@ -13,6 +17,7 @@ import {JsonMapType} from '@shopify/cli-kit/node/toml'
  */
 export async function unifiedConfigurationParserFactory(
   merged: RemoteAwareExtensionSpecification & FlattenedRemoteSpecification,
+  handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties = 'strip',
 ) {
   const contractJsonSchema = merged.validationSchema?.jsonSchema
   if (contractJsonSchema === undefined || isEmpty(JSON.parse(contractJsonSchema))) {
@@ -30,7 +35,12 @@ export async function unifiedConfigurationParserFactory(
     const subjectForAjv = zodValidatedData ?? (config as JsonMapType)
 
     const subjectForAjvWithoutFirstClassFields = configWithoutFirstClassFields(subjectForAjv)
-    const jsonSchemaParse = jsonSchemaValidate(subjectForAjvWithoutFirstClassFields, contract, extensionIdentifier)
+    const jsonSchemaParse = jsonSchemaValidate(
+      subjectForAjvWithoutFirstClassFields,
+      contract,
+      handleInvalidAdditionalProperties,
+      extensionIdentifier,
+    )
 
     // Finally, we de-duplicate the error set from both validations -- identical messages for identical paths are removed
     let errors = zodParse.errors || []
@@ -55,7 +65,7 @@ export async function unifiedConfigurationParserFactory(
     }
     return {
       state: 'ok',
-      data: zodValidatedData as BaseConfigType,
+      data: jsonSchemaParse.data as BaseConfigType,
       errors: undefined,
     }
   }

packages/cli-kit/src/public/node/json-schema.test.ts

@@ -52,7 +52,7 @@ describe('jsonSchemaValidate', () => {
       zod.object({foo: zod.number().max(99)}),
       {foo: 100},
     ],
-  ])('matches the zod behaviour for %s', (name, contract, zodVersion, subject) => {
+  ])('matches the zod behaviour for %s', (_name, contract, zodVersion, subject) => {
     const zodParsed = zodVersion.safeParse(subject)
     expect(zodParsed.success).toBe(false)
     if (zodParsed.success) {
@@ -61,7 +61,7 @@ describe('jsonSchemaValidate', () => {
 
     const zodErrors = zodParsed.error.errors.map((error) => ({path: error.path, message: error.message}))
 
-    const schemaParsed = jsonSchemaValidate(subject, contract, `test-${name}`)
+    const schemaParsed = jsonSchemaValidate(subject, contract, 'strip')
     expect(schemaParsed.state).toBe('error')
     expect(schemaParsed.errors, `Converting ${JSON.stringify(schemaParsed.rawErrors)}`).toEqual(zodErrors)
   })
@@ -77,10 +77,39 @@ describe('jsonSchemaValidate', () => {
       },
       'x-taplo': {foo: 'bar'},
     }
-    const schemaParsed = jsonSchemaValidate(subject, contract, 'test2')
+    const schemaParsed = jsonSchemaValidate(subject, contract, 'strip')
     expect(schemaParsed.state).toBe('ok')
   })
 
+  test('removes additional properties', () => {
+    const subject = {
+      foo: 'bar',
+      baz: 'qux',
+    }
+    const contract = {additionalProperties: false, type: 'object', properties: {foo: {type: 'string'}}}
+    const schemaParsed = jsonSchemaValidate(subject, contract, 'strip')
+    expect(schemaParsed.state).toBe('ok')
+    expect(schemaParsed.data).toEqual({foo: 'bar'})
+    // confirm we don't mutate input parameters
+    expect(schemaParsed.data).not.toEqual(subject)
+  })
+
+  test('fails on additional properties', () => {
+    const subject = {
+      foo: 'bar',
+      baz: 'qux',
+    }
+    const contract = {additionalProperties: false, type: 'object', properties: {foo: {type: 'string'}}}
+    const schemaParsed = jsonSchemaValidate(subject, contract, 'fail')
+    expect(schemaParsed.state).toBe('error')
+    expect(schemaParsed.errors).toEqual([
+      {
+        path: [],
+        message: 'must NOT have additional properties',
+      },
+    ])
+  })
+
   test('deals with a union mismatch with a preferred branch', () => {
     const subject = {
       root: {
@@ -121,7 +150,7 @@ describe('jsonSchemaValidate', () => {
         },
       },
     }
-    const schemaParsed = jsonSchemaValidate(subject, contract, 'test3')
+    const schemaParsed = jsonSchemaValidate(subject, contract, 'strip')
     expect(schemaParsed.state).toBe('error')
     expect(schemaParsed.errors).toEqual([
       {
@@ -175,7 +204,7 @@ describe('jsonSchemaValidate', () => {
         },
       },
     }
-    const schemaParsed = jsonSchemaValidate(subject, contract, 'test4')
+    const schemaParsed = jsonSchemaValidate(subject, contract, 'fail')
     expect(schemaParsed.state).toBe('error')
     expect(schemaParsed.errors).toEqual([
       {

packages/cli-kit/src/public/node/json-schema.ts

@@ -1,9 +1,13 @@
 /* eslint-disable @typescript-eslint/no-non-null-assertion */
 import {ParseConfigurationResult} from './schema.js'
+import {randomUUID} from './crypto.js'
 import {getPathValue} from '../common/object.js'
 import {capitalize} from '../common/string.js'
 import {Ajv, ErrorObject, SchemaObject, ValidateFunction} from 'ajv'
 import $RefParser from '@apidevtools/json-schema-ref-parser'
+import cloneDeep from 'lodash/cloneDeep.js'
+
+export type HandleInvalidAdditionalProperties = 'strip' | 'fail'
 
 type AjvError = ErrorObject<string, {[key: string]: unknown}>
 
@@ -22,6 +26,29 @@ export async function normaliseJsonSchema(schema: string): Promise<SchemaObject>
   return parsedSchema
 }
 
+function createAjvValidator(
+  handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties,
+  schema: SchemaObject,
+) {
+  // allowUnionTypes: Allows types like `type: ["string", "number"]`
+  // removeAdditional: Removes extraneous properties from the subject if `additionalProperties: false` is specified
+  let removeAdditional
+  switch (handleInvalidAdditionalProperties) {
+    case 'strip':
+      removeAdditional = true
+      break
+    case 'fail':
+      removeAdditional = undefined
+      break
+  }
+  const ajv = new Ajv({allowUnionTypes: true, removeAdditional})
+  ajv.addKeyword('x-taplo')
+
+  const validator = ajv.compile(schema)
+
+  return validator
+}
+
 const validatorsCache = new Map<string, ValidateFunction>()
 
 /**
@@ -31,27 +58,29 @@ const validatorsCache = new Map<string, ValidateFunction>()
  *
  * @param subject - The object to validate.
  * @param schema - The JSON schema to validate against.
+ * @param handleInvalidAdditionalProperties - Whether to strip or fail on invalid additional properties.
  * @param identifier - The identifier of the schema being validated, used to cache the validator.
  * @returns The result of the validation. If the state is 'error', the errors will be in a zod-like format.
  */
 export function jsonSchemaValidate(
   subject: object,
   schema: SchemaObject,
-  identifier: string,
+  handleInvalidAdditionalProperties: HandleInvalidAdditionalProperties,
+  identifier?: string,
 ): ParseConfigurationResult<unknown> & {rawErrors?: AjvError[]} {
-  const ajv = new Ajv({allowUnionTypes: true})
+  const subjectToModify = cloneDeep(subject)
 
-  ajv.addKeyword('x-taplo')
+  const cacheKey = identifier ?? randomUUID()
 
-  const validator = validatorsCache.get(identifier) ?? ajv.compile(schema)
-  validatorsCache.set(identifier, validator)
+  const validator = validatorsCache.get(cacheKey) ?? createAjvValidator(handleInvalidAdditionalProperties, schema)
+  validatorsCache.set(cacheKey, validator)
 
-  validator(subject)
+  validator(subjectToModify)
 
   // Errors from the contract are post-processed to be more zod-like and to deal with unions better
   let jsonSchemaErrors
   if (validator.errors && validator.errors.length > 0) {
-    jsonSchemaErrors = convertJsonSchemaErrors(validator.errors, subject, schema)
+    jsonSchemaErrors = convertJsonSchemaErrors(validator.errors, subjectToModify, schema)
     return {
       state: 'error',
       data: undefined,
@@ -61,7 +90,7 @@ export function jsonSchemaValidate(
   }
   return {
     state: 'ok',
-    data: subject,
+    data: subjectToModify,
     errors: undefined,
     rawErrors: undefined,
   }
@@ -162,8 +191,6 @@ function convertJsonSchemaErrors(rawErrors: AjvError[], subject: object, schema:
  * @returns A simplified list of errors.
  */
 function simplifyUnionErrors(rawErrors: AjvError[], subject: object, schema: SchemaObject): AjvError[] {
-  const ajv = new Ajv({allowUnionTypes: true})
-  ajv.addKeyword('x-taplo')
   let errors = rawErrors
 
   const resolvedUnionErrors = new Set()
@@ -191,7 +218,7 @@ function simplifyUnionErrors(rawErrors: AjvError[], subject: object, schema: Sch
       // we know that none of the union schemas are correct, but for each of them we can measure how wrong they are
       const correctValuesAndErrors = unionSchemas
         .map((candidateSchemaFromUnion: SchemaObject) => {
-          const candidateSchemaValidator = ajv.compile(candidateSchemaFromUnion)
+          const candidateSchemaValidator = createAjvValidator('fail', candidateSchemaFromUnion)
           candidateSchemaValidator(subjectValue)
 
           let score = 0
@@ -202,7 +229,7 @@ function simplifyUnionErrors(rawErrors: AjvError[], subject: object, schema: Sch
               const subSchema = candidateSchemaFromUnion.properties[propertyName] as SchemaObject
               const subjectValueSlice = getPathValue(subjectValue, propertyName)
 
-              const subValidator = ajv.compile(subSchema)
+              const subValidator = createAjvValidator('fail', subSchema)
               if (subValidator(subjectValueSlice)) {
                 return acc + 1
               }