Skip to content

postgres module: DROP/CREATE DATABASE identifiers and Snapshot()'s datname literal aren't quote-escaped #3908

Description

@cipherprofessor

Context

While fixing #3233 in PR #3907, CodeRabbit's review caught that the new pg_terminate_backend calls in restoreCommands() built a datname = '%s' SQL string literal by interpolating snapshotName/c.dbName directly, unescaped (review comment). That was fixed there with a quoteLiteral() helper (doubles embedded single quotes; wraps in E'...' with doubled backslashes when the value contains one, so it's correct regardless of the server's standard_conforming_strings setting).

The same underlying class of bug — building SQL by direct fmt.Sprintf interpolation with no escaping — is still present in a few other spots in modules/postgres/postgres.go on current main, none of which were touched by #3907:

  1. Snapshot() (~line 280) has the exact pre-existing version of the literal-quoting bug just fixed in Restore():

    fmt.Sprintf(`UPDATE pg_database SET datistemplate = FALSE WHERE datname = '%s'`, snapshotName),

    A snapshotName containing a single quote (or, on a server with standard_conforming_strings=off, a trailing backslash) breaks out of the literal the same way it did in Restore().

  2. Double-quoted identifiers are never escaped, in both Snapshot() and Restore()/restoreCommands():

    fmt.Sprintf(`DROP DATABASE IF EXISTS "%s"`, snapshotName)                                  // Snapshot()
    fmt.Sprintf(`CREATE DATABASE "%s" WITH TEMPLATE "%s" OWNER "%s"`, snapshotName, c.dbName, c.user) // Snapshot()
    fmt.Sprintf(`ALTER DATABASE "%s" WITH is_template = TRUE`, snapshotName)                    // Snapshot()
    fmt.Sprintf(`DROP DATABASE IF EXISTS "%s" with (FORCE)`, c.dbName)                          // restoreCommands()
    fmt.Sprintf(`CREATE DATABASE "%s" WITH TEMPLATE "%s" OWNER "%s"`, c.dbName, snapshotName, c.user) // restoreCommands()

    A name containing a double quote (a valid Postgres identifier once itself quoted, e.g. via CREATE DATABASE "a""b") breaks out of the "%s" wrapper. Postgres identifier-escaping is "double the embedded double-quote," analogous to the single-quote doubling quoteLiteral() already does for literals.

Impact

dbName/snapshotName/user here come from the calling Go test's own configuration (WithDatabase, SnapshotOption, WithUsername), not untrusted network input, so this isn't a remote attack surface in typical use — but it is a real correctness bug (a legitimately-quoted database/user name breaks these statements) and a latent injection risk wherever those values are themselves derived from something less trusted (e.g. a name templated from CI metadata).

Suggested fix

Reuse the quoteLiteral() helper added in #3907 for the two literal contexts, and add an equivalent quoteIdentifier() (double embedded ") for the five identifier contexts above. Happy to send a PR for this if it's welcome — flagging first since it touches multiple call sites across both Snapshot() and Restore() and I wanted to check scope/appetite before doing the same TDD red/green + review pass on identifier quoting specifically.

Found while working on #3907; not blocking it.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions