[Security] Contain theme file operations within the theme directory - #8600
Draft
github-actions[bot] wants to merge 1 commit into
Draft
github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
Theme file keys originate from remote API responses, but were joined onto the theme root without verifying the result stayed inside it. Resolve each key and reject any that escapes the theme directory before reading, writing, or removing the file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or 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
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.
WHY are these changes introduced?
Theme file keys used by
theme pull,theme dev, and reconciliation originate from remote Admin API responses, which are outside the CLI's trust boundary. Those keys were joined onto the theme root without verifying the result stayed inside it, so a key containing parent-directory segments could resolve to a location outside the theme directory. This is a path traversal issue in the theme filesystem layer, affecting local read, write, and delete operations.WHAT is this pull request doing?
Adds a
resolveThemeFilePathhelper inpackages/theme/src/cli/utilities/theme-fs.tsthat resolves a theme file key against the theme root and rejects it with anAbortErrorwhen the result falls outside that root.writeThemeFile,readThemeFile, andremoveThemeFilenow go through it, so the containment check happens at the boundary rather than at each call site.The check uses the existing
isSubpathhelper from@shopify/cli-kit/node/path, matching how the extension asset middleware already guards its served paths. Legitimate keys resolve exactly as before, so normal theme operations are unchanged.A regression test covers the rejected case and confirms no file is written outside the theme root.
How to manually test your changes?
Both should sync theme files as before, with files written only inside the theme directory.
Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add