Skip to content

Commit 2d686d8

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
1 parent 6ab6856 commit 2d686d8

File tree

3 files changed

+82
-11
lines changed

3 files changed

+82
-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

+51-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,54 @@ 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 not report an offense if url is listed in allowedDomains', async () => {
236+
const themeFiles: MockTheme = {
237+
'layout/theme.liquid': `
238+
<script src="https://domain.com" defer></script>
239+
`,
240+
};
241+
242+
const offenses = await check(
243+
themeFiles,
244+
[RemoteAsset],
245+
{},
246+
{
247+
RemoteAsset: {
248+
enabled: true,
249+
allowedDomains: [
250+
'domain.com',
251+
'https://domain.com',
252+
'http://domain.com',
253+
'https://www.domain.com',
254+
'www.domain.com',
255+
],
256+
},
257+
},
258+
);
259+
260+
expect(offenses).to.be.empty;
261+
});
212262
});

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

+26-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,11 @@ function valueIsShopifyHosted(attr: ValuedHtmlAttribute): boolean {
96100
});
97101
}
98102

99-
export const RemoteAsset: LiquidCheckDefinition = {
103+
const schema = {
104+
allowedDomains: SchemaProp.array(SchemaProp.string()).optional(),
105+
};
106+
107+
export const RemoteAsset: LiquidCheckDefinition<typeof schema> = {
100108
meta: {
101109
code: 'RemoteAsset',
102110
aliases: ['AssetUrlFilters'],
@@ -108,11 +116,13 @@ export const RemoteAsset: LiquidCheckDefinition = {
108116
},
109117
type: SourceCodeType.LiquidHtml,
110118
severity: Severity.WARNING,
111-
schema: {},
119+
schema,
112120
targets: [],
113121
},
114122

115123
create(context) {
124+
const allowedDomains = context.settings.allowedDomains || [];
125+
116126
function checkHtmlNode(node: HtmlVoidElement | HtmlRawNode) {
117127
if (!RESOURCE_TAGS.includes(node.name)) return;
118128

@@ -124,11 +134,14 @@ export const RemoteAsset: LiquidCheckDefinition = {
124134

125135
const isShopifyUrl = urlAttribute.value
126136
.filter((node): node is TextNode => node.type === NodeTypes.TextNode)
127-
.some((textNode) => isUrlHostedbyShopify(textNode.value));
137+
.some((textNode) => isUrlHostedbyShopify(textNode.value, allowedDomains));
128138

129139
if (isShopifyUrl) return;
130140

131-
const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted(urlAttribute);
141+
const hasDefinitelyARemoteAssetUrl = valueIsDefinitelyNotShopifyHosted(
142+
urlAttribute,
143+
allowedDomains,
144+
);
132145
if (hasDefinitelyARemoteAssetUrl) {
133146
context.report({
134147
message: 'Asset should be served by the Shopify CDN for better performance.',
@@ -167,7 +180,10 @@ export const RemoteAsset: LiquidCheckDefinition = {
167180
if (hasAsset) return;
168181

169182
const urlNode = parentNode.expression;
170-
if (urlNode.type === NodeTypes.String && !isUrlHostedbyShopify(urlNode.value)) {
183+
if (
184+
urlNode.type === NodeTypes.String &&
185+
!isUrlHostedbyShopify(urlNode.value, allowedDomains)
186+
) {
171187
context.report({
172188
message: 'Asset should be served by the Shopify CDN for better performance.',
173189
startIndex: urlNode.position.start,

0 commit comments

Comments
 (0)