Merge 7064c27a4c into ffc83eae36

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

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)