From e8156a8e4bcc5236f97b20e51f7390a4845cdb23 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 08:43:23 +0200 Subject: [PATCH 1/2] fix(sec): percent-encode or quote the database password in remaining DSN builders Encode the password with jq -sRr @uri in the CI migrate step and the DEPLOYMENT.md migrate commands, build the testutil connection string with url.UserPassword, and single-quote keyword DSN passwords that contain whitespace, quotes or backslashes. Round-trip tests parse the result back with pgx. --- .github/workflows/ci.yml | 4 +++- docs/DEPLOYMENT.md | 11 ++++++++-- internal/database/config.go | 14 +++++++++++- internal/database/config_test.go | 20 ++++++++++++++++++ internal/testutil/postgres.go | 12 +++++++++-- internal/testutil/postgres_test.go | 34 ++++++++++++++++++++++++++++++ 6 files changed, 89 insertions(+), 6 deletions(-) create mode 100644 internal/testutil/postgres_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 17f59ecb5..c7a917abd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -397,8 +397,10 @@ jobs: DB_PASSWORD: test_password # CI-only throwaway password — not used in any real environment run: | if [ -d "internal/database/postgres/migrations" ]; then + # Percent-encode the password: it may contain @ : / ? # % or spaces. + ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) migrate -path internal/database/postgres/migrations \ - -database "pgx5://${DB_USER}:${DB_PASSWORD}@${DB_HOST}:${DB_PORT}/${DB_NAME}?sslmode=disable" \ + -database "pgx5://${DB_USER}:${ENCODED_PASSWORD}@${DB_HOST}:${DB_PORT}/${DB_NAME}?sslmode=disable" \ up fi diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index af8a8dfd2..8f548af26 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -666,13 +666,18 @@ If `DB_AUTO_MIGRATE=true` (the default), migration 000011 runs automatically on > driver by scheme, and `migrate` is built here with `-tags pgx5` so it does not > link `lib/pq` (issue #1849). A `postgres://` URL fails with > `unknown driver postgres`. +> +> The password is percent-encoded with `jq -sRr @uri` because it may contain +> `@`, `:`, `/`, `%`, `#`, `?` or spaces, which break the URL when left raw. ```bash DB_PASSWORD=$(aws secretsmanager get-secret-value --secret-id cudly-dev-db-password-* --query SecretString --output text | jq -r .password) RDS_ENDPOINT=$(cd terraform/environments/aws && terraform output -raw database_proxy_endpoint) +ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) + migrate -path internal/database/postgres/migrations \ - -database "pgx5://cudly:${DB_PASSWORD}@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up + -database "pgx5://cudly:${ENCODED_PASSWORD}@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up ``` --- @@ -685,8 +690,10 @@ Check: `DB_AUTO_MIGRATE=true`, `DB_MIGRATIONS_PATH=/app/internal/database/postgr DB_PASSWORD=$(aws secretsmanager get-secret-value --secret-id cudly-dev-db-password-* --query SecretString --output text) RDS_ENDPOINT=$(cd terraform/environments/aws && terraform output -raw database_proxy_endpoint) +ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) + migrate -path internal/database/postgres/migrations \ - -database "pgx5://cudly:${DB_PASSWORD}@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up + -database "pgx5://cudly:${ENCODED_PASSWORD}@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up ``` ### Terraform State Lock diff --git a/internal/database/config.go b/internal/database/config.go index a4ed4c46b..21bbb4b95 100644 --- a/internal/database/config.go +++ b/internal/database/config.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "strconv" + "strings" "time" ) @@ -145,13 +146,24 @@ func (c *Config) dsn(password string) string { c.Host, c.Port, c.User, - password, + quoteDSNValue(password), c.Database, c.SSLMode, int(c.ConnectTimeout.Seconds()), ) } +// quoteDSNValue quotes a keyword/value DSN value per libpq rules when it is +// empty or contains whitespace, a single quote or a backslash; otherwise it +// returns the value unchanged. +func quoteDSNValue(v string) string { + if v != "" && !strings.ContainsAny(v, " \t\n\r\f\v'\\") { + return v + } + r := strings.NewReplacer(`\`, `\\`, `'`, `\'`) + return "'" + r.Replace(v) + "'" +} + // DSN generates a PostgreSQL connection string // If passwordOverride is provided, it's used instead of config.Password. func (c *Config) DSN(passwordOverride string) string { diff --git a/internal/database/config_test.go b/internal/database/config_test.go index 49041d478..e7f887a90 100644 --- a/internal/database/config_test.go +++ b/internal/database/config_test.go @@ -5,6 +5,7 @@ import ( "testing" "time" + "github.com/jackc/pgx/v5/pgconn" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -822,3 +823,22 @@ func TestValidatePoolSettings(t *testing.T) { }) } } + +// trickyPassword holds every character that breaks an unquoted keyword DSN or +// an unescaped URL userinfo. +const trickyPassword = `p@ss:w/rd%#? "q" 'it\s'` + +func TestConfigDSN_PasswordRoundTrip(t *testing.T) { + cfg := &Config{ + Host: "localhost", Port: 5432, User: "u", Database: "d", + SSLMode: "disable", ConnectTimeout: 10 * time.Second, + } + for _, pw := range []string{trickyPassword, "has space", "it's", `back\slash`, "", "plain"} { + parsed, err := pgconn.ParseConfig(cfg.DSN(pw)) + require.NoError(t, err, "password %q", pw) + assert.Equal(t, pw, parsed.Password) + assert.Equal(t, "u", parsed.User) + assert.Equal(t, "d", parsed.Database) + assert.Equal(t, "localhost", parsed.Host) + } +} diff --git a/internal/testutil/postgres.go b/internal/testutil/postgres.go index 2b0db1d9c..7e1ed2eaa 100644 --- a/internal/testutil/postgres.go +++ b/internal/testutil/postgres.go @@ -6,6 +6,8 @@ package testutil import ( "context" "fmt" + "net" + "net/url" "testing" "time" @@ -76,8 +78,14 @@ func SetupPostgresContainer(ctx context.Context, t *testing.T) (*PostgresContain // ConnectionString returns a PostgreSQL connection string. func (pc *PostgresContainer) ConnectionString() string { - return fmt.Sprintf("postgresql://%s:%s@%s:%s/%s?sslmode=disable", - pc.Username, pc.Password, pc.Host, pc.Port, pc.Database) + u := url.URL{ + Scheme: "postgresql", + User: url.UserPassword(pc.Username, pc.Password), + Host: net.JoinHostPort(pc.Host, pc.Port), + Path: "/" + pc.Database, + RawQuery: "sslmode=disable", + } + return u.String() } // Config returns a database configuration for the test container. diff --git a/internal/testutil/postgres_test.go b/internal/testutil/postgres_test.go new file mode 100644 index 000000000..6b3a13ff0 --- /dev/null +++ b/internal/testutil/postgres_test.go @@ -0,0 +1,34 @@ +//go:build integration +// +build integration + +package testutil + +import ( + "net/url" + "testing" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConnectionString_PasswordRoundTrip(t *testing.T) { + pw := `p@ss:w/rd%#? "q" 'it\s'` + pc := &PostgresContainer{ + Host: "localhost", Port: "5432", Database: "cudly_test", + Username: "cudly_test", Password: pw, + } + + u, err := url.Parse(pc.ConnectionString()) + require.NoError(t, err) + got, _ := u.User.Password() + assert.Equal(t, pw, got) + + cfg, err := pgconn.ParseConfig(pc.ConnectionString()) + require.NoError(t, err) + assert.Equal(t, pw, cfg.Password) + assert.Equal(t, "cudly_test", cfg.User) + assert.Equal(t, "localhost", cfg.Host) + assert.Equal(t, uint16(5432), cfg.Port) + assert.Equal(t, "cudly_test", cfg.Database) +} From e88480122af66a686fccdbb90edae92bbfbe5ced Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 6 Oct 2026 08:51:21 +0200 Subject: [PATCH 2/2] test(database): pin the quoted DSN text for backslash and quote passwords Make the second DEPLOYMENT.md migrate block extract .password like the first. --- docs/DEPLOYMENT.md | 2 +- internal/database/config_test.go | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index 8f548af26..cabb92952 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -687,7 +687,7 @@ migrate -path internal/database/postgres/migrations \ Check: `DB_AUTO_MIGRATE=true`, `DB_MIGRATIONS_PATH=/app/internal/database/postgres/migrations`, correct credentials. Run manually: ```bash -DB_PASSWORD=$(aws secretsmanager get-secret-value --secret-id cudly-dev-db-password-* --query SecretString --output text) +DB_PASSWORD=$(aws secretsmanager get-secret-value --secret-id cudly-dev-db-password-* --query SecretString --output text | jq -r .password) RDS_ENDPOINT=$(cd terraform/environments/aws && terraform output -raw database_proxy_endpoint) ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) diff --git a/internal/database/config_test.go b/internal/database/config_test.go index e7f887a90..3e701dd0d 100644 --- a/internal/database/config_test.go +++ b/internal/database/config_test.go @@ -842,3 +842,14 @@ func TestConfigDSN_PasswordRoundTrip(t *testing.T) { assert.Equal(t, "localhost", parsed.Host) } } + +func TestConfigDSN_PasswordQuotingText(t *testing.T) { + cfg := &Config{ + Host: "localhost", Port: 5432, User: "u", Database: "d", + SSLMode: "disable", ConnectTimeout: 10 * time.Second, + } + // libpq unescapes a backslash in quoted values only, and pgx does not + // reject an unquoted one, so the round trip alone cannot catch its omission. + assert.Contains(t, cfg.DSN(`back\slash`), ` password='back\\slash' `) + assert.Contains(t, cfg.DSN("it's"), ` password='it\'s' `) +}