Conversation
Coverage report
Test suite run success3800 tests passing in 1455 suites. Report generated by 🧪jest coverage report action from b255e6d |
e165a9d to
5b7340f
Compare
cd9fc6e to
4dd73b7
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4a80cd2 to
ae0d089
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
ae0d089 to
510502e
Compare
510502e to
289a2c6
Compare
289a2c6 to
510502e
Compare
510502e to
f9f6e10
Compare
|
/snapit |
|
🫰✨ Thanks @elanalynn! Your snapshot has been published to npm. Test the snapshot by installing your package globally: Caution After installing, validate the version by running |
ff3f4e0 to
635fedb
Compare
635fedb to
d296cb0
Compare
| copyStaticAssets: async (config, directory, outputPath) => { | ||
| if (!config.static_root) return | ||
| const sourceDir = joinPath(directory, config.static_root) | ||
| const outputDir = dirname(outputPath) |
There was a problem hiding this comment.
Validate static_root to prevent path traversal/arbitrary file copying
static_root is taken directly from config and joined into sourceDir (joinPath(directory, config.static_root)). If misconfigured or attacker-controlled, values like ../.., absolute paths, or symlinks can escape the app directory and copy arbitrary filesystem contents into the deploy bundle. This is risky even if developer-controlled (CI agents often have adjacent secrets). Recursive copy may amplify impact, especially if symlinks are followed.
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - No issues 📋 History✅ 1 findings → ✅ 1 findings → ✅ No issues |
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - No issues 📋 History✅ 1 findings → ✅ 1 findings → ✅ No issues |
d296cb0 to
635fedb
Compare
|
|
||
| return copyDirectoryContents(sourceDir, outputDir).catch((error) => { | ||
| throw new Error(`Failed to copy static assets from ${sourceDir} to ${outputDir}: ${error.message}`) | ||
| }) |
There was a problem hiding this comment.
Catch handler assumes error.message exists and can mask root cause
The catch handler assumes error has a .message field. If the underlying promise rejects with a non-Error (string/object), this code can throw a secondary error (e.g., reading message), masking the original failure.
Evidence:
.catch((error) => {
throw new Error(`...: ${error.message}`)
})
Impact:
- Failures become harder to debug; incorrect/noisy diagnostics.
635fedb to
66deccf
Compare
66deccf to
6b8ca49
Compare
6b8ca49 to
ae7ed38
Compare
ae7ed38 to
b255e6d
Compare

WHY are these changes introduced?
Related to Hosted app project.
Related to https://github.com/shop/world/pull/382743
Closes https://github.com/shop/issues-admin-extensibility/issues/2186
WHAT is this pull request doing?
This PR introduces the asset pipeline flow for hosted static apps, enabling static asset copying during the build process.
app_config_hosted_app_home.ts): Adds a configuration extension specification for hosted apps with astatic_rootfieldstatic_appbuild mode: Extends the build config modes to includestatic_app, which triggers static asset copyingHow it works
When a hosted app defines a static_root in its configuration:
hosted_appspecification is loaded with build modestatic_appcopyStaticAssets()is invokedstatic_rootdirectory are copied to the output directoryHow to test your changes?
static_rootpointing to a static assets directoryshopify app deployand verify assets are copied to the output directory -.shopify/deploy-bundleMeasuring impact
How do we know this change was effective? Please choose one:
Checklist