On Fri, Jan 13, 2023 at 09:19:10AM +0100, Jelte Fennema wrote: >> Even if folks applying quotes >> would not be able anymore to replace the pattern, the risk seems a bit >> remote? > > Yeah I agree the risk is remote. To be clear, the main pattern I'm > worried about breaking is simply "\1". Where people had put > quotes around \1 for no reason. All in all, I'm fine if 0003 gets > merged, but I'd also be fine with it if it doesn't. Both the risk > and the advantage seem fairly small.
Still, I am having a few second thoughts about 0003 after thinking about it over the weekend. Except if I am missing something, there are no issues with 0004 if we keep the current behavior of always replacing \1 even if pg-user is quoted? I would certainly add a new test case either way. >> I don't see how much that's different from the recent discussion with >> regexps added for databases and users to pg_hba.conf. And consistency >> sounds pretty good to me here. > > It's not much different, except that here also all and + change their meaning > (for pg_hba.conf those special cases already existed). Mainly I called it out > because I realised this discussion was called out in that commit too. > >> Regexps can have commas > > That's a really good reason to allow quoted regexes indeed. Even for pg_ident > entries, commas in unquoted regexes would cause the AuthToken parsing to fail. > > Is there anything you still want to see changed about any of the patches? + /* + * Mark the token as quoted, so it will only be compared literally + * and not for special meanings like, such as "all" and membership + * checks using the + prefix. + */ + expanded_pg_user_token = make_auth_token(expanded_pg_user, true); It is critical to quote this AuthToken after the replacement, indeed. Or we are in big trouble. - /* no substitution, so copy the match */ - expanded_pg_user = pstrdup(identLine->pg_user->string); + expanded_pg_user_token = identLine->pg_user; Perhaps it would be simpler to use copy_auth_token() in this code path and always free the resulting token? In the code path where system-user is a regexp, could it be better to skip the replacement of \1 in the new AuthToken if pg-user is itself a regexp? The compiled regexp would be the same, but it could be considered as a bit confusing, as it can be thought that the compiled regexp of pg-user happened after the replacement? No issues with 0002 after a second look, so applied to move on. -- Michael
signature.asc
Description: PGP signature