Skip to content

Commit 880b993

Browse files
committed
fix(runtime): search the whole rule for stray modules, not two directories
`strayModules` listed the immediate entries of the rule root and `captures/` and nothing else, so any nested directory evaded it completely. Create `<rule>/lib/helper.ts`, have `check.ts` import it, and the rule discovered as clean while unsigned code ran and `check.ts` still matched its blessed signature. The threat model was stated correctly and implemented as something narrower. "One import away" is not "one directory away": `import "./lib/helper.ts"` reaches an arbitrarily deep relative path in a single hop, exactly like `import "./helper.ts"`. Bounding the search to two directories left the hole this slice exists to close, in the slice that closes it. Now walks the whole rule directory, excluding `.tests/` at any depth for the reason already documented: fixtures are data a check reads, and a fixture that is itself TypeScript is the normal case. Also considers only files. `readdir` returns directories too, so a directory named `helper.ts` would have been reported as a stray module — a false positive refusing a fine rule with an error naming something that cannot run. `withFileTypes` plus `isFile()` was needed for the recursion anyway. An unreadable directory now propagates instead of being swallowed, matching `isMissingDirectory`'s contract: reading `EACCES` as "nothing here" would make an unsearchable rule indistinguishable from a clean one, which is the same failure this whole guard exists to prevent. Four new cases cover the gap — `lib/helper.ts`, `captures/nested/helper.js`, `a/b/c/deep.mjs`, and a directory named `helper.ts`. Reverting the recursion fails exactly those and leaves the three root-level cases passing, so they detect the gap rather than the guard's existence. Found by the review on #217.
1 parent b0ca3cd commit 880b993

4 files changed

Lines changed: 80 additions & 17 deletions

File tree

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ relative import away — so tampering with it bypasses the gate while `check.ts`
6969
still matches its blessed digest. Such a rule is refused, and `verify` names
7070
the file.
7171

72-
Every extension a check could import is covered, not only `.ts`. A rule's
73-
`.tests/` fixtures are excluded, since a check reads real files under a root
74-
and a fixture that is itself TypeScript is ordinary.
72+
Every extension a check could import is covered, not only `.ts`, and the
73+
search is recursive: "one import away" is not "one directory away", so a
74+
nested directory would otherwise carry unsigned code straight past the check.
75+
A rule's `.tests/` fixtures are excluded at any depth, since a check reads
76+
real files under a root and a fixture that is itself TypeScript is ordinary.

‎openspec/changes/generator-payload-alignment/specs/cli-runtime-rule-execution/spec.md‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,20 @@ this requirement makes the guarantee enforced rather than trusted.
5656
"Module file" SHALL cover every extension a check could import, not only `.ts`: the loader
5757
transpiles TypeScript, but a `.js` sibling resolves just as readily.
5858

59+
The search SHALL be recursive. "One import away" is not "one directory away" — a single
60+
`import "./lib/helper.ts"` reaches an arbitrarily deep relative path in one hop — so a search
61+
bounded to the rule root would let any nested directory carry unsigned code past the check while
62+
`check.ts` still matched its blessed signature. The search SHALL also consider only files, so a
63+
directory whose name ends in a module extension is not reported as one.
64+
5965
The rule's `.tests/` directory SHALL be excluded from this search. A runtime check reads real
6066
files under a root, so a fixture that is itself TypeScript is the normal case rather than a
6167
smuggled helper, and refusing it would break correct rules — a worse failure than the one being
6268
prevented.
6369

6470
#### Scenario: A helper module beside check.ts is refused
6571

66-
- **WHEN** a runtime rule directory contains `check.ts` and any other module file, at its root or under `captures/`
72+
- **WHEN** a runtime rule directory contains `check.ts` and any other module file, at any depth
6773
- **THEN** the CLI SHALL refuse the rule and name the offending file
6874
- **AND** SHALL NOT execute its `check.ts`
6975

‎packages/cli/src/rules/runtime/discover.ts‎

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,14 @@ import { join } from "node:path";
33

44
import { parse } from "yaml";
55

6+
import { isMissingDirectory } from "../errno";
7+
68
import {
79
MATCH_MODES,
810
SUPPORTED_METADATA_VERSIONS,
911
} from "../../types/runtime-rule";
1012
import type { CaptureRule, MatchMode } from "../../types/runtime-rule";
11-
import { RULES_DIRECTORY } from "../layout";
13+
import { RULES_DIRECTORY, RULE_TESTS_DIRECTORY } from "../layout";
1214

