From e2d5de15003d17e2f5c91a5c9151528031a97014 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 29 Nov 2020 15:22:04 -0500 Subject: [PATCH] Fix recently-introduced breakage in psql's \connect command. Through my misreading of what the existing code actually did, commits 85c54287a et al. broke psql's behavior for the case where "\c connstring" provides a password in the connstring. We should use that password in such a case, but as of 85c54287a we ignored it (and instead, prompted for a password). Commit 94929f1cf fixed that in HEAD, but since I thought it was cleaning up a longstanding misbehavior and not one I'd just created, I didn't back-patch it. Hence, back-patch the portions of 94929f1cf having to do with password management. In addition to fixing the introduced bug, this means that "\c -reuse-previous=on connstring" will allow re-use of an existing connection's password if the connstring doesn't change user/host/port. That didn't happen before, but it seems like a bug fix, and anyway I'm loath to have significant differences in this code across versions. Also fix an error with the same root cause about whether or not to override a connstring's setting of client_encoding. As of 85c54287a we always did so; restore the previous behavior of overriding only when stdin/stdout are a terminal and there's no environment setting of PGCLIENTENCODING. (I find that definition a bit surprising, but right now doesn't seem like the time to revisit it.) Per bug #16746 from Krzysztof Gradek. As with the previous patch, back-patch to all supported branches. Discussion: https://postgr.es/m/16746-44b30e2edf4335d4@postgresql.org --- doc/src/sgml/ref/psql-ref.sgml | 2 ++ src/bin/psql/command.c | 66 +++++++++++++++++++++++++--------- 2 files changed, 51 insertions(+), 17 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index b26de20f5d..3509abc0f7 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -844,6 +844,8 @@ testdb=> is changed from its previous value using the positional syntax, any hostaddr setting present in the existing connection's parameters is dropped. + Also, any password used for the existing connection will be re-used + only if the user, host, and port settings are not changed. When the command neither specifies nor reuses a particular parameter, the libpq default is used. diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 2c5ad9e305..4fcdc0b5ac 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -1636,6 +1636,7 @@ do_connect(enum trivalue reuse_previous_specification, int nconnopts = 0; bool same_host = false; char *password = NULL; + char *client_encoding; bool success = true; bool keep_password = true; bool has_connection_string; @@ -1706,6 +1707,7 @@ do_connect(enum trivalue reuse_previous_specification, { PQconninfoOption *ci; PQconninfoOption *replci; + bool have_password = false; for (ci = cinfo, replci = replcinfo; ci->keyword && replci->keyword; @@ -1724,6 +1726,26 @@ do_connect(enum trivalue reuse_previous_specification, replci->val = ci->val; ci->val = swap; + + /* + * Check whether connstring provides options affecting + * password re-use. While any change in user, host, + * hostaddr, or port causes us to ignore the old + * connection's password, we don't force that for + * dbname, since passwords aren't database-specific. + */ + if (replci->val == NULL || + strcmp(ci->val, replci->val) != 0) + { + if (strcmp(replci->keyword, "user") == 0 || + strcmp(replci->keyword, "host") == 0 || + strcmp(replci->keyword, "hostaddr") == 0 || + strcmp(replci->keyword, "port") == 0) + keep_password = false; + } + /* Also note whether connstring contains a password. */ + if (strcmp(replci->keyword, "password") == 0) + have_password = true; } } Assert(ci->keyword == NULL && replci->keyword == NULL); @@ -1733,8 +1755,13 @@ do_connect(enum trivalue reuse_previous_specification, PQconninfoFree(replcinfo); - /* We never re-use a password with a conninfo string. */ - keep_password = false; + /* + * If the connstring contains a password, tell the loop below + * that we may use it, regardless of other settings (i.e., + * cinfo's password is no longer an "old" password). + */ + if (have_password) + keep_password = true; /* Don't let code below try to inject dbname into params. */ dbname = NULL; @@ -1815,14 +1842,16 @@ do_connect(enum trivalue reuse_previous_specification, { password = prompt_for_password(user); } - else if (o_conn && keep_password) - { - password = PQpass(o_conn); - if (password && *password) - password = pg_strdup(password); - else - password = NULL; - } + + /* + * Consider whether to force client_encoding to "auto" (overriding + * anything in the connection string). We do so if we have a terminal + * connection and there is no PGCLIENTENCODING environment setting. + */ + if (pset.notty || getenv("PGCLIENTENCODING")) + client_encoding = NULL; + else + client_encoding = "auto"; /* Loop till we have a connection or fail, which we might've already */ while (success) @@ -1834,12 +1863,12 @@ do_connect(enum trivalue reuse_previous_specification, /* * Copy non-default settings into the PQconnectdbParams parameter - * arrays; but override any values specified old-style, as well as the - * password and a couple of fields we want to set forcibly. + * arrays; but inject any values specified old-style, as well as any + * interactively-obtained password, and a couple of fields we want to + * set forcibly. * * If you change this code, see also the initial-connection code in - * main(). For no good reason, a connection string password= takes - * precedence in main() but not here. + * main(). */ for (ci = cinfo; ci->keyword; ci++) { @@ -1858,12 +1887,15 @@ do_connect(enum trivalue reuse_previous_specification, } else if (port && strcmp(ci->keyword, "port") == 0) values[paramnum++] = port; - else if (strcmp(ci->keyword, "password") == 0) + /* If !keep_password, we unconditionally drop old password */ + else if ((password || !keep_password) && + strcmp(ci->keyword, "password") == 0) values[paramnum++] = password; else if (strcmp(ci->keyword, "fallback_application_name") == 0) values[paramnum++] = pset.progname; - else if (strcmp(ci->keyword, "client_encoding") == 0) - values[paramnum++] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto"; + else if (client_encoding && + strcmp(ci->keyword, "client_encoding") == 0) + values[paramnum++] = client_encoding; else if (ci->val) values[paramnum++] = ci->val; /* else, don't bother making libpq parse this keyword */