refactor(auth): avatar reclaim through storage

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

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', () => {