Skip to content

Commit

Permalink
Add allowedDomains check for RemoteAsset theme checker
Browse files Browse the repository at this point in the history
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
  • Loading branch information
zacariec committed Jan 28, 2025
1 parent fced038 commit 3c85c95
Show file tree
Hide file tree
Showing 3 changed files with 123 additions and 11 deletions.
5 changes: 5 additions & 0 deletions .changeset/spicy-pumas-sing.md
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -209,4 +209,72 @@ 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': `
<script src="https://domain.com" defer></script>
`,
};

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': `
<script src="https://domain.com" defer></script>
<script src="https://www.domain.com" defer></script>
<script src="www.domain.com" defer></script>
`,
};

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': `
<script src="https://domain.com" defer></script>
`,
};

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;
});
});
59 changes: 49 additions & 10 deletions packages/theme-check-common/src/checks/remote-asset/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
SourceCodeType,
LiquidCheckDefinition,
LiquidHtmlNode,
SchemaProp,
} from '../../types';
import { isAttr, isValuedHtmlAttribute, isNodeOfType, ValuedHtmlAttribute } from '../utils';
import { last } from '../../utils';
Expand Down Expand Up @@ -42,15 +43,18 @@ function isLiquidVariable(node: LiquidHtmlNode | string): node is LiquidVariable
return typeof node !== 'string' && node.type === NodeTypes.LiquidVariable;
}

function isUrlHostedbyShopify(url: string): boolean {
function isUrlHostedbyShopify(url: string, allowedDomains: string[] = []): boolean {
const urlObj = new URL(url);
return SHOPIFY_CDN_DOMAINS.includes(urlObj.hostname);
return [...SHOPIFY_CDN_DOMAINS, ...allowedDomains].includes(urlObj.hostname);
}

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;
}
}
Expand All @@ -60,7 +64,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;
}
}
Expand Down Expand Up @@ -96,7 +100,34 @@ 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[] {
const urlRegex = new RegExp(
/^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,5}(:[0-9]{1,5})?(\/.*)?$/,
);
return allowedDomains
.map((domain) => {
if (urlRegex.test(domain) === false) {
return undefined;
}

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<typeof schema> = {
meta: {
code: 'RemoteAsset',
aliases: ['AssetUrlFilters'],
Expand All @@ -108,11 +139,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;

Expand All @@ -124,11 +157,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.',
Expand Down Expand Up @@ -167,7 +203,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,
Expand Down

0 comments on commit 3c85c95

Please sign in to comment.