Skip to content

Commit 4d22cc2

Browse files
fix(security): harden file permissions, remove info leaks (#3363, #3365, #3360) (#3375)
* fix(security): harden file permissions, remove info leaks (#3363, #3365, #3360) - #3363: Set mode 0o600 on all state file writes (state.json, pipelines.json, analytics-cache.json, acp-local-storage.json). Previously created world-readable (644), exposing session metadata. - #3365: Remove allowed directories list from workDir validation error messages. The error still says 'not in allowed list' but no longer reveals which directories are allowed on the host. - #3360: Return 404 (not 403) on cross-tenant session access attempts. Previously the error said 'session belongs to another tenant', confirming the session's existence. Now returns the same 'Session not found' as write endpoints for consistency. Refs: #3363, #3365, #3360 * fix(tests): update 403 to 404 for cross-tenant session access (#3360) #3360 changed cross-tenant session access from 403 to 404 to avoid tenant enumeration. Two tests still asserted 403.
1 parent bfdae11 commit 4d22cc2

8 files changed

Lines changed: 17 additions & 20 deletions

File tree

src/__tests__/dashboard-session-auth.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ describe('dashboard session cookie request auth', () => {
200200
url: '/v1/owned/other-tenant',
201201
headers: { cookie: cookieHeader(dashboardSession.sessionId) },
202202
});
203-
expect(otherTenant.statusCode).toBe(403);
203+
expect(otherTenant.statusCode).toBe(404);
204204
});
205205

