SET LOCAL with parameters fails with syntax error

[gregwoodfill]

Describe the bug
When calling conn.Exec to set a local postgres with parameters, a syntax error is returned.
This started happening in v5.5.4 and looks to be related to adding parentheses around parameter values from this commit: c543134
Issuing SET LOCAL with a value wrapped in parentheses is not valid Postgres syntax.

To Reproduce
Steps to reproduce the behavior:

package main

import (
	"context"
	"github.com/jackc/pgx/v5"
	"log"
	"os"
)

func main() {
	conn, err := pgx.Connect(context.Background(), os.Getenv("DATABASE_URL"))
	if err != nil {
		log.Fatal(err)
	}
	defer conn.Close(context.Background())
	connMetadata := "some metadata"
	_, err = conn.Exec(context.Background(), "set local someMeta to $1", pgx.QueryExecModeSimpleProtocol, connMetadata)
	if err != nil {
		log.Fatal(err)
	}
}

Please run your example with the race detector enabled. For example, go run -race main.go or go test -race.

2024/03/05 16:52:02 ERROR: syntax error at or near "(" (SQLSTATE 42601)

Expected behavior
conn.Exec executes the statement without error.

Actual behavior
conn.Exec returns a syntax error.

syntax error at or near "(" (SQLSTATE 42601)

Version

• Go: v1.22.0
• PostgreSQL: PostgreSQL 16.1 (Debian 16.1-1.pgdg120+1) on x86_64-pc-linux-gnu, compiled by gcc (Debian 12.2.0-14) 12.2.0, 64-bit
• pgx: v5.5.4

Additional context
Add any other context about the problem here.

Appears to have been introduced here: c543134
PostgreSQL documentation on SET LOCAL: https://www.postgresql.org/docs/current/sql-set.html

[michaelcmelton]

Same issue and error is returned with the INTERVAL syntax as well.

[nickpalmer]

This change:
c543134#diff-65a99c1d9a3b358d8aebc18eb1f6d77c6142f66cdb2f330b66577681b38365ffR69

Should probably be something like:

if strings.HasPrefix(str, '-') || strings.Contains(str, '--') {
  str = '(' + str + ')'
}

But reading the CVE I think neither the original patch, nor this "don't break various queries version" may not solve the problem correctly.

A test should be added that directly addresses the CVE because it seems like a value could be sent that includes a closing paren which would bypass this mitigation.

I think the second part of the above or clause is not entirely correct. I think we have to actually parse the string for quotes because a value like '--' could be okay.

[nickpalmer]

I ended up going with a different approach using the postgresql extension of escape strings.

[jackc]

@gregwoodfill I'm willing to try to get this working again, but it's more or less a happy accident that it ever did work. To be honest, I never anticipated people purposely using the simple protocol to get parameter substitution in cases where PostgreSQL doesn't allow it. I had intended it to match the capabilities of the extended protocol / prepared statements in cases where the simple protocol was in use.

@michaelcmelton Can you give an example?

[michaelcmelton]

Hey @jackc ! Thanks for the quick response! So we're running queries via gorm which has it's own postgres driver that's entirely dependent on your library. In these queries, we're doing some date calculations on the fly with INTERVAL, and using parameter substitution to pass a value to the query for interval. Effectively, with gorm's abstractions above it, it looks something similar to this:

result = db.Where(`column >= (?::DATE - INTERVAL ?, date, validIntervalString)...`.Find(&variable)

The resultant query on the DB side is logged and looks something similar to this:

SELECT * FROM table WHERE column >= (('2024-03-07')::DATE - INTERVAL ('7 DAYS')

Given the changes in the sanitizer, even with a properly formatted parameter, SQL returns a syntax error pointing directly to the substituted interval string everything else seems valid.

[gregwoodfill]

@gregwoodfill I'm willing to try to get this working again, but it's more or less a happy accident that it ever did work. To be honest, I never anticipated people purposely using the simple protocol to get parameter substitution in cases where PostgreSQL doesn't allow it. I had intended it to match the capabilities of the extended protocol / prepared statements in cases where the simple protocol was in use.

@jackc thank you for the reply.

If the design of PGX is to follow parameter substitution only where PostgreSQL allows, then I think it's fair that this is now the expected behavior.

If this was changed in a future update, we would go back to using it, but for now we have a workaround for our one case that we were using simple protocol with parameter substitution.

[jackc]

I've released v5.5.5 and v4.18.3 which now use spaces instead of parentheses to ensure a minus can't be directly in front of a negative number leading to a line comment (--). This should restore the previous functionality.

@michaelcmelton Your INTERVAL use should work again. But it's interesting to note that it will fail if that query is ever run via the extended protocol / prepared statement. As far as PostgreSQL is concerned the entire INTERVAL '7 days' is the literal value, not the '7 days'.

e.g.

postgres@[local]:5015 pgx_test=# prepare s as select INTERVAL $1;
ERROR:  42601: syntax error at or near "$1"
LINE 1: prepare s as select INTERVAL $1;

It might be better would be to pass in a Go time.Duration which pgx automatically encodes as a PostgreSQL interval or to use the make_interval function.