From 1443a40e77e2b74a97015aeb35df4194ec6a981e Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Fri, 18 Sep 2026 10:36:57 +0200 Subject: [PATCH] fix(scripts): format only a path that names one file in the checkout (#15050) The pre-commit formatter rewrites the file it is given, and accepted a staged path whenever its last component was a regular file. That is not enough to know the bytes it rewrites belong to the commit. rustfmt truncates rather than replaces, so the inode survives and every other name for it is rewritten too. Git stages a hardlink like any other file and reports the tree clean, so an in-checkout `.rs` link to a file anywhere on the machine reached the formatter, and formatting the commit's own file rewrote the other one, with nothing in `git status` or the diff to say so. The paths that resolve out of the checkout were not reachable: git refuses to index one beyond a symbolic link, and reports one whose directory became a link as deleted from the working tree, which the unstaged check already holds back. The rule is the script's now rather than a consequence of those behaviors and the order of two checks. A staged path is formatted only when it names one regular file inside the checkout, and one that leads nowhere is named rather than fatal: `ENOTDIR` and `ELOOP` used to escape as a stat error and abort the commit. The formatter receives the resolved path, so it does not walk the links again; the window between the check and the open stays open, as Node offers no way to open a path one component at a time. Follow-up to https://github.com/pnpm/pnpm/pull/15045, which merged before the review comments this answers. --- pnpm/scripts/format-staged-rust.mjs | 50 ++++++++++---- pnpm/scripts/format-staged-rust.test.mjs | 85 +++++++++++++++++++++++- 2 files changed, 120 insertions(+), 15 deletions(-) diff --git a/pnpm/scripts/format-staged-rust.mjs b/pnpm/scripts/format-staged-rust.mjs index 2d4e9ada01..8bd1a170bf 100644 --- a/pnpm/scripts/format-staged-rust.mjs +++ b/pnpm/scripts/format-staged-rust.mjs @@ -31,14 +31,14 @@ export function formatStagedRust (repo, { format = pinnedRustfmt } = {}) { console.error(`pre-commit: not formatting these files, each has unstaged changes too:\n${indent(withheld)}`) } if (foreign.length > 0) { - console.error(`pre-commit: not formatting these paths, none is a regular file in the checkout:\n${indent(foreign)}`) + console.error(`pre-commit: not formatting these paths, each must name one regular file inside the checkout:\n${indent(foreign)}`) } - if (formattable.length === 0) return 0 + if (formattable.size === 0) return 0 - const status = format(formattable.map(file => path.join(root, file))) + const status = format([...formattable.values()]) if (status !== 0) return status - const reformatted = formattable.filter(file => git(repo, ['diff', '--quiet', '--', literal(file)]).status !== 0) + const reformatted = [...formattable.keys()].filter(file => git(repo, ['diff', '--quiet', '--', literal(file)]).status !== 0) if (reformatted.length === 0) return 0 checkedGit(repo, ['add', '--', ...reformatted.map(literal)]) @@ -51,29 +51,51 @@ export function formatStagedRust (repo, { format = pinnedRustfmt } = {}) { * * A file that also has unstaged changes is held back: formatting the working * tree and staging the result would commit the part of that file the author - * deliberately kept out of this commit. A path that is not a regular file is - * not a source at all — rustfmt writes through a symlink, so a staged `.rs` - * link pointing out of the checkout would have it rewrite a file the commit - * never touches, and the repository would show nothing changed. + * deliberately kept out of this commit. A path that does not lead to a regular + * file inside the checkout is not a source at all — rustfmt writes through a + * symlink, so such a path would have it rewrite a file the commit never + * touches, and the repository would show nothing changed. */ function partitionStaged (root, staged, unstaged) { const alsoUnstaged = new Set(unstaged) - const formattable = [] + const formattable = new Map() const withheld = [] const foreign = [] for (const file of staged) { - if (!isRegularFile(path.join(root, file))) foreign.push(file) + const source = checkedOutSource(root, file) + if (source == null) foreign.push(file) else if (alsoUnstaged.has(file)) withheld.push(file) - else formattable.push(file) + else formattable.set(file, source) } return { formattable, withheld, foreign } } -function isRegularFile (absolute) { +/** + * The resolved path of a staged file that the formatter may rewrite in place, + * or null when the staged path is not one. + * + * rustfmt truncates the file it is given, so every other name that reaches the + * same bytes is rewritten with it. A path qualifies only when it names one + * regular file inside the checkout: not a symbolic link, not a second name for + * an inode that something outside the repository also holds, and not a path + * that resolves beyond `root`, which must already be resolved itself. + * + * The resolved path is what the caller hands the formatter. Passing the staged + * path on instead would have the formatter walk the links again, which is a + * second chance to arrive somewhere else. + */ +export function checkedOutSource (root, file) { + const absolute = path.join(root, file) try { - return fs.lstatSync(absolute).isFile() + const stats = fs.lstatSync(absolute) + if (!stats.isFile() || stats.nlink > 1) return null + const real = fs.realpathSync(absolute) + return real.startsWith(root + path.sep) ? real : null } catch (error) { - if (error.code === 'ENOENT') return false + // The three ways a path can fail to lead anywhere: nothing of that name, + // a non-directory used as one, a cycle of links. Each is a staged path to + // reject and report, not a reason to fail the commit. + if (['ENOENT', 'ENOTDIR', 'ELOOP'].includes(error.code)) return null throw error } } diff --git a/pnpm/scripts/format-staged-rust.test.mjs b/pnpm/scripts/format-staged-rust.test.mjs index dbf6ed8e18..1c054dcb79 100644 --- a/pnpm/scripts/format-staged-rust.test.mjs +++ b/pnpm/scripts/format-staged-rust.test.mjs @@ -1,8 +1,10 @@ import assert from 'node:assert/strict' +import console from 'node:console' import fs from 'node:fs' +import os from 'node:os' import path from 'node:path' import { test } from 'node:test' -import { formatStagedRust } from './format-staged-rust.mjs' +import { checkedOutSource, formatStagedRust } from './format-staged-rust.mjs' import { git, temporaryRepo } from './git-fixture.mjs' // Names git hands back verbatim that a shell would have split or expanded, or @@ -126,3 +128,84 @@ test('refuses to format a staged symlink', { skip: process.platform === 'win32' assert.deepEqual(seen, []) assert.equal(fs.readFileSync(outside, 'utf8'), 'fn outside() {}\n') }) + +test('refuses a staged path whose directory became a link out of the checkout', { skip: process.platform === 'win32' }, (context) => { + const repo = repoWithSources(context, ['real.rs']) + fs.mkdirSync(path.join(repo, 'dir')) + fs.writeFileSync(path.join(repo, 'dir', 'nested.rs'), 'fn nested() {}\n') + git(repo, 'add', '--all') + + const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'pnpm-format-outside-')) + context.after(() => fs.rmSync(outside, { recursive: true, force: true })) + fs.writeFileSync(path.join(outside, 'nested.rs'), 'fn nested() {}\n') + fs.rmSync(path.join(repo, 'dir'), { recursive: true }) + fs.symlinkSync(outside, path.join(repo, 'dir')) + + const reported = context.mock.method(console, 'error', () => {}) + const { seen, format } = reformatter() + assert.equal(formatStagedRust(repo, { format }), 0) + + assert.deepEqual(seen, []) + assert.match(reported.mock.calls[0].arguments[0], /each must name one regular file inside the checkout:\n {2}dir\/nested\.rs/) + assert.equal(fs.readFileSync(path.join(outside, 'nested.rs'), 'utf8'), 'fn nested() {}\n') +}) + +test('rejects a source path that resolves outside the checkout', { skip: process.platform === 'win32' }, (context) => { + const repo = temporaryRepo(context, 'pnpm-format-contain-') + fs.writeFileSync(path.join(repo, 'inside.rs'), 'fn inside() {}\n') + const outside = `${repo}-outside.rs` + fs.writeFileSync(outside, 'fn outside() {}\n') + context.after(() => fs.rmSync(outside, { force: true })) + + fs.mkdirSync(path.join(repo, 'real-dir')) + fs.writeFileSync(path.join(repo, 'real-dir', 'nested.rs'), 'fn nested() {}\n') + fs.symlinkSync(path.join(repo, 'real-dir'), path.join(repo, 'link-dir')) + + assert.equal(checkedOutSource(repo, 'inside.rs'), path.join(repo, 'inside.rs')) + // The path the formatter is handed is the resolved one, not the one that + // still has a link to walk. + assert.equal(checkedOutSource(repo, 'link-dir/nested.rs'), path.join(repo, 'real-dir', 'nested.rs')) + assert.equal(checkedOutSource(repo, `../${path.basename(outside)}`), null) + assert.equal(checkedOutSource(repo, 'missing.rs'), null) +}) + +test('refuses to format a staged hardlink to a file outside the checkout', { skip: process.platform === 'win32' }, (context) => { + const repo = repoWithSources(context, ['real.rs']) + const outside = `${repo}-outside.rs` + fs.writeFileSync(outside, 'fn outside() {}\n') + context.after(() => fs.rmSync(outside, { force: true })) + fs.linkSync(outside, path.join(repo, 'linked.rs')) + git(repo, 'add', '--all') + + const reported = context.mock.method(console, 'error', () => {}) + const { seen, format } = reformatter() + assert.equal(formatStagedRust(repo, { format }), 0) + + assert.deepEqual(seen, []) + assert.match(reported.mock.calls[0].arguments[0], /must name one regular file inside the checkout:\n {2}linked\.rs/) + assert.equal(fs.readFileSync(outside, 'utf8'), 'fn outside() {}\n') +}) + +// Each replacement makes `dir/nested.rs` fail to resolve with a different +// errno, and every one of them used to abort the commit. +for (const [errno, replaceDirectory] of [ + ['ENOTDIR', (dir) => fs.writeFileSync(dir, 'no longer a directory\n')], + ['ELOOP', (dir) => fs.symlinkSync(path.basename(dir), dir)], +]) { + test(`reports a staged path that stops resolving with ${errno}`, { skip: process.platform === 'win32' }, (context) => { + const repo = repoWithSources(context, ['real.rs']) + fs.mkdirSync(path.join(repo, 'dir')) + fs.writeFileSync(path.join(repo, 'dir', 'nested.rs'), 'fn nested() {}\n') + git(repo, 'add', '--all') + fs.rmSync(path.join(repo, 'dir'), { recursive: true }) + replaceDirectory(path.join(repo, 'dir')) + assert.throws(() => fs.lstatSync(path.join(repo, 'dir', 'nested.rs')), { code: errno }) + + const reported = context.mock.method(console, 'error', () => {}) + const { seen, format } = reformatter() + assert.equal(formatStagedRust(repo, { format }), 0) + + assert.deepEqual(seen, []) + assert.match(reported.mock.calls[0].arguments[0], /must name one regular file inside the checkout:\n {2}dir\/nested\.rs/) + }) +}