Skip to content

Commit 8f6550f

Browse files
enoch85enoch85
committed
fix(logs): block path traversal in log file download endpoint
The safeLogFileRegex was unanchored, allowing any string containing a maintainerr-YYYY-MM-DD.log substring to pass validation. Combined with path.join, an attacker could read arbitrary files via URL-encoded traversal segments (e.g. maintainerr-2026-01-01.log%2F..%2F..%2Fetc%2Fpasswd). Anchor the regex and add a defense-in-depth canonical-path check that rejects symlinks and verifies the resolved path stays inside the logs directory.
1 parent 35a4d44 commit 8f6550f

2 files changed

Lines changed: 140 additions & 10 deletions

File tree

‎apps/server/src/modules/logging/logs.controller.spec.ts‎

Lines changed: 112 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,58 @@
11
import {
22
MessageEvent as NestMessageEvent,
3+
HttpException,
4+
HttpStatus,
35
RawBodyRequest,
6+
StreamableFile,
47
} from '@nestjs/common';
58
import { EventEmitter2 } from '@nestjs/event-emitter';
69
import { Response } from 'express';
710
import { EventEmitter } from 'events';
11+
import * as fs from 'fs';
12+
import { lstat, realpath } from 'fs/promises';
813
import { IncomingMessage } from 'http';
14+
import { PassThrough } from 'stream';
915
import { createMockLogger } from '../../../test/utils/data';
1016
import { LogSettingsService } from './logs.service';
1117
import { LogsController } from './logs.controller';
1218

19+
jest.mock('fs', () => {
20+
const actual = jest.requireActual('fs');
21+
return {
22+
...actual,
23+
createReadStream: jest.fn(),
24+
readdir: jest.fn(
25+
(
26+
_dir: string,
27+
callback: (
28+
error: NodeJS.ErrnoException | null,
29+
files: string[],
30+
) => void,
31+
) => {
32+
callback(null, []);
33+
},
34+
),
35+
};
36+
});
37+
38+
jest.mock('fs/promises', () => {
39+
const actual = jest.requireActual('fs/promises');
40+
return {
41+
...actual,
42+
lstat: jest.fn(),
43+
readdir: jest.fn(),
44+
realpath: jest.fn(),
45+
stat: jest.fn(),
46+
};
47+
});
48+
49+
const createReadStreamMock = fs.createReadStream as jest.MockedFunction<
50+
typeof fs.createReadStream
51+
>;
52+
53+
const lstatMock = jest.mocked(lstat);
54+
const realpathMock = jest.mocked(realpath);
55+
1356
class MockSocket extends EventEmitter {
1457
setKeepAlive = jest.fn();
1558
setNoDelay = jest.fn();
@@ -40,14 +83,20 @@ const buildRequest = (): RawBodyRequest<IncomingMessage> =>
4083
socket: new MockSocket(),
4184
}) as unknown as RawBodyRequest<IncomingMessage>;
4285

