Skip to content

fix(postgres,pgx): error instead of panicking on unquoted MigrationsTable - #1455

Open
xianyu-sheng wants to merge 1 commit into
golang-migrate:masterfrom
xianyu-sheng:fix/quoted-table-panic
Open

xianyu-sheng wants to merge 1 commit into
golang-migrate:masterfrom
xianyu-sheng:fix/quoted-table-panic

Conversation

@xianyu-sheng

Copy link
Copy Markdown

fix(postgres,pgx): error instead of panicking on unquoted MigrationsTable

Problem

When MigrationsTableQuoted is enabled, the PostgreSQL drivers extract the
quoted identifiers with:

result := re.FindAllStringSubmatch(config.MigrationsTable, -1)
config.migrationsTableName = result[len(result)-1][1]

If MigrationsTable contains no double quotes — including when it is left
empty and defaults to schema_migrations — result is empty and the index
expression panics:

--- FAIL: TestWithInstanceQuotedTableWithoutQuotes
panic: runtime error: index out of range [-1]

This reproduces in all three drivers: database/postgres, database/pgx and
database/pgx/v5.

Fix

Return a configuration error when no quoted identifier is found, consistent
with the existing "too many dot characters" error path:

if len(result) == 0 {
    return nil, fmt.Errorf("\"%s\" MigrationsTable is not quoted", config.MigrationsTable)
}

Regression tests

One test per driver (database/**/*_quoted_test.go) using a ping-only fake
database/sql driver, so no real database is needed:

Before:

panic: runtime error: index out of range [-1]
FAIL	github.com/golang-migrate/migrate/v4/database/postgres
FAIL	github.com/golang-migrate/migrate/v4/database/pgx
FAIL	github.com/golang-migrate/migrate/v4/database/pgx/v5

After:

--- PASS: TestWithInstanceQuotedTableWithoutQuotes
ok  	github.com/golang-migrate/migrate/v4/database/postgres
ok  	github.com/golang-migrate/migrate/v4/database/pgx
ok  	github.com/golang-migrate/migrate/v4/database/pgx/v5

go build ./... passes.

Detection

Found during a SmartBench audit of
the repository. Its evidence-gated LLM review on the vulnerable revision flags
the empty-match indexing in the quoted-table parsing; the regression tests
above confirm the finding independently.

Copilot AI balanced review requested due to automatic review settings October 15, 2026 12:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@xianyu-sheng
xianyu-sheng force-pushed the fix/quoted-table-panic branch from f9b6bf0 to 7da4e3a Compare October 15, 2026 13:08
@coveralls

coveralls commented Oct 15, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 54.988%. first build — xianyu-sheng:fix/quoted-table-panic into golang-migrate:master

@xianyu-sheng

Copy link
Copy Markdown
Author

For reference, the panic is reachable without the Go API too. The URL guard only fires when x-migrations-table is non-empty:

if (len(migrationsTable) > 0) && (migrationsTableQuoted) && ((migrationsTable[0] != '"') || ...) {
    return nil, fmt.Errorf("x-migrations-table must be quoted ...")
}

So this URL reaches the panic (the table then defaults to schema_migrations, which contains no quotes):

migrate -database "postgres://user:pass@localhost:5432/dbname?x-migrations-table-quoted=true" -path ./migrations up

Same for pgx:// and pgx/v5://. The regression tests in this PR cover the equivalent programmatic path (WithInstance with MigrationsTableQuoted: true), which is what produced the panic before the fix.

@xianyu-sheng
xianyu-sheng force-pushed the fix/quoted-table-panic branch from 7da4e3a to 07e0e44 Compare October 15, 2026 13:32
…able

When MigrationsTableQuoted is enabled and MigrationsTable contains no
double quotes (including when it is left empty and defaults to
schema_migrations), the quote matcher returns no matches and
result[len(result)-1] panics with an out-of-range index. Return a
configuration error instead, matching the existing too many dot
characters path.

Add regression tests for the postgres, pgx and pgx/v5 drivers.
@xianyu-sheng
xianyu-sheng force-pushed the fix/quoted-table-panic branch from 07e0e44 to 5084604 Compare October 15, 2026 13:55
@xianyu-sheng

Copy link
Copy Markdown
Author

CI status update: lint ✅ and test (1.25.x) ✅ pass in the current run (runs/38746851739), but test (1.26.x) has been stuck in its test step for ~50 minutes (comparable runs finish in ~20 min).

The failures seen in earlier runs of the same job are all in packages unrelated to this change:

  • database/sqlserver: testOpenWithPasswordAndMSI/mcr.microsoft.com/mssql/server:2025-latest times out after 60s while starting the container — run 2, run 3
  • source/github: TestDefaultClient fails with 403 API rate limit exceeded for the runner IP — run 2

Both packages passed in run 1 and test (1.25.x) passes in the current run. Locally go test ./database/postgres/ ./database/pgx/ ./database/pgx/v5/ -run TestWithInstanceQuotedTableWithoutQuotes passes; none of the changed files touch sqlserver or source/github.

Would a maintainer mind re-running the test (1.26.x) job when convenient? Thanks!

@xianyu-sheng

Copy link
Copy Markdown
Author

CI summary across the last few runs on this branch:

  • lint ✅ (after the errcheck fix)
  • test (1.25.x) ✅ in every completed run
  • test (1.26.x) ✅ in runs/38850853540, ❌ in runs/38852927263 on an unrelated infrastructure flake: database/sqlserver testMsiTrue/mcr.microsoft.com/mssql/server:2025-latest times out after 60s while starting the container (earlier runs also hit a source/github 403 rate limit from the runner IP).

So both test jobs have passed at least once on this exact commit, and the failures are always in packages this PR does not touch. Could a maintainer re-run the failed test (1.26.x) job when convenient? Happy to adjust anything on the change itself.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants