From dfb8f8a8a492057a432f3a840f3a840712cf9fcd Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 17 Aug 2026 05:47:31 +0200 Subject: [PATCH] refactor(auth): avatar reclaim through storage --- server/src/nest/auth/user-profile.service.ts | 25 ++++++------ .../unit/nest/user-profile.service.test.ts | 40 ++++++++++++++++++- 2 files changed, 52 insertions(+), 13 deletions(-) diff --git a/server/src/nest/auth/user-profile.service.ts b/server/src/nest/auth/user-profile.service.ts index 0c374340a..97ad8a6af 100644 --- a/server/src/nest/auth/user-profile.service.ts +++ b/server/src/nest/auth/user-profile.service.ts @@ -1,11 +1,10 @@ import { Injectable } from '@nestjs/common'; -import path from 'path'; -import fs from 'fs'; import { readEnv, getAppUrl } from '../../app-config'; import { DatabaseService } from '../database/database.service'; +import { StorageService } from '../storage/storage.service'; import { decrypt_api_key, maybe_encrypt_api_key } from '../common/crypto/apiKeyCrypto'; import { avatarUrl } from '../common/avatarUrl'; -import { EMAIL_REGEX, avatarDir, mask_stored_api_key } from './auth.helpers'; +import { EMAIL_REGEX, mask_stored_api_key } from './auth.helpers'; import { splitManagedKeys, MANAGED_LOCKED_PROFILE_KEYS } from '../common/managed'; import { User } from '../../types'; @@ -25,7 +24,10 @@ import { User } from '../../types'; */ @Injectable() export class UserProfileService { - constructor(private readonly db: DatabaseService) {} + constructor( + private readonly db: DatabaseService, + private readonly storage: StorageService, + ) {} /** * On a centrally administered install the three key columns belong to the @@ -182,12 +184,12 @@ export class UserProfileService { async saveAvatar(userId: number, filename: string) { const current = this.db.get<{ avatar: string | null }>('SELECT avatar FROM users WHERE id = ?', userId); // Only a locally uploaded file has something to clean up. An OIDC picture URL - // (#1399) has no file on disk, so skip the rm — path.join on a URL is meaningless. + // (#1399) has no storage object, so skip the delete entirely. if (current?.avatar && !/^https:\/\//i.test(current.avatar)) { - // Fire-and-forget: leftover files are harmless; the DB update is - // the source of truth for which avatar is current. - const oldPath = path.join(avatarDir, current.avatar); - await fs.promises.rm(oldPath, { force: true }).catch(() => {}); + // Fire-and-forget parity: leftover objects are harmless; the DB update is + // the source of truth for which avatar is current. The catch also + // swallows a hostile stored value the central key validation rejects. + await this.storage.delete('avatars', current.avatar).catch(() => {}); } this.db.run('UPDATE users SET avatar = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', filename, userId); @@ -198,10 +200,9 @@ export class UserProfileService { async deleteAvatar(userId: number) { const current = this.db.get<{ avatar: string | null }>('SELECT avatar FROM users WHERE id = ?', userId); - // An OIDC picture URL (#1399) has no local file — only rm an uploaded one. + // An OIDC picture URL (#1399) has no storage object — only delete an uploaded one. if (current?.avatar && !/^https:\/\//i.test(current.avatar)) { - const filePath = path.join(avatarDir, current.avatar); - await fs.promises.rm(filePath, { force: true }).catch(() => {}); + await this.storage.delete('avatars', current.avatar).catch(() => {}); } this.db.run('UPDATE users SET avatar = NULL, updated_at = CURRENT_TIMESTAMP WHERE id = ?', userId); return { success: true }; diff --git a/server/tests/unit/nest/user-profile.service.test.ts b/server/tests/unit/nest/user-profile.service.test.ts index 01d3d58fa..94b9bfde5 100644 --- a/server/tests/unit/nest/user-profile.service.test.ts +++ b/server/tests/unit/nest/user-profile.service.test.ts @@ -38,10 +38,14 @@ import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; import { createUser, createAdmin } from '../../helpers/factories'; +import fs from 'node:fs'; +import path from 'node:path'; import { UserProfileService } from '../../../src/nest/auth/user-profile.service'; +import { makeStorageFixture } from '../../helpers/storage-fixture'; import { DatabaseService } from '../../../src/nest/database/database.service'; -const profile = new UserProfileService(new DatabaseService(testDb)); +const avatarsFx = makeStorageFixture('avatars/'); +const profile = new UserProfileService(new DatabaseService(testDb), avatarsFx.storage); beforeAll(() => { createTables(testDb); @@ -276,6 +280,40 @@ describe('updateMapsKey / avatar', () => { const row = testDb.prepare('SELECT avatar FROM users WHERE id = ?').get(user.id) as { avatar: string | null }; expect(row.avatar).toBeNull(); }); + + function writeAvatar(name: string): string { + const dir = path.join(avatarsFx.root, 'avatars'); + fs.mkdirSync(dir, { recursive: true }); + const fp = path.join(dir, name); + fs.writeFileSync(fp, 'png-bytes'); + return fp; + } + + it('AUTH-DB-069b: saveAvatar reclaims the previous uploaded avatar file', async () => { + const { user } = createUser(testDb); + const old = writeAvatar('old.png'); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('old.png', user.id); + + await profile.saveAvatar(user.id, 'new.png'); + expect(fs.existsSync(old)).toBe(false); + }); + + it('AUTH-DB-070b: deleteAvatar removes the uploaded avatar file', async () => { + const { user } = createUser(testDb); + const fp = writeAvatar('mine.png'); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('mine.png', user.id); + + expect(await profile.deleteAvatar(user.id)).toEqual({ success: true }); + expect(fs.existsSync(fp)).toBe(false); + }); + + it('AUTH-DB-070c: a hostile stored avatar value is swallowed, never thrown', async () => { + const { user } = createUser(testDb); + testDb.prepare('UPDATE users SET avatar = ? WHERE id = ?').run('../../etc/passwd', user.id); + // Central key validation rejects the value; the delete swallows it exactly + // like the old rm().catch did — the DB update still wins. + expect(await profile.deleteAvatar(user.id)).toEqual({ success: true }); + }); }); describe('profile quirk fixes', () => {