Skip to content

Commit ae443ee

Browse files
Chris Shellenbargerclaude
andcommitted
Harden path safety with safePath(), add CodeQL config, add tests
- Replace join() with safePath() at all 25 path construction sites where user-controlled or readdir-sourced data flows into filesystem operations (sessions.ts, message-store.ts, agent-context.ts, privacy.ts) - Add validatePathSegment() to readdir loops in deleteUserData() and runRetentionCleanup() for consistency with exportUserData() - Add .github/codeql/codeql-config.yml excluding js/missing-rate-limiting (false positive: Fastify plugin-based rate limiting not recognized) - Add .github/workflows/codeql.yml for advanced CodeQL setup with config - Add src/path-safety.test.ts with 27 tests covering all validators - Test suite now at 110 tests, all passing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1a03d0b commit ae443ee

8 files changed

Lines changed: 218 additions & 42 deletions

File tree

.github/codeql/codeql-config.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
name: "CodeQL config for mastersof-ai/harness"
2+
3+
# Exclude queries that produce false positives for Fastify's architecture.
4+
# Fastify uses plugin-based global rate limiting (@fastify/rate-limit),
5+
# not per-route middleware. CodeQL's js/missing-rate-limiting rule is
6+
# Express-centric and doesn't recognize this pattern.
7+
query-filters:
8+
- exclude:
9+
id: js/missing-rate-limiting

.github/workflows/codeql.yml

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
name: CodeQL
2+
3+
on:
4+
push:
5+
branches: [master]
6+
pull_request:
7+
branches: [master]
8+
schedule:
9+
- cron: "0 6 * * 1" # Weekly Monday 6am UTC
10+
11+
jobs:
12+
analyze:
13+
runs-on: ubuntu-latest
14+
permissions:
15+
security-events: write
16+
contents: read
17+
18+
steps:
19+
- uses: actions/checkout@v4
20+
21+
- uses: github/codeql-action/init@v3
22+
with:
23+
languages: javascript-typescript
24+
config-file: .github/codeql/codeql-config.yml
25+
26+
- uses: github/codeql-action/analyze@v3