1315
/**
1416
* Directory (relative to `.taskless/`) that holds runtime rules — the
@@ -82,22 +84,46 @@ const MODULE_EXTENSIONS = [".ts", ".mts", ".cts", ".js", ".mjs", ".cjs"];
8284

8385
export async function strayModules(ruleDirectory: string): Promise<string[]> {
8486
const stray: string[] = [];
85-
for (const relative of ["", CAPTURES_DIRECTORY]) {
86-
const directory =
87-
relative === "" ? ruleDirectory : join(ruleDirectory, relative);
88-
let entries: string[];
87+
88+
// RECURSIVE, because "one import away" is not "one directory away". A single
89+
// `import "./lib/helper.ts"` reaches an arbitrarily deep relative path in one
90+
// hop, so scanning only the rule root and `captures/` would let any nested
91+
// directory carry unsigned code straight past this check while `check.ts`
92+
// still matched its blessed signature.
93+
async function walk(directory: string, prefix: string): Promise<void> {
94+
let entries;
8995
try {
90-
entries = await readdir(directory);
91-
} catch {
92-
continue;
96+
entries = await readdir(directory, { withFileTypes: true });
97+
} catch (error) {
98+
// A directory that is genuinely absent contributes nothing. Anything
99+
// else is a real IO problem and must not be read as "nothing here",
100+
// because that is indistinguishable from a clean rule.
101+
if (isMissingDirectory(error)) return;
102+
throw error;
93103
}
94-
for (const name of entries) {
95-
if (relative === "" && name === CHECK_FILE) continue;
96-
if (MODULE_EXTENSIONS.some((extension) => name.endsWith(extension))) {
97-
stray.push(relative === "" ? name : `${relative}/${name}`);
104+
for (const entry of entries) {
105+
const relative = prefix === "" ? entry.name : `${prefix}/${entry.name}`;
106+
if (entry.isDirectory()) {
107+
// Fixtures are data the check reads, not modules it imports; see the
108+
// note above on why searching them would refuse correct rules.
109+
if (relative === RULE_TESTS_DIRECTORY) continue;
110+
await walk(join(directory, entry.name), relative);
111+
continue;
112+
}
113+
// `isFile()` rather than a name test: a DIRECTORY named `helper.ts`
114+
// is not an executable file, and reporting it would refuse a fine rule
115+
// with an error naming something that cannot run.
116+
if (!entry.isFile()) continue;
117+
if (relative === CHECK_FILE) continue;
118+
if (
119+
MODULE_EXTENSIONS.some((extension) => entry.name.endsWith(extension))
120+
) {
121+
stray.push(relative);
98122
}
99123
}
100124
}
125+
126+
await walk(ruleDirectory, "");
101127
return stray.toSorted();
102128
}
103129

‎packages/cli/test/engine-dispatch.test.ts‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,11 @@ describe("engine dispatch by directory", () => {
440440
["helper.mjs", "helper.mjs"],
441441
// Reachable from check.ts by a relative import just the same.
442442
["captures/helper.ts", "captures/helper.ts"],
443+
// "One import away" is not "one directory away": a single
444+
// `import "./lib/helper.ts"` reaches any depth in one hop.
445+
["lib/helper.ts", "lib/helper.ts"],
446+
["captures/nested/helper.js", "captures/nested/helper.js"],
447+
["a/b/c/deep.mjs", "a/b/c/deep.mjs"],
443448
])(
444449
"refuses a runtime rule carrying %s beside check.ts",
445450
async (path, named) => {
@@ -455,6 +460,12 @@ describe("engine dispatch by directory", () => {
455460
"utf8"
456461
);
457462
await writeFile(join(rule, "check.ts"), RUNTIME_CHECK, "utf8");
463+
// Parent computed from the delivered path rather than via `dirname`, to
464+
// keep this test off the shared import line.
465+
const slash = path.lastIndexOf("/");
466+
if (slash > 0) {
467+
await mkdir(join(rule, path.slice(0, slash)), { recursive: true });
468+
}
458469
await writeFile(join(rule, path), "export const x = 1;\n", "utf8");
459470

460471
// Only check.ts is signed, so a second module is code reachable from a
@@ -470,6 +481,23 @@ describe("engine dispatch by directory", () => {
470481
}
471482
);
472483

484+
it("does not report a directory whose name ends in a module extension", async () => {
485+
// `readdir` returns directories too. A directory named `helper.ts` cannot
486+
// execute, so refusing the rule would be a false positive naming something
487+
// that is not a file.
488+
const rule = ruleDirectory(temporaryDirectory, "runtime", "logs-abc12345");
489+
await mkdir(join(rule, "captures"), { recursive: true });
490+
await mkdir(join(rule, "helper.ts"), { recursive: true });
491+
await writeFile(
492+
join(rule, "captures", "logs.yml"),
493+
RUNTIME_CAPTURE,
494+
"utf8"
495+
);
496+
await writeFile(join(rule, "check.ts"), RUNTIME_CHECK, "utf8");
497+
498+
expect(await discoverRuntimeRules(temporaryDirectory)).toHaveLength(1);
499+
});
500+
473501
it("allows a TypeScript fixture under .tests/", async () => {
474502
// A runtime check reads real files under a root, so a fixture that is
475503
// legitimately TypeScript is the normal case rather than a smuggled
@@ -484,8 +512,9 @@ describe("engine dispatch by directory", () => {
484512
"utf8"
485513
);
486514
await writeFile(join(rule, "check.ts"), RUNTIME_CHECK, "utf8");
515+
await mkdir(join(rule, ".tests", "valid", "deep"), { recursive: true });
487516
await writeFile(
488-
join(rule, ".tests", "valid", "sample.ts"),
517+
join(rule, ".tests", "valid", "deep", "sample.ts"),
489518
"console.log('x');\n",
490519
"utf8"
491520
);

0 commit comments

Comments
 (0)