diff --git a/.changeset/spicy-pumas-sing.md b/.changeset/spicy-pumas-sing.md new file mode 100644 index 000000000..65a739c43 --- /dev/null +++ b/.changeset/spicy-pumas-sing.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-check-common': patch +--- + +Add RemoteAsset allowedDomains check to validate CDN and approved domain usage for better performance and developer experience diff --git a/packages/theme-check-common/src/checks/remote-asset/index.spec.ts b/packages/theme-check-common/src/checks/remote-asset/index.spec.ts index de505ae21..b0529b01a 100644 --- a/packages/theme-check-common/src/checks/remote-asset/index.spec.ts +++ b/packages/theme-check-common/src/checks/remote-asset/index.spec.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { runLiquidCheck, highlightedOffenses } from '../../test'; +import { runLiquidCheck, highlightedOffenses, check, MockTheme } from '../../test'; import { RemoteAsset } from './index'; describe('Module: RemoteAsset', () => { @@ -209,4 +209,71 @@ describe('Module: RemoteAsset', () => { const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses); expect(highlights).to.be.empty; }); + + it('should report an offense if url is not listed in allowedDomains', async () => { + const themeFiles: MockTheme = { + 'layout/theme.liquid': ` + + `, + }; + + const offenses = await check( + themeFiles, + [RemoteAsset], + {}, + { + RemoteAsset: { + enabled: true, + allowedDomains: ['someotherdomain.com'], + }, + }, + ); + + expect(offenses).to.have.length(1); + }); + + it('should report an offense if the url in the config is malformed/missing protocol', async () => { + const themeFiles: MockTheme = { + 'layout/theme.liquid': ` + + + `, + }; + + const offenses = await check( + themeFiles, + [RemoteAsset], + {}, + { + RemoteAsset: { + enabled: true, + allowedDomains: ['www.domain.com', 'domain.com'], + }, + }, + ); + + expect(offenses).to.have.length(2); + }); + + it('should not report an offense if url is listed in allowedDomains', async () => { + const themeFiles: MockTheme = { + 'layout/theme.liquid': ` + + `, + }; + + const offenses = await check( + themeFiles, + [RemoteAsset], + {}, + { + RemoteAsset: { + enabled: true, + allowedDomains: ['https://domain.com', 'http://domain.com', 'https://www.domain.com'], + }, + }, + ); + + expect(offenses).to.be.empty; + }); }); diff --git a/packages/theme-check-common/src/checks/remote-asset/index.ts b/packages/theme-check-common/src/checks/remote-asset/index.ts index cfa28d063..535019083 100644 --- a/packages/theme-check-common/src/checks/remote-asset/index.ts +++ b/packages/theme-check-common/src/checks/remote-asset/index.ts @@ -12,6 +12,7 @@ import { SourceCodeType, LiquidCheckDefinition, LiquidHtmlNode, + SchemaProp, } from '../../types'; import { isAttr, isValuedHtmlAttribute, isNodeOfType, ValuedHtmlAttribute } from '../utils'; import { last } from '../../utils'; @@ -42,15 +43,24 @@ function isLiquidVariable(node: LiquidHtmlNode | string): node is LiquidVariable return typeof node !== 'string' && node.type === NodeTypes.LiquidVariable; } -function isUrlHostedbyShopify(url: string): boolean { - const urlObj = new URL(url); - return SHOPIFY_CDN_DOMAINS.includes(urlObj.hostname); +function isUrlHostedbyShopify(url: string, allowedDomains: string[] = []): boolean { + try { + const urlObj = new URL(url); + return [...SHOPIFY_CDN_DOMAINS, ...allowedDomains].includes(urlObj.hostname); + } catch (_error) { + // Return false for any invalid URLs (missing protocol, malformed URLs, invalid characters etc.) + // Since we're validating if URLs are Shopify-hosted, any invalid URL should return false + return false; + } } -function valueIsDefinitelyNotShopifyHosted(attr: ValuedHtmlAttribute): boolean { +function valueIsDefinitelyNotShopifyHosted( + attr: ValuedHtmlAttribute, + allowedDomains: string[] = [], +): boolean { return attr.value.some((node) => { if (node.type === NodeTypes.TextNode && /^(https?:)?\/\//.test(node.value)) { - if (!isUrlHostedbyShopify(node.value)) { + if (!isUrlHostedbyShopify(node.value, allowedDomains)) { return true; } } @@ -60,7 +70,7 @@ function valueIsDefinitelyNotShopifyHosted(attr: ValuedHtmlAttribute): boolean { if (isLiquidVariable(variable)) { const expression = variable.expression; if (expression.type === NodeTypes.String && /^https?:\/\//.test(expression.value)) { - if (!isUrlHostedbyShopify(expression.value)) { + if (!isUrlHostedbyShopify(expression.value, allowedDomains)) { return true; } } @@ -96,7 +106,27 @@ function valueIsShopifyHosted(attr: ValuedHtmlAttribute): boolean { }); } -export const RemoteAsset: LiquidCheckDefinition = { +// Takes a list of allowed domains, and normalises them into an expected domain: www.domain.com -> domain.com for equality checks. +function normaliseAllowedDomains(allowedDomains: string[]): string[] { + return allowedDomains + .map((domain) => { + try { + const url = new URL(domain); + // Hostname can still return www. from https://www.domain.com we want it to be https://www.domain.com -> domain.com + return url.hostname.replace(/^www\./, ''); + } catch (_error) { + // we shouldn't return the malformed domain - should be strict and stick to web standards (new URL validation). + return undefined; + } + }) + .filter((domain): domain is string => domain !== undefined); +} + +const schema = { + allowedDomains: SchemaProp.array(SchemaProp.string()).optional(), +}; + +export const RemoteAsset: LiquidCheckDefinition = { meta: { code: 'RemoteAsset', aliases: ['AssetUrlFilters'], @@ -108,11 +138,13 @@ export const RemoteAsset: LiquidCheckDefinition = { }, type: SourceCodeType.LiquidHtml, severity: Severity.WARNING, - schema: {}, + schema, targets: [], }, create(context) { + const allowedDomains = normaliseAllowedDomains(context.settings.allowedDomains || []); + function checkHtmlNode(node: HtmlVoidElement | HtmlRawNode) { if (!RESOURCE_TAGS.includes(node.name)) return; @@ -124,11 +156,14 @@ export const RemoteAsset: LiquidCheckDefinition = { const isShopifyUrl = urlAttribute.value .filter((node): node is TextNode => node.type === NodeTypes.TextNode) - .some((textNode) => isUrlHostedbyShopify(textNode.value)); + .some((textNode) => isUrlHostedbyShopify(textNode.value, allowedDomains)); if (isShopifyUrl) return; - const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted(urlAttribute); + const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted( + urlAttribute, + allowedDomains, + ); if (hasDefinitelyARemoteAssetUrl) { context.report({ message: 'Asset should be served by the Shopify CDN for better performance.', @@ -167,7 +202,10 @@ export const RemoteAsset: LiquidCheckDefinition = { if (hasAsset) return; const urlNode = parentNode.expression; - if (urlNode.type === NodeTypes.String && !isUrlHostedbyShopify(urlNode.value)) { + if ( + urlNode.type === NodeTypes.String && + !isUrlHostedbyShopify(urlNode.value, allowedDomains) + ) { context.report({ message: 'Asset should be served by the Shopify CDN for better performance.', startIndex: urlNode.position.start,