Skip to content

Commit e09e72d

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(file-safety)(#1918 u151): restrict the staging directory before writing content into it
On Windows the 0o700 mode handed to the staging directory is no restriction at all. Measured here, a directory created that way and chmod-synced still listed read for BUILTIN\Users and modify for NT AUTHORITY\Authenticated Users, and a file written into it inherited exactly those rights, so the new content sat readable by every user on the machine until the commit renamed it into place. Step 0 now sets an access list on this write's own staging directory - inheritance dropped, full control for the account the process runs as - before Step 1 opens the staged file. When the restriction command itself fails the step reports it through onWarning and the write goes ahead, which is how the target DACL steps already behave for an icacls that fails. Both shapes below were measured before the step was written. A trustee icacls cannot resolve leaves the directory exactly as it was: a name mapping to no SID exits 1332 with "No mapping between account names and security IDs was done." and an empty principal exits 87 printing its usage text, so the notice names the exit code and the write continues under the rights the directory had. Second, a grant addressed to an account this process does not carry leaves the directory with no right for it at all, and listing, opening and writing inside it then each answer EPERM. The staged open is where that shows up: if it fails with EPERM or EACCES after this write applied the restriction, the step resets the directory it narrowed, says so, and stages under the restored rights; if this write never applied the restriction, the lockout is not its own to undo and the failure reaches the caller unchanged. The mocked rows assert the restriction against the staged open, both trap shapes, and the control in which an unapplied restriction propagates. The NTFS row builds a parent whose access list carries read for other users, narrows the target below it, and reads the staging directory and the staged file's own lists before and after the step.
1 parent c80512a commit e09e72d

3 files changed

Lines changed: 691 additions & 61 deletions

File tree

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

Lines changed: 251 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,49 @@ vi.mock("child_process", () => ({
5555
// Minimal stand-in for the ChildProcess that callback-form execFile returns.
5656
const fakeChild = { kill: () => true } as unknown as ChildProcess
5757

58+
// The writer takes the account name for its Windows grant from os.userInfo(), which on a hosted lane
59+
// is whichever user that lane runs as. This file mocks os.userInfo() with a plain stand-in - a spy on
60+
// an ES module namespace is refused at runtime - so the account named in the grant below is named
61+
// here instead of being inherited from the host, and every row in the file sees the same one.
62+
const ACL_TEST_ACCOUNT = "acl-test-account"
63+
let aclAccountName = ACL_TEST_ACCOUNT
64+
65+
vi.mock("os", async () => {
66+
const actual = await vi.importActual<typeof import("os")>("os")
67+
return {
68+
...actual,
69+
userInfo: () => ({ uid: 0, gid: 0, username: aclAccountName, homedir: "", shell: "" }),
70+
}
71+
})
72+
73+
function _useAccount(name: string): void {
74+
aclAccountName = name
75+
}
76+
77+
// The icacls verbs one write asked for, in the order it asked for them. A win32 self-staged write
78+
// asks for "narrow" (the staging directory, before any content goes into it), then "save" and
79+
// "restore" for the target's DACL, and "reset" only when a narrowing left this process no right to
80+
// stage into the directory at all.
81+
function _icaclsVerb(call: readonly unknown[]): string {
82+
const parts = call
83+
.slice(1)
84+
.flatMap((part: unknown) => (Array.isArray(part) ? part.map((item: unknown) => String(item)) : [String(part)]))
85+
if (parts.includes("/inheritance:r")) {
86+
return "narrow"
87+
}
88+
if (parts.includes("/save")) {
89+
return "save"
90+
}
91+
if (parts.includes("/restore")) {
92+
return "restore"
93+
}
94+
return parts.includes("/reset") ? "reset" : "other"
95+
}
96+
97+
function _icaclsVerbs(calls: readonly (readonly unknown[])[]): string[] {
98+
return calls.map(_icaclsVerb)
99+
}
100+
58101
// Helper that mirrors safeWriteText's path resolution exactly
59102
function _resolvedTarget(filePath: string): string {
60103
return path.resolve(filePath)
@@ -233,10 +276,10 @@ describe("safeWriteText", () => {
233276
expect(fs.rmdir).toHaveBeenCalledTimes(1)
234277
expect(fs.rmdir).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"))
235278
if (process.platform === "win32") {
236-
// default platform is win32: the DACL save + restore still ran
237-
// through the default icacls path (options?.execFileRunner must
238-
// not throw when options is undefined)
239-
expect(vi.mocked(execFile)).toHaveBeenCalledTimes(2)
279+
// default platform is win32: the staging narrowing + the DACL save + restore still ran
280+
// through the default icacls path (options?.execFileRunner must not throw when options is
281+
// undefined)
282+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save", "restore"])
240283
expect(vi.mocked(fs.unlink)).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl"))
241284
}
242285
})
@@ -698,8 +741,9 @@ describe("safeWriteText", () => {
698741
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
699742
await safeWriteText(targetPath, "data", { platform: "win32" })
700743

701-
// icacls dump + restore were called (execFile is callback-based mock)
702-
expect(execFile).toHaveBeenCalledTimes(2)
744+
// the staging directory was narrowed, then the target DACL was dumped and restored onto
745+
// the published file (execFile is a callback-based mock)
746+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save", "restore"])
703747
})
704748

705749
it("non-win32: DACL path is unreachable when platform is not win32", async () => {
@@ -727,8 +771,9 @@ describe("safeWriteText", () => {
727771

728772
// write succeeded despite icacls failure (fallback to plain rename)
729773
expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath)
730-
// one icacls attempt only: a failed DACL apply must not try to restore
731-
expect(execFile).toHaveBeenCalledTimes(1)
774+
// the staging narrowing ran, and then the dump failed: a failed DACL apply must not try to
775+
// restore, so two calls is the whole story here
776+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save"])
732777
})
733778

734779
it("win32: reports that access rights may change when the DACL cannot be saved", async () => {
@@ -760,7 +805,7 @@ describe("safeWriteText", () => {
760805
await safeWriteText(targetPath, "data", { platform: "win32" })
761806

762807
const calls = vi.mocked(execFile).mock.calls
763-
expect(calls).toHaveLength(2)
808+
expect(_icaclsVerbs(calls)).toEqual(["narrow", "save", "restore"])
764809
for (const call of calls) {
765810
expect(call[0]).toBe("icacls")
766811
// An execFile without a timeout waits for its child forever. While it waits,
@@ -792,9 +837,11 @@ describe("safeWriteText", () => {
792837
safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }),
793838
).resolves.toBeUndefined()
794839

795-
expect(execFile).toHaveBeenCalledTimes(1)
840+
// both attempts timed out, and neither is fatal: two bounded calls and no restore.
841+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save"])
796842
expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath)
797843
expect(warnings.filter((m) => m.includes("different access rights"))).toHaveLength(1)
844+
expect(warnings.filter((m) => m.includes("Could not narrow"))).toHaveLength(1)
798845
})
799846

800847
it("win32: reports when the target cannot be checked for DACL preservation", async () => {
@@ -812,7 +859,9 @@ describe("safeWriteText", () => {
812859

813860
await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) })
814861

815-
expect(execFile).not.toHaveBeenCalled()
862+
// The target could not be checked, so nothing was dumped and nothing was restored. The
863+
// staging narrowing answers a different question and still ran.
864+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow"])
816865
expect(warnings.filter((m) => m.includes("Could not check"))).toHaveLength(1)
817866
})
818867

@@ -830,11 +879,12 @@ describe("safeWriteText", () => {
830879

831880
await safeWriteText(targetPath, "data", { platform: "win32" })
832881

833-
// write committed; only the save was attempted (no restore from a failed dump)
882+
// write committed; only the narrowing and the save were attempted (no restore from a
883+
// failed dump)
834884
expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining("safeWriteText_"), targetPath)
835885
expect(fs.rename).toHaveBeenCalledTimes(1)
836-
expect(execFile).toHaveBeenCalledTimes(1)
837-
const saveArgs = vi.mocked(execFile).mock.calls[0]?.[1]
886+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save"])
887+
const saveArgs = vi.mocked(execFile).mock.calls[1]?.[1]
838888
expect(saveArgs?.[1]).toBe("/save")
839889
// the dump path (possibly partially created by icacls) was unlinked
840890
expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining("safeWriteText.acl"))
@@ -847,18 +897,18 @@ describe("safeWriteText", () => {
847897

848898
await safeWriteText(targetPath, "data", { backup: true, platform: "win32" })
849899

850-
// icacls was called twice (save + restore)
851-
expect(execFile).toHaveBeenCalledTimes(2)
900+
// The staging directory is narrowed first, then the target DACL is dumped and restored
901+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save", "restore"])
852902

853-
// First call: save DACL from target before backup rename
854-
const firstCall = vi.mocked(execFile).mock.calls[0]
855-
expect(firstCall[0]).toBe("icacls")
856-
expect(firstCall[1]).toEqual([targetPath, "/save", expect.stringContaining("safeWriteText.acl"), "/T"])
903+
// The dump: save DACL from the target before the backup rename
904+
const dumpCall = vi.mocked(execFile).mock.calls[1]
905+
expect(dumpCall[0]).toBe("icacls")
906+
expect(dumpCall[1]).toEqual([targetPath, "/save", expect.stringContaining("safeWriteText.acl"), "/T"])
857907

858-
// Second call: restore DACL onto directory after commit rename
859-
const secondCall = vi.mocked(execFile).mock.calls[1]
860-
expect(secondCall[0]).toBe("icacls")
861-
expect(secondCall[1]).toEqual([
908+
// The restore: DACL onto the directory after the commit rename
909+
const restoreCall = vi.mocked(execFile).mock.calls[2]
910+
expect(restoreCall[0]).toBe("icacls")
911+
expect(restoreCall[1]).toEqual([
862912
expect.stringContaining("/tmp/test-dir"),
863913
"/restore",
864914
expect.stringContaining("safeWriteText.acl"),
@@ -879,13 +929,18 @@ describe("safeWriteText", () => {
879929
await safeWriteText(targetPath, "data", { backup: true, platform: "win32" })
880930

881931
const callOrder = vi.mocked(execFile).mock.invocationCallOrder
932+
const verbs = _icaclsVerbs(vi.mocked(execFile).mock.calls)
882933
const copyOrder = vi.mocked(fs.copyFile).mock.invocationCallOrder
883934
const renameOrder = vi.mocked(fs.rename).mock.invocationCallOrder
884-
const saveCall = callOrder[0]
885-
const restoreCall = callOrder[1]
935+
const narrowCall = callOrder[verbs.indexOf("narrow")]
936+
const saveCall = callOrder[verbs.indexOf("save")]
937+
const restoreCall = callOrder[verbs.indexOf("restore")]
886938
const backupCopy = copyOrder[0]
887939
const commitRename = renameOrder[0]
888940

941+
// The staging directory is narrowed before anything is staged in it, and the dump is taken
942+
// before the copy that could race a concurrent publish.
943+
expect(narrowCall).toBeLessThan(saveCall)
889944
expect(saveCall).toBeLessThan(backupCopy)
890945
expect(backupCopy).toBeLessThan(commitRename)
891946
expect(commitRename).toBeLessThan(restoreCall)
@@ -897,12 +952,11 @@ describe("safeWriteText", () => {
897952
vi.mocked(fsSync.openSync).mockReturnValue(1)
898953
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
899954

900-
// icacls save succeeds, restore fails
901-
let callCount = 0
902-
vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => {
903-
callCount++
955+
// icacls narrows the staging directory and dumps the target DACL; only the restore fails.
956+
vi.mocked(execFile).mockImplementation((_cmd, args, _opts, cb) => {
957+
const failing = String(args).includes("/restore")
904958
if (typeof cb === "function") {
905-
cb(callCount === 1 ? null : new Error("icacls restore error"), "", "")
959+
cb(failing ? new Error("icacls restore error") : null, "", "")
906960
}
907961
return fakeChild
908962
})
@@ -935,14 +989,178 @@ describe("safeWriteText", () => {
935989

936990
await safeWriteText(targetPath, "data", { platform: "win32" })
937991

938-
// icacls was NOT called (target absent → skip DACL entirely)
939-
expect(execFile).not.toHaveBeenCalled()
992+
// The target was absent, so there was no DACL to save and nothing to restore. Only the
993+
// staging narrowing ran.
994+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow"])
940995

941996
// no dump file created or unlinked
942997
expect(fs.unlink).not.toHaveBeenCalled()
943998
})
944999
})
9451000

1001+
// ── Test 5b: win32 staging-directory access rights ───────────────────────
1002+
1003+
describe("win32 staging DACL", () => {
1004+
// The platform here is an argument, not the host, so these rows run on every lane and assert
1005+
// the order the staging narrowing keeps against the staged write. The row that runs the real
1006+
// icacls against a real NTFS volume lives beside this file as
1007+
// safeWriteText.stagingDacl.spec.ts.
1008+
it("win32: narrows the staging directory before any content is staged in it", async () => {
1009+
const targetPath = "/tmp/test-dir/target.txt"
1010+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1011+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1012+
const narrowCalls: string[] = []
1013+
const openSyncCallsAtNarrow: number[] = []
1014+
vi.mocked(execFile).mockImplementation((_cmd, args, _opts, cb) => {
1015+
if (String(args).includes("/inheritance:r")) {
1016+
// Nothing may have been opened in staging yet: the narrowing has to beat the staged
1017+
// open, because Windows hands a new file the rights its directory holds at the
1018+
// moment the file is created.
1019+
narrowCalls.push(String(args))
1020+
openSyncCallsAtNarrow.push(vi.mocked(fsSync.openSync).mock.calls.length)
1021+
}
1022+
if (typeof cb === "function") {
1023+
cb(null, "", "")
1024+
}
1025+
return fakeChild
1026+
})
1027+
const warnings: string[] = []
1028+
const account = ACL_TEST_ACCOUNT
1029+
1030+
await safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) })
1031+
1032+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save", "restore"])
1033+
expect(narrowCalls).toHaveLength(1)
1034+
expect(narrowCalls[0]).toContain(".file-safety-staging")
1035+
expect(narrowCalls[0]).toContain("/inheritance:r")
1036+
expect(narrowCalls[0]).toMatch(/:\(OI\)\(CI\)\(F\)$/)
1037+
expect(narrowCalls[0]).toContain(account)
1038+
expect(openSyncCallsAtNarrow).toEqual([0])
1039+
expect(warnings).toEqual([])
1040+
})
1041+
1042+
it("win32: an icacls that cannot map the principal is named by its exit code and does not stop the write", async () => {
1043+
const targetPath = "/tmp/test-dir/target.txt"
1044+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1045+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1046+
// Measured on this machine: a name that maps to no SID makes icacls exit 1332 with that
1047+
// message, and an empty principal makes it exit 87 printing its usage text. Neither applies
1048+
// an access right, so the write proceeds on the rights the directory already had.
1049+
const unmapped = Object.assign(new Error("No mapping between account names and security IDs was done."), {
1050+
code: 1332,
1051+
})
1052+
vi.mocked(execFile).mockImplementation((_cmd, _args, _opts, cb) => {
1053+
if (typeof cb === "function") {
1054+
cb(unmapped, "", "")
1055+
}
1056+
return fakeChild
1057+
})
1058+
const warnings: string[] = []
1059+
1060+
await expect(
1061+
safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }),
1062+
).resolves.toBeUndefined()
1063+
1064+
expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath)
1065+
expect(warnings.filter((m) => m.includes("Could not narrow"))).toHaveLength(1)
1066+
expect(String(warnings[0])).toContain("1332")
1067+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "save"])
1068+
})
1069+
1070+
it("win32: with no account name to grant to, no child is started and the write still commits", async () => {
1071+
const targetPath = "/tmp/test-dir/target.txt"
1072+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1073+
vi.mocked(fsSync.openSync).mockReturnValue(1)
1074+
_useAccount("")
1075+
const savedUsername = process.env.USERNAME
1076+
const savedLogname = process.env.LOGNAME
1077+
process.env.USERNAME = ""
1078+
process.env.LOGNAME = ""
1079+
const warnings: string[] = []
1080+
try {
1081+
await expect(
1082+
safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }),
1083+
).resolves.toBeUndefined()
1084+
} finally {
1085+
process.env.USERNAME = savedUsername
1086+
process.env.LOGNAME = savedLogname
1087+
_useAccount(ACL_TEST_ACCOUNT)
1088+
}
1089+
1090+
expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath)
1091+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["save", "restore"])
1092+
expect(warnings.filter((m) => m.includes("no account name"))).toHaveLength(1)
1093+
})
1094+
1095+
it("win32: a narrowing that leaves no right for this process resets the directory and still stages", async () => {
1096+
const targetPath = "/tmp/test-dir/target.txt"
1097+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1098+
const resetCalls: string[] = []
1099+
// Measured on this machine: with the grant addressed to an account this process does not
1100+
// carry, listing, opening and writing inside the staging directory each answered EPERM. The
1101+
// staged open is where that shows up, so the first open fails and the open after the reset
1102+
// succeeds.
1103+
let firstOpenFailed = false
1104+
vi.mocked(fsSync.openSync).mockImplementation((target: unknown) => {
1105+
if (String(target).includes(".file-safety-staging") && !firstOpenFailed) {
1106+
firstOpenFailed = true
1107+
throw Object.assign(new Error("EPERM: operation not permitted, open"), { code: "EPERM" })
1108+
}
1109+
return 1
1110+
})
1111+
vi.mocked(execFile).mockImplementation((_cmd, args, _opts, cb) => {
1112+
if (String(args).includes("/reset")) {
1113+
resetCalls.push(String(args))
1114+
}
1115+
if (typeof cb === "function") {
1116+
cb(null, "", "")
1117+
}
1118+
return fakeChild
1119+
})
1120+
const warnings: string[] = []
1121+
1122+
await expect(
1123+
safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }),
1124+
).resolves.toBeUndefined()
1125+
1126+
expect(resetCalls).toHaveLength(1)
1127+
expect(resetCalls[0]).toContain(".file-safety-staging")
1128+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow", "reset", "save", "restore"])
1129+
expect(warnings.filter((m) => m.includes("inherited rights were restored"))).toHaveLength(1)
1130+
expect(fs.rename).toHaveBeenCalledWith(expect.stringContaining(".file-safety-staging"), targetPath)
1131+
})
1132+
1133+
it("win32: an EPERM on the staged open still fails the write when this write never narrowed the directory", async () => {
1134+
const targetPath = "/tmp/test-dir/target.txt"
1135+
vi.mocked(fs.realpath).mockResolvedValue(targetPath)
1136+
const resetCalls: string[] = []
1137+
vi.mocked(fsSync.openSync).mockImplementation(() => {
1138+
throw Object.assign(new Error("EPERM: operation not permitted, open"), { code: "EPERM" })
1139+
})
1140+
vi.mocked(execFile).mockImplementation((_cmd, args, _opts, cb) => {
1141+
if (String(args).includes("/reset")) {
1142+
resetCalls.push(String(args))
1143+
}
1144+
if (typeof cb === "function") {
1145+
cb(new Error("icacls error"), "", "")
1146+
}
1147+
return fakeChild
1148+
})
1149+
const warnings: string[] = []
1150+
1151+
await expect(
1152+
safeWriteText(targetPath, "data", { platform: "win32", onWarning: (m) => warnings.push(m) }),
1153+
).rejects.toThrow("EPERM")
1154+
1155+
// The narrowing was attempted and failed, so a lockout is not this write's to undo: no
1156+
// reset, no retry, and the original failure reaches the caller.
1157+
expect(resetCalls).toEqual([])
1158+
expect(_icaclsVerbs(vi.mocked(execFile).mock.calls)).toEqual(["narrow"])
1159+
expect(fs.rename).not.toHaveBeenCalled()
1160+
expect(warnings.filter((m) => m.includes("inherited rights were restored"))).toHaveLength(0)
1161+
})
1162+
})
1163+
9461164
// ── Test 6: pre-written temp path (tempPath option) ──────────────────────
9471165

9481166
describe("pre-written temp path", () => {

0 commit comments

Comments
 (0)