Skip to content

Commit dc8c9fd

Browse files
committed
Add allowedDomains check for RemoteAsset theme checker
Enhances developer experience by validating the remote assets (scripts, stylesheets) are loaded not only from Shopify CDNs but now approved domains. This prevents potential performance risks and incorrect errors from loading resources from untrusted sources, forcing developers to go and enable a source if it's _required_ The implementation includes: - Schema property for configuring allowedDomains - Domain validation against configured allow list - Test coverage for both allowed and non-allowed domains Reverted minor changeset to patch bump Fixed prettier issues Refactor domain list from global to parameter Changes domain validation to accept allowedDomains as a parameter instead of mutating a global array. This improves code clarity and testability by making data flow explicit. The change reduces side effets by: - Removing relince on shared mutable state - Making dpendencies clear through function signatures - Enabling better unit testing of domain validation logic Added file formatting Add normalisaztion & tests. - Added normalization to domains - Added extra tests to test against and for normalisation of strings. Update domains in test refactor: remove redundant regex validation in normaliseAllowedDomains - Remove regex pre-validation since new URL() already enforces strict URL standards - Simplify code by relying solely on new URL() for validation and normalisation Fixed issue where malformed URLs were throwing type errors. The test case urls were malformed as expected, but the new URL constructor was throwing Type Errors, which is expected if not wrapped in a try...catch block. Fixed by wrapping in a try...catch and returning false when malformed.
1 parent def4cf4 commit dc8c9fd

File tree

3 files changed

+122
-12
lines changed

3 files changed

+122
-12
lines changed

.changeset/spicy-pumas-sing.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme-check-common': patch
3+
---
4+
5+
Add RemoteAsset allowedDomains check to validate CDN and approved domain usage for better performance and developer experience

packages/theme-check-common/src/checks/remote-asset/index.spec.ts

+68-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { describe, it, expect } from 'vitest';
2-
import { runLiquidCheck, highlightedOffenses } from '../../test';
2+
import { runLiquidCheck, highlightedOffenses, check, MockTheme } from '../../test';
33
import { RemoteAsset } from './index';
44

55
describe('Module: RemoteAsset', () => {
@@ -209,4 +209,71 @@ describe('Module: RemoteAsset', () => {
209209
const highlights = highlightedOffenses({ 'file.liquid': sourceCode }, offenses);
210210
expect(highlights).to.be.empty;
211211
});
212+
213+
it('should report an offense if url is not listed in allowedDomains', async () => {
214+
const themeFiles: MockTheme = {
215+
'layout/theme.liquid': `
216+
<script src="https://domain.com" defer></script>
217+
`,
218+
};
219+
220+
const offenses = await check(
221+
themeFiles,
222+
[RemoteAsset],
223+
{},
224+
{
225+
RemoteAsset: {
226+
enabled: true,
227+
allowedDomains: ['someotherdomain.com'],
228+
},
229+
},
230+
);
231+
232+
expect(offenses).to.have.length(1);
233+
});
234+
235+
it('should report an offense if the url in the config is malformed/missing protocol', async () => {
236+
const themeFiles: MockTheme = {
237+
'layout/theme.liquid': `
238+
<script src="https://domain.com" defer></script>
239+
<script src="https://www.domain.com" defer></script>
240+
`,
241+
};
242+
243+
const offenses = await check(
244+
themeFiles,
245+
[RemoteAsset],
246+
{},
247+
{
248+
RemoteAsset: {
249+
enabled: true,
250+
allowedDomains: ['www.domain.com', 'domain.com'],
251+
},
252+
},
253+
);
254+
255+
expect(offenses).to.have.length(2);
256+
});
257+
258+
it('should not report an offense if url is listed in allowedDomains', async () => {
259+
const themeFiles: MockTheme = {
260+
'layout/theme.liquid': `
261+
<script src="https://domain.com" defer></script>
262+
`,
263+
};
264+
265+
const offenses = await check(
266+
themeFiles,
267+
[RemoteAsset],
268+
{},
269+
{
270+
RemoteAsset: {
271+
enabled: true,
272+
allowedDomains: ['https://domain.com', 'http://domain.com', 'https://www.domain.com'],
273+
},
274+
},
275+
);
276+
277+
expect(offenses).to.be.empty;
278+
});
212279
});

packages/theme-check-common/src/checks/remote-asset/index.ts

+49-11
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
SourceCodeType,
1313
LiquidCheckDefinition,
1414
LiquidHtmlNode,
15+
SchemaProp,
1516
} from '../../types';
1617
import { isAttr, isValuedHtmlAttribute, isNodeOfType, ValuedHtmlAttribute } from '../utils';
1718
import { last } from '../../utils';
@@ -42,15 +43,24 @@ function isLiquidVariable(node: LiquidHtmlNode | string): node is LiquidVariable
4243
return typeof node !== 'string' && node.type === NodeTypes.LiquidVariable;
4344
}
4445

