Skip to content

Preserve literal passwords in PostgreSQL service files #1631

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jackwalkerlabs wants to merge 1 commit into
base: main
Choose a base branch
from
+24 −7
Open

Preserve literal passwords in PostgreSQL service files #1631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ Contributors:
* Chris (ChrisJr404)
* Pieter Ouwerkerk (pouwerkerk)

* jackwalkerlabs (Codex-assisted)

@dbaty dbaty Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


Creator:
--------
Amjith Ramanujam
3 changes: 3 additions & 0 deletions changelog.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ Features:

Bug fixes:
----------
* 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.
Comment on lines +25 to +27

@dbaty dbaty Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make it clearer that it's a breaking change. Also, I would not reference ConfigObj, which is an implementation detail. Suggestion:

Suggested change
* 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?

* Fix special commands being broken while explain mode (F5) is on. Every input
was prefixed with ``EXPLAIN (...)`` and sent to the server as SQL, including
backslash commands and the bare words ``exit``/``quit``, so ``\q``, ``\d``,
Expand Down
13 changes: 6 additions & 7 deletions pgcli/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from zoneinfo import ZoneInfoNotFoundError
from configobj import ConfigObj, ParseError
from configparser import ConfigParser

@dbaty dbaty Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

from pgspecial.namedqueries import NamedQueries
from .config import skip_initial_comment

Expand Down Expand Up @@ -2213,14 +2213,13 @@ def parse_service_info(service):
return None, service_file
with open(service_file, newline="") as f:
skipped_lines = skip_initial_comment(f)
try:
service_file_config = ConfigObj(f)
except ParseError as err:
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.

@dbaty dbaty Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
# signs are part of the value, not ConfigObj comments or syntax.
# signs are part of the value.

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)

@dbaty dbaty Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use of itertools.repeat("\n", skipped_lines)? Surely the parser could do without the empty lines, couldn't it?

if service not in service_file_config:
return None, service_file
service_conf = service_file_config.get(service)
service_conf = service_file_config[service]

@dbaty dbaty Sep 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return service_conf, service_file


Expand Down
13 changes: 13 additions & 0 deletions tests/test_main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
get_connect_timeout,
get_editor,
notify_callback,
parse_service_info,
PGCli,
OutputSettings,
COLOR_CODE_REGEX,
Expand Down Expand Up @@ -590,6 +591,18 @@ def test_quoted_db_uri(tmpdir):
mock_connect.assert_called_with(database="testdb[", host="baz.com", user="bar^", passwd="]foo")


@pytest.mark.parametrize("password", ["abc#def", "#leading", "abc #def", "abc%def", "a,b", '"quoted"', "'quoted'"])
def test_pg_service_password_is_literal(tmp_path, monkeypatch, password):
service_file = tmp_path / "pg_service.conf"
service_file.write_text(f"# comment\n[myservice]\npassword={password}\n")
monkeypatch.setenv("PGSERVICEFILE", str(service_file))

config, filename = parse_service_info("myservice")

assert filename == str(service_file)
assert config["password"] == password


def test_pg_service_file(tmpdir):
with mock.patch.object(PGCli, "connect") as mock_connect:
cli = PGCli(pgclirc_file=str(tmpdir.join("rcfile")))
Expand Down
Loading