fix(server): hide database password from backup process arguments - #30757
Open
NoiceHax wants to merge 1 commit into
Open
fix(server): hide database password from backup process arguments#30757NoiceHax wants to merge 1 commit into
NoiceHax wants to merge 1 commit into
Conversation
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 immich-app#30333
|
Label error. Requires exactly 1 of: changelog:.*. Found: 🗄️server, auto-closed:template. A maintainer will add the required label. |
Contributor
|
This PR has been automatically closed as the description doesn't follow our template. After you edit it to match the template, the PR will automatically be reopened. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When
DB_URLis set, the whole connection string was passed topg_dumpandpsqlas a plain argument, so the password in it showed up inpsfor anyone able to list processes. The password was already going throughPGPASSWORDas well, so the copy in the arguments was not needed.Now the password is taken out of the URL before it becomes an argument, both the
user:passpart and a?password=query parameter, and onlyPGPASSWORDcarries it.URLgives the userinfo password back percent-encoded, so it is decoded first because libpq wants the raw value.Fixes #30333
How Has This Been Tested?
Two new unit tests in
server/src/services/database-backup.service.spec.ts. One checkspg_dumpis spawned with a password-free URL andPGPASSWORDset to the decoded password. The other checks thepasswordquery parameter is stripped. Three existing inline snapshots changed because the URL no longer carries the password. Both new tests fail onmain.Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
This code was written alongside AI. The change and its tests were reviewed against the behaviour described in the linked issue before opening, and I am happy to walk through the reasoning or rework any part of it.