[Feature] Relative url patching on contract app extensions

.changeset/flow-trigger-lifecycle-callback-relative-url.md

@@ -0,0 +1,5 @@
+---
+'@shopify/app': minor
+---
+
+Allow Flow trigger lifecycle callback `url`s to be relative to the app's `application_url`

packages/app/src/cli/models/app/app.test.ts

@@ -17,12 +17,16 @@ import {
   testAppAccessConfigExtension,
   testAppHomeConfigExtension,
   testAppProxyConfigExtension,
+  testDeveloperPlatformClient,
+  testOrganizationApp,
 } from './app.test-data.js'
 import {ExtensionInstance} from '../extensions/extension-instance.js'
 import {FunctionConfigType} from '../extensions/specifications/function.js'
 import {WebhooksConfig} from '../extensions/specifications/types/app_config_webhook.js'
 import {EditorExtensionCollectionType} from '../extensions/specifications/editor_extension_collection.js'
 import {ApplicationURLs} from '../../services/dev/urls.js'
+import {RemoteSpecification} from '../../api/graphql/extension_specifications.js'
+import {fetchSpecifications} from '../../services/generate/fetch-extension-specifications.js'
 import {describe, expect, test, vi} from 'vitest'
 import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs'
 import {joinPath} from '@shopify/cli-kit/node/path'
@@ -779,6 +783,156 @@ describe('manifest', () => {
       ],
     })
   })