86+
const createController = () =>
87+
new LogsController(
88+
{} as unknown as LogSettingsService,
89+
new EventEmitter2(),
90+
createMockLogger(),
91+
);
92+
4393
describe('LogsController', () => {
94+
beforeEach(() => {
95+
jest.clearAllMocks();
96+
});
97+
4498
it('removes SSE clients when writing a log event to a closed stream fails', async () => {
45-
const eventEmitter = new EventEmitter2();
46-
const controller = new LogsController(
47-
{} as unknown as LogSettingsService,
48-
eventEmitter,
49-
createMockLogger(),
50-
);
99+
const controller = createController();
51100
const response = new MockResponse();
52101

53102
await controller.stream(response as unknown as Response, buildRequest());
@@ -72,4 +121,61 @@ describe('LogsController', () => {
72121
expect(() => controller.sendDataToClient(clientId, message)).not.toThrow();
73122
expect(controller.connectedClients.size).toBe(0);
74123
});
124+
125+
it('rejects filenames that only contain a valid log filename as a substring', async () => {
126+
const controller = createController();
127+
128+
await controller
129+
.getFile('maintainerr-2026-04-28.log.bak')
130+
.catch((error: HttpException) => {
131+
expect(error.getStatus()).toBe(HttpStatus.BAD_REQUEST);
132+
expect(error.message).toBe('Invalid file');
133+
});
134+
135+
expect(lstatMock).not.toHaveBeenCalled();
136+
expect(realpathMock).not.toHaveBeenCalled();
137+
expect(createReadStreamMock).not.toHaveBeenCalled();
138+
});
139+
140+
it('rejects symlinked log files before opening the stream', async () => {
141+
const controller = createController();
142+
lstatMock.mockResolvedValue({
143+
isFile: () => false,
144+
isSymbolicLink: () => true,
145+
} as Awaited<ReturnType<typeof lstat>>);
146+
147+
await controller
148+
.getFile('maintainerr-2026-04-28.log')
149+
.catch((error: HttpException) => {
150+
expect(error.getStatus()).toBe(HttpStatus.BAD_REQUEST);
151+
expect(error.message).toBe('Invalid file');
152+
});
153+
154+
expect(realpathMock).not.toHaveBeenCalled();
155+
expect(createReadStreamMock).not.toHaveBeenCalled();
156+
});
157+
158+
it('streams regular log files from the resolved canonical path', async () => {
159+
const controller = createController();
160+
const stream = new PassThrough();
161+
lstatMock.mockResolvedValue({
162+
isFile: () => true,
163+
isSymbolicLink: () => false,
164+
} as Awaited<ReturnType<typeof lstat>>);
165+
realpathMock
166+
.mockResolvedValueOnce('/workspaces/Maintainerr/data/logs')
167+
.mockResolvedValueOnce(
168+
'/workspaces/Maintainerr/data/logs/maintainerr-2026-04-28.log',
169+
);
170+
createReadStreamMock.mockReturnValue(
171+
stream as unknown as ReturnType<typeof fs.createReadStream>,
172+
);
173+
174+
const response = await controller.getFile('maintainerr-2026-04-28.log');
175+
176+
expect(response).toBeInstanceOf(StreamableFile);
177+
expect(createReadStreamMock).toHaveBeenCalledWith(
178+
'/workspaces/Maintainerr/data/logs/maintainerr-2026-04-28.log',
179+
);
180+
});
75181
});

‎apps/server/src/modules/logging/logs.controller.ts‎

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {
2222
import { EventEmitter2 } from '@nestjs/event-emitter';
2323
import { Response } from 'express';
2424
import { createReadStream, readdir } from 'fs';
25-
import { readdir as readdirp, stat } from 'fs/promises';
25+
import { lstat, readdir as readdirp, realpath, stat } from 'fs/promises';
2626
import { IncomingMessage } from 'http';
2727
import mime from 'mime-types';
2828
import { ZodValidationPipe } from 'nestjs-zod';
@@ -51,7 +51,10 @@ const logsDirectory =
5151
? '/opt/data/logs'
5252
: path.join(__dirname, `../../../../../data/logs`);
5353

54-
const safeLogFileRegex = /maintainerr-\d{4}-\d{2}-\d{2}\.log(\.gz)?/;
54+
const safeLogFileRegex = /^maintainerr-\d{4}-\d{2}-\d{2}\.log(\.gz)?$/;
55+
56+
const isPathInsideDirectory = (directoryPath: string, candidatePath: string) =>
57+
candidatePath.startsWith(`${directoryPath}${path.sep}`);
5558

5659
@Controller('/api/logs')
5760
export class LogsController implements BeforeApplicationShutdown {
@@ -269,8 +272,29 @@ export class LogsController implements BeforeApplicationShutdown {
269272
}
270273

271274
const filePath = path.join(logsDirectory, file);
272-
const fileMimeType = mime.lookup(filePath);
273-
const fileStream: Readable = createReadStream(filePath);
275+
const fileStats = await lstat(filePath).catch(
276+
(error: NodeJS.ErrnoException) => {
277+
if (error.code === 'ENOENT') {
278+
throw new HttpException('File not found', HttpStatus.NOT_FOUND);
279+
}
280+
281+
throw error;
282+
},
283+
);
284+
if (fileStats.isSymbolicLink() || !fileStats.isFile()) {
285+
throw new HttpException('Invalid file', HttpStatus.BAD_REQUEST);
286+
}
287+
288+
const [resolvedDir, resolvedFilePath] = await Promise.all([
289+
realpath(logsDirectory),
290+
realpath(filePath),
291+
]);
292+
if (!isPathInsideDirectory(resolvedDir, resolvedFilePath)) {
293+
throw new HttpException('Invalid file', HttpStatus.BAD_REQUEST);
294+
}
295+
296+
const fileMimeType = mime.lookup(resolvedFilePath);
297+
const fileStream: Readable = createReadStream(resolvedFilePath);
274298

275299
return new StreamableFile(fileStream, {
276300
type: fileMimeType !== false ? fileMimeType : 'application/octet-stream',

0 commit comments

Comments
 (0)