From 9a0cc883a67abe993c852b7d15597a0ff0fc13cd Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Tue, 8 Sep 2026 08:45:29 -0500 Subject: [PATCH 1/3] fix(notifications): reject unsafe notification paths Validate notification filenames before filesystem access for delete, archive, restore, and creation. Preserve supported filenames and malformed notification cleanup. Work intent: FeatureOS security report 482091. Regression coverage reproduces deletion outside the notification directory on the original implementation. --- .../notifications.service.spec.ts | 94 ++++++++++++++++++- .../notifications/notifications.service.ts | 28 ++++-- 2 files changed, 113 insertions(+), 9 deletions(-) diff --git a/api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.ts b/api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.ts index dcf624a278..ac4a91ab14 100644 --- a/api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.ts +++ b/api/src/unraid-api/graph/resolvers/notifications/notifications.service.spec.ts @@ -10,9 +10,12 @@ import type { TestingModule } from '@nestjs/testing'; import { ConfigService } from '@nestjs/config'; import { Test } from '@nestjs/testing'; import { existsSync } from 'fs'; -import { mkdir } from 'fs/promises'; +import { mkdir, readFile, rm, writeFile } from 'fs/promises'; +import { join } from 'path'; +import { PrefixedID } from '@unraid/shared/prefixed-id-scalar.js'; import { execa } from 'execa'; +import { Kind } from 'graphql'; import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { NotificationIni } from '@app/core/types/states/notification.js'; @@ -283,6 +286,95 @@ describe.sequential('NotificationsService', () => { expect.soft(overview.unread.total).toEqual(0); }); + describe('notification path validation', () => { + const operations = [ + { + name: 'delete unread', + run: (id: string) => service.deleteNotification({ id, type: NotificationType.UNREAD }), + }, + { + name: 'delete archived', + run: (id: string) => service.deleteNotification({ id, type: NotificationType.ARCHIVE }), + }, + { name: 'archive', run: (id: string) => service.archiveNotification({ id }) }, + { name: 'restore', run: (id: string) => service.markAsUnread({ id }) }, + ]; + + it.each(operations)('$name rejects traversal and preserves an outside file', async ({ run }) => { + const outsidePath = join(basePath, 'outside.notify'); + const contents = 'unrelated=value\n'; + await writeFile(outsidePath, contents); + const overview = service.getOverview(); + try { + await expect(run('../outside.notify')).rejects.toThrow(); + expect(await readFile(outsidePath, 'utf8')).toBe(contents); + expect(service.getOverview()).toEqual(overview); + } finally { + await rm(outsidePath, { force: true }); + } + }); + + it.each(operations)('$name rejects invalid IDs as bad requests', async ({ run }) => { + const invalidIds = [ + '', + '.', + '..', + '../outside', + 'nested/file.notify', + 'nested/../file.notify', + '/absolute.notify', + '..\\outside', + 'C:\\outside', + 'server:extra:outside', + 'bad\0.notify', + ]; + for (const id of invalidIds) { + await expect(run(id)).rejects.toMatchObject({ status: 400 }); + } + }); + + it.each(['variable', 'literal'])( + 'rejects prefixed traversal from a GraphQL %s', + async (inputKind) => { + const scalar = new PrefixedID(); + const value = '1:../outside.notify'; + const id = + inputKind === 'variable' + ? scalar.parseValue(value) + : scalar.parseLiteral({ kind: Kind.STRING, value }); + const outsidePath = join(basePath, 'outside.notify'); + const contents = 'unrelated=value\n'; + await writeFile(outsidePath, contents); + try { + await expect( + service.deleteNotification({ id, type: NotificationType.UNREAD }) + ).rejects.toThrow(); + expect(await readFile(outsidePath, 'utf8')).toBe(contents); + } finally { + await rm(outsidePath, { force: true }); + } + } + ); + + it('allows deletion of a malformed notification inside the notification directory', async () => { + const id = 'invalid.notify'; + const path = join(testPaths.UNREAD, id); + await writeFile(path, 'unrelated=value\n'); + await service.deleteNotification({ id, type: NotificationType.UNREAD }); + expect(existsSync(path)).toBe(false); + }); + + it('preserves valid filenames through archive, restore, and deletion', async () => { + const notification = await createNotification({ title: 'Überwachung v1.2 (NAS)' }); + const archived = await service.archiveNotification(notification); + expect(existsSync(join(testPaths.ARCHIVE, notification.id))).toBe(true); + const restored = await service.markAsUnread(archived); + expect(existsSync(join(testPaths.UNREAD, notification.id))).toBe(true); + await service.deleteNotification(restored); + expect(existsSync(join(testPaths.UNREAD, notification.id))).toBe(false); + }); + }); + it.each(notificationImportance)('loadNotifications respects %s filter', async (importance) => { const notifications = await Promise.all([ createNotification({ importance: NotificationImportance.ALERT }), diff --git a/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts b/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts index 7a32c7b15f..eee798e42d 100644 --- a/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts +++ b/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts @@ -1,7 +1,7 @@ import { Injectable, Logger } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { mkdir, readdir, readFile, rename, stat, unlink, writeFile } from 'fs/promises'; -import { basename, join } from 'path'; +import { basename, dirname, join, resolve } from 'path'; import type { Stats } from 'fs'; import { FSWatcher, watch } from 'chokidar'; @@ -93,6 +93,19 @@ export class NotificationsService { }; } + private notificationPath(id: string, type: NotificationType): string { + if (!id || id === '.' || id === '..' || /[/\\:\0]/u.test(id)) { + throw new AppError('Invalid notification ID', 400); + } + + const directory = resolve(this.paths()[type]); + const path = resolve(directory, id); + if (dirname(path) !== directory) { + throw new AppError('Invalid notification ID', 400); + } + return path; + } + private initializeNotificationsState(basePath: string, recreate = false) { const initialize = async () => { await this.ensureNotificationDirectories(basePath); @@ -311,7 +324,7 @@ export class NotificationsService { this.logger.debug(`[createNotification] legacy notifier failed: ${error}`); this.logger.verbose(`[createNotification] Writing: ${JSON.stringify(fileData, null, 4)}`); - const path = join(this.paths().UNREAD, id); + const path = this.notificationPath(id, NotificationType.UNREAD); const ini = encodeIni(fileData); // this.logger.debug(`[createNotification] INI: ${ini}`); await writeFile(path, ini); @@ -395,7 +408,7 @@ export class NotificationsService { *------------------------------------------------------------------------**/ public async deleteNotification({ id, type }: Pick) { - const path = join(this.paths()[type], id); + const path = this.notificationPath(id, type); // we don't want to update the overview stats if the deletion (unlink) fails // so we do the file system ops first @@ -471,12 +484,11 @@ export class NotificationsService { snapshot?: NotificationOverview; }) { const { from, to, snapshot } = params; - const paths = this.paths(); const fromStatKey = from.toLowerCase(); const toStatKey = to.toLowerCase(); return async (notification: Notification) => { - const currentPath = join(paths[from], notification.id); - const targetPath = join(paths[to], notification.id); + const currentPath = this.notificationPath(notification.id, from); + const targetPath = this.notificationPath(notification.id, to); /**----------------------- * Event, PubSub, & Overview Update logic @@ -518,7 +530,7 @@ export class NotificationsService { } public async archiveNotification({ id }: Pick): Promise { - const unreadPath = join(this.paths().UNREAD, id); + const unreadPath = this.notificationPath(id, NotificationType.UNREAD); // We expect to only archive 'unread' notifications, but it's possible that the notification // has already been archived or deleted (e.g. retry logic, spike in network latency). @@ -554,7 +566,7 @@ export class NotificationsService { } public async markAsUnread({ id }: Pick): Promise { - const archivePath = join(this.paths().ARCHIVE, id); + const archivePath = this.notificationPath(id, NotificationType.ARCHIVE); // the target notification might not be in the archive! if (!(await fileExists(archivePath))) { this.logger.warn(`[markAsUnread] Could not find notification in archive: ${id}`); From be84f44ca8b7953759ee4308634b1777ba177ab0 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Tue, 8 Sep 2026 08:59:44 -0500 Subject: [PATCH 2/3] fix(api): guard log and API-key file boundaries Reuse notification filename validation at filesystem consumers. Reject directory-only log targets and unsafe stored key IDs, and validate deletion batches before unlinking. Work intent: extend FeatureOS report 482091 remediation with a filesystem-boundary audit. Verified 177 focused tests and API type checks. --- .../files/resolve-file-in-directory.spec.ts | 38 +++++++++++++++++++ .../utils/files/resolve-file-in-directory.ts | 16 ++++++++ .../unraid-api/auth/api-key.service.spec.ts | 28 +++++++++++++- api/src/unraid-api/auth/api-key.service.ts | 19 +++++++--- .../graph/resolvers/logs/logs.service.spec.ts | 19 ++++++++++ .../graph/resolvers/logs/logs.service.ts | 5 ++- .../notifications/notifications.service.ts | 14 ++----- 7 files changed, 119 insertions(+), 20 deletions(-) create mode 100644 api/src/core/utils/files/resolve-file-in-directory.spec.ts create mode 100644 api/src/core/utils/files/resolve-file-in-directory.ts diff --git a/api/src/core/utils/files/resolve-file-in-directory.spec.ts b/api/src/core/utils/files/resolve-file-in-directory.spec.ts new file mode 100644 index 0000000000..f280204ef6 --- /dev/null +++ b/api/src/core/utils/files/resolve-file-in-directory.spec.ts @@ -0,0 +1,38 @@ +import { join, resolve } from 'node:path'; + +import { describe, expect, it } from 'vitest'; + +import { resolveFileInDirectory } from '@app/core/utils/files/resolve-file-in-directory.js'; + +describe('resolveFileInDirectory', () => { + it.each([ + '', + '.', + '..', + '../outside', + '../keys-other/file', + '/outside', + 'nested/file', + 'nested/../file', + '..\\outside', + 'C:\\outside', + 'server:file', + 'bad\0file', + ])('rejects unsafe filename %j', (filename) => { + expect(() => resolveFileInDirectory('/keys', filename)).toThrow(); + }); + + it.each([ + 'normal.notify', + 'key-123.json', + 'Überwachung v1.2 (NAS).notify', + '..not-traversal', + '%2e%2e%2ffile', + ])('preserves a literal safe filename %j', (filename) => { + expect(resolveFileInDirectory('/keys', filename)).toBe(join('/keys', filename)); + }); + + it('resolves relative base directories', () => { + expect(resolveFileInDirectory('keys', 'key.json')).toBe(resolve('keys/key.json')); + }); +}); diff --git a/api/src/core/utils/files/resolve-file-in-directory.ts b/api/src/core/utils/files/resolve-file-in-directory.ts new file mode 100644 index 0000000000..865ade8a49 --- /dev/null +++ b/api/src/core/utils/files/resolve-file-in-directory.ts @@ -0,0 +1,16 @@ +import { dirname, resolve } from 'node:path'; + +import { AppError } from '@app/core/errors/app-error.js'; + +export function resolveFileInDirectory(directory: string, filename: string): string { + if (!filename || filename === '.' || filename === '..' || /[/\\:\0]/u.test(filename)) { + throw new AppError('Invalid filename', 400); + } + + const basePath = resolve(directory); + const filePath = resolve(basePath, filename); + if (dirname(filePath) !== basePath) { + throw new AppError('Invalid filename', 400); + } + return filePath; +} diff --git a/api/src/unraid-api/auth/api-key.service.spec.ts b/api/src/unraid-api/auth/api-key.service.spec.ts index 900b4ad4af..7b147a1e19 100644 --- a/api/src/unraid-api/auth/api-key.service.spec.ts +++ b/api/src/unraid-api/auth/api-key.service.spec.ts @@ -1,5 +1,5 @@ import { Logger } from '@nestjs/common'; -import { readdir, readFile, writeFile } from 'fs/promises'; +import { readdir, readFile, unlink, writeFile } from 'fs/promises'; import { join } from 'path'; import { AuthAction, Resource, Role } from '@unraid/shared/graphql.model.js'; @@ -27,6 +27,7 @@ vi.mock('fs/promises', async () => ({ readdir: vi.fn().mockResolvedValue(['key1.json', 'key2.json', 'notakey.txt']), readFile: vi.fn(), writeFile: vi.fn(), + unlink: vi.fn(), })); vi.mock('fs-extra', () => ({ @@ -100,6 +101,31 @@ describe('ApiKeyService', () => { vi.clearAllMocks(); }); + describe('file path validation', () => { + it.each(['../outside', 'nested/key', '..\\outside', '/absolute/key'])( + 'rejects unsafe stored key ID %j before writing', + async (id) => { + await expect(apiKeyService.saveApiKey({ ...mockApiKey, id })).rejects.toThrow(); + expect(writeFile).not.toHaveBeenCalled(); + } + ); + + it('excludes unsafe IDs loaded from disk', async () => { + vi.mocked<(path: string) => Promise>(readdir).mockResolvedValue(['key.json']); + vi.mocked(readFile).mockResolvedValue(JSON.stringify({ ...mockApiKey, id: '../outside' })); + expect(await apiKeyService.loadAllFromDisk()).toEqual([]); + }); + + it('rejects an unsafe deletion batch before deleting any key', async () => { + const unsafeKey = { ...mockApiKey, id: '../outside' }; + vi.spyOn(apiKeyService, 'findByField').mockImplementation((_field, id) => + id === unsafeKey.id ? unsafeKey : mockApiKey + ); + await expect(apiKeyService.deleteApiKeys([mockApiKey.id, unsafeKey.id])).rejects.toThrow(); + expect(unlink).not.toHaveBeenCalled(); + }); + }); + describe('initialization', () => { it('should ensure directory exists', async () => { new ApiKeyService(); diff --git a/api/src/unraid-api/auth/api-key.service.ts b/api/src/unraid-api/auth/api-key.service.ts index 4a9a091dda..13d926ed60 100644 --- a/api/src/unraid-api/auth/api-key.service.ts +++ b/api/src/unraid-api/auth/api-key.service.ts @@ -1,7 +1,6 @@ import { Injectable, Logger, OnModuleInit } from '@nestjs/common'; import crypto from 'crypto'; import { readdir, readFile, unlink, writeFile } from 'fs/promises'; -import { join } from 'path'; import { AuthAction, Resource, Role } from '@unraid/shared/graphql.model.js'; import { normalizeLegacyActions } from '@unraid/shared/util/permissions.js'; @@ -11,6 +10,7 @@ import { ensureDirSync } from 'fs-extra'; import { GraphQLError } from 'graphql'; import { v4 as uuidv4 } from 'uuid'; +import { resolveFileInDirectory } from '@app/core/utils/files/resolve-file-in-directory.js'; import { environment } from '@app/environment.js'; import { getters } from '@app/store/index.js'; import { @@ -217,7 +217,7 @@ export class ApiKeyService implements OnModuleInit { */ private async loadApiKeyFile(file: string): Promise { try { - const content = await readFile(join(this.basePath, file), 'utf8'); + const content = await readFile(resolveFileInDirectory(this.basePath, file), 'utf8'); // First convert all the strings in roles and permissions to uppercase (this ensures that casing is never an issue) const parsedContent = JSON.parse(content); @@ -235,7 +235,9 @@ export class ApiKeyService implements OnModuleInit { })); } - return await validateObject(ApiKey, parsedContent); + const apiKey = await validateObject(ApiKey, parsedContent); + resolveFileInDirectory(this.basePath, `${apiKey.id}.json`); + return apiKey; } catch (error) { if (error instanceof SyntaxError) { this.logger.error(`Corrupted key file: ${file}`); @@ -299,7 +301,7 @@ export class ApiKeyService implements OnModuleInit { }, {} as ApiKey); await writeFile( - join(this.basePath, `${validatedApiKey.id}.json`), + resolveFileInDirectory(this.basePath, `${validatedApiKey.id}.json`), JSON.stringify(sortedApiKey, null, 2) ); } catch (error: unknown) { @@ -333,6 +335,11 @@ export class ApiKeyService implements OnModuleInit { * @throws Array if errors occur during the file deletion. */ public async deleteApiKeys(ids: string[]): Promise { + const keyFiles = ids.map((id) => ({ + id, + path: resolveFileInDirectory(this.basePath, `${id}.json`), + })); + // First verify all keys exist const missingKeys = ids.filter((id) => !this.findByField('id', id)); if (missingKeys.length > 0) { @@ -340,8 +347,8 @@ export class ApiKeyService implements OnModuleInit { } // Delete all files in parallel - const { errors, data: deletedIds } = await batchProcess(ids, async (id) => { - await unlink(join(this.basePath, `${id}.json`)); + const { errors, data: deletedIds } = await batchProcess(keyFiles, async ({ id, path }) => { + await unlink(path); return id; }); diff --git a/api/src/unraid-api/graph/resolvers/logs/logs.service.spec.ts b/api/src/unraid-api/graph/resolvers/logs/logs.service.spec.ts index 2071b25f31..a0686e7459 100644 --- a/api/src/unraid-api/graph/resolvers/logs/logs.service.spec.ts +++ b/api/src/unraid-api/graph/resolvers/logs/logs.service.spec.ts @@ -65,6 +65,25 @@ describe('LogsService', () => { vi.clearAllMocks(); }); + it.each(['', '.', '..', '../..', '/var/log/..', 'bad\0.log'])( + 'rejects an invalid log subscription path %j before registration', + (path) => { + expect(() => service.registerLogFileSubscription(path)).toThrow(); + expect(subscriptionTracker.registerTopic).not.toHaveBeenCalled(); + expect(chokidar.watch).not.toHaveBeenCalled(); + } + ); + + it.each(['.', '..', '/var/log/..'])('rejects log directory reads for %j', async (path) => { + await expect(service.getLogFileContent(path)).rejects.toThrow(); + }); + + it('accepts a full log path returned by the log listing', () => { + expect(service.registerLogFileSubscription('/var/log/syslog')).toEqual( + service.registerLogFileSubscription('syslog') + ); + }); + it('should be defined', () => { expect(service).toBeDefined(); }); diff --git a/api/src/unraid-api/graph/resolvers/logs/logs.service.ts b/api/src/unraid-api/graph/resolvers/logs/logs.service.ts index 051725a914..29563e31d8 100644 --- a/api/src/unraid-api/graph/resolvers/logs/logs.service.ts +++ b/api/src/unraid-api/graph/resolvers/logs/logs.service.ts @@ -7,6 +7,7 @@ import { createInterface } from 'node:readline'; import * as chokidar from 'chokidar'; import { pubsub } from '@app/core/pubsub.js'; +import { resolveFileInDirectory } from '@app/core/utils/files/resolve-file-in-directory.js'; import { getters } from '@app/store/index.js'; import { LogWatcherManager } from '@app/unraid-api/graph/resolvers/logs/log-watcher-manager.service.js'; import { SubscriptionTrackerService } from '@app/unraid-api/graph/services/subscription-tracker.service.js'; @@ -84,7 +85,7 @@ export class LogsService { ): Promise { try { // Validate that the path is within the log directory - const normalizedPath = join(this.logBasePath, basename(path)); + const normalizedPath = resolveFileInDirectory(this.logBasePath, basename(path)); // Count total lines const totalLines = await this.countFileLines(normalizedPath); @@ -119,7 +120,7 @@ export class LogsService { * @returns The subscription topic key */ registerLogFileSubscription(path: string): string { - const normalizedPath = join(this.logBasePath, basename(path)); + const normalizedPath = resolveFileInDirectory(this.logBasePath, basename(path)); const topicKey = this.getTopicKey(normalizedPath); // Register the topic if not already registered diff --git a/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts b/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts index eee798e42d..04834ecef8 100644 --- a/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts +++ b/api/src/unraid-api/graph/resolvers/notifications/notifications.service.ts @@ -1,7 +1,7 @@ import { Injectable, Logger } from '@nestjs/common'; import { ConfigService } from '@nestjs/config'; import { mkdir, readdir, readFile, rename, stat, unlink, writeFile } from 'fs/promises'; -import { basename, dirname, join, resolve } from 'path'; +import { basename, join } from 'path'; import type { Stats } from 'fs'; import { FSWatcher, watch } from 'chokidar'; @@ -16,6 +16,7 @@ import { AppError } from '@app/core/errors/app-error.js'; import { pubsub, PUBSUB_CHANNEL } from '@app/core/pubsub.js'; import { NotificationIni } from '@app/core/types/states/notification.js'; import { fileExists } from '@app/core/utils/files/file-exists.js'; +import { resolveFileInDirectory } from '@app/core/utils/files/resolve-file-in-directory.js'; import { parseConfig } from '@app/core/utils/misc/parse-config.js'; import { CHOKIDAR_USEPOLLING } from '@app/environment.js'; import { getters } from '@app/store/index.js'; @@ -94,16 +95,7 @@ export class NotificationsService { } private notificationPath(id: string, type: NotificationType): string { - if (!id || id === '.' || id === '..' || /[/\\:\0]/u.test(id)) { - throw new AppError('Invalid notification ID', 400); - } - - const directory = resolve(this.paths()[type]); - const path = resolve(directory, id); - if (dirname(path) !== directory) { - throw new AppError('Invalid notification ID', 400); - } - return path; + return resolveFileInDirectory(this.paths()[type], id); } private initializeNotificationsState(basePath: string, recreate = false) { From ba02e3e328553ee700eb2395a572f3c561b83aa8 Mon Sep 17 00:00:00 2001 From: Eli Bosley Date: Tue, 8 Sep 2026 09:17:19 -0500 Subject: [PATCH 3/3] fix(api): remove unused unrestricted boot writer --- api/src/core/utils/write-to-boot.ts | 17 ----------------- 1 file changed, 17 deletions(-) delete mode 100644 api/src/core/utils/write-to-boot.ts diff --git a/api/src/core/utils/write-to-boot.ts b/api/src/core/utils/write-to-boot.ts deleted file mode 100644 index afa68b6323..0000000000 --- a/api/src/core/utils/write-to-boot.ts +++ /dev/null @@ -1,17 +0,0 @@ -import fs from 'fs'; -import path from 'path'; - -import { convert } from 'convert'; - -import { logger } from '@app/core/log.js'; - -const writeFile = async (filePath: string, fileContents: string | Buffer) => { - logger.debug(`Writing ${convert(fileContents.length, 'bytes').to('kilobytes')} to ${filePath}`); - await fs.promises.writeFile(filePath, fileContents); -}; - -export const writeToBoot = async (filePath: string, fileContents: string | Buffer) => { - const basePath = '/boot/config/plugins/dynamix/'; - const resolvedPath = path.resolve(basePath, filePath); - await writeFile(resolvedPath, fileContents); -};