Skip to content

Commit bae313d

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(file-safety): refuse to publish a staging entry replaced after validation (#2391)
1 parent a39f461 commit bae313d

2 files changed

Lines changed: 322 additions & 1 deletion

File tree

‎src/services/file-safety/__tests__/safeWriteText.spec.ts‎

Lines changed: 251 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@ import { execFile } from "child_process"
44
import type { ChildProcess } from "child_process"
55
import * as path from "path"
66

7-
import { PostCommitDurabilityError, resolveLockKey, safeWriteText, StagingPathError } from "../safeWriteText"
7+
import {
8+
PostCommitDurabilityError,
9+
resolveLockKey,
10+
safeWriteText,
11+
StagingPathError,
12+
StagingReplacedError,
13+
} from "../safeWriteText"
814

915
// Full mock for fs/promises — all methods are vi.fn() stubs
1016
vi.mock("fs/promises", () => ({
@@ -71,6 +77,37 @@ function _fileStatsWithIdentity(ino: bigint, dev: bigint): fsSync.BigIntStats {
7177
s.dev = dev
7278
return s
7379
}
80+
// The same stand-in for a stat that carries no bigint identity: pass numbers or undefined
81+
// here to model a platform whose lstat answer has no bigint ino or dev to compare.
82+
function _statsCarrying(ino: unknown, dev: unknown): fsSync.BigIntStats {
83+
// Double assertion: the runtime value is the Stats stand-in, the type is the bigint
84+
// variant. The fields come from unknown on purpose, so a test can hand over a value
85+
// that is not bigint and watch the comparison decline to run.
86+
const stats = _fileStats(false) as unknown as fsSync.BigIntStats
87+
stats.ino = ino as bigint
88+
stats.dev = dev as bigint
89+
return stats
90+
}
91+
92+
// Stands in for fs.lstat and answers only the identity reads - the calls that pass
93+
// { bigint: true } - from a queue, in the order this write performs them: the
94+
// staging guard's file, the target that file must not be, the staged entry once this
95+
// write's content landed, the entry at the commit rename, and the entry that landed on
96+
// the target. Every other read gets a stat without identity. A dry queue fails the test
97+
// loudly instead of passing quietly.
98+
function _lstatIdentityReads(identityReads: fsSync.BigIntStats[]) {
99+
return async (readPath: unknown, options?: unknown): Promise<fsSync.BigIntStats> => {
100+
const wantsBigint = typeof options === "object" && options !== null && Reflect.get(options, "bigint") === true
101+
if (!wantsBigint) {
102+
return _statsCarrying(undefined, undefined)
103+
}
104+
const next = identityReads.shift()
105+
if (next === undefined) {
106+
throw new Error(`no identity read left in the queue for ${String(readPath)}`)
107+
}
108+
return next
109+
}
110+
}
74111

75112
function mockDefaults(): void {
76113
vi.resetAllMocks()
@@ -1278,6 +1315,219 @@ describe("caller-supplied staging path", () => {
12781315
expect(fs.rename).not.toHaveBeenCalled()
12791316
})
12801317

1318+
it("refuses to publish when the caller's staging entry is replaced before the commit rename", async () => {
1319+
const targetPath = "/tmp/test-dir/target.txt"
1320+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1321+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1322+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1323+
vi.mocked(fs.lstat).mockImplementation(
1324+
_lstatIdentityReads([
1325+
_fileStatsWithIdentity(42n, 7n),
1326+
_fileStatsWithIdentity(43n, 7n),
1327+
_fileStatsWithIdentity(42n, 7n),
1328+
_fileStatsWithIdentity(99n, 7n),
1329+
]),
1330+
)
1331+
1332+
let failure: unknown
1333+
await safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }).catch(
1334+
(error: unknown) => {
1335+
failure = error
1336+
},
1337+
)
1338+
if (!(failure instanceof StagingReplacedError)) {
1339+
throw new Error(`expected a StagingReplacedError, got ${String(failure)}`)
1340+
}
1341+
expect(failure.name).toBe("StagingReplacedError")
1342+
expect(failure.stage).toBe("before-commit")
1343+
expect(failure.message).toContain(stagingPath)
1344+
expect(failure.message).toContain("no longer names")
1345+
1346+
// The publish was skipped, not merely reported as a failure: no rename of any kind ran,
1347+
// so the peer's file was never committed onto the target.
1348+
expect(fs.rename).not.toHaveBeenCalled()
1349+
// The write got as far as staging its content, so this is not the early rejection the
1350+
// staging guard performs before any work.
1351+
expect(fsSync.openSync).toHaveBeenCalledTimes(1)
1352+
})
1353+
1354+
it("reports a replaced staging entry when the swap lands on the target through the commit rename", async () => {
1355+
const targetPath = "/tmp/test-dir/target.txt"
1356+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1357+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1358+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1359+
vi.mocked(fs.lstat).mockImplementation(
1360+
_lstatIdentityReads([
1361+
_fileStatsWithIdentity(42n, 7n),
1362+
_fileStatsWithIdentity(43n, 7n),
1363+
_fileStatsWithIdentity(42n, 7n),
1364+
_fileStatsWithIdentity(42n, 7n),
1365+
_fileStatsWithIdentity(99n, 7n),
1366+
]),
1367+
)
1368+
1369+
let failure: unknown
1370+
await safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }).catch(
1371+
(error: unknown) => {
1372+
failure = error
1373+
},
1374+
)
1375+
if (!(failure instanceof StagingReplacedError)) {
1376+
throw new Error(`expected a StagingReplacedError, got ${String(failure)}`)
1377+
}
1378+
expect(failure.name).toBe("StagingReplacedError")
1379+
expect(failure.stage).toBe("after-commit")
1380+
expect(failure.message).toContain(stagingPath)
1381+
expect(failure.message).toContain("was replaced before the commit rename landed")
1382+
1383+
// The swap landed inside the window between the pre-commit comparison and the rename, so
1384+
// the rename ran and moved a file this write did not stage.
1385+
expect(fs.rename).toHaveBeenCalledTimes(1)
1386+
expect(fs.rename).toHaveBeenCalledWith(stagingPath, targetPath)
1387+
})
1388+
1389+
it("publishes when the entry at the commit rename is still the entry this write staged", async () => {
1390+
const targetPath = "/tmp/test-dir/target.txt"
1391+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1392+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1393+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1394+
vi.mocked(fs.lstat).mockImplementation(
1395+
_lstatIdentityReads([
1396+
_fileStatsWithIdentity(42n, 7n),
1397+
_fileStatsWithIdentity(43n, 7n),
1398+
_fileStatsWithIdentity(42n, 7n),
1399+
_fileStatsWithIdentity(42n, 7n),
1400+
_fileStatsWithIdentity(42n, 7n),
1401+
]),
1402+
)
1403+
1404+
await expect(
1405+
safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }),
1406+
).resolves.toBeUndefined()
1407+
expect(fs.rename).toHaveBeenCalledTimes(1)
1408+
expect(fs.rename).toHaveBeenCalledWith(stagingPath, targetPath)
1409+
})
1410+
1411+
it("refuses to publish when the staged entry and the entry at the commit share an inode but differ in device", async () => {
1412+
const targetPath = "/tmp/test-dir/target.txt"
1413+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1414+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1415+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1416+
vi.mocked(fs.lstat).mockImplementation(
1417+
_lstatIdentityReads([
1418+
_fileStatsWithIdentity(42n, 7n),
1419+
_fileStatsWithIdentity(43n, 7n),
1420+
_fileStatsWithIdentity(42n, 7n),
1421+
_fileStatsWithIdentity(42n, 8n),
1422+
]),
1423+
)
1424+
1425+
let failure: unknown
1426+
await safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }).catch(
1427+
(error: unknown) => {
1428+
failure = error
1429+
},
1430+
)
1431+
if (!(failure instanceof StagingReplacedError)) {
1432+
throw new Error(`expected a StagingReplacedError, got ${String(failure)}`)
1433+
}
1434+
expect(failure.name).toBe("StagingReplacedError")
1435+
expect(failure.stage).toBe("before-commit")
1436+
expect(failure.message).toContain(stagingPath)
1437+
expect(failure.message).toContain("no longer names")
1438+
1439+
// Same inode on a different device is a different file: comparing inodes alone would publish it.
1440+
})
1441+
1442+
it("skips the comparison when the staged entry's stats carry no bigint device", async () => {
1443+
const targetPath = "/tmp/test-dir/target.txt"
1444+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1445+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1446+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1447+
vi.mocked(fs.lstat).mockImplementation(
1448+
_lstatIdentityReads([
1449+
_statsCarrying(42n, 7),
1450+
_statsCarrying(43n, 7),
1451+
_statsCarrying(42n, 7),
1452+
_statsCarrying(42n, 8),
1453+
]),
1454+
)
1455+
1456+
await expect(
1457+
safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }),
1458+
).resolves.toBeUndefined()
1459+
expect(fs.rename).toHaveBeenCalledTimes(1)
1460+
expect(fs.rename).toHaveBeenCalledWith(stagingPath, targetPath)
1461+
1462+
// Nothing bigint to compare, so this is the same reading the staging-alias guard already applies.
1463+
})
1464+
1465+
it("skips the comparison when the staged entry's stats carry no bigint inode", async () => {
1466+
const targetPath = "/tmp/test-dir/target.txt"
1467+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1468+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1469+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1470+
vi.mocked(fs.lstat).mockImplementation(
1471+
_lstatIdentityReads([
1472+
_statsCarrying(42, 7n),
1473+
_statsCarrying(43, 7n),
1474+
_statsCarrying(42, 7n),
1475+
_statsCarrying(43, 7n),
1476+
]),
1477+
)
1478+
1479+
await expect(
1480+
safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }),
1481+
).resolves.toBeUndefined()
1482+
expect(fs.rename).toHaveBeenCalledTimes(1)
1483+
expect(fs.rename).toHaveBeenCalledWith(stagingPath, targetPath)
1484+
})
1485+
1486+
it("publishes when the entry at the commit rename cannot be compared", async () => {
1487+
const targetPath = "/tmp/test-dir/target.txt"
1488+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1489+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1490+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1491+
vi.mocked(fs.lstat).mockImplementation(
1492+
_lstatIdentityReads([
1493+
_fileStatsWithIdentity(42n, 7n),
1494+
_fileStatsWithIdentity(43n, 7n),
1495+
_fileStatsWithIdentity(42n, 7n),
1496+
_statsCarrying(undefined, undefined),
1497+
_statsCarrying(undefined, undefined),
1498+
]),
1499+
)
1500+
1501+
await expect(
1502+
safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }),
1503+
).resolves.toBeUndefined()
1504+
expect(fs.rename).toHaveBeenCalledTimes(1)
1505+
expect(fs.rename).toHaveBeenCalledWith(stagingPath, targetPath)
1506+
1507+
// The guard declines rather than guessing: an unreadable comparison must not refuse a sound write.
1508+
})
1509+
1510+
it("publishes when the staged entry carries no identity but the entry at the commit does", async () => {
1511+
const targetPath = "/tmp/test-dir/target.txt"
1512+
const stagingPath = "/tmp/test-dir/caller-staged.txt"
1513+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1514+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1515+
vi.mocked(fs.lstat).mockImplementation(
1516+
_lstatIdentityReads([
1517+
_statsCarrying(undefined, undefined),
1518+
_statsCarrying(43n, 7n),
1519+
_statsCarrying(undefined, undefined),
1520+
_fileStatsWithIdentity(99n, 7n),
1521+
_fileStatsWithIdentity(99n, 7n),
1522+
]),
1523+
)
1524+
1525+
await expect(
1526+
safeWriteText(targetPath, "data", { tempPath: stagingPath, platform: "linux" }),
1527+
).resolves.toBeUndefined()
1528+
expect(fs.rename).toHaveBeenCalledTimes(1)
1529+
expect(fs.rename).toHaveBeenCalledWith(stagingPath, targetPath)
1530+
})
12811531
it("rejects a staging path that is a symlink", async () => {
12821532
const targetPath = "/tmp/test-dir/target.txt"
12831533
vi.mocked(fs.realpath).mockResolvedValue(targetPath)

‎src/services/file-safety/safeWriteText.ts‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,52 @@ export class PostCommitDurabilityError extends Error {
8686
this.backupPath = backupPath
8787
}
8888
}
89+
/**
90+
* The staging entry this write validated, wrote and fsynced was found to name another
91+
* file at the commit. The commit rename publishes whatever the staging name holds the
92+
* moment it runs, so a peer that replaced the entry between validation and publication
93+
* would otherwise have its own content committed onto the target while this write
94+
* reported success. Reported as its own error so a failed write cannot claim a publish
95+
* it did not make.
96+
*/
97+
export class StagingReplacedError extends Error {
98+
readonly stagingPath: string
99+
/** Where it was noticed: before anything was published, or after the commit rename landed. */
100+
readonly stage: "before-commit" | "after-commit"
101+
102+
constructor(stagingPath: string, stage: "before-commit" | "after-commit") {
103+
super(
104+
stage === "before-commit"
105+
? `Staging entry ${stagingPath} no longer names the file this write staged; the commit rename was skipped and the target was left as it was.`
106+
: `Staging entry ${stagingPath} was replaced before the commit rename landed; the target now holds a file this write did not stage.`,
107+
)
108+
this.name = "StagingReplacedError"
109+
this.stagingPath = stagingPath
110+
this.stage = stage
111+
}
112+
}
113+
89114
// -- helpers ---------------------------------------------------------------
90115

116+
/**
117+
* The device and inode a path names at this moment, or undefined when no comparison can
118+
* be made: the read failed, or the platform's stats do not carry bigint identity. The
119+
* caller reads undefined as "nothing to compare", the same reading this module already
120+
* applies to the staging-alias guard, which rejects only when both stats are bigint.
121+
*/
122+
async function _entryIdentity(absolutePath: string): Promise<{ dev: bigint; ino: bigint } | undefined> {
123+
const stats = await fs.lstat(absolutePath, { bigint: true }).catch(() => undefined)
124+
if (stats === undefined || typeof stats.dev !== "bigint" || typeof stats.ino !== "bigint") {
125+
return undefined
126+
}
127+
return { dev: stats.dev, ino: stats.ino }
128+
}
129+
130+
/** Both call sites have already refused a comparison when either side is missing, */
131+
function _sameEntry(a: { dev: bigint; ino: bigint }, b: { dev: bigint; ino: bigint }): boolean {
132+
return a.ino === b.ino && a.dev === b.dev
133+
}
134+
91135
/** Generate a unique temp file name in the given directory. */
92136
function _tempName(dir: string, prefix: string): string {
93137
return path.join(dir, "." + prefix + "_" + Date.now() + "_" + Math.random().toString(36).substring(2) + ".tmp")
@@ -289,6 +333,8 @@ export async function safeWriteText(
289333
// with an empty .file-safety-staging directory behind. Track the directory this
290334
// write created so its cleanup removes its own directory, not a shared one.
291335
let stagingDir: string | null = null
336+
// Identity of the entry this write staged, captured after our own write and fsync landed.
337+
let stagedEntryIdentity: { dev: bigint; ino: bigint } | undefined
292338
let tempPath: string
293339
if (options?.tempPath) {
294340
// A caller-supplied staging file is only safe when it is the file this
@@ -422,6 +468,12 @@ export async function safeWriteText(
422468
}
423469
}
424470

471+
// Record what the staging name holds now that our content is written and fsynced. The
472+
// commit renames by path, so this is the identity the publish is checked against: an
473+
// entry swapped out from under this write has to show up here or at the commit, not
474+
// pass unreported.
475+
stagedEntryIdentity = await _entryIdentity(tempPath)
476+
425477
// -- Step 2 (win32): save DACL BEFORE the backup copy -----------
426478
const platform = options?.platform ?? process.platform
427479
// Warning delivery must never abort the write: the notices below describe a
@@ -533,9 +585,28 @@ export async function safeWriteText(
533585
}
534586

535587
// -- Step 4: atomic rename temp -> target ---------------------
588+
// The name about to be renamed must still hold the file this write staged, by device
589+
// and inode: the same identity the staging guard already compares.
590+
if (stagedEntryIdentity !== undefined) {
591+
const entryAtCommit = await _entryIdentity(tempPath)
592+
if (entryAtCommit !== undefined && !_sameEntry(stagedEntryIdentity, entryAtCommit)) {
593+
throw new StagingReplacedError(tempPath, "before-commit")
594+
}
595+
}
536596
await fs.rename(tempPath, targetPath)
537597
committed = true
538598

599+
// And the file that landed on the target must be the file this write staged. A
600+
// replacement that slipped inside the window above still shows up here, because the
601+
// commit moves our own inode: a different device and inode at the target means the
602+
// publish carried somebody else's content, and the caller must not be told otherwise.
603+
if (stagedEntryIdentity !== undefined) {
604+
const landedIdentity = await _entryIdentity(targetPath)
605+
if (landedIdentity !== undefined && !_sameEntry(stagedEntryIdentity, landedIdentity)) {
606+
throw new StagingReplacedError(tempPath, "after-commit")
607+
}
608+
}
609+
539610
// -- Step 4b (POSIX): fsync the parent directory so the directory entry
540611
// changed by the commit rename is durable, not just the file content.
541612
if (platform !== "win32") {

0 commit comments

Comments
 (0)