Simplify theme capabilities and reject encoded IP origins

Author: isaacroldan

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

@@ -1,4 +1,4 @@
-import {BaseConfigType, MAX_EXTENSION_HANDLE_LENGTH, MAX_UID_LENGTH} from './schemas.js'
+import {BaseConfigType, BaseExtensionConfigType, MAX_EXTENSION_HANDLE_LENGTH, MAX_UID_LENGTH} from './schemas.js'
 import {FunctionConfigType} from './specifications/function.js'
 import {
   DevSessionUpdateContext,
@@ -58,7 +58,7 @@ export const DEFAULT_DEV_SESSION_UPDATE_MESSAGE = 'Configuration accepted'
  *
  * This class holds the public interface to interact with extensions
  */
-export class ExtensionInstance<TConfiguration extends BaseConfigType = BaseConfigType> {
+export class ExtensionInstance<TConfiguration extends BaseExtensionConfigType = BaseConfigType> {
   entrySourceFilePath: string
   devUUID: string
   localIdentifier: string

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

@@ -21,11 +21,8 @@ const IframeCapabilitySchema = zod.object({
   sources: zod.array(zod.string()).optional(),
 })
 
-export const CapabilitiesSchema = zod.object({
-  // Theme app extensions declare `network_access` as a table that lists the origins they contact (see the theme
-  // specification). Every other extension declares a boolean, which is all this shared schema accepts. The wider
-  // type lets the theme specification reuse `BaseConfigType`.
-  network_access: zod.boolean().optional() as ZodSchemaType<boolean | {allowed_sources: string[]} | undefined>,
+const CapabilitiesSchema = zod.object({
+  network_access: zod.boolean().optional(),
   block_progress: zod.boolean().optional(),
   api_access: zod.boolean().optional(),
   collect_buyer_consent: CollectBuyerConsentCapabilitySchema.optional(),
@@ -157,5 +154,7 @@ export const UnifiedSchema = zod.object({
 
 export type NewExtensionPointSchemaType = zod.infer<typeof NewExtensionPointSchema>
 
-// Base config type that all config schemas must extend.
 export type BaseConfigType = zod.infer<typeof BaseSchema>
+
+// Base config type that all config schemas must extend.
+export type BaseExtensionConfigType = Omit<BaseConfigType, 'capabilities'>

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

@@ -1,4 +1,4 @@
-import {ZodSchemaType, BaseConfigType, BaseSchema} from './schemas.js'
+import {ZodSchemaType, BaseConfigType, BaseExtensionConfigType, BaseSchema} from './schemas.js'
 import {ExtensionInstance} from './extension-instance.js'
 import {patchAppRelativeUrls} from './specifications/validation/app_relative_urls.js'
 import {blocks} from '../../constants.js'
@@ -81,7 +81,7 @@ export interface DevSessionUpdateContext {
 /**
  * Extension specification with all the needed properties and methods to load an extension.
  */
-export interface ExtensionSpecification<TConfiguration extends BaseConfigType = BaseConfigType> {
+export interface ExtensionSpecification<TConfiguration extends BaseExtensionConfigType = BaseConfigType> {
   identifier: string
   externalIdentifier: string
   externalName: string
@@ -179,7 +179,7 @@ export interface DevSessionWatchConfig {
 /**
  * Extension specification, explicitly marked as having taken remote configuration values into account.
  */
-export type RemoteAwareExtensionSpecification<TConfiguration extends BaseConfigType = BaseConfigType> =
+export type RemoteAwareExtensionSpecification<TConfiguration extends BaseExtensionConfigType = BaseConfigType> =
   ExtensionSpecification<TConfiguration> & {
     loadedRemoteSpecs: true
   }
@@ -202,7 +202,7 @@ type ForbiddenFields =
 /**
  * Partial ExtensionSpec type used when creating a new ExtensionSpec, the only mandatory field is the identifier
  */
-interface CreateExtensionSpecType<TConfiguration extends BaseConfigType = BaseConfigType> extends Partial<
+interface CreateExtensionSpecType<TConfiguration extends BaseExtensionConfigType = BaseConfigType> extends Partial<
   Omit<ExtensionSpecification<TConfiguration>, ForbiddenFields>
 > {
   identifier: string
@@ -229,9 +229,10 @@ interface CreateExtensionSpecType<TConfiguration extends BaseConfigType = BaseCo
  * hasExtensionPointTarget?: (configuration: TConfiguration, target: string) => boolean // function to determine if the extension has a given extension point target
  * ```
  */
-export function createExtensionSpecification<TConfiguration extends BaseConfigType = BaseConfigType>(
+export function createExtensionSpecification<TConfiguration extends BaseExtensionConfigType = BaseConfigType>(
   spec: CreateExtensionSpecType<TConfiguration>,
 ): ExtensionSpecification<TConfiguration> {
+  const baseSchema: ZodSchemaType<BaseExtensionConfigType> = BaseSchema
   const defaults = {
     // these two fields are going to be overridden by the extension specification API response,
     // but we need them to have a default value for tests
@@ -240,7 +241,7 @@ export function createExtensionSpecification<TConfiguration extends BaseConfigTy
     externalName: capitalize(spec.identifier.replace(/_/g, ' ')),
     surface: 'test-surface',
     partnersWebIdentifier: spec.identifier,
-    schema: BaseSchema as ZodSchemaType<TConfiguration>,
+    schema: baseSchema as ZodSchemaType<TConfiguration>,
     registrationLimit: blocks.extensions.defaultRegistrationLimit,
     transform: spec.transformLocalToRemote,
     reverseTransform: spec.transformRemoteToLocal,
@@ -292,7 +293,9 @@ export function createExtensionSpecification<TConfiguration extends BaseConfigTy
  *   - App config extensions have default transform and reverseTransform functions
 
  */
-export function createConfigExtensionSpecification<TConfiguration extends BaseConfigType = BaseConfigType>(spec: {
+export function createConfigExtensionSpecification<
+  TConfiguration extends BaseExtensionConfigType = BaseConfigType,
+>(spec: {
   identifier: string
   schema: ZodSchemaType<TConfiguration>
   clientSteps?: ClientSteps
@@ -325,7 +328,7 @@ export function createConfigExtensionSpecification<TConfiguration extends BaseCo
   })
 }
 
-export function createContractBasedModuleSpecification<TConfiguration extends BaseConfigType = BaseConfigType>(
+export function createContractBasedModuleSpecification<TConfiguration extends BaseExtensionConfigType = BaseConfigType>(
   spec: Pick<
     CreateExtensionSpecType<TConfiguration>,
     | 'identifier'

packages/app/src/cli/models/extensions/specifications/theme.test.ts

@@ -52,11 +52,20 @@ describe('theme', () => {
       expect(parse({}).data?.capabilities?.network_access).toBeUndefined()
     })
 
-    test('ignores a boolean network_access, as before', () => {
-      const result = parse({network_access: true})
+    test.each([true, false])('rejects a boolean network_access of %s', (networkAccess) => {
+      const result = parse({network_access: networkAccess})
+
+      expect(result.state).toBe('error')
+      expect(errorMessages(result)).toEqual([
+        'network_access must be a table with an allowed_sources list for theme app extensions',
+      ])
+    })
+
+    test('ignores capabilities that do not apply to theme extensions', () => {
+      const result = parse({network_access: {allowed_sources: []}, api_access: true, block_progress: true})
 
       expect(result.state).toBe('ok')
-      expect(result.data?.capabilities?.network_access).toBeUndefined()
+      expect(result.data?.capabilities).toEqual({network_access: {allowed_sources: []}})
     })
 
     test('requires allowed_sources when the network_access table is declared', () => {
@@ -77,7 +86,7 @@ describe('theme', () => {
       ])
     })
 
-    test('rejects a network_access value that is neither a table nor a boolean', () => {
+    test('rejects a network_access value that is not a table', () => {
       const result = parse({network_access: 'https://api.example.com'})
 
       expect(result.state).toBe('error')
@@ -99,6 +108,13 @@ describe('theme', () => {
       ['a wildcard that is not the leading label', 'https://api.*.example.com'],
       ['a single-label host', 'https://localhost'],
       ['an IP address', 'https://127.0.0.1'],
+      ['an IP address with a hexadecimal final label', 'https://127.0.0x1'],
+      ['an IP address in hexadecimal notation', 'https://0x7f.0x1'],
+      ['a link-local IP address in hexadecimal notation', 'https://0xa9.0xfe.0xa9.0xfe'],
+      ['a wildcard on an IP address', 'https://*.127.0.0.1'],
+      ['a wildcard on an IP address in hexadecimal notation', 'https://*.0x7f.0x1'],
+      ['a host that cannot be parsed as a URL', 'https://example.0xabc'],
+      ['a wildcard on a host that cannot be parsed as a URL', 'https://*.example.0xabc'],
       ['an invalid label', 'https://-api.example.com'],
       ['an empty label', 'https://api..example.com'],
       ['a trailing dot', 'https://api.example.com./'],
@@ -149,8 +165,8 @@ describe('theme', () => {
       await expect(themeSpec.deployConfig?.(config, '', '', undefined)).resolves.toEqual({theme_extension: {files: {}}})
     })
 
-    test('sends only the theme extension files when network_access is a boolean', async () => {
-      const config = parse({network_access: true, block_progress: true}).data!
+    test('sends only the theme extension files when other capabilities are declared', async () => {
+      const config = parse({api_access: true, block_progress: true}).data!
 
       await expect(themeSpec.deployConfig?.(config, '', '', undefined)).resolves.toEqual({theme_extension: {files: {}}})
     })
@@ -175,16 +191,16 @@ describe('theme', () => {
   })
 
   describe('UI extensions', () => {
-    test('still accept a boolean network_access', () => {
+    test.each([true, false])('still accept a boolean network_access of %s', (networkAccess) => {
       const result = UIExtensionSchema.safeParse({
         name: 'ui extension',
         type: 'ui_extension',
         extension_points: [],
-        capabilities: {network_access: true},
+        capabilities: {network_access: networkAccess},
       })
 
       expect(result.success).toBe(true)
-      expect(result.data?.capabilities?.network_access).toBe(true)
+      expect(result.data?.capabilities?.network_access).toBe(networkAccess)
     })
 
     test('reject a network_access table', () => {

packages/app/src/cli/models/extensions/specifications/theme.ts

@@ -1,12 +1,14 @@
 import {createExtensionSpecification} from '../specification.js'
-import {BaseSchema, CapabilitiesSchema} from '../schemas.js'
+import {BaseSchema} from '../schemas.js'
 import {themeExtensionFiles} from '../../../utilities/extensions/theme.js'
 import {ExtensionInstance} from '../extension-instance.js'
 import {fileSize} from '@shopify/cli-kit/node/fs'
 import {dirname, relativePath} from '@shopify/cli-kit/node/path'
 import {AbortError} from '@shopify/cli-kit/node/error'
 import {outputContent, outputToken} from '@shopify/cli-kit/node/output'
 import {zod} from '@shopify/cli-kit/node/schema'
+import {safeParseURL} from '@shopify/cli-kit/common/url'
+import {isIP} from 'node:net'
 
 export const MAX_ALLOWED_SOURCES = 20
 const MAX_HOST_LENGTH = 253
@@ -34,8 +36,8 @@ export function normalizeAllowedSource(source: string): string | undefined {
 
   const labels = host.split('.')
   if (labels.length < 2 || !labels.every((label) => HOST_LABEL_PATTERN.test(label))) return undefined
-  // A numeric last label means an IP address (or a TLD that doesn't exist).
-  if (/^\d+$/.test(labels[labels.length - 1]!)) return undefined
+  const parsedOrigin = safeParseURL(`https://${host}`)
+  if (!parsedOrigin || isIP(parsedOrigin.hostname) !== 0) return undefined
 
   let portSuffix = ''
   if (port !== undefined) {
@@ -71,12 +73,8 @@ const ThemeNetworkAccessSchema = zod.object(
   {invalid_type_error: 'network_access must be a table with an allowed_sources list for theme app extensions'},
 )
 
-const ThemeCapabilitiesSchema = CapabilitiesSchema.extend({
-  // A boolean `network_access` (used by UI extensions) never had an effect on theme app extensions. Keep ignoring it.
-  network_access: zod.preprocess(
-    (value) => (typeof value === 'boolean' ? undefined : value),
-    ThemeNetworkAccessSchema.optional(),
-  ),
+const ThemeCapabilitiesSchema = zod.object({
+  network_access: ThemeNetworkAccessSchema.optional(),
 })
 
 const ThemeSchema = BaseSchema.extend({
@@ -103,7 +101,7 @@ const themeSpec = createExtensionSpecification({
   deployConfig: async (config) => {
     const networkAccess = config.capabilities?.network_access
     // Only send capabilities when they're declared, so existing payloads stay the same.
-    if (typeof networkAccess !== 'object') return {theme_extension: {files: {}}}
+    if (!networkAccess) return {theme_extension: {files: {}}}
 
     return {
       theme_extension: {files: {}},
@@ -155,7 +153,7 @@ const SUPPORTED_EXTS: {[dirname: string]: FilenameValidation} = {
 }
 const SUPPORTED_BUCKETS = Object.keys(SUPPORTED_EXTS)
 
-async function validateThemeExtension(extension: ExtensionInstance): Promise<void> {
+async function validateThemeExtension(extension: Pick<ExtensionInstance, 'directory'>): Promise<void> {
   const themeFiles = await themeExtensionFiles(extension)
   const liquidBytes: number[] = []
   const extensionBytes: number[] = []

packages/app/src/cli/services/dev/extension/payload.ts

@@ -98,7 +98,7 @@ export async function getUIExtensionPayload(
       },
       capabilities: {
         blockProgress: extension.configuration.capabilities?.block_progress ?? false,
-        networkAccess: extension.configuration.capabilities?.network_access === true,
+        networkAccess: extension.configuration.capabilities?.network_access ?? false,
         apiAccess: extension.configuration.capabilities?.api_access ?? false,
         collectBuyerConsent: {
           smsMarketing: extension.configuration.capabilities?.collect_buyer_consent?.sms_marketing ?? false,

packages/app/src/cli/utilities/extensions/theme.ts

@@ -22,7 +22,7 @@ const ignoredFilePatterns = [
   '*.toml',
 ]
 
-export async function themeExtensionFiles(themeExtension: ExtensionInstance): Promise<string[]> {
+export async function themeExtensionFiles(themeExtension: Pick<ExtensionInstance, 'directory'>): Promise<string[]> {
   const filename = '.shopifyignore'
   const filepath = joinPath(themeExtension.directory, filename)
   const ignore = ignoredFilePatterns.map((pattern) => joinPath('*', pattern))