Skip to content

fix: preserve sslmode when serializing connection config in ToPostgresURL #5958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PR1709 wants to merge 1 commit into from
+63 −4
Closed

fix: preserve sslmode when serializing connection config in ToPostgresURL #5958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/cli-go/internal/db/diff/explicit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func TestResolveExplicitDatabaseRef(t *testing.T) {
ref, err := resolveExplicitDatabaseRef(context.Background(), "local", fsys, nil, nil)

require.NoError(t, err)
assert.Equal(t, "postgresql://postgres:postgres@127.0.0.1:54322/postgres?connect_timeout=10", ref)
assert.Equal(t, "postgresql://postgres:postgres@127.0.0.1:54322/postgres?connect_timeout=10&sslmode=disable", ref)
})

t.Run("passes through database url", func(t *testing.T) {
Expand All @@ -45,7 +45,7 @@ func TestResolveExplicitDatabaseRef(t *testing.T) {
}, nil)

require.NoError(t, err)
assert.Equal(t, "postgresql://postgres:secret@db.abcdefghijklmnopqrst.supabase.co:5432/postgres?connect_timeout=10", ref)
assert.Equal(t, "postgresql://postgres:secret@db.abcdefghijklmnopqrst.supabase.co:5432/postgres?connect_timeout=10&sslmode=disable", ref)
})

t.Run("rejects unknown target", func(t *testing.T) {
Expand Down
23 changes: 23 additions & 0 deletions apps/cli-go/internal/utils/connect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ func toPostgresURL(config pgconn.Config, userinfo *url.Userinfo) string {
timeoutSecond = 10
}
queryParams := fmt.Sprintf("connect_timeout=%d", timeoutSecond)
if _, ok := config.RuntimeParams["sslmode"]; !ok {
queryParams += fmt.Sprintf("&sslmode=%s", getSSLMode(config))
Comment on lines +46 to +47

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid emitting an sslmode that defeats the TLS probe

Whenever this serialized URL is passed to GetRootCA, it now already contains an sslmode, but isRequireSSL still appends another &sslmode=require in internal/gen/types/types.go. pgconn reads the first value for duplicate URL parameters, so a derived disable or prefer remains effective; against a plaintext-only database the probe can connect without TLS and incorrectly report that TLS is required, causing downstream pg-meta/pg-delta setup to receive CA/TLS configuration for a server that does not support it. The probe should replace the query parameter rather than append a duplicate.

Useful? React with 👍 / 👎.

}
for k, v := range config.RuntimeParams {
queryParams += fmt.Sprintf("&%s=%s", k, url.QueryEscape(v))
}
Expand All @@ -61,6 +64,26 @@ func toPostgresURL(config pgconn.Config, userinfo *url.Userinfo) string {
)
}

func getSSLMode(config pgconn.Config) string {
if mode, ok := config.RuntimeParams["sslmode"]; ok {
return mode
}
if config.TLSConfig == nil {
if len(config.Fallbacks) > 0 && config.Fallbacks[0].TLSConfig != nil {
return "allow"
}
return "disable"
Comment on lines +71 to +75

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid disabling TLS for manually constructed remote configs

Many remote configs are constructed directly rather than by pgconn.ParseConfig—for example NewDbConfigWithPassword in internal/utils/flags/db_url.go and the direct branch URL in internal/branches/get/get.go—so a nil TLSConfig means no TLS policy has been materialized, not that the user explicitly selected disable. Returning sslmode=disable here makes linked CLI operations and generated POSTGRES_URL_NON_POOLING values use plaintext; before this change, omitting the parameter let pgconn default to prefer, after which ConnectByUrl removed the insecure fallback and effectively required TLS. Preserve that secure default unless disabling TLS was explicit.

Useful? React with 👍 / 👎.

@depthfirst-app depthfirst-app Bot Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Severity: MEDIUM

getSSLMode returns "disable" for any pgconn.Config with TLSConfig == nil — including configs constructed programmatically (not from sslmode=disable in a URL). NewDbConfigWithPassword builds cloud Supabase configs without setting TLSConfig, so ToPostgresURL now writes sslmode=disable into the serialized URL. This URL is stored in .env (as POSTGRES_URL) by the bootstrap command and passed to pgdelta/declarative tools. Applications that read this URL (Prisma, pg, etc.) will connect to the cloud DB without TLS — even if their own default would have been require or prefer — potentially exposing credentials in cleartext.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: The root cause is that getSSLMode cannot distinguish between a pgconn.Config that had sslmode=disable explicitly set in a URL (which pgconn.ParseConfig resolves by setting TLSConfig = nil and removing sslmode from RuntimeParams) and a programmatically-built config (like NewDbConfigWithPassword) where TLSConfig was simply never set. Both result in the same state: TLSConfig == nil, Fallbacks empty, and sslmode absent from RuntimeParams. Changing line 75 alone to return a different value would break round-trip preservation for explicitly-disabled TLS connections.

The correct fix is in NewDbConfigWithPassword (in internal/utils/flags/db_url.go): add an explicit TLSConfig when constructing the cloud Supabase pgconn.Config. Add "crypto/tls" to imports, then set TLSConfig: &tls.Config{ServerName: utils.GetSupabaseDbHost(projectRef)} on the struct literal. With a non-nil TLSConfig, getSSLMode will correctly return "require" (because InsecureSkipVerify is false and there are no plaintext fallbacks), so ToPostgresURL will serialize sslmode=require into the cloud URL and the .env file written by bootstrap will carry a proper TLS requirement to downstream clients such as Prisma and pg.

}
if len(config.Fallbacks) > 0 && config.Fallbacks[0].TLSConfig == nil {
return "prefer"
}
if !config.TLSConfig.InsecureSkipVerify {
return "verify-full"
}
return "require"
Comment on lines +80 to +83

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve verify-ca instead of downgrading it to require

When an input --db-url uses sslmode=verify-ca, pgconn represents it with TLSConfig.InsecureSkipVerify plus a custom certificate-verification callback. This branch conflates that state with require and serializes sslmode=require; because the CA configuration is not serialized either, reparsing the URL can silently remove the certificate verification the user requested. Distinguish the verification callback state and cover verify-ca in the round-trip test.

Useful? React with 👍 / 👎.

}


var ErrPrimaryNotFound = errors.New("primary database not found")

func GetPoolerConfigPrimary(ctx context.Context, ref string) (api.SupavisorConfigResponse, error) {
Expand Down
40 changes: 38 additions & 2 deletions apps/cli-go/internal/utils/connect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package utils

import (
"context"
"fmt"
"io"
"net"
"net/http"
Expand Down Expand Up @@ -392,7 +393,7 @@ func TestPostgresURL(t *testing.T) {
"options": "test",
},
})
assert.Equal(t, `postgresql://postgres:%21%40%23$%25%5E&%2A%28%29@[2406:da18:4fd:9b0d:80ec:9812:3e65:450b]:5432/?connect_timeout=10&options=test`, url)
assert.Equal(t, `postgresql://postgres:%21%40%23$%25%5E&%2A%28%29@[2406:da18:4fd:9b0d:80ec:9812:3e65:450b]:5432/?connect_timeout=10&sslmode=disable&options=test`, url)
}