package.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@
3535
"scripts": {
3636
"start": "tsx src/index.tsx",
3737
"dev": "tsx watch src/index.tsx",
38-
"test": "tsx --test src/manifest.test.ts src/agent-context.test.ts src/tools/index.test.ts src/prompt.test.ts src/types/ask-user.test.ts src/integration.test.ts",
39-
"test:unit": "tsx --test src/manifest.test.ts src/agent-context.test.ts src/tools/index.test.ts src/prompt.test.ts src/types/ask-user.test.ts",
38+
"test": "tsx --test src/manifest.test.ts src/agent-context.test.ts src/path-safety.test.ts src/tools/index.test.ts src/prompt.test.ts src/types/ask-user.test.ts src/integration.test.ts",
39+
"test:unit": "tsx --test src/manifest.test.ts src/agent-context.test.ts src/path-safety.test.ts src/tools/index.test.ts src/prompt.test.ts src/types/ask-user.test.ts",
4040
"test:integration": "tsx --test src/integration.test.ts",
4141
"lint": "biome check",
4242
"lint:fix": "biome check --write",

src/agent-context.ts

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { existsSync, mkdirSync, readdirSync } from "node:fs";
22
import { join } from "node:path";
33
import { getHomeDir } from "./config.js";
44
import { type AgentManifest, loadAgentManifest } from "./manifest.js";
5-
import { validateName } from "./path-safety.js";
5+
import { safePath, validateName } from "./path-safety.js";
66

77
export interface AgentContext {
88
name: string;
@@ -36,7 +36,8 @@ export function getAgentsDir(): string {
3636

3737
export function resolveAgent(name: string): AgentContext {
3838
validateName(name, "agent name");
39-
const agentDir = join(getAgentsDir(), name);
39+
const agentsDir = getAgentsDir();
40+
const agentDir = safePath(agentsDir, name);
4041
const identityPath = join(agentDir, "IDENTITY.md");
4142

4243
if (!existsSync(agentDir)) {
@@ -46,7 +47,7 @@ export function resolveAgent(name: string): AgentContext {
4647
throw new AgentNotFoundError(name, `IDENTITY.md not found: ${identityPath}`);
4748
}
4849

49-
const stateDir = join(getHomeDir(), "state", name);
50+
const stateDir = safePath(getHomeDir(), "state", name);
5051
const workspaceDir = join(agentDir, "workspace");
5152
mkdirSync(workspaceDir, { recursive: true });
5253

@@ -77,7 +78,8 @@ export function resolveRemoteAgent(name: string, userId: string): AgentContext {
7778
validateName(name, "agent name");
7879
validateName(userId, "user ID");
7980

80-
const agentDir = join(getAgentsDir(), name);
81+
const agentsDir = getAgentsDir();
82+
const agentDir = safePath(agentsDir, name);
8183
const identityPath = join(agentDir, "IDENTITY.md");
8284

8385
if (!existsSync(agentDir)) {
@@ -87,9 +89,9 @@ export function resolveRemoteAgent(name: string, userId: string): AgentContext {
8789
throw new AgentNotFoundError(name, `IDENTITY.md not found: ${identityPath}`);
8890
}
8991

90-
const stateDir = join(getHomeDir(), "state", name);
91-
const userWorkspaceDir = join(agentDir, "workspace", userId);
92-
const userMemoryDir = join(agentDir, "memory", userId);
92+
const stateDir = safePath(getHomeDir(), "state", name);
93+
const userWorkspaceDir = safePath(agentDir, "workspace", userId);
94+
const userMemoryDir = safePath(agentDir, "memory", userId);
9395

9496
mkdirSync(userWorkspaceDir, { recursive: true, mode: 0o700 });
9597
mkdirSync(userMemoryDir, { recursive: true, mode: 0o700 });
@@ -123,8 +125,8 @@ export async function listAgents(): Promise<AgentManifest[]> {
123125
}
124126

125127
const agentDirs = entries
126-
.filter((e) => e.isDirectory() && existsSync(join(agentsDir, e.name, "IDENTITY.md")))
127-
.map((e) => join(agentsDir, e.name));
128+
.filter((e) => e.isDirectory() && existsSync(safePath(agentsDir, e.name, "IDENTITY.md")))
129+
.map((e) => safePath(agentsDir, e.name));
128130

129131
const results = await Promise.all(
130132
agentDirs.map(async (dir) => {

src/message-store.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { appendFile, mkdir, readFile } from "node:fs/promises";
2-
import { join } from "node:path";
3-
import { validateSessionId } from "./path-safety.js";
2+
import { safePath, validateSessionId } from "./path-safety.js";
43
import type { SessionDirs } from "./sessions.js";
54

65
export interface PersistedMessage {
@@ -12,12 +11,12 @@ export interface PersistedMessage {
1211

1312
function messagesPath(dirs: SessionDirs, sessionId: string): string {
1413
validateSessionId(sessionId);
15-
return join(dirs.sessionsDir, sessionId, "messages.jsonl");
14+
return safePath(dirs.sessionsDir, sessionId, "messages.jsonl");
1615
}
1716

1817
export async function appendMessage(dirs: SessionDirs, sessionId: string, message: PersistedMessage): Promise<void> {
1918
const filePath = messagesPath(dirs, sessionId);
20-
await mkdir(join(dirs.sessionsDir, sessionId), { recursive: true });
19+
await mkdir(safePath(dirs.sessionsDir, sessionId), { recursive: true });
2120
await appendFile(filePath, `${JSON.stringify(message)}\n`, "utf-8");
2221
}
2322

src/path-safety.test.ts

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
import assert from "node:assert";
2+
import { resolve } from "node:path";
3+
import { describe, it } from "node:test";
4+
import { safePath, validateName, validatePathSegment, validateSessionId } from "./path-safety.js";
5+
6+
describe("validatePathSegment", () => {
7+
it("accepts normal strings", () => {
8+
assert.strictEqual(validatePathSegment("hello", "test"), "hello");
9+
assert.strictEqual(validatePathSegment("my-file", "test"), "my-file");
10+
assert.strictEqual(validatePathSegment("abc123", "test"), "abc123");
11+
});
12+
13+
it("returns the segment on success", () => {
14+
const result = validatePathSegment("valid-segment", "label");
15+
assert.strictEqual(result, "valid-segment");
16+
});
17+
18+
it("rejects empty string", () => {
19+
assert.throws(() => validatePathSegment("", "test"), /Invalid test/);
20+
});
21+
22+
it("rejects strings containing '..'", () => {
23+
assert.throws(() => validatePathSegment("..", "test"), /Invalid test/);
24+
assert.throws(() => validatePathSegment("foo/../bar", "test"), /Invalid test/);
25+
});
26+
27+
it("rejects strings containing '/'", () => {
28+
assert.throws(() => validatePathSegment("foo/bar", "test"), /Invalid test/);
29+
assert.throws(() => validatePathSegment("/absolute", "test"), /Invalid test/);
30+
});
31+
32+
it("rejects strings containing '\\'", () => {
33+
assert.throws(() => validatePathSegment("foo\\bar", "test"), /Invalid test/);
34+
});
35+
36+
it("rejects strings containing null byte", () => {
37+
assert.throws(() => validatePathSegment("foo\0bar", "test"), /Invalid test/);
38+
});
39+
});
40+
41+
describe("validateSessionId", () => {
42+
it("accepts valid UUID v4", () => {
43+
const uuid = "550e8400-e29b-41d4-a716-446655440000";
44+
assert.strictEqual(validateSessionId(uuid), uuid);
45+
});
46+
47+
it("accepts uppercase UUID", () => {
48+
const uuid = "550E8400-E29B-41D4-A716-446655440000";
49+
assert.strictEqual(validateSessionId(uuid), uuid);
50+
});
51+
52+
it("returns the id on success", () => {
53+
const uuid = "a1b2c3d4-e5f6-7890-abcd-ef1234567890";
54+
assert.strictEqual(validateSessionId(uuid), uuid);
55+
});
56+
57+
it("rejects empty string", () => {
58+
assert.throws(() => validateSessionId(""), /Invalid session ID/);
59+
});
60+
61+
it("rejects non-UUID strings", () => {
62+
assert.throws(() => validateSessionId("not-a-uuid"), /Invalid session ID/);
63+
assert.throws(() => validateSessionId("hello"), /Invalid session ID/);
64+
assert.throws(() => validateSessionId("../etc"), /Invalid session ID/);
65+
});
66+
67+
it("rejects partial UUIDs", () => {
68+
assert.throws(() => validateSessionId("550e8400-e29b-41d4"), /Invalid session ID/);
69+
assert.throws(() => validateSessionId("550e8400"), /Invalid session ID/);
70+
});
71+
});
72+
73+
describe("validateName", () => {
74+
it("accepts valid names", () => {
75+
assert.strictEqual(validateName("cofounder", "agent"), "cofounder");
76+
assert.strictEqual(validateName("my-agent", "agent"), "my-agent");
77+
assert.strictEqual(validateName("agent_1", "agent"), "agent_1");
78+
assert.strictEqual(validateName("ab", "agent"), "ab");
79+
});
80+
81+
it("returns the name on success", () => {
82+
assert.strictEqual(validateName("researcher", "agent"), "researcher");
83+
});
84+
85+
it("rejects single character", () => {
86+
assert.throws(() => validateName("a", "agent"), /Invalid agent/);
87+
});
88+
89+
it("rejects names with dots", () => {
90+
assert.throws(() => validateName("my.agent", "agent"), /Invalid agent/);
91+
});
92+
93+
it("rejects names with slashes", () => {
94+
assert.throws(() => validateName("my/agent", "agent"), /Invalid agent/);
95+
});
96+
97+
it("rejects names starting with hyphen", () => {
98+
assert.throws(() => validateName("-agent", "agent"), /Invalid agent/);
99+
});
100+
101+
it("rejects names longer than 64 characters", () => {
102+
const longName = "a".repeat(65);
103+
assert.throws(() => validateName(longName, "agent"), /Invalid agent/);
104+
});
105+
106+
it("rejects empty string", () => {
107+
assert.throws(() => validateName("", "agent"), /Invalid agent/);
108+
});
109+
110+
it("rejects path traversal", () => {
111+
assert.throws(() => validateName("../etc", "agent"), /Invalid agent/);
112+
});
113+
});
114+
115+
describe("safePath", () => {
116+
it("returns resolved path for valid segments", () => {
117+
const result = safePath("/tmp/base", "sub", "file.txt");
118+
assert.strictEqual(result, resolve("/tmp/base", "sub", "file.txt"));
119+
});
120+
121+
it("throws on traversal attempt", () => {
122+
assert.throws(() => safePath("/tmp/base", "..", "etc", "passwd"), /Path traversal detected/);
123+
});
124+
125+
it("throws on absolute path injection", () => {
126+
assert.throws(() => safePath("/tmp/base", "/etc/passwd"), /Path traversal detected/);
127+
});
128+
129+
it("accepts base directory itself (no segments)", () => {
130+
const result = safePath("/tmp/base");
131+
assert.strictEqual(result, resolve("/tmp/base"));
132+
});
133+
134+
it("works with nested valid segments", () => {
135+
const result = safePath("/tmp/base", "level1", "level2", "level3");
136+
assert.strictEqual(result, resolve("/tmp/base", "level1", "level2", "level3"));
137+
});
138+
});

0 commit comments

Comments
 (0)