Skip to content

Commit 1dabcfa

Browse files
committed
fix(rules): reject delivered paths that collide before writing any of them
Two ways a file set passed assessment and then failed mid-write, leaving the half-written rule directory this module exists to make impossible. ONE PATH BEING AN ANCESTOR OF ANOTHER. `check.ts` and `check.ts/nested.ts` are not duplicates, neither is unsafe, and the set can still be complete — so every check passed. Then the same name has to be a file and a directory at once: `mkdir` throws `ENOTDIR` when the parent was already written as a file, `writeFile` throws `EISDIR` when a recursive `mkdir` got there first. Which one depends on array order. In the second order the rule ends up with its captures written and no `check.ts` file, which is the "never blessed, sits in unknown[]" case the completeness check exists to prevent, reached with debris on disk instead of a clean refusal. TWO PATHS DIFFERING ONLY IN CASE. APFS and NTFS are case-insensitive by default, so `captures/Logs.yml` and `captures/logs.yml` are distinct strings and one file: the second write clobbers the first and the rule silently loses a capture. Capture names are arbitrary, unlike `check.ts` and the engine's config file, so no other check would have caught it. The duplicate check is now case-folded, and the ancestor check folds too. Both proven to bite: removing the ancestor check fails both orderings and nothing else; removing the case folding fails only the case test. Found by the review on #219.
1 parent 6d103ef commit 1dabcfa

3 files changed

Lines changed: 84 additions & 8 deletions

File tree

‎.changeset/generator-payload-alignment.md‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,9 @@ runtime rule with no `check.ts` is never blessed and is held, and a Vale rule
8989
with no `.vale.ini` has no matcher enabling it and never fires. Both leave
9090
`check` exiting 0.
9191

92-
Delivered paths are refused before anything is written — absolute paths, `..`
93-
segments, backslashes, unnormalized segments, duplicates. The whole set is
94-
assessed as a unit, so a refused delivery leaves no directory behind rather
95-
than a half-written rule that verifies as broken two steps from the cause.
92+
Delivered paths are refused before anything is written: absolute paths, `..`
93+
segments, backslashes, unnormalized segments, duplicates (including two paths
94+
differing only in case, which are one file on a case-insensitive filesystem),
95+
and one path being an ancestor of another. The whole set is assessed as a
96+
unit, so a refused delivery leaves no directory behind rather than a
97+
half-written rule that verifies as broken two steps from the cause.

‎packages/cli/src/rules/deliver.ts‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -148,16 +148,49 @@ export function assessDelivery(
148148
}
149149
const directory = ruleDirectory(cwd, engine, ruleId);
150150

151-
const seen = new Set<string>();
151+
// Case-FOLDED as well as exact. APFS and NTFS are case-insensitive by
152+
// default, so `captures/Logs.yml` and `captures/logs.yml` are two distinct
153+
// strings that become one file on disk: the second write silently clobbers
154+
// the first and the rule is missing a capture nobody was told was dropped.
155+
// Capture names are arbitrary, so nothing else would catch it.
156+
const folded = new Set<string>();
152157
for (const file of files) {
153158
const unsafe = describeUnsafePath(directory, file.path);
154159
if (unsafe !== undefined) {
155160
return { ok: false, reason: `delivered a path that ${unsafe}` };
156161
}
157-
if (seen.has(file.path)) {
158-
return { ok: false, reason: `delivered ${file.path} twice` };
162+
const key = file.path.toLowerCase();
163+
if (folded.has(key)) {
164+
return {
165+
ok: false,
166+
reason:
167+
key === file.path
168+
? `delivered ${file.path} twice`
169+
: `delivered ${file.path} twice, differing only in case, which is one file on a case-insensitive filesystem`,
170+
};
171+
}
172+
folded.add(key);
173+
}
174+
175+
// One delivered path being an ANCESTOR of another is the same file needing
176+
// to be a file and a directory at once. It passes every check above — no
177+
// path is unsafe, none is a duplicate, the set can still be complete — and
178+
// then fails mid-write: `mkdir` throws `ENOTDIR` when the parent was already
179+
// written as a file, and `writeFile` throws `EISDIR` when a recursive
180+
// `mkdir` got there first. Which one depends on array order, and either way
181+
// the rule directory is left half-written, which is precisely the state this
182+
// module exists to make impossible.
183+
for (const file of files) {
184+
const segments = file.path.split("/");
185+
for (let index = 1; index < segments.length; index += 1) {
186+
const ancestor = segments.slice(0, index).join("/");
187+
if (folded.has(ancestor.toLowerCase())) {
188+
return {
189+
ok: false,
190+
reason: `delivered ${ancestor} as both a file and a directory (with ${file.path})`,
191+
};
192+
}
159193
}
160-
seen.add(file.path);
161194
}
162195

163196
const incomplete = describeIncompleteSet(engine, ruleId, files);

‎packages/cli/test/deliver.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,47 @@ describe("delivering a rule as a file set", () => {
165165
).rejects.toThrow(/twice/);
166166
});
167167

168+
it.each([
169+
// Either order fails mid-write, differently: `mkdir` throws ENOTDIR when
170+
// the parent was already written as a file, `writeFile` throws EISDIR when
171+
// a recursive mkdir got there first.
172+
["file before directory", ["check.ts", "check.ts/nested.ts"]],
173+
["directory before file", ["check.ts/nested.ts", "check.ts"]],
174+
])(
175+
"refuses a path that is an ancestor of another (%s)",
176+
async (_label, [first, second]) => {
177+
await expect(
178+
writeRuleFile(
179+
cwd,
180+
delivered("runtime", "logs-abc12345", [
181+
{ path: first as string, content: "a\n" },
182+
{ path: second as string, content: "b\n" },
183+
{ path: "captures/logs.yml", content: CAPTURE },
184+
])
185+
)
186+
).rejects.toThrow(/both a file and a directory/);
187+
188+
expect(existsSync(ruleDirectory(cwd, "runtime", "logs-abc12345"))).toBe(
189+
false
190+
);
191+
}
192+
);
193+
194+
it("refuses two capture paths differing only in case", async () => {
195+
// On APFS and NTFS these are one file: the second write clobbers the
196+
// first, and the rule loses a capture nobody was told was dropped.
197+
await expect(
198+
writeRuleFile(
199+
cwd,
200+
delivered("runtime", "logs-abc12345", [
201+
{ path: "check.ts", content: "export default async () => [];\n" },
202+
{ path: "captures/logs.yml", content: CAPTURE },
203+
{ path: "captures/Logs.yml", content: CAPTURE },
204+
])
205+
)
206+
).rejects.toThrow(/differing only in case/);
207+
});
208+
168209
it("refuses a payload carrying both files and content", async () => {
169210
const rule = {
170211
id: "logs-abc12345",

0 commit comments

Comments
 (0)