[Theme Check] Add allowedDomains check for RemoteAsset theme checker #711
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zacariec
changed the title
charlespwd
reviewed
Jan 16, 2025
charlespwd
reviewed
Jan 16, 2025
charlespwd
reviewed
Jan 17, 2025
| const urlObj = new URL(url); | ||
| return SHOPIFY_CDN_DOMAINS.includes(urlObj.hostname); | ||
| return [...SHOPIFY_CDN_DOMAINS, ...allowedDomains].includes(urlObj.hostname); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'll update the PR template based on the revised commit message that focuses on developer experience and performance:
What are you adding in this PR?
Fixes #410
This PR adds domain validation to the RemoteAsset theme checker to ensure remote assets (scripts, stylesheets) are loaded from Shopify CDNs and approved domains. When a remote asset is referenced from an unlisted domain, the checker will provide guidance to help developers optimize performance and follow best practices.
Key changes:
allowedDomainsconfiguration in RemoteAsset check schemaWhat's next? Any followup issues?
Potential follow-ups:
What did you learn?
The variety of ways developers include remote assets highlighted the need for clear guidance on performance best practices. The test cases helped identify common patterns in how external resources are referenced in themes.
Before you deploy
changesetallChecksarray insrc/checks/index.tsyarn buildand committed the updated configuration filestheme-app-extension.ymlconfig (N/A - performance check applies to all themes)changeset