Skip to content

Commit 21fbba9

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
1 parent ed3c437 commit 21fbba9

File tree

3 files changed

+116
-11
lines changed

3 files changed

+116
-11
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

+69-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,72 @@ 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+
<script src="www.domain.com" defer></script>
241+
`,
242+
};
243+
244+
const offenses = await check(
245+
themeFiles,
246+
[RemoteAsset],
247+
{},
248+
{
249+
RemoteAsset: {
250+
enabled: true,
251+
allowedDomains: ['www.domain.com', 'domain.com'],
252+
},
253+
},
254+
);
255+
256+
expect(offenses).to.have.length(2);
257+
});
258+
259+
it('should not report an offense if url is listed in allowedDomains', async () => {
260+
const themeFiles: MockTheme = {
261+
'layout/theme.liquid': `
262+
<script src="https://domain.com" defer></script>
263+
`,
264+
};
265+
266+
const offenses = await check(
267+
themeFiles,
268+
[RemoteAsset],
269+
{},
270+
{
271+
RemoteAsset: {
272+
enabled: true,
273+
allowedDomains: ['https://domain.com', 'http://domain.com', 'https://www.domain.com'],
274+
},
275+
},
276+
);
277+
278+
expect(offenses).to.be.empty;
279+
});
212280
});

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

+42-10
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,18 @@ 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+
function isUrlHostedbyShopify(url: string, allowedDomains: string[] = []): boolean {
4647
const urlObj = new URL(url);
47-
return SHOPIFY_CDN_DOMAINS.includes(urlObj.hostname);
48+
return [...SHOPIFY_CDN_DOMAINS, ...allowedDomains].includes(urlObj.hostname);
4849
}
4950

50-
function valueIsDefinitelyNotShopifyHosted(attr: ValuedHtmlAttribute): boolean {
51+
function valueIsDefinitelyNotShopifyHosted(
52+
attr: ValuedHtmlAttribute,
53+
allowedDomains: string[] = [],
54+
): boolean {
5155
return attr.value.some((node) => {
5256
if (node.type === NodeTypes.TextNode && /^(https?:)?\/\//.test(node.value)) {
53-
if (!isUrlHostedbyShopify(node.value)) {
57+
if (!isUrlHostedbyShopify(node.value, allowedDomains)) {
5458
return true;
5559
}
5660
}
@@ -60,7 +64,7 @@ function valueIsDefinitelyNotShopifyHosted(attr: ValuedHtmlAttribute): boolean {
6064
if (isLiquidVariable(variable)) {
6165
const expression = variable.expression;
6266
if (expression.type === NodeTypes.String && /^https?:\/\//.test(expression.value)) {
63-
if (!isUrlHostedbyShopify(expression.value)) {
67+
if (!isUrlHostedbyShopify(expression.value, allowedDomains)) {
6468
return true;
6569
}
6670
}
@@ -96,7 +100,27 @@ function valueIsShopifyHosted(attr: ValuedHtmlAttribute): boolean {
96100
});
97101
}
98102

99-
export const RemoteAsset: LiquidCheckDefinition = {
103+
// Takes a list of allowed domains, and normalises them into an expected domain: www.domain.com -> domain.com for equality checks.
104+
function normaliseAllowedDomains(allowedDomains: string[]): string[] {
105+
return allowedDomains
106+
.map((domain) => {
107+
try {
108+
const url = new URL(domain);
109+
// Hostname can still return www. from https://www.domain.com we want it to be https://www.domain.com -> domain.com
110+
return url.hostname.replace(/^www\./, '');
111+
} catch (_error) {
112+
// we shouldn't return the malformed domain - should be strict and stick to web standards (new URL validation).
113+
return undefined;
114+
}
115+
})
116+
.filter((domain): domain is string => domain !== undefined);
117+
}
118+
119+
const schema = {
120+
allowedDomains: SchemaProp.array(SchemaProp.string()).optional(),
121+
};
122+
123+
export const RemoteAsset: LiquidCheckDefinition<typeof schema> = {
100124
meta: {
101125
code: 'RemoteAsset',
102126
aliases: ['AssetUrlFilters'],
@@ -108,11 +132,13 @@ export const RemoteAsset: LiquidCheckDefinition = {
108132
},
109133
type: SourceCodeType.LiquidHtml,
110134
severity: Severity.WARNING,
111-
schema: {},
135+
schema,
112136
targets: [],
113137
},
114138

115139
create(context) {
140+
const allowedDomains = normaliseAllowedDomains(context.settings.allowedDomains || []);
141+
116142
function checkHtmlNode(node: HtmlVoidElement | HtmlRawNode) {
117143
if (!RESOURCE_TAGS.includes(node.name)) return;
118144

@@ -124,11 +150,14 @@ export const RemoteAsset: LiquidCheckDefinition = {
124150

125151
const isShopifyUrl = urlAttribute.value
126152
.filter((node): node is TextNode => node.type === NodeTypes.TextNode)
127-
.some((textNode) => isUrlHostedbyShopify(textNode.value));
153+
.some((textNode) => isUrlHostedbyShopify(textNode.value, allowedDomains));
128154

129155
if (isShopifyUrl) return;
130156

131-
const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted(urlAttribute);
157+
const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted(
158+
urlAttribute,
159+
allowedDomains,
160+
);
132161
if (hasDefinitelyARemoteAssetUrl) {
133162
context.report({
134163
message: 'Asset should be served by the Shopify CDN for better performance.',
@@ -167,7 +196,10 @@ export const RemoteAsset: LiquidCheckDefinition = {
167196
if (hasAsset) return;
168197

169198
const urlNode = parentNode.expression;
170-
if (urlNode.type === NodeTypes.String && !isUrlHostedbyShopify(urlNode.value)) {
199+
if (
200+
urlNode.type === NodeTypes.String &&
201+
!isUrlHostedbyShopify(urlNode.value, allowedDomains)
202+
) {
171203
context.report({
172204
message: 'Asset should be served by the Shopify CDN for better performance.',
173205
startIndex: urlNode.position.start,

0 commit comments

Comments
 (0)