From 7064c27a4c65940cc97e14e88b60658ded602b01 Mon Sep 17 00:00:00 2001 From: NoiceHax Date: Thu, 13 Aug 2026 22:34:08 +0530 Subject: [PATCH] fix(server): hide database password from backup process arguments When `DB_URL` is used, the connection string was forwarded verbatim to the spawned `pg_dump`/`psql` processes, so the password contained in it was readable by anyone able to list processes. The credentials are now stripped from the URL (both the userinfo section and the `password` parameter) and only handed over through `PGPASSWORD`, which is already being set. The value is percent-decoded first, since libpq expects the raw password in the environment variable. Closes #30333 --- .../services/database-backup.service.spec.ts | 80 ++++++++++++++++++- .../src/services/database-backup.service.ts | 20 ++++- 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/server/src/services/database-backup.service.spec.ts b/server/src/services/database-backup.service.spec.ts index 37964e7b6f..aef7201817 100644 --- a/server/src/services/database-backup.service.spec.ts +++ b/server/src/services/database-backup.service.spec.ts @@ -209,13 +209,46 @@ describe(DatabaseBackupService.name, () => { const args = call[1] as string[]; expect(args).toMatchInlineSnapshot(` [ - "postgresql://postgres:pwd@host:5432/immich?sslmode=require", + "postgresql://postgres@host:5432/immich?sslmode=require", "--clean", "--if-exists", ] `); }); + it('should pass the DB_URL password via PGPASSWORD instead of the process arguments', async () => { + // the password would otherwise be readable by anyone able to list processes + const dbUrl = 'postgresql://postgres:p%40ssword@host:5432/immich'; + const configMock = { + getEnv: () => ({ database: { config: { connectionType: 'url', url: dbUrl }, skipMigrations: false } }), + getWorker: () => ImmichWorker.Api, + isDev: () => false, + } as unknown as any; + + sut = new DatabaseBackupService( + mocks.logger as never, + mocks.storage as never, + configMock as never, + mocks.systemMetadata as never, + mocks.process, + mocks.database as never, + mocks.user as never, + mocks.cron as never, + mocks.job as never, + void 0 as never, + ); + + mocks.database.getPostgresVersion.mockResolvedValue('14.10'); + + await sut.handleBackupDatabase(); + + expect(mocks.process.spawnDuplexStream).toHaveBeenCalledWith( + '/usr/lib/postgresql/14/bin/pg_dump', + ['postgresql://postgres@host:5432/immich', '--clean', '--if-exists'], + { env: { PATH: process.env.PATH, PGPASSWORD: 'p@ssword' } }, + ); + }); + it('should run a database backup successfully', async () => { const result = await sut.handleBackupDatabase(); expect(result).toBe(JobStatus.Success); @@ -488,7 +521,7 @@ describe(DatabaseBackupService.name, () => { await expect(sut.buildPostgresLaunchArguments('pg_dump')).resolves.toMatchInlineSnapshot(` { "args": [ - "postgresql://mypg:mypwd@myhost:1234/myimmich?sslmode=require", + "postgresql://mypg@myhost:1234/myimmich?sslmode=require", "--clean", "--if-exists", ], @@ -507,7 +540,7 @@ describe(DatabaseBackupService.name, () => { { "args": [ "--dbname", - "postgresql://mypg:mypwd@myhost:1234/myimmich?sslmode=require", + "postgresql://mypg@myhost:1234/myimmich?sslmode=require", "--single-transaction", "--set", "ON_ERROR_STOP=on", @@ -524,6 +557,47 @@ describe(DatabaseBackupService.name, () => { }); }); + describe('using URL with credentials as parameters', () => { + beforeEach(() => { + const dbUrl = 'postgresql://myhost:1234/myimmich?user=mypg&password=my%2Fpwd'; + const configMock = { + getEnv: () => ({ database: { config: { connectionType: 'url', url: dbUrl }, skipMigrations: false } }), + getWorker: () => ImmichWorker.Api, + isDev: () => false, + } as unknown as any; + + sut = new DatabaseBackupService( + mocks.logger as never, + mocks.storage as never, + configMock as never, + mocks.systemMetadata as never, + mocks.process, + mocks.database as never, + mocks.user as never, + mocks.cron as never, + mocks.job as never, + void 0 as never, + ); + }); + + it('should remove the password parameter', async () => { + await expect(sut.buildPostgresLaunchArguments('pg_dump')).resolves.toMatchInlineSnapshot(` + { + "args": [ + "postgresql://myhost:1234/myimmich?user=mypg", + "--clean", + "--if-exists", + ], + "bin": "/usr/lib/postgresql/14/bin/pg_dump", + "databaseMajorVersion": 14, + "databasePassword": "my/pwd", + "databaseUsername": "mypg", + "databaseVersion": "14.10 (Debian 14.10-1.pgdg120+1)", + } + `); + }); + }); + describe('using bad URL', () => { beforeEach(() => { const dbUrl = 'post://gresql://mypg:myp@wd@myhos:t:1234/myimmich?sslmode=require&uselibpqcompat=true'; diff --git a/server/src/services/database-backup.service.ts b/server/src/services/database-backup.service.ts index a277b028a5..39be28a31a 100644 --- a/server/src/services/database-backup.service.ts +++ b/server/src/services/database-backup.service.ts @@ -129,6 +129,7 @@ export class DatabaseBackupService { const args: string[] = []; let databaseUsername; + let databasePassword; if (isUrlConnection) { if (bin !== 'pg_dump') { @@ -142,16 +143,24 @@ export class DatabaseBackupService { parsedUrl.searchParams.delete('uselibpqcompat'); databaseUsername = parsedUrl.username || parsedUrl.searchParams.get('user'); + databasePassword = decodeUrlComponent(parsedUrl.password) || parsedUrl.searchParams.get('password'); + + // the password is handed over via `PGPASSWORD`, so that it is not visible + // in the arguments of the spawned process + parsedUrl.password = ''; + parsedUrl.searchParams.delete('password'); url = parsedUrl.href; } // assume typical values if we can't parse URL or not present databaseUsername ??= 'postgres'; + databasePassword ??= ''; args.push(url); } else { databaseUsername = databaseConfig.username; + databasePassword = databaseConfig.password; args.push( '--username', @@ -214,7 +223,7 @@ export class DatabaseBackupService { bin: `/usr/lib/postgresql/${databaseMajorVersion}/bin/${bin}`, args, databaseUsername, - databasePassword: isUrlConnection ? new URL(databaseConfig.url).password : databaseConfig.password, + databasePassword, databaseVersion, databaseMajorVersion, }; @@ -461,6 +470,15 @@ export class DatabaseBackupService { } } +// `URL` exposes the userinfo section percent-encoded, while `PGPASSWORD` expects the raw value +function decodeUrlComponent(value: string) { + try { + return decodeURIComponent(value); + } catch { + return value; + } +} + const SQL_DROP_CONNECTIONS = ` -- drop all other database connections SELECT pg_terminate_backend(pid)