45-
function isUrlHostedbyShopify(url: string): boolean {
46-
const urlObj = new URL(url);
47-
return SHOPIFY_CDN_DOMAINS.includes(urlObj.hostname);
46+
function isUrlHostedbyShopify(url: string, allowedDomains: string[] = []): boolean {
47+
try {
48+
const urlObj = new URL(url);
49+
return [...SHOPIFY_CDN_DOMAINS, ...allowedDomains].includes(urlObj.hostname);
50+
} catch (_error) {
51+
// Return false for any invalid URLs (missing protocol, malformed URLs, invalid characters etc.)
52+
// Since we're validating if URLs are Shopify-hosted, any invalid URL should return false
53+
return false;
54+
}
4855
}
4956

50-
function valueIsDefinitelyNotShopifyHosted(attr: ValuedHtmlAttribute): boolean {
57+
function valueIsDefinitelyNotShopifyHosted(
58+
attr: ValuedHtmlAttribute,
59+
allowedDomains: string[] = [],
60+
): boolean {
5161
return attr.value.some((node) => {
5262
if (node.type === NodeTypes.TextNode && /^(https?:)?\/\//.test(node.value)) {
53-
if (!isUrlHostedbyShopify(node.value)) {
63+
if (!isUrlHostedbyShopify(node.value, allowedDomains)) {
5464
return true;
5565
}
5666
}
@@ -60,7 +70,7 @@ function valueIsDefinitelyNotShopifyHosted(attr: ValuedHtmlAttribute): boolean {
6070
if (isLiquidVariable(variable)) {
6171
const expression = variable.expression;
6272
if (expression.type === NodeTypes.String && /^https?:\/\//.test(expression.value)) {
63-
if (!isUrlHostedbyShopify(expression.value)) {
73+
if (!isUrlHostedbyShopify(expression.value, allowedDomains)) {
6474
return true;
6575
}
6676
}
@@ -96,7 +106,27 @@ function valueIsShopifyHosted(attr: ValuedHtmlAttribute): boolean {
96106
});
97107
}
98108

99-
export const RemoteAsset: LiquidCheckDefinition = {
109+
// Takes a list of allowed domains, and normalises them into an expected domain: www.domain.com -> domain.com for equality checks.
110+
function normaliseAllowedDomains(allowedDomains: string[]): string[] {
111+
return allowedDomains
112+
.map((domain) => {
113+
try {
114+
const url = new URL(domain);
115+
// Hostname can still return www. from https://www.domain.com we want it to be https://www.domain.com -> domain.com
116+
return url.hostname.replace(/^www\./, '');
117+
} catch (_error) {
118+
// we shouldn't return the malformed domain - should be strict and stick to web standards (new URL validation).
119+
return undefined;
120+
}
121+
})
122+
.filter((domain): domain is string => domain !== undefined);
123+
}
124+
125+
const schema = {
126+
allowedDomains: SchemaProp.array(SchemaProp.string()).optional(),
127+
};
128+
129+
export const RemoteAsset: LiquidCheckDefinition<typeof schema> = {
100130
meta: {
101131
code: 'RemoteAsset',
102132
aliases: ['AssetUrlFilters'],
@@ -108,11 +138,13 @@ export const RemoteAsset: LiquidCheckDefinition = {
108138
},
109139
type: SourceCodeType.LiquidHtml,
110140
severity: Severity.WARNING,
111-
schema: {},
141+
schema,
112142
targets: [],
113143
},
114144

115145
create(context) {
146+
const allowedDomains = normaliseAllowedDomains(context.settings.allowedDomains || []);
147+
116148
function checkHtmlNode(node: HtmlVoidElement | HtmlRawNode) {
117149
if (!RESOURCE_TAGS.includes(node.name)) return;
118150

@@ -124,11 +156,14 @@ export const RemoteAsset: LiquidCheckDefinition = {
124156

125157
const isShopifyUrl = urlAttribute.value
126158
.filter((node): node is TextNode => node.type === NodeTypes.TextNode)
127-
.some((textNode) => isUrlHostedbyShopify(textNode.value));
159+
.some((textNode) => isUrlHostedbyShopify(textNode.value, allowedDomains));
128160

129161
if (isShopifyUrl) return;
130162

131-
const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted(urlAttribute);
163+
const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted(
164+
urlAttribute,
165+
allowedDomains,
166+
);
132167
if (hasDefinitelyARemoteAssetUrl) {
133168
context.report({
134169
message: 'Asset should be served by the Shopify CDN for better performance.',
@@ -167,7 +202,10 @@ export const RemoteAsset: LiquidCheckDefinition = {
167202
if (hasAsset) return;
168203

169204
const urlNode = parentNode.expression;
170-
if (urlNode.type === NodeTypes.String && !isUrlHostedbyShopify(urlNode.value)) {
205+
if (
206+
urlNode.type === NodeTypes.String &&
207+
!isUrlHostedbyShopify(urlNode.value, allowedDomains)
208+
) {
171209
context.report({
172210
message: 'Asset should be served by the Shopify CDN for better performance.',
173211
startIndex: urlNode.position.start,

0 commit comments

Comments
 (0)