diff --git a/DESIGN.md b/DESIGN.md index d75c668f5a..1e0c5c42bf 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -264,3 +264,26 @@ replicability metadata is deferred to the cluster-level-config work (CORE-3018), schema. Per-peer failures never reject: they come back as `{status: 'failed', reason, node}` entries in `response.replicated[]`, and `message` still reads as success (same contract as drop_schema), so operators must inspect the array for per-node outcomes. + +## A dangling symlink silently truncates the deploy tarball (`components/packageComponent.ts`) + +Packaging uses `tar-fs.pack(dir, { dereference: true })` by default (`skip_symlinks` off). +tar-fs's own walker calls `fs.stat` (not `lstat`) on every discovered entry when dereferencing; a +dangling symlink's target throws `ENOENT`, and tar-fs's `statAll` loop treats _any_ `ENOENT` from +a walk-discovered (not explicitly-requested) entry as end-of-stream — it calls `pack.finalize()` +immediately, silently dropping every entry still queued (BFS order) after the link. No error is +ever emitted, so `packStream.on('error', ...)` never fires and `deploy_component` reports success +on a truncated archive. `scanPackageDirectory()` now pre-walks the tree once (async) to build a +skip-set of dangling symlinks, which `streamPackagedDirectory`'s `tar.pack({ ignore })` consults via +a synchronous `Set.has()` — **`ignore` is called synchronously by tar-fs with no Promise support**, +so any fix here has to resolve the dangling set _before_ constructing `tar.pack`, not from inside +the callback (an earlier draft used `lstatSync`/`statSync` per entry there, which would have added +blocking I/O to a path that also runs inline on the Harper server's event loop via the +`package_component` operation). The scan recurses into _valid_ symlinked directories the same way +tar-fs's dereferenced walk does (readdir through the link), since a dangling symlink nested inside +one is just as capable of tripping the same early-finalize — skipping recursion into symlinked +dirs there would silently reintroduce the bug for that case. Circular directory symlinks are not +guarded against (in the scan or in tar-fs's own pack walk); that's a pre-existing tar-fs limitation +this fix doesn't attempt to solve. `deploy_component`/`package_component` still never validate that +declared entry points (`jsResource`/`graphqlSchema`) survived extraction — a truncation from some +other future cause would still report success silently; that's a deferred, separate fix. diff --git a/bin/cliOperations.ts b/bin/cliOperations.ts index 7e25ba0943..9ff41eef52 100644 --- a/bin/cliOperations.ts +++ b/bin/cliOperations.ts @@ -9,7 +9,7 @@ import { httpRequest } from '../utility/common_utils.ts'; import * as path from 'path'; import * as fs from 'fs-extra'; import * as YAML from 'yaml'; -import { streamPackagedDirectory, getPackagedDirectorySize, packageDirectory } from '../components/packageComponent.ts'; +import { streamPackagedDirectory, packageDirectory, scanPackageDirectory } from '../components/packageComponent.ts'; import { encode as encodeCbor } from 'cbor-x'; import { buildMultipartBody } from './multipartBuilder.ts'; import { parseSSE } from './sseConsumer.ts'; @@ -113,9 +113,20 @@ const PREPARE_OPERATION: any = { // so the pre-gzip onBytes callback can be wired directly to renderer.countUploadBytes. req._projectPath = projectPath; req._packageOptions = packageOptions; - // Pre-walk the directory for an uncompressed-size estimate. Both the progress counter - // and this total are in uncompressed units so the bar tracks to 100% naturally. - req._uploadSizeEstimate = await getPackagedDirectorySize(projectPath, packageOptions); + // Pre-walk the directory once for both the uncompressed-size estimate (progress bar + // total) and the dangling-symlink list — a dangling symlink would otherwise silently + // truncate the tarball (tar-fs finalizes early on the broken target). Packaging skips + // them; the list is reused below (no second walk) and warns the user which links were + // skipped so the omission is visible. + const scan = await scanPackageDirectory(projectPath, packageOptions); + req._uploadSizeEstimate = scan.totalSize; + req._danglingSymlinks = scan.danglingSymlinks; + if (scan.danglingSymlinks.length) { + process.stderr.write( + `warning: skipping ${scan.danglingSymlinks.length} broken symlink(s) — their linked content will NOT be deployed:\n` + + scan.danglingSymlinks.map((p) => ` ${p}\n`).join('') + ); + } req._multipart = true; }, }; @@ -306,7 +317,8 @@ async function cliOperations(req: any, skipResponseLog = false) { const packageStream = streamPackagedDirectory( req._projectPath, req._packageOptions, - renderer ? (n) => renderer.countUploadBytes(n) : undefined + renderer ? (n) => renderer.countUploadBytes(n) : undefined, + req._danglingSymlinks ); const fields = operationFields(req); const multipart = buildMultipartBody(fields, { diff --git a/components/packageComponent.ts b/components/packageComponent.ts index 957cd80f56..2501d12cc5 100644 --- a/components/packageComponent.ts +++ b/components/packageComponent.ts @@ -1,6 +1,6 @@ import { join, relative, sep } from 'node:path'; import { stat, readdir } from 'node:fs/promises'; -import { Readable } from 'node:stream'; +import { Readable, pipeline } from 'node:stream'; import tar from 'tar-fs'; import { createGzip } from 'node:zlib'; @@ -18,7 +18,7 @@ const WEBPACK_CACHE_SEGMENT = join('cache', 'webpack'); * `skip_node_modules` is set. The path is first made relative to `directory`, so packaging a component * that itself lives under a `node_modules/` path — i.e. any npm-installed component — does not match * every entry. tar-fs invokes `ignore` with the absolute path, so a substring test on it wrongly - * excluded the whole tree. Shared by the stream packer and the size walk so the two cannot diverge. + * excluded the whole tree. Shared by the stream packer and the directory walk so they cannot diverge. */ function isExcluded(directory: string, fullPath: string, options: PackageOptions): boolean { if (!options.skip_node_modules) return false; @@ -26,53 +26,39 @@ function isExcluded(directory: string, fullPath: string, options: PackageOptions return rel.split(sep).includes('node_modules') || rel.includes(WEBPACK_CACHE_SEGMENT); } -/** - * Package a directory into a tar+gzip stream. The returned Readable can be - * piped directly into an HTTP request body, avoiding the Node.js 2GB Buffer - * cap that the buffered variant runs into for large components. - * - * @param onBytes - Optional callback invoked with the byte length of each raw - * tar chunk *before* gzip compression. Useful for tracking upload progress - * against an uncompressed-size total (e.g. from `getPackagedDirectorySize`). - */ -export function streamPackagedDirectory( - directory: string, - options: PackageOptions = DEFAULT_OPTIONS, - onBytes?: (n: number) => void -): Readable { - const packStream = tar.pack(directory, { - dereference: !options.skip_symlinks, - ignore: (name: string) => isExcluded(directory, name, options), - map: (header) => { - if (header.type === 'directory') { - header.mode = 0o755; - } - return header; - }, - }); - const gzip = createGzip(); - // Propagate pack errors onto the gzip stream so a single consumer can listen - packStream.on('error', (err) => gzip.destroy(err)); - if (onBytes) { - // Attaching a 'data' listener after pipe() is safe — the stream is already - // in flowing mode and Node's EventEmitter supports multiple listeners. - packStream.on('data', (chunk: Buffer) => onBytes(chunk.length)); - } - return packStream.pipe(gzip); +export interface PackageDirectoryScan { + /** Total uncompressed byte size of the files `streamPackagedDirectory` would pack. */ + totalSize: number; + /** + * Relative paths of symlinks whose target does not exist. Under tar-fs's `dereference: true` + * (our default), such a link makes the walker's stat() throw ENOENT, which tar-fs treats as + * end-of-stream: it *finalizes the archive early*, silently dropping every entry queued after + * the link. `streamPackagedDirectory` skips these paths so the walk continues past them. + */ + danglingSymlinks: string[]; } /** - * Walk `directory` and return the total uncompressed size of all files that - * `streamPackagedDirectory` would include with the same options. Used by the - * CLI to give the upload progress bar a realistic total. The uncompressed size - * won't equal the gzipped wire size, but it gives the bar a steady trajectory: - * the bar moves as bytes are sent and snaps to 100% when the upload finishes. + * Walk `directory` once, computing both the packable size total and the dangling-symlink list — + * the two pieces of metadata the CLI's pre-deploy checks and the packer's ignore-set both need. + * A single shared walker keeps size accounting and dangling-link detection from silently + * diverging (e.g. one recursing into symlinked directories and the other not). + * + * Recurses into a *valid* symlinked directory the same way tar-fs's own dereferenced walk does + * (readdir through the link), so a dangling symlink nested inside one is still found — otherwise + * tar-fs would hit that nested ENOENT and truncate the archive regardless of this scan's result. + * This mirrors tar-fs's own lack of symlink-cycle protection under `dereference`; a circular + * symlink can already hang tar-fs's real pack walk today, so this scan doesn't guard against it + * either — fixing that is a separate, pre-existing limitation outside this change's scope. */ -export async function getPackagedDirectorySize( +export async function scanPackageDirectory( directory: string, options: PackageOptions = DEFAULT_OPTIONS -): Promise { - let total = 0; +): Promise { + let totalSize = 0; + const danglingSymlinks: string[] = []; + const dereference = !options.skip_symlinks; + const walk = async (dir: string): Promise => { let entries; try { @@ -82,16 +68,28 @@ export async function getPackagedDirectorySize( } for (const entry of entries) { const fullPath = join(dir, entry.name); - if (isExcluded(directory, fullPath, options)) { - continue; - } - if (entry.isDirectory()) { + if (isExcluded(directory, fullPath, options)) continue; + if (entry.isSymbolicLink()) { + // Without dereference, tar-fs packs the link literally and never stats the + // target, so an unresolved link is neither a size contributor nor a hazard. + if (!dereference) continue; + let target; + try { + target = await stat(fullPath); // follows the link; throws if missing + } catch { + danglingSymlinks.push(relative(directory, fullPath)); + continue; + } + if (target.isDirectory()) { + await walk(fullPath); + } else { + totalSize += target.size; + } + } else if (entry.isDirectory()) { await walk(fullPath); } else { - if (options.skip_symlinks && entry.isSymbolicLink()) continue; try { - const s = await stat(fullPath); // follows symlinks, matching tar dereference - total += s.size; + totalSize += (await stat(fullPath)).size; } catch { // inaccessible file — skip } @@ -99,7 +97,91 @@ export async function getPackagedDirectorySize( } }; await walk(directory); - return total; + return { totalSize, danglingSymlinks }; +} + +/** + * Package a directory into a tar+gzip stream. The returned Readable can be + * piped directly into an HTTP request body, avoiding the Node.js 2GB Buffer + * cap that the buffered variant runs into for large components. + * + * @param onBytes - Optional callback invoked with the byte length of each raw + * tar chunk *before* gzip compression. Useful for tracking upload progress + * against an uncompressed-size total (e.g. from `getPackagedDirectorySize`). + * @param knownDanglingSymlinks - Skip this function's own directory scan and reuse a + * dangling-symlink list the caller already computed (e.g. via `scanPackageDirectory` + * during a prior CLI step), avoiding a second full-tree walk before packing starts. + */ +export function streamPackagedDirectory( + directory: string, + options: PackageOptions = DEFAULT_OPTIONS, + onBytes?: (n: number) => void, + knownDanglingSymlinks?: string[] +): Readable { + const dereference = !options.skip_symlinks; + const gzip = createGzip(); + // Resolving the dangling-symlink set up front — rather than stat-ing candidates + // synchronously from tar-fs's `ignore` callback — keeps the pack walk free of blocking + // I/O. That matters here because this also runs server-side, inline on the shared event + // loop, via the package_component operation. + const dangling = knownDanglingSymlinks + ? Promise.resolve(knownDanglingSymlinks) + : scanPackageDirectory(directory, options).then((scan) => scan.danglingSymlinks); + dangling + .then((danglingSymlinks) => { + // The consumer may have already aborted (destroying gzip) while the prescan was + // still resolving — skip starting a directory walk nothing will read. + if (gzip.destroyed) return; + const danglingSet = new Set(danglingSymlinks); + const packStream = tar.pack(directory, { + dereference, + ignore: (name: string) => + isExcluded(directory, name, options) || (dereference && danglingSet.has(relative(directory, name))), + map: (header) => { + if (header.type === 'directory') { + header.mode = 0o755; + } + return header; + }, + }); + if (onBytes) { + packStream.on('data', (chunk: Buffer) => onBytes(chunk.length)); + } + // pipeline (rather than a bare .pipe()) destroys both streams on either side + // erroring or the consumer aborting mid-stream, so an aborted upload doesn't leave + // the directory walk running in the background. The callback is a no-op: pipeline + // already surfaces the failure by destroying `gzip` with the error itself. + pipeline(packStream, gzip, () => {}); + }) + .catch((err) => gzip.destroy(err)); + return gzip; +} + +/** + * Total uncompressed size of the files `streamPackagedDirectory` would include with the same + * options. Used by the CLI to give the upload progress bar a realistic total. The uncompressed + * size won't equal the gzipped wire size, but it gives the bar a steady trajectory: the bar + * moves as bytes are sent and snaps to 100% when the upload finishes. + */ +export async function getPackagedDirectorySize( + directory: string, + options: PackageOptions = DEFAULT_OPTIONS +): Promise { + return (await scanPackageDirectory(directory, options)).totalSize; +} + +/** + * Relative paths of dangling symlinks that `streamPackagedDirectory` would skip. The CLI uses + * this to warn the user before deploy, since a dangling link means its intended content is + * absent from the package (and, before the skip guard, would have silently truncated the whole + * tarball). Returns `[]` when `skip_symlinks` is set, because links are then packed literally + * with no dereference. + */ +export async function findDanglingSymlinks( + directory: string, + options: PackageOptions = DEFAULT_OPTIONS +): Promise { + return (await scanPackageDirectory(directory, options)).danglingSymlinks; } /** diff --git a/unitTests/bin/cliOperations.test.js b/unitTests/bin/cliOperations.test.js index 1e76888ade..82c70e22da 100644 --- a/unitTests/bin/cliOperations.test.js +++ b/unitTests/bin/cliOperations.test.js @@ -102,18 +102,18 @@ describe('cliOperations', () => { describe('deploy_component cross-version compatibility', () => { const target = 'https://example.com:9925/'; let originalPackageDirectory; - let originalGetSize; + let originalScan; beforeEach(() => { saveCredentials(target, { operation_token: 'valid-token', refresh_token: 'refresh-token' }); tokenAuthModule.isJWTExpired = () => false; originalPackageDirectory = packageComponentModule.packageDirectory; - originalGetSize = packageComponentModule.getPackagedDirectorySize; + originalScan = packageComponentModule.scanPackageDirectory; }); afterEach(() => { packageComponentModule.packageDirectory = originalPackageDirectory; - packageComponentModule.getPackagedDirectorySize = originalGetSize; + packageComponentModule.scanPackageDirectory = originalScan; }); // Streams an SSE `done` event so the modern (>= 5.1) deploy path can read its result. @@ -155,7 +155,10 @@ describe('cliOperations', () => { it('downgrades a directory deploy to a CBOR binary payload when the target is < 5.1', async () => { const fakeTarball = Buffer.from('fake-tarball-bytes'); - packageComponentModule.getPackagedDirectorySize = async () => fakeTarball.length; + packageComponentModule.scanPackageDirectory = async () => ({ + totalSize: fakeTarball.length, + danglingSymlinks: [], + }); packageComponentModule.packageDirectory = async () => fakeTarball; const calls = []; diff --git a/unitTests/components/packageComponent.test.js b/unitTests/components/packageComponent.test.js index 43dd6ce342..615f39182c 100644 --- a/unitTests/components/packageComponent.test.js +++ b/unitTests/components/packageComponent.test.js @@ -10,7 +10,7 @@ const { Readable } = require('node:stream'); const testUtils = require('../testUtils.js'); testUtils.preTestPrep(); -const { streamPackagedDirectory, packageDirectory } = require('#src/components/packageComponent'); +const { streamPackagedDirectory, packageDirectory, findDanglingSymlinks } = require('#src/components/packageComponent'); const { buildMultipartBody } = require('#src/bin/multipartBuilder'); const { parseMultipartRequest } = require('#src/server/serverHelpers/multipartParser'); const gunzip = require('gunzip-maybe'); @@ -133,6 +133,102 @@ describe('streamPackagedDirectory round-trip', () => { await fs.rm(extractDir, { recursive: true, force: true }); } }); + + it('does not truncate the archive when a dangling symlink is present (regression)', async function () { + this.timeout(15000); + // A dangling symlink used to silently truncate the tarball: under dereference tar-fs + // stat()s the missing target, gets ENOENT, and finalizes the archive early — dropping + // every entry queued after the link, with no error. The packager must now skip the + // broken link and ship the full tree. + const sourceFiles = { + 'package.json': '{"name":"demo","version":"1.0.0"}\n', + 'src/resources/index.js': 'exports.x = 1;\n', + 'tests/a.test.js': 'test();\n', + '.github/workflows/ci.yml': 'name: ci\n', + }; + const sourceDir = await makeFixture(sourceFiles); + // Broken link + a valid link (to real content) that must still be dereferenced. + await fs.mkdir(path.join(sourceDir, '.claude'), { recursive: true }); + await fs.symlink('../../.agents/skills/nope', path.join(sourceDir, '.claude', 'broken')); + await fs.symlink(path.join(sourceDir, 'src'), path.join(sourceDir, 'linked-src')); + const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'pkg-dangling-out-')); + try { + await pipeline( + streamPackagedDirectory(sourceDir, { skip_node_modules: true }), + gunzip(), + tar.extract(extractDir) + ); + const extracted = await readDirTree(extractDir); + // Every real file survives (nothing dropped after the broken link)... + for (const [rel, content] of Object.entries(sourceFiles)) { + assert.strictEqual(extracted[rel], content, `missing/altered ${rel}`); + } + // ...the valid symlink's target content is dereferenced in... + assert.strictEqual(extracted['linked-src/resources/index.js'], sourceFiles['src/resources/index.js']); + // ...and the dangling link itself is not packed as a broken entry. + assert.ok(!Object.keys(extracted).some((k) => k.includes('broken')), 'dangling link should be skipped'); + } finally { + await fs.rm(sourceDir, { recursive: true, force: true }); + await fs.rm(extractDir, { recursive: true, force: true }); + } + }); + + it('findDanglingSymlinks reports broken links and ignores valid ones and node_modules', async function () { + this.timeout(15000); + const sourceDir = await makeFixture({ 'index.js': 'x\n', 'src/a.js': 'a\n' }); + await fs.symlink('./does-not-exist', path.join(sourceDir, 'broken-root')); + await fs.symlink('../nope', path.join(sourceDir, 'src', 'broken-nested')); + await fs.symlink(path.join(sourceDir, 'src'), path.join(sourceDir, 'good')); + // A dangling link inside node_modules must be ignored when skipping node_modules. + await fs.mkdir(path.join(sourceDir, 'node_modules'), { recursive: true }); + await fs.symlink('./gone', path.join(sourceDir, 'node_modules', 'broken-dep')); + try { + const dangling = (await findDanglingSymlinks(sourceDir, { skip_node_modules: true })).sort(); + // 'src/broken-nested' is reported twice — once directly, once via the 'good' alias + // symlink (which points at 'src') — since tar-fs's dereferenced walk would try to + // pack (and fail on) both as distinct archive entries. + assert.deepStrictEqual(dangling, [ + 'broken-root', + path.join('good', 'broken-nested'), + path.join('src', 'broken-nested'), + ]); + // With skip_symlinks the archive packs links literally (no dereference), so nothing to warn. + assert.deepStrictEqual(await findDanglingSymlinks(sourceDir, { skip_symlinks: true }), []); + } finally { + await fs.rm(sourceDir, { recursive: true, force: true }); + } + }); + + it('finds a dangling symlink nested inside a validly-linked directory, and packs the rest (regression)', async function () { + this.timeout(15000); + // tar-fs's dereferenced walk readdirs *through* a valid symlinked directory just like a + // real one, so a dangling link nested inside it is just as capable of truncating the + // archive as one at the top level. The scan must recurse into valid symlinked + // directories to catch this, or packaging will still hit the original bug. + const sourceDir = await makeFixture({ 'real/nested/deep.txt': 'deep\n', 'real/sibling.txt': 'sib\n' }); + await fs.symlink('./gone', path.join(sourceDir, 'real', 'nested', 'broken')); + await fs.symlink(path.join(sourceDir, 'real'), path.join(sourceDir, 'link-to-real')); + const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'pkg-nested-dangling-out-')); + try { + const dangling = (await findDanglingSymlinks(sourceDir)).sort(); + // Reachable — and reported — via both the real path and the symlink alias, matching + // the two distinct archive entries tar-fs would otherwise try (and fail) to stat. + assert.deepStrictEqual(dangling, [ + path.join('link-to-real', 'nested', 'broken'), + path.join('real', 'nested', 'broken'), + ]); + + await pipeline(streamPackagedDirectory(sourceDir), gunzip(), tar.extract(extractDir)); + const extracted = await readDirTree(extractDir); + assert.strictEqual(extracted['real/nested/deep.txt'], 'deep\n'); + assert.strictEqual(extracted['real/sibling.txt'], 'sib\n'); + assert.strictEqual(extracted['link-to-real/nested/deep.txt'], 'deep\n'); + assert.strictEqual(extracted['link-to-real/sibling.txt'], 'sib\n'); + } finally { + await fs.rm(sourceDir, { recursive: true, force: true }); + await fs.rm(extractDir, { recursive: true, force: true }); + } + }); }); // keep eslint happy in case Readable isn't directly used in some branch