Preserve literal passwords in PostgreSQL service files - #1631
Preserve literal passwords in PostgreSQL service files#1631jackwalkerlabs wants to merge 1 commit into
Conversation
Fixes dbcli#1512. Implemented and validated with Codex.
| err.line_number += skipped_lines | ||
| raise err | ||
| # libpq treats values literally: hashes, commas, quotes and percent | ||
| # signs are part of the value, not ConfigObj comments or syntax. |
There was a problem hiding this comment.
It makes sense in the diff, but it looks odd to reference ConfigObj in a comment here while the module makes not reference to it at all. I'd remove that part.
| # signs are part of the value, not ConfigObj comments or syntax. | |
| # signs are part of the value. |
| * Chris (ChrisJr404) | ||
| * Pieter Ouwerkerk (pouwerkerk) | ||
|
|
||
| * jackwalkerlabs (Codex-assisted) |
There was a problem hiding this comment.
You can remove the empty line above this line. Also, the "Codex-assisted" comment is not warranted. I appreciate that you mentioned it in the pull request description. But here in the changelog it's... you: I suppose that you do have an existence outside Codex. And who knows, maybe you could contribute without Codex, someday! ;)
(I suppose that Codex wrote that comment. That makes me a bit sad, actually.)
| * Preserve hashes, percent signs, commas and quotes in service-file passwords, | ||
| matching libpq. Quotes around service-file values are now literal; remove | ||
| quotes previously added solely to escape ConfigObj syntax. |
There was a problem hiding this comment.
I would make it clearer that it's a breaking change. Also, I would not reference ConfigObj, which is an implementation detail. Suggestion:
| * Preserve hashes, percent signs, commas and quotes in service-file passwords, | |
| matching libpq. Quotes around service-file values are now literal; remove | |
| quotes previously added solely to escape ConfigObj syntax. | |
| * [breaking change] Password in service files (usually ``~/.pg_service.conf``) are now read as they appear, as `libpq` and `psql` do. Previously, hashes, percent signs and other values had to be quoted. They must not be quoted anymore. |
@j-bennet: it's a (small) breaking change, I think it would warrant a new minor version (from 4.6.0 to 4.7.0). Do you agree?
| if service not in service_file_config: | ||
| return None, service_file | ||
| service_conf = service_file_config.get(service) | ||
| service_conf = service_file_config[service] |
There was a problem hiding this comment.
This changes the behaviour: previously, parse_service_info() returned (None, service_file) upon an unknown service name. The caller would detect it and print an error message. Now this function raises a KeyError.
The previous behaviour should be kept.
| # libpq treats values literally: hashes, commas, quotes and percent | ||
| # signs are part of the value, not ConfigObj comments or syntax. | ||
| service_file_config = ConfigParser(interpolation=None, delimiters=("=",), comment_prefixes=("#",)) | ||
| service_file_config.read_file(itertools.chain(itertools.repeat("\n", skipped_lines), f), source=service_file) |
There was a problem hiding this comment.
What's the use of itertools.repeat("\n", skipped_lines)? Surely the parser could do without the empty lines, couldn't it?
| @@ -1,5 +1,5 @@ | |||
| from zoneinfo import ZoneInfoNotFoundError | |||
| from configobj import ConfigObj, ParseError | |||
| from configparser import ConfigParser | |||
There was a problem hiding this comment.
I'd love to drop the configobj dependency. pgcli still uses it to parse its main configuration file, but we could probably replace that by configparser as well. There may be incompatibilities/differences between the two, they would have to be listed first (maybe we can ignore them, maybe we cant'). Are you willing to give it a try? (That should be in a separate pull request.)
2 participants
Service-file passwords such as
abc#def,a,b, and literal quoted values were changed by ConfigObj before connecting. Read the service file with interpolation and inline comments disabled so these values reach the connection unchanged. Leading pre-section comments remain supported, and parser errors retain their original file line numbers.Fixes #1512. As discussed there, quotes are now literal, matching libpq; the changelog calls out removing quotes previously used only to escape ConfigObj syntax.
Validation: six new regression cases fail on the original code; all seven pass with the change. Full suite against an isolated PostgreSQL 17 instance: 2,784 passed, 1 xfailed, 1 xpassed. Ruff lint and formatting hooks pass. Python 3.12 on macOS.
Implemented and tested using Codex under the submitting account's authorization.
Checklist