On Sun, 6 Apr 2025 at 20:53, Álvaro Herrera <
alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Apr-06, Andrew Dunstan wrote:
>
> > On 2025-03-28 Fr 10:43 AM, Nathan Bossart wrote:
>
> > > Taking a step back, are we sure that 1) this is the right place to do these
> > > checks and 2) we shouldn't apply the same restrictions to all names? I'm
> > > wondering if it would be better to add these checks to the grammar instead
> > > of trying to patch up all the various places they are used in the tree.
> >
> > Maybe. I don't think there is time for that for v18, so we'd have to defer
> > this for now. I can live with that - it's been like this for a long time.
>
> Grumble. I'd rather introduce a partial restriction now only for names
> that affect the tools failing outright (pg_dumpall in particular), than
> do nothing for this release. If we feel the need to extend the
> restriction later, that's easy to do and bothers nobody. We've wanted
> these characters to be forbidden on database names for a long time, but
> nobody has cared as much for their use on other types of names. I'm not
> even sure we'd support the idea of forbidding them on all names (though
> the SQL standard doesn't allow control chars in identifiers.)
>
> Another point is that we can easily have pg_upgrade check for invalid
> database and role names, but checking *all* names would be more onerous.
>
> I don't like the present implementation though, on translability
> grounds. I think the error message should appear at each callsite
> rather than be hardcoded in the new function, to avoid string building.
> I think it'd be cleaner if the new function (maybe "name_contains_crlf"
> or "is_identifier_awful") just returned a boolean based on strpbrk(),
> and the callsite throws the error.
>
> I wonder why does the patch restrict both database and role names. Does
> a user with a newline also cause pg_upgrade to fail? I mean, this
> thread started with a consideration for database names only, and the
> usage on role names seemed to have appeared out of nowhere in [1].
In my testing, pg_dumpall was not failing with roles/user (\n\r) in my machine but due to the below comment in code, I restricted roles also.
+++ b/src/fe_utils/string_utils.c
@@ -568,12 +568,6 @@ appendByteaLiteral(PQExpBuffer buf, const unsigned char *str, size_t length,
* Append the given string to the shell command being built in the buffer,
* with shell-style quoting as needed to create exactly one argument.
*
- * Forbid LF or CR characters, which have scant practical use beyond designing
- * security breaches. The Windows command shell is unusable as a conduit for
- * arguments containing LF or CR characters. A future major release should
- * reject those characters in CREATE ROLE and CREATE DATABASE, because use
- * there eventually leads to errors here.
- *
* appendShellString() simply prints an error and dies if LF or CR appears.