+
+  test.each([
+    {mode: 'deploy', devApplicationURLs: undefined, expectedAppUrl: 'https://my-app.example.com'},
+    {
+      mode: 'dev',
+      devApplicationURLs: {applicationUrl: 'https://my-tunnel.example.com', redirectUrlWhitelist: []},
+      expectedAppUrl: 'https://my-tunnel.example.com',
+    },
+  ])('preserves admin links alongside Flow extensions during $mode', async ({devApplicationURLs, expectedAppUrl}) => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      const adminLinkRemoteSpec: RemoteSpecification = {
+        name: 'Admin link',
+        externalName: 'Admin link',
+        identifier: 'admin_link',
+        externalIdentifier: 'admin_link',
+        experience: 'extension',
+        managementExperience: 'cli',
+        gated: false,
+        registrationLimit: 1,
+        uidStrategy: 'uuid',
+        validationSchema: {
+          jsonSchema: JSON.stringify({
+            type: 'object',
+            properties: {
+              name: {type: 'string'},
+              targeting: {
+                type: 'array',
+                items: {
+                  type: 'object',
+                  properties: {target: {type: 'string'}, url: {type: 'string'}},
+                  required: ['target', 'url'],
+                  additionalProperties: false,
+                },
+              },
+              localization: {type: 'object'},
+            },
+            required: ['targeting'],
+            additionalProperties: false,
+          }),
+        },
+      }
+      const lifecycleCallbackRemoteSpec: RemoteSpecification = {
+        name: 'Flow trigger lifecycle callback',
+        externalName: 'Flow trigger lifecycle callback',
+        identifier: 'flow_trigger_lifecycle_callback',
+        externalIdentifier: 'flow_trigger_lifecycle_callback',
+        experience: 'extension',
+        managementExperience: 'cli',
+        gated: false,
+        registrationLimit: 1,
+        uidStrategy: 'uuid',
+        validationSchema: {
+          jsonSchema: JSON.stringify({
+            type: 'object',
+            properties: {
+              name: {type: 'string'},
+              url: {type: 'string', pattern: '^(https://|/[^/])'},
+            },
+            required: ['url'],
+            additionalProperties: false,
+          }),
+        },
+      }
+      const remoteApp = testOrganizationApp()
+      const remoteSpecs = await testDeveloperPlatformClient().specifications(remoteApp)
+      const specifications = await fetchSpecifications({
+        developerPlatformClient: testDeveloperPlatformClient({
+          specifications: async () => [...remoteSpecs, adminLinkRemoteSpec, lifecycleCallbackRemoteSpec],
+        }),
+        app: remoteApp,
+      })
+      const locale = JSON.stringify({title: 'Open app'})
+      await mkdir(joinPath(tmpDir, 'admin_link', 'locales'))
+      await writeFile(joinPath(tmpDir, 'admin_link', 'locales', 'en.default.json'), locale)
+
+      const configurations = [
+        {
+          type: 'admin_link',
+          handle: 'admin-link',
+          name: 'Admin link',
+          targeting: [{target: 'admin.product-details.action.link', url: '/products'}],
+        },
+        {
+          type: 'flow_action',
+          handle: 'flow-action',
+          name: 'Flow action',
+          runtime_url: '/execute',
+          validation_url: 'https://validation.example.com/validate',
+        },
+        {type: 'flow_trigger', handle: 'flow-trigger', name: 'Flow trigger'},
+        {
+          type: 'flow_trigger_lifecycle_callback',
+          handle: 'lifecycle-callback',
+          name: 'Lifecycle callback',
+          url: '/callback',
+        },
+        {type: 'app_home', application_url: 'https://my-app.example.com', embedded: true},
+      ]
+      const extensions = await Promise.all(
+        configurations.map(async (configuration) => {
+          const specification = specifications.find((spec) => spec.identifier === configuration.type)!
+          const parsed = specification.parseConfigurationObject(configuration)
+          if (parsed.state !== 'ok') throw new Error(`Couldn't parse ${configuration.type} configuration`)
+
+          const directory = joinPath(tmpDir, configuration.type)
+          await mkdir(directory)
+          return new ExtensionInstance({
+            configuration: parsed.data,
+            directory,
+            specification,
+            configurationPath: joinPath(directory, 'shopify.extension.toml'),
+            entryPath: '',
+          })
+        }),
+      )
+      const app = testApp({
+        directory: tmpDir,
+        allExtensions: extensions,
+        configuration: {...DEFAULT_CONFIG, application_url: 'https://my-app.example.com'},
+        devApplicationURLs,
+      })
+
+      const manifest = await app.manifest(undefined)
+
+      expect(manifest.modules).toMatchObject([
+        {
+          type: 'admin_link',
+          config: {
+            name: 'Admin link',
+            targeting: [{target: 'admin.product-details.action.link', url: '/products'}],
+            localization: {default_locale: 'en', translations: {en: Buffer.from(locale).toString('base64')}},
+          },
+        },
+        {
+          type: 'flow_action',
+          config: {
+            title: 'Flow action',
+            url: `${expectedAppUrl}/execute`,
+            validation_url: 'https://validation.example.com/validate',
+          },
+        },
+        {type: 'flow_trigger', config: {title: 'Flow trigger', fields: [], schema_patch: ''}},
+        {
+          type: 'flow_trigger_lifecycle_callback',
+          config: {name: 'Lifecycle callback', url: `${expectedAppUrl}/callback`},
+        },
+        {type: 'app_home', config: {app_url: expectedAppUrl, embedded: true}},
+      ])
+    })
+  })
 })
 
 describe('generateExtensionTypes', () => {

packages/app/src/cli/models/app/validation/common.ts

@@ -10,6 +10,16 @@ export function validateRelativeUrl(zodType: zod.ZodString, {message = 'URL must
   return zodType.refine((value) => value.startsWith('/') || isValidUrl(value, true), {message})
 }
 
+/**
+ * Characters that are never legal in a URL, and that would let a malformed configuration value smuggle extra content
+ * into a request when the URL is later interpolated.
+ */
+export const URL_CONTROL_CHARACTERS = /[\r\n\t]/
+
+export function isHttpsUrl(url: string): boolean {
+  return isValidUrl(url, true)
+}
+
 function isValidUrl(input: string, httpsOnly: boolean) {
   try {
     const url = new URL(input)

packages/app/src/cli/models/extensions/extension-instance.test.ts

@@ -911,7 +911,13 @@ describe('getDevSessionUpdateMessages', () => {
         .map((specification) => specification.identifier)
         .sort()
 
-      expect(matching).toEqual(['editor_extension_collection', 'flow_action', 'flow_trigger', 'payments_extension'])
+      expect(matching).toEqual([
+        'editor_extension_collection',
+        'flow_action',
+        'flow_trigger',
+        'flow_trigger_lifecycle_callback',
+        'payments_extension',
+      ])
     })
   })
 })

packages/app/src/cli/models/extensions/load-specifications.ts

@@ -17,6 +17,7 @@ import checkoutSpec from './specifications/checkout_ui_extension.js'
 import flowActionSpecification from './specifications/flow_action.js'
 import flowTemplateSpec from './specifications/flow_template.js'
 import flowTriggerSpecification from './specifications/flow_trigger.js'
+import flowTriggerLifecycleCallbackSpec from './specifications/flow_trigger_lifecycle_callback.js'
 import functionSpec from './specifications/function.js'
 import paymentExtensionSpec from './specifications/payments_app_extension.js'
 import posUISpec from './specifications/pos_ui_extension.js'
@@ -70,6 +71,7 @@ function loadSpecifications() {
     flowActionSpecification,
     flowTemplateSpec,
     flowTriggerSpecification,
+    flowTriggerLifecycleCallbackSpec,
     functionSpec,
     paymentExtensionSpec,
     posUISpec,

packages/app/src/cli/models/extensions/specification.integration.test.ts

@@ -5,9 +5,11 @@ import {
   createConfigExtensionSpecification,
   createExtensionSpecification,
 } from './specification.js'
-import {BaseSchema} from './schemas.js'
+import {BaseConfigType, BaseSchema} from './schemas.js'
+import {placeholderAppConfiguration} from '../app/app.test-data.js'
 import {ClientSteps} from '../../services/build/client-steps.js'
 import {AppSchema} from '../app/app.js'
+import {AbortError} from '@shopify/cli-kit/node/error'
 import {describe, test, expect, beforeAll} from 'vitest'
 
 // If the AppSchema is not instanced, the dynamic loading of loadLocalExtensionsSpecifications is not working
@@ -95,6 +97,89 @@ describe('createContractBasedModuleSpecification', () => {
     // Then
     expect(got.clientSteps).toBeUndefined()
   })
+
+  describe('app relative URLs', () => {
+    interface CallbackConfig extends BaseConfigType {
+      url: string
+      other_url?: string
+    }
+
+    const callbackSpec = () =>
+      createContractBasedModuleSpecification<CallbackConfig>({
+        identifier: 'test_callback',
+        uidStrategy: 'uuid',
+        experience: 'extension',
+        appModuleFeatures: () => [],
+        appRelativeUrlFields: ['url'],
+      })
+
+    test('resolves declared deployment URLs without mutating the original configuration', async () => {
+      const spec = callbackSpec()
+      const config = {type: 'test_callback', url: '/callback', other_url: '/leave-alone'}
+
+      const got = await spec.deployConfig!(config, './my-extension', 'api-key', undefined, {
+        appConfiguration: {...placeholderAppConfiguration, application_url: 'https://my-app.example.com'},
+      })
+
+      expect(got).toEqual({url: 'https://my-app.example.com/callback', other_url: '/leave-alone'})
+      expect(config).toEqual({type: 'test_callback', url: '/callback', other_url: '/leave-alone'})
+    })
+
+    test('leaves an absolute deployment URL untouched without app configuration', async () => {
+      const spec = callbackSpec()
+
+      const got = await spec.deployConfig!(
+        {type: 'test_callback', url: 'https://my-prod-host.example.com/callback'},
+        './my-extension',
+        'api-key',
+      )
+
+      expect(got).toEqual({url: 'https://my-prod-host.example.com/callback'})
+    })
+
+    test('resolves declared URLs against the dev tunnel URL', () => {
+      const spec = callbackSpec()
+      const config = {type: 'test_callback', url: '/callback', other_url: '/leave-alone'}
+
+      spec.patchWithAppDevURLs!(config, {applicationUrl: 'https://my-tunnel.example.com', redirectUrlWhitelist: []})
+
+      expect(config).toEqual({
+        type: 'test_callback',
+        url: 'https://my-tunnel.example.com/callback',
+        other_url: '/leave-alone',
+      })
+    })
+
+    test.each([
+      {appRelativeUrlFields: undefined, description: 'omitted'},
+      {appRelativeUrlFields: [], description: 'empty'},
+    ])('does not opt in by identifier when URL fields are $description', async ({appRelativeUrlFields}) => {
+      const spec = createContractBasedModuleSpecification<CallbackConfig>({
+        identifier: 'flow_trigger_lifecycle_callback',
+        uidStrategy: 'uuid',
+        experience: 'extension',
+        appModuleFeatures: () => [],
+        appRelativeUrlFields,
+      })
+      const config = {type: 'flow_trigger_lifecycle_callback', url: '/callback'}
+
+      spec.patchWithAppDevURLs?.(config, {applicationUrl: 'https://my-tunnel.example.com', redirectUrlWhitelist: []})
+      const got = await spec.deployConfig!(config, './my-extension', 'api-key', undefined, {
+        appConfiguration: {...placeholderAppConfiguration, application_url: 'https://my-app.example.com'},
+      })
+
+      expect(config).toEqual({type: 'flow_trigger_lifecycle_callback', url: '/callback'})
+      expect(got).toEqual({url: '/callback'})
+    })
+
+    test('rejects a relative deployment URL without app configuration', async () => {
+      const spec = callbackSpec()
+
+      await expect(
+        spec.deployConfig!({type: 'test_callback', url: '/callback'}, './my-extension', 'api-key'),
+      ).rejects.toThrow(AbortError)
+    })
+  })
 })
 
 describe('createExtensionSpecification', () => {

packages/app/src/cli/models/extensions/specification.ts

@@ -1,5 +1,6 @@
 import {ZodSchemaType, BaseConfigType, BaseSchema} from './schemas.js'
 import {ExtensionInstance} from './extension-instance.js'
+import {patchAppRelativeUrls} from './specifications/validation/app_relative_urls.js'
 import {blocks} from '../../constants.js'
 import {ClientSteps} from '../../services/build/client-steps.js'
 
@@ -98,6 +99,8 @@ export interface ExtensionSpecification<TConfiguration extends BaseConfigType =
   hasExtensionPointTarget?(config: TConfiguration, target: string): boolean
   appModuleFeatures: (config?: TConfiguration) => ExtensionFeature[]
   getDevSessionUpdateMessages?: (config: TConfiguration, context: DevSessionUpdateContext) => Promise<string[]>
+  /** Top-level URL fields to resolve against the app URL. The remote contract must accept relative values. */
+  appRelativeUrlFields?: ReadonlyArray<keyof TConfiguration & string>
   patchWithAppDevURLs?: (config: TConfiguration, urls: ApplicationURLs) => void
 
   /**
@@ -302,13 +305,16 @@ export function createContractBasedModuleSpecification<TConfiguration extends Ba
     CreateExtensionSpecType<TConfiguration>,
     | 'identifier'
     | 'appModuleFeatures'
+    | 'appRelativeUrlFields'
     | 'uidStrategy'
     | 'clientSteps'
     | 'experience'
     | 'transformRemoteToLocal'
     | 'devSessionWatchConfig'
   >,
 ) {
+  const appRelativeUrlFields = spec.appRelativeUrlFields
+
   return createExtensionSpecification({
     identifier: spec.identifier,
     schema: zod.any({}) as unknown as ZodSchemaType<TConfiguration>,
@@ -318,8 +324,25 @@ export function createContractBasedModuleSpecification<TConfiguration extends Ba
     uidStrategy: spec.uidStrategy,
     transformRemoteToLocal: spec.transformRemoteToLocal,
     devSessionWatchConfig: spec.devSessionWatchConfig,
-    deployConfig: async (config, directory) => {
+    appRelativeUrlFields,
+    patchWithAppDevURLs: appRelativeUrlFields?.length
+      ? (config, urls) => {
+          patchAppRelativeUrls(
+            capitalize(spec.identifier.replace(/_/g, ' ')),
+            appRelativeUrlFields,
+            config,
+            urls.applicationUrl,
+          )
+        }
+      : undefined,
+    deployConfig: async (config, directory, _apiKey, _moduleId, context) => {
+      // configWithoutFirstClassFields returns a fresh object, so patching it in place cannot affect the caller.
       let parsedConfig = configWithoutFirstClassFields(config)
+      if (appRelativeUrlFields?.length) {
+        const applicationUrl = context?.appConfiguration?.application_url
+        const appUrl = typeof applicationUrl === 'string' ? applicationUrl : undefined
+        patchAppRelativeUrls(capitalize(spec.identifier.replace(/_/g, ' ')), appRelativeUrlFields, parsedConfig, appUrl)
+      }
       if (spec.appModuleFeatures().includes('localization')) {
         const localization = await loadLocalesConfig(directory, spec.identifier)
         parsedConfig = {...parsedConfig, localization}