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
  • Loading branch information
zacariec committed Jan 16, 2025
1 parent 4a429e1 commit 861e8c6
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/grumpy-countries-jog.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme-check-common': minor
---

Updated RemoteAsset to take an array of allowed domains
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,57 @@ 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 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: [
'domain.com',
'https://domain.com',
'http://domain.com',
'https://www.domain.com',
'www.domain.com',
],
},
},
);

expect(offenses).to.be.empty;
});

});
20 changes: 18 additions & 2 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 @@ -96,7 +97,11 @@ function valueIsShopifyHosted(attr: ValuedHtmlAttribute): boolean {
});
}

export const RemoteAsset: LiquidCheckDefinition = {
const schema = {
allowedDomains: SchemaProp.array(SchemaProp.string()),
};

export const RemoteAsset: LiquidCheckDefinition<typeof schema> = {
meta: {
code: 'RemoteAsset',
aliases: ['AssetUrlFilters'],
Expand All @@ -108,11 +113,18 @@ export const RemoteAsset: LiquidCheckDefinition = {
},
type: SourceCodeType.LiquidHtml,
severity: Severity.WARNING,
schema: {},
schema,
targets: [],
},

create(context) {
/* Push the allowed list of domains to our allowed domains */
if (context.settings.allowedDomains) {
for (const domain of context.settings.allowedDomains) {
SHOPIFY_CDN_DOMAINS.push(domain);
}
}

function checkHtmlNode(node: HtmlVoidElement | HtmlRawNode) {
if (!RESOURCE_TAGS.includes(node.name)) return;

Expand Down Expand Up @@ -168,6 +180,10 @@ export const RemoteAsset: LiquidCheckDefinition = {

const urlNode = parentNode.expression;
if (urlNode.type === NodeTypes.String && !isUrlHostedbyShopify(urlNode.value)) {
if (context.settings.allowedDomains && Array.isArray(context.settings.allowedDomains)) {
// const url = new URL(urlNode.value);
}

context.report({
message: 'Asset should be served by the Shopify CDN for better performance.',
startIndex: urlNode.position.start,
Expand Down

0 comments on commit 861e8c6

Please sign in to comment.