Add client steps implementation to support existing functionalities#6966
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
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. |
Coverage report
Test suite run success3814 tests passing in 1458 suites. Report generated by 🧪jest coverage report action from 6a3974c |
9c2e1fd to
7e48497
Compare
16768f3 to
59f0139
Compare
7e48497 to
159bb73
Compare
159bb73 to
e6f3577
Compare
| config.inclusions.map(async (entry) => { | ||
| if (entry.type === 'pattern') { | ||
| const sourceDir = entry.baseDir ? joinPath(extension.directory, entry.baseDir) : extension.directory | ||
| const destinationDir = entry.destination ? joinPath(outputDir, entry.destination) : outputDir |
There was a problem hiding this comment.
Path traversal: include_assets can write outside outputDir
joinPath(outputDir, entry.destination) is used directly with user-controlled config (destination). If destination is absolute (/etc) or contains ../.., it can escape outputDir and overwrite arbitrary files on the machine running the build (CI runner, developer machine). Same class of issue exists for entry.baseDir (controls sourceDir), copySourceEntry(...destination) (static destination), copyConfigKeyEntry(...destination) (configKey destination), and copyConfigKeyEntry where the config value itself can be ../../something.
Because build manifests/config are extension-provided, a malicious extension (or compromised repo) could cause the CLI to overwrite sensitive files or poison other build artifacts.
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - 2 findings 📋 History❌ Failed → ✅ 1 findings → ❌ Failed → ❌ Failed → ✅ 1 findings → ✅ 2 findings |
bbc0817 to
7b68eb7
Compare
e6f3577 to
3f28156
Compare
3f28156 to
44034c5
Compare
f9f9e63 to
59dcaa7
Compare
44034c5 to
094a68c
Compare
59dcaa7 to
fb4c01d
Compare
0478573 to
a8b39c0
Compare
68f4751 to
e79e9ec
Compare
74a2028 to
e39fbcd
Compare
61573e1 to
9257821
Compare
5e27f1f to
e03b479
Compare
596be94 to
e088f88
Compare
e03b479 to
ca267e2
Compare
e088f88 to
b42a87b
Compare
991bfa9 to
c673b4a
Compare
|
|
||
| const destDir = preserveStructure ? joinPath(outputDir, basename(sourcePath)) : outputDir | ||
| await copyDirectoryContents(sourcePath, destDir) | ||
| const copied = await glob(['**/*'], {cwd: destDir, absolute: false}) |
There was a problem hiding this comment.
Static include_assets copy treats sourcePath as directory when destination is omitted
In copySourceEntry, when destination is omitted it unconditionally calls copyDirectoryContents(sourcePath, ...). If sourcePath is a file (e.g., {type:'static', source:'README.md'} without destination), copyDirectoryContents will likely throw (ENOTDIR). Docs/schema imply static entries can be file/directory, but implementation only supports directory when destination is omitted.
c673b4a to
6a3974c
Compare
| * Copies static assets defined in the extension's build_manifest to the output directory. | ||
| * This is a no-op for extensions that do not define static assets. | ||
| */ | ||
| export async function executeCopyStaticAssetsStep(_step: LifecycleStep, context: BuildContext): Promise<void> { |
There was a problem hiding this comment.
are we ever using _step? why is it included?
| export async function executeCreateTaxStubStep(_step: LifecycleStep, context: BuildContext): Promise<void> { | ||
| const {extension} = context | ||
| await touchFile(extension.outputPath) | ||
| await writeFile(extension.outputPath, '(()=>{})();') |
There was a problem hiding this comment.
Tax stub step can overwrite a directory with a JS file
The step writes the stub to extension.outputPath directly. If outputPath is configured as a directory (which the include_assets step supports when “no extension”), this will attempt to write a file over a directory path and fail, or clobber expected directory structure.
Evidence:
await touchFile(extension.outputPath)
await writeFile(extension.outputPath, '(()=>{})();')|
|
||
| await mkdir(dirname(destPath)) | ||
| await copyFile(filepath, destPath) | ||
| }), |
There was a problem hiding this comment.
include_assets pattern copy can silently overwrite on filename collisions when flattening
When preserveStructure is false, it flattens to basename(filepath). If multiple matched files share a basename (e.g. index.js, styles.css in different folders), later copies overwrite earlier ones with no warning, producing nondeterministic artifacts depending on glob ordering.
Evidence:
const relPath = preserveStructure ? relativePath(sourceDir, filepath) : basename(filepath)
const destPath = joinPath(outputDir, relPath)
await copyFile(filepath, destPath)
Add build step infrastructure for extension asset management
WHY are these changes introduced?
This introduces a flexible build step system to handle various asset copying and processing needs for different extension types, replacing hardcoded build logic with configurable steps.
WHAT is this pull request doing?
'admin'to theCONFIG_EXTENSION_IDSarray for admin extensionsexecuteIncludeAssetsStep- handles pattern-based, static, and config-key-driven asset copying with comprehensive test coverageexecuteCopyStaticAssetsStep- copies static assets defined in extension build manifestsexecuteCreateTaxStubStep- generates minimal JavaScript stub files for tax calculation extensionsexecuteStepByType) that dispatches to appropriate handlers based on step typeHow to test your changes?
include_assetsstep type with different inclusion strategiesinclude-assets-step.test.tsMeasuring impact
How do we know this change was effective? Please choose one:
Checklist