func TestPostgresURLWithoutPassword(t *testing.T) {
Expand All @@ -408,6 +409,41 @@ func TestPostgresURLWithoutPassword(t *testing.T) {
url := ToPostgresURLWithoutPassword(config)
// Same as ToPostgresURL but with the password omitted from the userinfo, so a
// credential is never written to stdout by the db __shadow seam.
assert.Equal(t, `postgresql://postgres@[2406:da18:4fd:9b0d:80ec:9812:3e65:450b]:5432/?connect_timeout=10&options=test`, url)
assert.Equal(t, `postgresql://postgres@[2406:da18:4fd:9b0d:80ec:9812:3e65:450b]:5432/?connect_timeout=10&sslmode=disable&options=test`, url)
assert.NotContains(t, url, "%21%40%23")
}

func TestPostgresURL_SSLModePreserved(t *testing.T) {
modes := []struct {
mode string
expectedMode string
}{
{"disable", "disable"},
{"allow", "allow"},
{"prefer", "prefer"},
{"require", "require"},
{"verify-full", "verify-full"},
}

for _, tc := range modes {
t.Run(tc.mode, func(t *testing.T) {
inputURL := fmt.Sprintf("postgresql://postgres:password@localhost:5432/postgres?sslmode=%s", tc.mode)
config, err := pgconn.ParseConfig(inputURL)
require.NoError(t, err)

resURL := ToPostgresURL(*config)
assert.Contains(t, resURL, fmt.Sprintf("sslmode=%s", tc.expectedMode))

reparsedConfig, err := pgconn.ParseConfig(resURL)
require.NoError(t, err)

assert.Equal(t, config.TLSConfig != nil, reparsedConfig.TLSConfig != nil)
assert.Equal(t, len(config.Fallbacks), len(reparsedConfig.Fallbacks))
if config.TLSConfig != nil {
assert.Equal(t, config.TLSConfig.InsecureSkipVerify, reparsedConfig.TLSConfig.InsecureSkipVerify)
}
})
}
}


Loading