206206
it('filters global SSE events for tenant-scoped dashboard sessions', () => {

src/__tests__/multitenancy-1944.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ describe('Multi-tenancy (#1944) — requireOwnership with tenant', () => {
400400
const result = requireOwnership(sessions, 's-1', reply, 'globex-key', 'globex');
401401

402402
expect(result).toBeNull();
403-
expect(reply.status).toHaveBeenCalledWith(403);
403+
expect(reply.status).toHaveBeenCalledWith(404);
404404
});
405405

406406
it('allows same-tenant access', async () => {

src/permission-routes.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ function createPermissionHandler(
4141
if (callerTenantId && callerTenantId !== SYSTEM_TENANT && session.tenantId !== callerTenantId) {
4242
const effectiveAudit = getAuditLogger ? getAuditLogger() : audit;
4343
if (effectiveAudit) void effectiveAudit.log(keyId ?? 'system', 'session.action.denied', `Cross-tenant ${matchedPermission} denied on session ${session.id} (tenant: ${session.tenantId})`, session.id, callerTenantId);
44-
return reply.status(403).send({ error: 'SESSION_FORBIDDEN', message: 'Session belongs to another tenant' });
44+
// #3360: Return generic 404 to avoid leaking cross-tenant session existence.
45+
return reply.status(404).send({ error: 'SESSION_NOT_FOUND', message: 'Session not found' });
4546
}
4647

4748
// Feature flag check (#1910): skip enhanced ownership when disabled

src/routes/context.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,8 +221,9 @@ export function requireOwnership(
221221
if (keyId === 'master' || keyId === null || keyId === undefined) return session;
222222
// Issue #2267: Tenant scoping — reject cross-tenant access.
223223
// SYSTEM_TENANT callers bypass scoping. Tenant-scoped callers can only access their own sessions.
224+
// #3360: Return generic 404 to avoid leaking that a cross-tenant session exists.
224225
if (tenantId && tenantId !== SYSTEM_TENANT && session.tenantId !== tenantId) {
225-
reply.status(403).send({ error: 'Forbidden: session belongs to another tenant' });
226+
reply.status(404).send({ error: 'Session not found' });
226227
return null;
227228
}
228229
if (role === 'admin') return session;

src/services/acp/local-storage.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,16 +168,17 @@ export class FileAcpLocalStorageProfile implements AcpLocalStorageProfile {
168168
}
169169
// Issue #3045: atomic write to prevent truncation on SIGTERM/OOM kill
170170
// Issue #3366: error recovery — a failed write must not poison subsequent writes
171+
// #3363: restrict file permissions to owner-only
171172
const content = `${JSON.stringify(serializeState(this.state), null, 2)}\n`;
172173
const tmpFile = `${this.config.filePath}.tmp.${process.pid}`;
173174

174175
const prevChain = this.writeChain;
175176
this.writeChain = prevChain
176177
.then(
177178
// Previous write succeeded — do this write
178-
() => writeFile(tmpFile, content, 'utf8').then(() => rename(tmpFile, this.config.filePath)),
179+
() => writeFile(tmpFile, content, { mode: 0o600 }).then(() => rename(tmpFile, this.config.filePath)),
179180
// Previous write failed — still attempt this write
180-
() => writeFile(tmpFile, content, 'utf8').then(() => rename(tmpFile, this.config.filePath)),
181+
() => writeFile(tmpFile, content, { mode: 0o600 }).then(() => rename(tmpFile, this.config.filePath)),
181182
)
182183
.then(() => {
183184
this.persistError = null;
@@ -195,7 +196,6 @@ export class FileAcpLocalStorageProfile implements AcpLocalStorageProfile {
195196
// Reset chain so next persist() is not chained to a rejected promise
196197
this.writeChain = Promise.resolve();
197198
});
198-
199199
await this.writeChain;
200200
}
201201

src/services/metrics-cache.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ export class JsonFileBackend implements MetricsCacheBackend {
108108
await mkdir(dir, { recursive: true });
109109
}
110110
const tmpFile = `${this.filePath}.tmp`;
111-
await writeFile(tmpFile, JSON.stringify(data, null, 2));
111+
await writeFile(tmpFile, JSON.stringify(data, null, 2), { mode: 0o600 });
112112
await rename(tmpFile, this.filePath);
113113
}
114114
}

src/services/state/JsonFileStore.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
* Issue #1937: Pluggable SessionStore interface.
99
*/
1010

11-
import { readFile, writeFile, rename, mkdir } from 'node:fs/promises';
11+
import { readFile, writeFile, rename, mkdir, chmod } from 'node:fs/promises';
1212
import { existsSync, unlinkSync, readdirSync } from 'node:fs';
1313
import { join, dirname } from 'node:path';
1414
import { Mutex } from 'async-mutex';
@@ -85,7 +85,7 @@ export class JsonFileStore implements StateStore {
8585
if (this.isValidState(parsed)) {
8686
// Write backup of successfully loaded state
8787
try {
88-
await writeFile(`${this.stateFile}.bak`, raw);
88+
await writeFile(`${this.stateFile}.bak`, raw, { mode: 0o600 });
8989
} catch { /* non-critical */ }
9090
return parsed as SerializedSessionState;
9191
}
@@ -112,12 +112,13 @@ export class JsonFileStore implements StateStore {
112112
async save(state: SerializedSessionState): Promise<void> {
113113
// #2793: Use unique temp file per save to prevent ENOENT race when
114114
// concurrent saves (SessionManager.doSave + putSession) overlap.
115+
// #3363: State files contain sensitive metadata — restrict to owner-only.
115116
const dir = dirname(this.stateFile);
116117
if (!existsSync(dir)) {
117118
await mkdir(dir, { recursive: true });
118119
}
119120
const tmpFile = `${this.stateFile}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}`;
120-
await writeFile(tmpFile, JSON.stringify(state, null, 2));
121+
await writeFile(tmpFile, JSON.stringify(state, null, 2), { mode: 0o600 });
121122
await rename(tmpFile, this.stateFile);
122123
}
123124

@@ -199,7 +200,7 @@ export class JsonFileStore implements StateStore {
199200
}
200201

201202
const tmpFile = `${this.pipelineFile}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}`;
202-
await writeFile(tmpFile, JSON.stringify(state, null, 2));
203+
await writeFile(tmpFile, JSON.stringify(state, null, 2), { mode: 0o600 });
203204
await rename(tmpFile, this.pipelineFile);
204205
}
205206

src/validation.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -744,10 +744,7 @@ export async function validateWorkDir(
744744
const resolved = path.resolve(workDir);
745745
const preAllowed = candidateSafeDirs.some((dir) => isUnderOrEqual(resolved, dir));
746746
if (!preAllowed) {
747-
const allowedList = candidateSafeDirs.length <= 5
748-
? candidateSafeDirs.join(', ')
749-
: candidateSafeDirs.slice(0, 5).join(', ') + ` (+${candidateSafeDirs.length - 5} more)`;
750-
return { error: `workDir ${resolved} is not in the allowed directories list. Allowed: ${allowedList}`, code: 'INVALID_WORKDIR' };
747+
return { error: `workDir ${resolved} is not in the allowed directories list`, code: 'INVALID_WORKDIR' };
751748
}
752749

753750
// Step 3: Follow symlinks.
@@ -761,10 +758,7 @@ export async function validateWorkDir(
761758
// Step 4: Canonical allowlist check after symlink resolution.
762759
const allowed = candidateSafeDirs.some((dir) => isUnderOrEqual(realPath, dir));
763760
if (!allowed) {
764-
const allowedList = candidateSafeDirs.length <= 5
765-
? candidateSafeDirs.join(', ')
766-
: candidateSafeDirs.slice(0, 5).join(', ') + ` (+${candidateSafeDirs.length - 5} more)`;
767-
return { error: `workDir ${resolved} is not in the allowed directories list. Allowed: ${allowedList}`, code: 'INVALID_WORKDIR' };
761+
return { error: `workDir ${resolved} is not in the allowed directories list`, code: 'INVALID_WORKDIR' };
768762
}
769763

770764
// Step 5: Ensure workDir is a directory, not a file.

0 commit comments

Comments
 (0)