Skip to content

Commit f457c69

Browse files
steipetesteipete
committed
fix: pin standalone file reads
1 parent 8361123 commit f457c69

4 files changed

Lines changed: 181 additions & 14 deletions

File tree

‎src/json.ts‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import { randomUUID } from "node:crypto";
2-
import fsSync, { readFileSync } from "node:fs";
2+
import fsSync from "node:fs";
33
import fs from "node:fs/promises";
44
import path from "node:path";
5+
import { readRegularFile, readRegularFileSync } from "./regular-file.js";
56

67
const JSON_FILE_MODE = 0o600;
78
const JSON_DIR_MODE = 0o700;
@@ -102,7 +103,7 @@ function writeTempJsonFile(pathname: string, payload: string) {
102103

103104
export function loadJsonFile<T = unknown>(pathname: string): T | undefined {
104105
try {
105-
const raw = fsSync.readFileSync(pathname, "utf8");
106+
const raw = readRegularFileSync({ filePath: pathname }).buffer.toString("utf8");
106107
return JSON.parse(raw) as T;
107108
} catch {
108109
return undefined;
@@ -173,22 +174,31 @@ async function replaceFileWithWindowsFallback(tempPath: string, filePath: string
173174

174175
export async function readJsonFile<T>(filePath: string): Promise<T | null> {
175176
try {
176-
const raw = await fs.readFile(filePath, "utf8");
177+
const raw = (await readRegularFile({ filePath })).buffer.toString("utf8");
177178
return JSON.parse(raw) as T;
178179
} catch {
179180
return null;
180181
}
181182
}
182183

183184
export async function readJsonFileStrict<T>(filePath: string): Promise<T> {
184-
const raw = await fs.readFile(filePath, "utf8");
185-
return JSON.parse(raw) as T;
185+
let raw: string;
186+
try {
187+
raw = (await readRegularFile({ filePath })).buffer.toString("utf8");
188+
} catch (err) {
189+
throw new JsonFileReadError(filePath, "read", err);
190+
}
191+
try {
192+
return JSON.parse(raw) as T;
193+
} catch (err) {
194+
throw new JsonFileReadError(filePath, "parse", err);
195+
}
186196
}
187197

188198
export async function readDurableJsonFile<T>(filePath: string): Promise<T | null> {
189199
let raw: string;
190200
try {
191-
raw = await fs.readFile(filePath, "utf8");
201+
raw = (await readRegularFile({ filePath })).buffer.toString("utf8");
192202
} catch (err) {
193203
if (getErrorCode(err) === "ENOENT") {
194204
return null;
@@ -204,7 +214,7 @@ export async function readDurableJsonFile<T>(filePath: string): Promise<T | null
204214

205215
export function readJsonFileSync(filePath: string): unknown {
206216
try {
207-
const raw = readFileSync(filePath, "utf8");
217+
const raw = readRegularFileSync({ filePath }).buffer.toString("utf8");
208218
return JSON.parse(raw) as unknown;
209219
} catch {
210220
return null;

‎src/regular-file.ts‎

Lines changed: 79 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { Stats } from "node:fs";
22
import fsSync from "node:fs";
33
import fs from "node:fs/promises";
44
import path from "node:path";
5+
import { sameFileIdentity } from "./file-identity.js";
56
import { isNotFoundPathError } from "./path.js";
67
import { assertNoSymlinkParents, assertNoSymlinkParentsSync } from "./symlink-parents.js";
78

@@ -34,6 +35,15 @@ export function resolveRegularFileAppendFlags(
3435
);
3536
}
3637

38+
function resolveRegularFileReadFlags(): number {
39+
return (
40+
fsSync.constants.O_RDONLY |
41+
(typeof fsSync.constants.O_NOFOLLOW === "number" && process.platform !== "win32"
42+
? fsSync.constants.O_NOFOLLOW
43+
: 0)
44+
);
45+
}
46+
3747
export async function statRegularFile(filePath: string): Promise<RegularFileStatResult> {
3848
let stat: Stats;
3949
try {
@@ -77,11 +87,67 @@ export async function readRegularFile(params: {
7787
if (params.maxBytes !== undefined && result.stat.size > params.maxBytes) {
7888
throw new Error(`File exceeds ${params.maxBytes} bytes: ${params.filePath}`);
7989
}
80-
const buffer = await fs.readFile(params.filePath);
90+
91+
const handle = await fs.open(params.filePath, resolveRegularFileReadFlags());
92+
try {
93+
const stat = await handle.stat();
94+
verifyStableReadTarget({
95+
filePath: params.filePath,
96+
pathStat: await fs.lstat(params.filePath),
97+
postOpenStat: stat,
98+
preOpenStat: result.stat,
99+
});
100+
if (params.maxBytes !== undefined && stat.size > params.maxBytes) {
101+
throw new Error(`File exceeds ${params.maxBytes} bytes: ${params.filePath}`);
102+
}
103+
const buffer = await handle.readFile();
104+
if (params.maxBytes !== undefined && buffer.byteLength > params.maxBytes) {
105+
throw new Error(`File exceeds ${params.maxBytes} bytes: ${params.filePath}`);
106+
}
107+
return { buffer, stat };
108+
} finally {
109+
await handle.close();
110+
}
111+
}
112+
113+
function verifyStableReadTarget(params: {
114+
preOpenStat: Stats;
115+
postOpenStat: Stats;
116+
pathStat: Stats;
117+
filePath: string;
118+
}): void {
119+
if (!params.postOpenStat.isFile() || params.pathStat.isSymbolicLink() || !params.pathStat.isFile()) {
120+
throw new Error(`File is not a regular file: ${params.filePath}`);
121+
}
122+
if (
123+
!sameFileIdentity(params.preOpenStat, params.postOpenStat) ||
124+
!sameFileIdentity(params.pathStat, params.postOpenStat)
125+
) {
126+
throw new Error(`File changed during read: ${params.filePath}`);
127+
}
128+
}
129+
130+
function readOpenedRegularFileSync(params: {
131+
fd: number;
132+
filePath: string;
133+
preOpenStat: Stats;
134+
maxBytes?: number;
135+
}): { buffer: Buffer; stat: Stats } {
136+
const stat = fsSync.fstatSync(params.fd);
137+
verifyStableReadTarget({
138+
filePath: params.filePath,
139+
pathStat: fsSync.lstatSync(params.filePath),
140+
postOpenStat: stat,
141+
preOpenStat: params.preOpenStat,
142+
});
143+
if (params.maxBytes !== undefined && stat.size > params.maxBytes) {
144+
throw new Error(`File exceeds ${params.maxBytes} bytes: ${params.filePath}`);
145+
}
146+
const buffer = fsSync.readFileSync(params.fd);
81147
if (params.maxBytes !== undefined && buffer.byteLength > params.maxBytes) {
82148
throw new Error(`File exceeds ${params.maxBytes} bytes: ${params.filePath}`);
83149
}
84-
return { buffer, stat: result.stat };
150+
return { buffer, stat };
85151
}
86152

87153
export function readRegularFileSync(params: { filePath: string; maxBytes?: number }): {
@@ -95,11 +161,18 @@ export function readRegularFileSync(params: { filePath: string; maxBytes?: numbe
95161
if (params.maxBytes !== undefined && result.stat.size > params.maxBytes) {
96162
throw new Error(`File exceeds ${params.maxBytes} bytes: ${params.filePath}`);
97163
}
98-
const buffer = fsSync.readFileSync(params.filePath);
99-
if (params.maxBytes !== undefined && buffer.byteLength > params.maxBytes) {
100-
throw new Error(`File exceeds ${params.maxBytes} bytes: ${params.filePath}`);
164+
165+
const fd = fsSync.openSync(params.filePath, resolveRegularFileReadFlags());
166+
try {
167+
return readOpenedRegularFileSync({
168+
fd,
169+
filePath: params.filePath,
170+
preOpenStat: result.stat,
171+
maxBytes: params.maxBytes,
172+
});
173+
} finally {
174+
fsSync.closeSync(fd);
101175
}
102-
return { buffer, stat: result.stat };
103176
}
104177

105178
function verifyStableAppendTarget(params: {

‎test/json.test.ts‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import fs from "node:fs/promises";
22
import os from "node:os";
33
import path from "node:path";
4-
import { afterEach, describe, expect, it } from "vitest";
4+
import { afterEach, describe, expect, it, vi } from "vitest";
55
import {
66
JsonFileReadError,
77
createAsyncLock,
@@ -50,6 +50,36 @@ describe("json file helpers", () => {
5050
} satisfies Partial<JsonFileReadError>);
5151
});
5252

53+
it("does not follow symlink swaps while reading", async () => {
54+
const root = await tempRoot("fs-safe-json-swap-");
55+
const filePath = path.join(root, "state.json");
56+
const secretPath = path.join(root, "secret.json");
57+
await fs.writeFile(filePath, "{\"ok\":true}", "utf8");
58+
await fs.writeFile(secretPath, "{\"secret\":true}", "utf8");
59+
60+
const originalLstat = fs.lstat.bind(fs);
61+
let swapped = false;
62+
const lstatSpy = vi.spyOn(fs, "lstat").mockImplementation(async (...args) => {
63+
const stat = await originalLstat(...args);
64+
if (!swapped && args[0] === filePath) {
65+
swapped = true;
66+
await fs.rm(filePath, { force: true });
67+
await fs.symlink(secretPath, filePath);
68+
}
69+
return stat;
70+
});
71+
72+
try {
73+
await expect(readJsonFileStrict(filePath)).rejects.toMatchObject({
74+
name: "JsonFileReadError",
75+
reason: "read",
76+
} satisfies Partial<JsonFileReadError>);
77+
await expect(readJsonFile(filePath)).resolves.toBeNull();
78+
} finally {
79+
lstatSpy.mockRestore();
80+
}
81+
});
82+
5383
it("serializes work through createAsyncLock", async () => {
5484
const lock = createAsyncLock();
5585
const events: string[] = [];

‎test/new-primitives.test.ts‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import {
1414
import {
1515
appendRegularFile,
1616
appendRegularFileSync,
17+
readRegularFile,
18+
readRegularFileSync,
1719
resolveRegularFileAppendFlags,
1820
statRegularFile,
1921
} from "../src/regular-file.js";
@@ -229,6 +231,58 @@ describe("regular file append", () => {
229231
code: "ENOENT",
230232
});
231233
});
234+
235+
it("pins regular file reads against symlink swaps", async () => {
236+
const filePath = path.join(root, "read-target.txt");
237+
const secretPath = path.join(root, "read-secret.txt");
238+
await fs.writeFile(filePath, "safe", "utf8");
239+
await fs.writeFile(secretPath, "secret", "utf8");
240+
241+
const originalLstat = fs.lstat.bind(fs);
242+
let swapped = false;
243+
const lstatSpy = vi.spyOn(fs, "lstat").mockImplementation(async (...args) => {
244+
const stat = await originalLstat(...args);
245+
if (!swapped && args[0] === filePath) {
246+
swapped = true;
247+
await fs.rm(filePath, { force: true });
248+
await fs.symlink(secretPath, filePath);
249+
}
250+
return stat;
251+
});
252+
253+
try {
254+
await expect(readRegularFile({ filePath })).rejects.toThrow();
255+
await expect(fs.readFile(secretPath, "utf8")).resolves.toBe("secret");
256+
} finally {
257+
lstatSpy.mockRestore();
258+
}
259+
});
260+
261+
it("pins sync regular file reads against symlink swaps", async () => {
262+
const filePath = path.join(root, "sync-read-target.txt");
263+
const secretPath = path.join(root, "sync-read-secret.txt");
264+
await fs.writeFile(filePath, "safe", "utf8");
265+
await fs.writeFile(secretPath, "secret", "utf8");
266+
267+
const originalLstatSync = syncFs.lstatSync.bind(syncFs);
268+
let swapped = false;
269+
const lstatSpy = vi.spyOn(syncFs, "lstatSync").mockImplementation((...args) => {
270+
const stat = originalLstatSync(...args);
271+
if (!swapped && args[0] === filePath) {
272+
swapped = true;
273+
syncFs.rmSync(filePath, { force: true });
274+
syncFs.symlinkSync(secretPath, filePath);
275+
}
276+
return stat;
277+
});
278+
279+
try {
280+
expect(() => readRegularFileSync({ filePath })).toThrow();
281+
await expect(fs.readFile(secretPath, "utf8")).resolves.toBe("secret");
282+
} finally {
283+
lstatSpy.mockRestore();
284+
}
285+
});
232286
});
233287

234288
describe("atomic file replacement", () => {

0 commit comments

Comments
 (0)