diff --git a/c/driver/postgresql/database.cc b/c/driver/postgresql/database.cc index b416ee7c94b..f2adf73d847 100644 --- a/c/driver/postgresql/database.cc +++ b/c/driver/postgresql/database.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -187,7 +188,47 @@ AdbcStatusCode PostgresDatabase::Connect(PGconn** conn, struct AdbcError* error) "[libpq] Must set database option 'uri' before creating a connection"); return ADBC_STATUS_INVALID_STATE; } + char* parse_error = nullptr; + PQconninfoOption* options = PQconninfoParse(uri_.c_str(), &parse_error); + if (!options) { + if (!parse_error) { + InternalAdbcSetError(error, "[libpq] Failed to allocate connection options"); + return ADBC_STATUS_INTERNAL; + } + // Only emit fixed diagnostics: libpq messages may contain credentials, + // including in malformed option names. Unknown/localized messages stay generic. + std::string_view message(parse_error); + const char* diagnostic = nullptr; + if (message.starts_with("invalid percent-encoded token:")) { + diagnostic = "invalid percent-encoded token"; + } else if (message.starts_with("forbidden value %00 in percent-encoded value:")) { + diagnostic = "forbidden value %00 in percent-encoded value"; + } else if (message.starts_with("invalid URI query parameter:")) { + diagnostic = "invalid URI query parameter"; + } else if (message.starts_with("invalid connection option ")) { + diagnostic = "invalid connection option"; + } else if (message.starts_with("missing \"=\" after ")) { + diagnostic = "missing key/value separator in connection string"; + } else if (message.starts_with( + "missing key/value separator \"=\" in URI query parameter:")) { + diagnostic = "missing key/value separator in URI query parameter"; + } else if (message == "unterminated quoted string in connection info string\n") { + diagnostic = "unterminated quoted string in connection info string"; + } + PQfreemem(parse_error); + if (diagnostic) { + InternalAdbcSetError(error, "[libpq] Invalid connection string: %s", diagnostic); + } else { + InternalAdbcSetError(error, "[libpq] Invalid connection string"); + } + return ADBC_STATUS_INVALID_ARGUMENT; + } + PQconninfoFree(options); *conn = PQconnectdb(uri_.c_str()); + if (!*conn) { + InternalAdbcSetError(error, "[libpq] Failed to allocate connection"); + return ADBC_STATUS_INTERNAL; + } if (PQstatus(*conn) != CONNECTION_OK) { InternalAdbcSetError(error, "%s%s", "[libpq] Failed to connect: ", PQerrorMessage(*conn)); diff --git a/c/driver/postgresql/postgresql_test.cc b/c/driver/postgresql/postgresql_test.cc index 42e5df90567..bcf20f6daac 100644 --- a/c/driver/postgresql/postgresql_test.cc +++ b/c/driver/postgresql/postgresql_test.cc @@ -3707,3 +3707,29 @@ INSTANTIATE_TEST_SUITE_P(Decimal128FixtureRoundTrip, PostgresDecimalFixtureRound testing::Bool(), [](const testing::TestParamInfo& info) { return info.param ? "SlowPath" : "FastPath"; }); + +TEST(PostgresUriErrors, InvalidInputDoesNotLeak) { + for (const auto& [uri, diagnostic] : std::vector>{ + {"postgresql://user:sentinel%ZZ@localhost/db", + "invalid percent-encoded token"}, + {"host=localhost sentinel_secret", + "missing key/value separator in connection string"}, + {"postgresql://user:sentinel%00@localhost/db", + "forbidden value %00 in percent-encoded value"}, + {"postgresql://localhost/db?sentinel=value", "invalid URI query parameter"}, + {"sentinel=value", "invalid connection option"}, + {"postgresql://localhost/db?sentinel", + "missing key/value separator in URI query parameter"}, + {"password='sentinel", + "unterminated quoted string in connection info string"}}) { + struct AdbcError error = {}; + adbc_validation::Handle database; + ASSERT_EQ(ADBC_STATUS_OK, AdbcDatabaseNew(&database.value, &error)); + ASSERT_EQ(ADBC_STATUS_OK, AdbcDatabaseSetOption(&database.value, "uri", uri, &error)); + ASSERT_EQ(ADBC_STATUS_INVALID_ARGUMENT, AdbcDatabaseInit(&database.value, &error)); + ASSERT_NE(nullptr, error.message); + EXPECT_THAT(error.message, ::testing::Not(::testing::HasSubstr("sentinel"))); + EXPECT_THAT(error.message, ::testing::HasSubstr(diagnostic)); + if (error.release) error.release(&error); + } +} diff --git a/c/driver/sqlite/sqlite.cc b/c/driver/sqlite/sqlite.cc index e241797f422..9caffaf17ae 100644 --- a/c/driver/sqlite/sqlite.cc +++ b/c/driver/sqlite/sqlite.cc @@ -570,12 +570,24 @@ class SqliteDatabase : public driver::Database { /*zVfs=*/nullptr); if (rc != SQLITE_OK) { Status status; - if (conn_) { - status = status::fmt::IO("failed to open '{}': {}", uri_, sqlite3_errmsg(conn)); + if (conn) { + std::string_view message = sqlite3_errmsg(conn); + if ((rc == SQLITE_ERROR || rc == SQLITE_PERM) && + (message.starts_with("invalid uri authority:") || + message.starts_with("no such access mode:") || + message.starts_with("no such cache mode:") || + message.starts_with("access mode not allowed:") || + message.starts_with("cache mode not allowed:") || + message.starts_with("no such vfs:"))) { + status = status::fmt::InvalidArgument("invalid database URI: {}", + message.substr(0, message.find(':'))); + } else { + status = status::fmt::IO("failed to open database: {}", sqlite3_errstr(rc)); + } + (void)sqlite3_close(conn); } else { - status = status::fmt::IO("failed to open '{}': failed to allocate memory", uri_); + status = status::IO("failed to open database: failed to allocate memory"); } - (void)sqlite3_close(conn); return status; } return conn; diff --git a/c/driver/sqlite/sqlite_test.cc b/c/driver/sqlite/sqlite_test.cc index 22f7c8afe33..841a4745faa 100644 --- a/c/driver/sqlite/sqlite_test.cc +++ b/c/driver/sqlite/sqlite_test.cc @@ -1369,3 +1369,35 @@ INSTANTIATE_TEST_SUITE_P(FloatTypes, SqliteFloatParamTest, // NANOARROW_TYPE_FLOAT, NANOARROW_TYPE_DOUBLE)); + +TEST(SqliteUriErrors, InvalidInputDoesNotLeak) { + for (const char* uri : + {"file:test?mode=sentinel_secret", "sqlite://test?cache=sentinel_secret", + "file:test?vfs=sentinel_secret"}) { + struct AdbcError error = {}; + adbc_validation::Handle database; + ASSERT_EQ(ADBC_STATUS_OK, AdbcDatabaseNew(&database.value, &error)); + ASSERT_EQ(ADBC_STATUS_OK, AdbcDatabaseSetOption(&database.value, "uri", uri, &error)); + ASSERT_EQ(ADBC_STATUS_INVALID_ARGUMENT, AdbcDatabaseInit(&database.value, &error)); + ASSERT_NE(nullptr, error.message); + EXPECT_THAT(error.message, ::testing::Not(::testing::HasSubstr("sentinel"))); + if (error.release) error.release(&error); + } +} + +TEST(SqliteUriErrors, OpenFailureDoesNotLeak) { + struct AdbcError error = {}; + adbc_validation::Handle database; + const auto path = std::filesystem::temp_directory_path() / + "adbc-sentinel-nonexistent-parent" / "database.db"; + ASSERT_FALSE(std::filesystem::exists(path.parent_path())); + const std::string uri = "file:" + path.string() + "?mode=ro"; + ASSERT_EQ(ADBC_STATUS_OK, AdbcDatabaseNew(&database.value, &error)); + ASSERT_EQ(ADBC_STATUS_OK, + AdbcDatabaseSetOption(&database.value, "uri", uri.c_str(), &error)); + ASSERT_EQ(ADBC_STATUS_IO, AdbcDatabaseInit(&database.value, &error)); + ASSERT_NE(nullptr, error.message); + EXPECT_THAT(error.message, ::testing::Not(::testing::HasSubstr("sentinel"))); + EXPECT_THAT(error.message, ::testing::HasSubstr("failed to open database")); + if (error.release) error.release(&error); +} diff --git a/c/meson.build b/c/meson.build index f42c3b8d445..57a76342e0c 100644 --- a/c/meson.build +++ b/c/meson.build @@ -26,7 +26,7 @@ project( 'buildtype=release', 'c_std=c99', 'warning_level=2', - 'cpp_std=c++17', + 'cpp_std=c++20', ], ) diff --git a/ci/linux-packages/yum/almalinux-8/Dockerfile b/ci/linux-packages/yum/almalinux-8/Dockerfile index 4cbd0049031..79bf9bf7c42 100644 --- a/ci/linux-packages/yum/almalinux-8/Dockerfile +++ b/ci/linux-packages/yum/almalinux-8/Dockerfile @@ -18,7 +18,7 @@ ARG FROM=almalinux:8 FROM ${FROM} -ENV IMAGE_VERSION=2026-01-27 +ENV IMAGE_VERSION=2026-11-10 ARG DEBUG @@ -31,6 +31,7 @@ RUN \ ccache \ cmake \ gcc-c++ \ + gcc-toolset-14 \ gobject-introspection-devel \ libpq-devel \ make \ diff --git a/ci/linux-packages/yum/apache-arrow-adbc.spec.in b/ci/linux-packages/yum/apache-arrow-adbc.spec.in index eea668eb7c0..ee3d57072d8 100644 --- a/ci/linux-packages/yum/apache-arrow-adbc.spec.in +++ b/ci/linux-packages/yum/apache-arrow-adbc.spec.in @@ -43,6 +43,9 @@ Source0: https://www.apache.org/dyn/closer.lua?action=download&path=/arrow/arrow BuildRequires: arrow-glib-devel BuildRequires: cmake BuildRequires: gcc-c++ +%if %{rhel} == 8 +BuildRequires: gcc-toolset-14 +%endif BuildRequires: gobject-introspection-devel BuildRequires: libpq-devel BuildRequires: ninja-build @@ -57,6 +60,10 @@ Apache Arrow Database Connectivity (ADBC) is an Apache Arrow based database acce %setup -q -n apache-arrow-adbc-%{version} %build +%if %{rhel} == 8 +source /opt/rh/gcc-toolset-14/enable +%endif + cmake_build_type=RelWithDebInfo meson_build_type=debugoptimized @@ -88,6 +95,10 @@ LD_LIBRARY_PATH=$PWD/../c/%{adbc_cmake_builddir}/driver_manager \ cd - %install +%if %{rhel} == 8 +source /opt/rh/gcc-toolset-14/enable +%endif + cd c %adbc_cmake_install cd - diff --git a/go/driver/flightsql/flightsql_adbc_test.go b/go/driver/flightsql/flightsql_adbc_test.go index 67c94255bad..4bee896d78e 100644 --- a/go/driver/flightsql/flightsql_adbc_test.go +++ b/go/driver/flightsql/flightsql_adbc_test.go @@ -1379,7 +1379,7 @@ func TestFlightSQLSchemeInvalidURIs(t *testing.T) { name string uri string }{ - {"unrecognized transport", "flightsql://localhost:1234?transport=bogus"}, + {"unrecognized transport", "flightsql://user:sentinel@localhost:1234?transport=sentinel"}, {"host with unix transport", "flightsql://localhost:1234/socket?transport=unix"}, {"path with tcp transport", "flightsql://localhost:1234/socket?transport=tcp"}, {"path with default (tls) transport", "flightsql://localhost:1234/socket"}, @@ -1396,6 +1396,36 @@ func TestFlightSQLSchemeInvalidURIs(t *testing.T) { _, err = db.Open(context.Background()) require.Error(t, err) + require.NotContains(t, err.Error(), tt.uri) + require.NotContains(t, err.Error(), "sentinel") + var adbcErr adbc.Error + require.ErrorAs(t, err, &adbcErr) + require.Equal(t, adbc.StatusInvalidArgument, adbcErr.Code) + }) + } +} + +func TestMalformedURISecrets(t *testing.T) { + drv := driver.NewDriver(memory.DefaultAllocator) + for _, tc := range []struct{ uri, diagnostic string }{ + {"grpc://user:sentinel%ZZ@localhost", "invalid URL escape"}, + {"grpc://localhost:sentinel", "invalid port"}, + {"grpc://user:sentinel@bad host/db", "invalid character in host name"}, + {"grpc://user:sentinel@[::1/db", "missing ']' in host"}, + {":sentinel", "missing protocol scheme"}, + {"grpc://host/\nsentinel", "net/url: invalid control character in URL"}, + {"grpc://bad user:sentinel@host/db", "net/url: invalid userinfo"}, + {"1:sentinel", "first path segment in URL cannot contain colon"}, + } { + t.Run(tc.diagnostic, func(t *testing.T) { + _, err := drv.NewDatabase(map[string]string{adbc.OptionKeyURI: tc.uri}) + require.Error(t, err) + require.NotContains(t, err.Error(), "sentinel") + var adbcErr adbc.Error + require.ErrorAs(t, err, &adbcErr) + require.Equal(t, adbc.StatusInvalidArgument, adbcErr.Code) + require.Equal(t, "Invalid URI: "+tc.diagnostic, adbcErr.Msg) + require.Nil(t, errors.Unwrap(err)) }) } } diff --git a/go/driver/flightsql/flightsql_database.go b/go/driver/flightsql/flightsql_database.go index 06c94920a74..b4edb705dd9 100644 --- a/go/driver/flightsql/flightsql_database.go +++ b/go/driver/flightsql/flightsql_database.go @@ -400,9 +400,9 @@ func getFlightClient(ctx context.Context, loc string, d *databaseImpl, authMiddl } var uri *url.URL - uri, err = url.Parse(loc) + uri, err = parseLocation(loc) if err != nil { - return nil, adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': %s", loc, err), Code: adbc.StatusInvalidArgument} + return nil, err } creds := d.creds @@ -418,24 +418,24 @@ func getFlightClient(ctx context.Context, loc string, d *databaseImpl, authMiddl switch transport { case "", "tls": if uri.Path != "" { - return nil, adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': a socket path is only valid with transport=unix", loc), Code: adbc.StatusInvalidArgument} + return nil, adbc.Error{Msg: "Invalid URI: a socket path is only valid with transport=unix", Code: adbc.StatusInvalidArgument} } case "tcp": if uri.Path != "" { - return nil, adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': a socket path is only valid with transport=unix", loc), Code: adbc.StatusInvalidArgument} + return nil, adbc.Error{Msg: "Invalid URI: a socket path is only valid with transport=unix", Code: adbc.StatusInvalidArgument} } creds = insecure.NewCredentials() case "unix": if uri.Host != "" { - return nil, adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': a host is not valid with transport=unix", loc), Code: adbc.StatusInvalidArgument} + return nil, adbc.Error{Msg: "Invalid URI: a host is not valid with transport=unix", Code: adbc.StatusInvalidArgument} } if uri.Path == "" { - return nil, adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': transport=unix requires a socket path", loc), Code: adbc.StatusInvalidArgument} + return nil, adbc.Error{Msg: "Invalid URI: transport=unix requires a socket path", Code: adbc.StatusInvalidArgument} } creds = insecure.NewCredentials() target = "unix:" + uri.Path default: - return nil, adbc.Error{Msg: fmt.Sprintf("Invalid URI '%s': unrecognized transport '%s' (expected 'tls', 'tcp', or 'unix')", loc, transport), Code: adbc.StatusInvalidArgument} + return nil, adbc.Error{Msg: "Invalid URI: unrecognized transport (expected 'tls', 'tcp', or 'unix')", Code: adbc.StatusInvalidArgument} } } diff --git a/go/driver/flightsql/flightsql_driver.go b/go/driver/flightsql/flightsql_driver.go index 0dd20c52a4e..8102862c1f7 100644 --- a/go/driver/flightsql/flightsql_driver.go +++ b/go/driver/flightsql/flightsql_driver.go @@ -42,6 +42,7 @@ import ( "context" "errors" "net/url" + "strings" "time" "github.com/apache/arrow-adbc/go/adbc" @@ -170,8 +171,8 @@ func (d *driverImpl) NewDatabaseWithOptionsContext(ctx context.Context, opts map userDialOpts: userDialOpts, } - if db.uri, err = url.Parse(uri); err != nil { - return nil, adbc.Error{Msg: err.Error(), Code: adbc.StatusInvalidArgument} + if db.uri, err = parseLocation(uri); err != nil { + return nil, err } // Use WithMaxMsgSize(16 MiB) since Flight services tend to send large messages @@ -195,3 +196,38 @@ func (d *driverImpl) NewDatabase(opts map[string]string) (adbc.Database, error) func (d *driverImpl) NewDatabaseWithContext(ctx context.Context, opts map[string]string) (adbc.Database, error) { return d.NewDatabaseWithOptionsContext(ctx, opts) } + +// parseLocation preserves only known safe URL diagnostics, omitting input values. +func parseLocation(location string) (*url.URL, error) { + uri, err := url.Parse(location) + if err != nil { + message := "Invalid URI" + if urlErr, ok := err.(*url.Error); ok { + diagnostic := urlErr.Err.Error() + switch { + case strings.HasPrefix(diagnostic, "invalid URL escape "): + diagnostic = "invalid URL escape" + case strings.HasPrefix(diagnostic, "invalid character "): + diagnostic = "invalid character in host name" + case strings.HasPrefix(diagnostic, "invalid port "): + diagnostic = "invalid port" + case strings.HasPrefix(diagnostic, "invalid host: "): + diagnostic = "invalid host" + case diagnostic == "missing protocol scheme", + diagnostic == "net/url: invalid control character in URL", + diagnostic == "net/url: invalid userinfo", + diagnostic == "invalid IP-literal", + diagnostic == "missing ']' in host", + diagnostic == "first path segment in URL cannot contain colon": + // These diagnostics contain no input values. + default: + diagnostic = "" + } + if diagnostic != "" { + message += ": " + diagnostic + } + } + return nil, adbc.Error{Msg: message, Code: adbc.StatusInvalidArgument} + } + return uri, nil +} diff --git a/python/adbc_driver_postgresql/tests/test_lowlevel.py b/python/adbc_driver_postgresql/tests/test_lowlevel.py index 92e4659dbbb..6c2011642a5 100644 --- a/python/adbc_driver_postgresql/tests/test_lowlevel.py +++ b/python/adbc_driver_postgresql/tests/test_lowlevel.py @@ -52,7 +52,8 @@ def test_version() -> None: def test_failed_connection() -> None: with pytest.raises( - adbc_driver_manager.OperationalError, match=".*libpq.*Failed to connect.*" + adbc_driver_manager.ProgrammingError, + match=".*libpq.*Invalid connection string.*", ): adbc_driver_postgresql.connect("invalid")