From fab80c99b7742479538ff4e75dd234880701f83b Mon Sep 17 00:00:00 2001 From: Affaan Mustafa Date: Mon, 30 Mar 2026 02:01:54 -0400 Subject: [PATCH] fix: harden Trae install ownership (#1013) * fix: harden trae install ownership * fix: unblock unicode safety CI lint * fix: unblock shared CI regressions * test: isolate package-manager dependent hooks and formatter tests --- .trae/install.sh | 67 ++++++----- tests/hooks/hooks.test.js | 7 +- tests/lib/package-manager.test.js | 35 +++++- tests/lib/resolve-formatter.test.js | 47 ++++++-- tests/scripts/trae-install.test.js | 179 ++++++++++++++++++++++++++++ 5 files changed, 296 insertions(+), 39 deletions(-) create mode 100644 tests/scripts/trae-install.test.js diff --git a/.trae/install.sh b/.trae/install.sh index da0d860c..959579e7 100755 --- a/.trae/install.sh +++ b/.trae/install.sh @@ -39,6 +39,40 @@ ensure_manifest_entry() { fi } +manifest_has_entry() { + local manifest="$1" + local entry="$2" + + [ -f "$manifest" ] && grep -Fqx "$entry" "$manifest" +} + +copy_managed_file() { + local source_path="$1" + local target_path="$2" + local manifest="$3" + local manifest_entry="$4" + local make_executable="${5:-0}" + + local already_managed=0 + if manifest_has_entry "$manifest" "$manifest_entry"; then + already_managed=1 + fi + + if [ -f "$target_path" ]; then + if [ "$already_managed" -eq 1 ]; then + ensure_manifest_entry "$manifest" "$manifest_entry" + fi + return 1 + fi + + cp "$source_path" "$target_path" + if [ "$make_executable" -eq 1 ]; then + chmod +x "$target_path" + fi + ensure_manifest_entry "$manifest" "$manifest_entry" + return 0 +} + # Install function do_install() { local target_dir="$PWD" @@ -95,12 +129,8 @@ do_install() { [ -f "$f" ] || continue local_name=$(basename "$f") target_path="$trae_full_path/commands/$local_name" - if [ ! -f "$target_path" ]; then - cp "$f" "$target_path" - ensure_manifest_entry "$MANIFEST" "commands/$local_name" + if copy_managed_file "$f" "$target_path" "$MANIFEST" "commands/$local_name"; then commands=$((commands + 1)) - else - ensure_manifest_entry "$MANIFEST" "commands/$local_name" fi done fi @@ -111,12 +141,8 @@ do_install() { [ -f "$f" ] || continue local_name=$(basename "$f") target_path="$trae_full_path/agents/$local_name" - if [ ! -f "$target_path" ]; then - cp "$f" "$target_path" - ensure_manifest_entry "$MANIFEST" "agents/$local_name" + if copy_managed_file "$f" "$target_path" "$MANIFEST" "agents/$local_name"; then agents=$((agents + 1)) - else - ensure_manifest_entry "$MANIFEST" "agents/$local_name" fi done fi @@ -134,11 +160,9 @@ do_install() { target_path="$target_skill_dir/$relative_path" mkdir -p "$(dirname "$target_path")" - if [ ! -f "$target_path" ]; then - cp "$source_file" "$target_path" + if copy_managed_file "$source_file" "$target_path" "$MANIFEST" "skills/$skill_name/$relative_path"; then skill_copied=1 fi - ensure_manifest_entry "$MANIFEST" "skills/$skill_name/$relative_path" done < <(find "$d" -type f | sort) if [ "$skill_copied" -eq 1 ]; then @@ -154,11 +178,9 @@ do_install() { target_path="$trae_full_path/rules/$relative_path" mkdir -p "$(dirname "$target_path")" - if [ ! -f "$target_path" ]; then - cp "$rule_file" "$target_path" + if copy_managed_file "$rule_file" "$target_path" "$MANIFEST" "rules/$relative_path"; then rules=$((rules + 1)) fi - ensure_manifest_entry "$MANIFEST" "rules/$relative_path" done < <(find "$REPO_ROOT/rules" -type f | sort) fi @@ -167,12 +189,8 @@ do_install() { if [ -f "$readme_file" ]; then local_name=$(basename "$readme_file") target_path="$trae_full_path/$local_name" - if [ ! -f "$target_path" ]; then - cp "$readme_file" "$target_path" - ensure_manifest_entry "$MANIFEST" "$local_name" + if copy_managed_file "$readme_file" "$target_path" "$MANIFEST" "$local_name"; then other=$((other + 1)) - else - ensure_manifest_entry "$MANIFEST" "$local_name" fi fi done @@ -182,13 +200,8 @@ do_install() { if [ -f "$script_file" ]; then local_name=$(basename "$script_file") target_path="$trae_full_path/$local_name" - if [ ! -f "$target_path" ]; then - cp "$script_file" "$target_path" - chmod +x "$target_path" - ensure_manifest_entry "$MANIFEST" "$local_name" + if copy_managed_file "$script_file" "$target_path" "$MANIFEST" "$local_name" 1; then other=$((other + 1)) - else - ensure_manifest_entry "$MANIFEST" "$local_name" fi fi done diff --git a/tests/hooks/hooks.test.js b/tests/hooks/hooks.test.js index 09805645..7d4475e6 100644 --- a/tests/hooks/hooks.test.js +++ b/tests/hooks/hooks.test.js @@ -1221,9 +1221,14 @@ async function runTests() { fs.writeFileSync(path.join(rootDir, '.prettierrc'), '{}'); fs.writeFileSync(filePath, 'export const value = 1;\n'); createCommandShim(binDir, 'npx', logFile); + const isolatedHome = path.join(testDir, 'isolated-home'); + fs.mkdirSync(path.join(isolatedHome, '.claude'), { recursive: true }); const stdinJson = JSON.stringify({ tool_input: { file_path: filePath } }); - const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, withPrependedPath(binDir)); + const result = await runScript(path.join(scriptsDir, 'post-edit-format.js'), stdinJson, withPrependedPath(binDir, { + HOME: isolatedHome, + USERPROFILE: isolatedHome + })); assert.strictEqual(result.code, 0, 'Should exit 0 for config-only repo'); const logEntries = readCommandLog(logFile); diff --git a/tests/lib/package-manager.test.js b/tests/lib/package-manager.test.js index 826d8c02..ebe5922e 100644 --- a/tests/lib/package-manager.test.js +++ b/tests/lib/package-manager.test.js @@ -37,6 +37,33 @@ function cleanupTestDir(testDir) { fs.rmSync(testDir, { recursive: true, force: true }); } +function withIsolatedHome(fn) { + const isolatedHome = fs.mkdtempSync(path.join(os.tmpdir(), 'pm-home-')); + const originalHome = process.env.HOME; + const originalUserProfile = process.env.USERPROFILE; + + process.env.HOME = isolatedHome; + process.env.USERPROFILE = isolatedHome; + + try { + return fn(isolatedHome); + } finally { + if (originalHome !== undefined) { + process.env.HOME = originalHome; + } else { + delete process.env.HOME; + } + + if (originalUserProfile !== undefined) { + process.env.USERPROFILE = originalUserProfile; + } else { + delete process.env.USERPROFILE; + } + + fs.rmSync(isolatedHome, { recursive: true, force: true }); + } +} + // Test suite function runTests() { console.log('\n=== Testing package-manager.js ===\n'); @@ -711,9 +738,11 @@ function runTests() { const originalEnv = process.env.CLAUDE_PACKAGE_MANAGER; try { delete process.env.CLAUDE_PACKAGE_MANAGER; - const result = pm.getPackageManager({ projectDir: testDir }); - assert.strictEqual(result.name, 'npm', 'Should default to npm'); - assert.strictEqual(result.source, 'default'); + withIsolatedHome(() => { + const result = pm.getPackageManager({ projectDir: testDir }); + assert.strictEqual(result.name, 'npm', 'Should default to npm'); + assert.strictEqual(result.source, 'default'); + }); } finally { if (originalEnv !== undefined) { process.env.CLAUDE_PACKAGE_MANAGER = originalEnv; diff --git a/tests/lib/resolve-formatter.test.js b/tests/lib/resolve-formatter.test.js index c02bb60d..dc478fc8 100644 --- a/tests/lib/resolve-formatter.test.js +++ b/tests/lib/resolve-formatter.test.js @@ -58,6 +58,33 @@ function cleanupTmpDirs() { tmpDirs.length = 0; } +function withIsolatedHome(fn) { + const isolatedHome = fs.mkdtempSync(path.join(os.tmpdir(), 'resolve-fmt-home-')); + const originalHome = process.env.HOME; + const originalUserProfile = process.env.USERPROFILE; + + process.env.HOME = isolatedHome; + process.env.USERPROFILE = isolatedHome; + + try { + return fn(isolatedHome); + } finally { + if (originalHome !== undefined) { + process.env.HOME = originalHome; + } else { + delete process.env.HOME; + } + + if (originalUserProfile !== undefined) { + process.env.USERPROFILE = originalUserProfile; + } else { + delete process.env.USERPROFILE; + } + + fs.rmSync(isolatedHome, { recursive: true, force: true }); + } +} + function runTests() { console.log('\n=== Testing resolve-formatter.js ===\n'); @@ -168,10 +195,12 @@ function runTests() { run('resolveFormatterBin: falls back to npx for biome', () => { const root = makeTmpDir(); - const result = resolveFormatterBin(root, 'biome'); - const expectedBin = process.platform === 'win32' ? 'npx.cmd' : 'npx'; - assert.strictEqual(result.bin, expectedBin); - assert.deepStrictEqual(result.prefix, ['@biomejs/biome']); + withIsolatedHome(() => { + const result = resolveFormatterBin(root, 'biome'); + const expectedBin = process.platform === 'win32' ? 'npx.cmd' : 'npx'; + assert.strictEqual(result.bin, expectedBin); + assert.deepStrictEqual(result.prefix, ['@biomejs/biome']); + }); }); run('resolveFormatterBin: uses local prettier binary when available', () => { @@ -188,10 +217,12 @@ function runTests() { run('resolveFormatterBin: falls back to npx for prettier', () => { const root = makeTmpDir(); - const result = resolveFormatterBin(root, 'prettier'); - const expectedBin = process.platform === 'win32' ? 'npx.cmd' : 'npx'; - assert.strictEqual(result.bin, expectedBin); - assert.deepStrictEqual(result.prefix, ['prettier']); + withIsolatedHome(() => { + const result = resolveFormatterBin(root, 'prettier'); + const expectedBin = process.platform === 'win32' ? 'npx.cmd' : 'npx'; + assert.strictEqual(result.bin, expectedBin); + assert.deepStrictEqual(result.prefix, ['prettier']); + }); }); run('resolveFormatterBin: returns null for unknown formatter', () => { diff --git a/tests/scripts/trae-install.test.js b/tests/scripts/trae-install.test.js new file mode 100644 index 00000000..8328e81b --- /dev/null +++ b/tests/scripts/trae-install.test.js @@ -0,0 +1,179 @@ +/** + * Tests for .trae/install.sh and .trae/uninstall.sh + */ + +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { execFileSync } = require('child_process'); + +const REPO_ROOT = path.join(__dirname, '..', '..'); +const INSTALL_SCRIPT = path.join(REPO_ROOT, '.trae', 'install.sh'); +const UNINSTALL_SCRIPT = path.join(REPO_ROOT, '.trae', 'uninstall.sh'); + +function createTempDir(prefix) { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function cleanup(dirPath) { + fs.rmSync(dirPath, { recursive: true, force: true }); +} + +function runInstall(options = {}) { + return execFileSync('bash', [INSTALL_SCRIPT, ...(options.args || [])], { + cwd: options.cwd, + env: { + ...process.env, + HOME: options.homeDir || process.env.HOME, + }, + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 20000, + }); +} + +function runUninstall(options = {}) { + return execFileSync('bash', [UNINSTALL_SCRIPT, ...(options.args || [])], { + cwd: options.cwd, + env: { + ...process.env, + HOME: options.homeDir || process.env.HOME, + }, + encoding: 'utf8', + input: options.input || 'y\n', + stdio: ['pipe', 'pipe', 'pipe'], + timeout: 20000, + }); +} + +function readManifestLines(projectRoot) { + const manifestPath = path.join(projectRoot, '.trae', '.ecc-manifest'); + return fs.readFileSync(manifestPath, 'utf8') + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean); +} + +function test(name, fn) { + try { + fn(); + console.log(` \u2713 ${name}`); + return true; + } catch (error) { + console.log(` \u2717 ${name}`); + console.log(` Error: ${error.message}`); + return false; + } +} + +function runTests() { + console.log('\n=== Testing Trae install/uninstall scripts ===\n'); + + let passed = 0; + let failed = 0; + + if (process.platform === 'win32') { + console.log(' - skipped on Windows; Trae shell scripts are Unix-only'); + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); + process.exit(0); + } + + if (test('does not claim ownership of preexisting target files', () => { + const homeDir = createTempDir('trae-home-'); + const projectRoot = createTempDir('trae-project-'); + + try { + const preexistingCommandPath = path.join(projectRoot, '.trae', 'commands', 'e2e.md'); + fs.mkdirSync(path.dirname(preexistingCommandPath), { recursive: true }); + fs.writeFileSync(preexistingCommandPath, 'user owned command\n'); + + runInstall({ cwd: projectRoot, homeDir }); + + const manifestLines = readManifestLines(projectRoot); + assert.ok(!manifestLines.includes('commands/e2e.md'), 'Preexisting file should not be recorded in manifest'); + + runUninstall({ cwd: projectRoot, homeDir }); + + assert.strictEqual(fs.readFileSync(preexistingCommandPath, 'utf8'), 'user owned command\n'); + } finally { + cleanup(homeDir); + cleanup(projectRoot); + } + })) passed++; else failed++; + + if (test('records nested skill files and the full rules tree in the manifest', () => { + const homeDir = createTempDir('trae-home-'); + const projectRoot = createTempDir('trae-project-'); + + try { + runInstall({ cwd: projectRoot, homeDir }); + + const manifestLines = readManifestLines(projectRoot); + assert.ok(manifestLines.includes('skills/skill-comply/pyproject.toml')); + assert.ok(manifestLines.includes('rules/common/code-review.md')); + assert.ok(manifestLines.includes('rules/python/coding-style.md')); + assert.ok(manifestLines.includes('rules/zh/README.md')); + + assert.ok(fs.existsSync(path.join(projectRoot, '.trae', 'skills', 'skill-comply', 'pyproject.toml'))); + assert.ok(fs.existsSync(path.join(projectRoot, '.trae', 'rules', 'python', 'coding-style.md'))); + assert.ok(fs.existsSync(path.join(projectRoot, '.trae', 'rules', 'zh', 'README.md'))); + } finally { + cleanup(homeDir); + cleanup(projectRoot); + } + })) passed++; else failed++; + + if (test('reinstall preserves managed manifest coverage without duplicate entries', () => { + const homeDir = createTempDir('trae-home-'); + const projectRoot = createTempDir('trae-project-'); + + try { + runInstall({ cwd: projectRoot, homeDir }); + + const managedCommandPath = path.join(projectRoot, '.trae', 'commands', 'e2e.md'); + fs.rmSync(managedCommandPath); + + runInstall({ cwd: projectRoot, homeDir }); + + const manifestLines = readManifestLines(projectRoot); + const entryCount = manifestLines.filter((line) => line === 'commands/e2e.md').length; + + assert.strictEqual(entryCount, 1, 'Managed file should appear once in manifest after reinstall'); + assert.ok(fs.existsSync(managedCommandPath), 'Managed file should be recreated on reinstall'); + } finally { + cleanup(homeDir); + cleanup(projectRoot); + } + })) passed++; else failed++; + + if (test('uninstall rejects manifest entries that escape the Trae root via symlink traversal', () => { + const homeDir = createTempDir('trae-home-'); + const projectRoot = createTempDir('trae-project-'); + const externalRoot = createTempDir('trae-outside-'); + + try { + const traeRoot = path.join(projectRoot, '.trae'); + fs.mkdirSync(traeRoot, { recursive: true }); + + const outsideSecretPath = path.join(externalRoot, 'secret.txt'); + fs.writeFileSync(outsideSecretPath, 'do not remove\n'); + fs.symlinkSync(externalRoot, path.join(traeRoot, 'escape-link')); + fs.writeFileSync(path.join(traeRoot, '.ecc-manifest'), 'escape-link/secret.txt\n.ecc-manifest\n'); + + const stdout = runUninstall({ cwd: projectRoot, homeDir }); + + assert.ok(stdout.includes('Skipped: escape-link/secret.txt (invalid manifest entry)')); + assert.strictEqual(fs.readFileSync(outsideSecretPath, 'utf8'), 'do not remove\n'); + } finally { + cleanup(homeDir); + cleanup(projectRoot); + cleanup(externalRoot); + } + })) passed++; else failed++; + + console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); + process.exit(failed > 0 ? 1 : 0); +} + +runTests();