Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Theme Check] Add allowedDomains check for RemoteAsset theme checker #711

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+116 −11
Open

[Theme Check] Add allowedDomains check for RemoteAsset theme checker #711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/spicy-pumas-sing.md
View file
Open in desktop
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
70 changes: 69 additions & 1 deletion packages/theme-check-common/src/checks/remote-asset/index.spec.ts
View file
Open in desktop
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;
});
});
52 changes: 42 additions & 10 deletions packages/theme-check-common/src/checks/remote-asset/index.ts
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should allowedDomains be normalized before doing the include check? IIRC .includes compares using strict equality, this would fail in the case a user puts in http://domain.com in their config since urlObj.hostName would compare domain.com with http://domain.com which does not match exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlespwd good point, inline with how we check i've introduced a normalise func that takes an array of strings and returns a new array of mapped strings.

String normalisation follows web URL standard and returns the hostname for comparison.

}

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