fix: preserve sslmode when serializing connection config in ToPostgresURL - #5958
fix: preserve sslmode when serializing connection config in ToPostgresURL#5958PR1709 wants to merge 1 commit into
Conversation
|
👋 Thanks! The linked issue hasn't been marked A maintainer adds that label once an issue is triaged and ready to be worked on. Please wait for the label before opening a pull request. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e2325e228
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !config.TLSConfig.InsecureSkipVerify { | ||
| return "verify-full" | ||
| } | ||
| return "require" |
There was a problem hiding this comment.
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 👍 / 👎.
| if _, ok := config.RuntimeParams["sslmode"]; !ok { | ||
| queryParams += fmt.Sprintf("&sslmode=%s", getSSLMode(config)) |
There was a problem hiding this comment.
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 👍 / 👎.
| if config.TLSConfig == nil { | ||
| if len(config.Fallbacks) > 0 && config.Fallbacks[0].TLSConfig != nil { | ||
| return "allow" | ||
| } | ||
| return "disable" |
There was a problem hiding this comment.
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[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| if len(config.Fallbacks) > 0 && config.Fallbacks[0].TLSConfig != nil { | ||
| return "allow" | ||
| } | ||
| return "disable" |
There was a problem hiding this comment.
🟡 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.
1 participant
Fixes #5873
Summary
When
pgconn.ParseConfigparses a Postgres database URL containingsslmode(e.g.sslmode=requireorsslmode=disable),pgconnresolvessslmodeintoTLSConfigandFallbacksand stripssslmodefromRuntimeParams.When
utils.ToPostgresURL()subsequently serializedpgconn.Configback into a URL string, it only serializedconnect_timeoutandRuntimeParams, thereby droppingsslmode. As a result, downstream callers re-parsing the serialized URL reverted to defaultsslmodebehavior (prefer+ environment variable fallbacks), losing explicit--db-urlsslmodesettings.This PR updates
toPostgresURLto derive and append the appropriatesslmodequery parameter from thepgconn.Config'sTLSConfigandFallbacks(if not already specified inRuntimeParams), ensuringsslmoderequirements survive round-trips.Testing
TestPostgresURL_SSLModePreservedininternal/utils/connect_test.goverifying round-trip preservation across allsslmodesettings (disable,allow,prefer,require,verify-full).go test ./....