fix(api): confine notification, log, and API-key file operations

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'));
+    });
+});

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;
+}

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<string[]>>(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();

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<ApiKey | null> {
         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,15 +335,20 @@ export class ApiKeyService implements OnModuleInit {
      * @throws Array<Error> if errors occur during the file deletion.
      */
     public async deleteApiKeys(ids: string[]): Promise<void> {
+        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) {
             throw new Error(`API keys not found: ${missingKeys.join(', ')}`);
         }
 
         // 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;
         });
 

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();
     });

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<LogFileContent> {
         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

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 }),