On Tue, Feb 27, 2024 at 04:22:34PM -0800, Jeff Davis wrote: > Do we need a documentation update here? If so, where would be a good > place?
I'm afraid I don't have a better idea than adding a short note in each affected commands's page. > On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote: >> I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new >> value >> for these. > > Did you have a particular concern about PGC_S_SESSION? My only concern is that it could obscure the source of the search_path change, which in turn might cause confusion when things fail. > If it's less than PGC_S_SESSION, it won't work, because the caller's > SET command will override it, and the same manipulation is possible. > > And I don't think we want it higher than PGC_S_SESSION, otherwise the > function can't set its own search_path, if needed. Yeah, we would have to make it equivalent in priority to PGC_S_SESSION, which would likely require a bunch of special logic. I don't know if this is worth it, and this seems like something that could pretty easily be added in the future if it became necessary. >> > +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp" >> >> Is including pg_temp actually safe? I worry that a user could use >> their >> temporary schema to inject objects that would take the place of >> non-schema-qualified stuff in functions. > > pg_temp cannot (currently) be excluded. If it is omitted from the > string, it will be placed *first* in the search_path, which is more > dangerous. > > pg_temp does not take part in function or operator resolution, which > makes it safer than it first appears. There are potentially some risks > around tables, but it's not typical to access a table in a function > called as part of an index expression. > > If we determine that pg_temp is actually unsafe to include, we need to > do something like what I proposed here: > > https://www.postgresql.org/message-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.ca...@j-davis.com I don't doubt anything you've said, but I can't help but think that we might as well handle the pg_temp risk, too. Furthermore, I see that we use "" as a safe search_path for autovacuum and fe_utils/connect.h. Is there any way to unite these? IIUC it might be possible to combine the autovacuum and maintenance command cases (i.e., "!pg_temp"), but we might need to keep pg_temp for the frontend case. I think it's worth trying to add comments about why this setting is safe for some cases but not others, too. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com