From 31c2c8d90ef57dfac21100ab80d43e91a6f4712f Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Mon, 8 Sep 2025 18:28:02 -0700 Subject: [PATCH 1/9] Factor out uninstall code into common routine --- config/config.go | 35 +++++++++++++++++++++++++++++++++++ config/dirs_unixlike.go | 32 ++++---------------------------- config/dirs_windows.go | 27 ++------------------------- 3 files changed, 41 insertions(+), 53 deletions(-) diff --git a/config/config.go b/config/config.go index bc42a870..379f33df 100644 --- a/config/config.go +++ b/config/config.go @@ -13,6 +13,7 @@ import ( "os" "path" "path/filepath" + "regexp" "runtime" "slices" "strings" @@ -315,3 +316,37 @@ func decodeManifest(r io.Reader, driverName string, requireShared bool) (Manifes return result, nil } + +// Common, non-platform-specific code for uninstalling a driver. Called by +// platform-specific UninstallDriver function. +func UninstallDriverShared(cfg Config, info DriverInfo) error { + for sharedPath := range info.Driver.Shared.Paths() { + + // dbc installs drivers in a folder, other tools may not so we handle each + // differently. + if info.Source == "dbc" { + if err := os.RemoveAll(filepath.Dir(sharedPath)); err != nil { + // Ignore only when not found. This supports manifest-only drivers. + // TODO: Come up with a better mechanism to handle manifest-only drivers + // and remove this continue when we do + if errors.Is(err, fs.ErrNotExist) { + continue + } + return fmt.Errorf("error removing driver %s: %w", info.ID, err) + } + } else { + if err := os.Remove(sharedPath); err != nil { + // Ignore only when not found. This supports manifest-only drivers. + // TODO: Come up with a better mechanism to handle manifest-only drivers + // and remove this continue when we do + if errors.Is(err, fs.ErrNotExist) { + continue + } + return fmt.Errorf("error removing driver %s: %w", info.ID, err) + } + } + + } + + return nil +} diff --git a/config/dirs_unixlike.go b/config/dirs_unixlike.go index 98c32943..0486ea0c 100644 --- a/config/dirs_unixlike.go +++ b/config/dirs_unixlike.go @@ -5,9 +5,7 @@ package config import ( - "errors" "fmt" - "io/fs" "maps" "os" "path/filepath" @@ -90,36 +88,14 @@ func CreateManifest(cfg Config, driver DriverInfo) (err error) { } func UninstallDriver(cfg Config, info DriverInfo) error { - if info.Source == "dbc" { - for sharedPath := range info.Driver.Shared.Paths() { - if err := os.RemoveAll(filepath.Dir(sharedPath)); err != nil { - // Ignore only when not found. This supports manifest-only drivers. - // TODO: Come up with a better mechanism to handle manifest-only drivers - // and remove this continue when we do - if errors.Is(err, fs.ErrNotExist) { - continue - } - return fmt.Errorf("error removing driver %s: %w", info.ID, err) - } - } - } else { - for sharedPath := range info.Driver.Shared.Paths() { - if err := os.Remove(sharedPath); err != nil { - // Ignore only when not found. This supports manifest-only drivers. - // TODO: Come up with a better mechanism to handle manifest-only drivers - // and remove this continue when we do - if errors.Is(err, fs.ErrNotExist) { - continue - } - return fmt.Errorf("error removing driver %s: %w", info.ID, err) - } - } - } - manifest := filepath.Join(cfg.Location, info.ID+".toml") if err := os.Remove(manifest); err != nil { return fmt.Errorf("error removing manifest %s: %w", manifest, err) } + if err := UninstallDriverShared(cfg, info); err != nil { + return fmt.Errorf("failed to delete driver shared object: %w", err) + } + return nil } diff --git a/config/dirs_windows.go b/config/dirs_windows.go index 6e4ba793..6eab8153 100644 --- a/config/dirs_windows.go +++ b/config/dirs_windows.go @@ -5,7 +5,6 @@ package config import ( "errors" "fmt" - "io/fs" "log" "maps" "os" @@ -285,30 +284,8 @@ func UninstallDriver(cfg Config, info DriverInfo) error { return fmt.Errorf("failed to delete driver registry key: %w", err) } - if info.Source == "dbc" { - for sharedPath := range info.Driver.Shared.Paths() { - if err := os.RemoveAll(filepath.Dir(sharedPath)); err != nil { - // Ignore only when not found. This supports manifest-only drivers. - // TODO: Come up with a better mechanism to handle manifest-only drivers - // and remove this continue when we do - if errors.Is(err, fs.ErrNotExist) { - continue - } - return fmt.Errorf("error removing driver %s: %w", info.ID, err) - } - } - } else { - for sharedPath := range info.Driver.Shared.Paths() { - if err := os.Remove(sharedPath); err != nil { - // Ignore only when not found. This supports manifest-only drivers. - // TODO: Come up with a better mechanism to handle manifest-only drivers - // and remove this continue when we do - if errors.Is(err, fs.ErrNotExist) { - continue - } - return fmt.Errorf("error removing driver %s: %w", info.ID, err) - } - } + if err = UninstallDriverShared(cfg, info); err != nil { + return fmt.Errorf("failed to delete driver shared object: %w", err) } return nil From 03212f7e4bf241c5c63968da28360d4c4d6af4fc Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Mon, 8 Sep 2025 18:28:56 -0700 Subject: [PATCH 2/9] Make uninstall routine safer --- config/config.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/config/config.go b/config/config.go index 379f33df..288ff546 100644 --- a/config/config.go +++ b/config/config.go @@ -321,6 +321,19 @@ func decodeManifest(r io.Reader, driverName string, requireShared bool) (Manifes // platform-specific UninstallDriver function. func UninstallDriverShared(cfg Config, info DriverInfo) error { for sharedPath := range info.Driver.Shared.Paths() { + // Don't remove anything that isn't contained withing the found driver's + // config directory (i.e., avoid malicious driver manifests) + if !strings.HasPrefix(sharedPath, cfg.Location) { + continue + } + + // Skip any shared paths with more than one contiguous `.` in them to avoid + // path traversal in the middle of a path + matched, err := regexp.MatchString("\\.{2,}", sharedPath) + if matched || err != nil { // Also ignore errors which shouldn't happen with + // such a simple regex + continue + } // dbc installs drivers in a folder, other tools may not so we handle each // differently. From 9819db5b54137c9f5b52f8c5d4f85edbb388c9d8 Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Mon, 8 Sep 2025 18:29:34 -0700 Subject: [PATCH 3/9] Clean up driver install folder for manifest-only drivers --- config/config.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/config/config.go b/config/config.go index 288ff546..8a558683 100644 --- a/config/config.go +++ b/config/config.go @@ -361,5 +361,25 @@ func UninstallDriverShared(cfg Config, info DriverInfo) error { } + // Manifest only drivers can come with extra files such as a LICENSE and we + // create a folder next to the driver manifest to store them, same as we'd + // store the actual driver shared library. Above, we find the path of this + // folder by looking at the Driver.shared path. For manifest-only drivers, + // Driver.shared is not a valid path (it's just a name), so this trick doesn't + // work. We do want to clean this folder up so here we guess what it is and + // try to remove it e.g., "somedriver_macos_arm64_v1.2.3." + extra_folder := fmt.Sprintf("%s_%s_v%s", info.ID, platformTuple, info.Version) + // Return immediately if folder has repeated . chars (for safety) + matched, err := regexp.MatchString("\\.{2,}", extra_folder) + if matched || err != nil { + return nil + } + extra_path := filepath.Join(cfg.Location, extra_folder) + finfo, err := os.Stat(extra_path) + if err == nil && finfo.IsDir() { + _ = os.RemoveAll(extra_path) + // ignore errors + } + return nil } From 8b82752fe6e126ddac5a51841cbb9b00fd08a96e Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Tue, 9 Sep 2025 16:25:47 -0400 Subject: [PATCH 4/9] don't run snapshot on forks --- .github/workflows/release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 79727615..c77a5258 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -16,7 +16,7 @@ jobs: snapshot: runs-on: ubuntu-latest environment: snapshot - if: github.event_name == 'pull_request' + if: github.event_name == 'pull_request' && !github.event.pull_request.head.repo.fork steps: - uses: actions/checkout@v5 with: From 47ee415f15eb7d33d5dad298a59f3b076fcfb9e4 Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Mon, 15 Sep 2025 12:55:40 -0700 Subject: [PATCH 5/9] Finish test for new behavior --- cmd/dbc/install_test.go | 2 +- cmd/dbc/search_test.go | 4 ++-- cmd/dbc/testdata/test_manifest.yaml | 2 +- cmd/dbc/uninstall_test.go | 29 +++++++++++++++++++++++++++++ config/config.go | 21 +++++---------------- 5 files changed, 38 insertions(+), 20 deletions(-) diff --git a/cmd/dbc/install_test.go b/cmd/dbc/install_test.go index 3e488b1e..5bd943ef 100644 --- a/cmd/dbc/install_test.go +++ b/cmd/dbc/install_test.go @@ -120,7 +120,7 @@ func (suite *SubcommandTestSuite) TestInstallManifestOnlyDriver() { m := InstallCmd{Driver: "test-driver-manifest-only"}. GetModelCustom(baseModel{getDriverList: getTestDriverList, downloadPkg: downloadTestPkg}) suite.validateOutput("\r[✓] searching\r\n[✓] downloading\r\n[✓] installing\r\n[✓] verifying signature\r\n"+ - "\r\nInstalled test-driver-manifest-only 0.1.0 to "+suite.tempdir+"\r\n"+ + "\r\nInstalled test-driver-manifest-only 1.0.0 to "+suite.tempdir+"\r\n"+ "\r\nMust have libtest_driver installed to load this driver\r\n", suite.runCmd(m)) if runtime.GOOS != "windows" { suite.FileExists(filepath.Join(suite.tempdir, "test-driver-manifest-only.toml")) diff --git a/cmd/dbc/search_test.go b/cmd/dbc/search_test.go index 5037269a..82a20957 100644 --- a/cmd/dbc/search_test.go +++ b/cmd/dbc/search_test.go @@ -41,7 +41,7 @@ func (suite *SubcommandTestSuite) TestSearchCmdVerbose() { "Available Versions:\r\n ├── 2.0.0\r\n ╰── 2.1.0\r\n"+ "• test-driver-manifest-only\r\n Title: Test Driver Manifest Only\r\n "+ "Description: This is manifest-only driver\r\n License: Apache-2.0\r\n "+ - "Available Versions:\r\n ╰── 0.1.0\r\n\r ", suite.runCmd(m)) + "Available Versions:\r\n ╰── 1.0.0\r\n\r ", suite.runCmd(m)) } func (suite *SubcommandTestSuite) TestSearchCmdVerboseWithInstalled() { @@ -64,5 +64,5 @@ func (suite *SubcommandTestSuite) TestSearchCmdVerboseWithInstalled() { " Description: This is manifest-only driver\r\n"+ " License: Apache-2.0\r\n"+ " Available Versions:\r\n"+ - " ╰── 0.1.0\r\n\r ", suite.runCmd(m)) + " ╰── 1.0.0\r\n\r ", suite.runCmd(m)) } diff --git a/cmd/dbc/testdata/test_manifest.yaml b/cmd/dbc/testdata/test_manifest.yaml index c394740a..ba182fe3 100644 --- a/cmd/dbc/testdata/test_manifest.yaml +++ b/cmd/dbc/testdata/test_manifest.yaml @@ -54,7 +54,7 @@ drivers: license: Apache-2.0 path: test-driver-manifest-only pkginfo: - - version: v0.1.0 + - version: v1.0.0 packages: - platform: linux_amd64 - platform: macos_amd64 diff --git a/cmd/dbc/uninstall_test.go b/cmd/dbc/uninstall_test.go index f0a5dfe1..b1f24b74 100644 --- a/cmd/dbc/uninstall_test.go +++ b/cmd/dbc/uninstall_test.go @@ -3,6 +3,7 @@ package main import ( + "fmt" "os" "path" "path/filepath" @@ -124,3 +125,31 @@ func (suite *SubcommandTestSuite) TestUninstallMultipleLocationsNonDefault() { suite.FileExists(filepath.Join(suite.tempdir, "test-driver-1.toml")) suite.NoFileExists(filepath.Join(installModel.cfg.Location, "test-driver-1.toml")) } + +func (suite *SubcommandTestSuite) TestUninstallManifestOnlyDriver() { + m := InstallCmd{Driver: "test-driver-manifest-only"}. + GetModelCustom(baseModel{getDriverList: getTestDriverList, downloadPkg: downloadTestPkg}) + suite.validateOutput("\r[✓] searching\r\n[✓] downloading\r\n[✓] installing\r\n[✓] verifying signature\r\n"+ + "\r\nInstalled test-driver-manifest-only 1.0.0 to "+suite.tempdir+"\r\n"+ + "\r\nMust have libtest_driver installed to load this driver\r\n", suite.runCmd(m)) + if runtime.GOOS != "windows" { + suite.FileExists(filepath.Join(suite.tempdir, "test-driver-manifest-only.toml")) + } + + // Verify the sidecar folder exists before we uninstall + new_sidecar_path := fmt.Sprintf("test-driver-manifest-only_%s_v1.0.0", config.PlatformTuple()) + err := os.Rename(filepath.Join(suite.tempdir, "test-driver-manifest-only"), filepath.Join(suite.tempdir, new_sidecar_path)) + if err != nil { + suite.Fail("Failed to rename sidecar folder. Something is wrong with this test.") + } + suite.DirExists(filepath.Join(suite.tempdir, new_sidecar_path)) + + // Now uninstall and verify we clean up + m = UninstallCmd{Driver: "test-driver-manifest-only"}. + GetModelCustom(baseModel{getDriverList: getTestDriverList, downloadPkg: downloadTestPkg}) + suite.validateOutput("Driver `test-driver-manifest-only` uninstalled successfully!\r\n\r\n\r ", suite.runCmd(m)) + if runtime.GOOS != "windows" { + suite.NoFileExists(filepath.Join(suite.tempdir, "test-driver-manifest-only.toml")) + } + suite.NoDirExists(filepath.Join(suite.tempdir, new_sidecar_path)) +} diff --git a/config/config.go b/config/config.go index 8a558683..b51115da 100644 --- a/config/config.go +++ b/config/config.go @@ -13,7 +13,6 @@ import ( "os" "path" "path/filepath" - "regexp" "runtime" "slices" "strings" @@ -321,20 +320,15 @@ func decodeManifest(r io.Reader, driverName string, requireShared bool) (Manifes // platform-specific UninstallDriver function. func UninstallDriverShared(cfg Config, info DriverInfo) error { for sharedPath := range info.Driver.Shared.Paths() { + // Run filepath.Clean on sharedPath mainly to catch inner ".." in the path + sharedPath = filepath.Clean(sharedPath) + // Don't remove anything that isn't contained withing the found driver's // config directory (i.e., avoid malicious driver manifests) if !strings.HasPrefix(sharedPath, cfg.Location) { continue } - // Skip any shared paths with more than one contiguous `.` in them to avoid - // path traversal in the middle of a path - matched, err := regexp.MatchString("\\.{2,}", sharedPath) - if matched || err != nil { // Also ignore errors which shouldn't happen with - // such a simple regex - continue - } - // dbc installs drivers in a folder, other tools may not so we handle each // differently. if info.Source == "dbc" { @@ -358,7 +352,6 @@ func UninstallDriverShared(cfg Config, info DriverInfo) error { return fmt.Errorf("error removing driver %s: %w", info.ID, err) } } - } // Manifest only drivers can come with extra files such as a LICENSE and we @@ -369,14 +362,10 @@ func UninstallDriverShared(cfg Config, info DriverInfo) error { // work. We do want to clean this folder up so here we guess what it is and // try to remove it e.g., "somedriver_macos_arm64_v1.2.3." extra_folder := fmt.Sprintf("%s_%s_v%s", info.ID, platformTuple, info.Version) - // Return immediately if folder has repeated . chars (for safety) - matched, err := regexp.MatchString("\\.{2,}", extra_folder) - if matched || err != nil { - return nil - } + extra_folder = filepath.Clean(extra_folder) extra_path := filepath.Join(cfg.Location, extra_folder) finfo, err := os.Stat(extra_path) - if err == nil && finfo.IsDir() { + if err == nil && finfo.IsDir() && extra_path != "." { _ = os.RemoveAll(extra_path) // ignore errors } From 4e5fcb517c198bb783970d00af9b9ce04fb771ec Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Tue, 16 Sep 2025 12:51:14 -0700 Subject: [PATCH 6/9] Add err to msg --- cmd/dbc/uninstall_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/dbc/uninstall_test.go b/cmd/dbc/uninstall_test.go index b1f24b74..f45f6af4 100644 --- a/cmd/dbc/uninstall_test.go +++ b/cmd/dbc/uninstall_test.go @@ -140,7 +140,7 @@ func (suite *SubcommandTestSuite) TestUninstallManifestOnlyDriver() { new_sidecar_path := fmt.Sprintf("test-driver-manifest-only_%s_v1.0.0", config.PlatformTuple()) err := os.Rename(filepath.Join(suite.tempdir, "test-driver-manifest-only"), filepath.Join(suite.tempdir, new_sidecar_path)) if err != nil { - suite.Fail("Failed to rename sidecar folder. Something is wrong with this test.") + suite.Fail("Failed to rename sidecar folder. Something is wrong with this test: %v", err) } suite.DirExists(filepath.Join(suite.tempdir, new_sidecar_path)) From 6f94bb22c0335e0257b338d84c6ae854d27e204f Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Tue, 16 Sep 2025 15:54:44 -0400 Subject: [PATCH 7/9] ADBC_DRIVER_PATH doesn't write a registry key --- config/dirs_windows.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/config/dirs_windows.go b/config/dirs_windows.go index 6eab8153..1d157a48 100644 --- a/config/dirs_windows.go +++ b/config/dirs_windows.go @@ -274,17 +274,19 @@ func CreateManifest(cfg Config, driver DriverInfo) (err error) { } func UninstallDriver(cfg Config, info DriverInfo) error { - k, err := registry.OpenKey(cfg.Level.key(), regKeyADBC, registry.WRITE) - if err != nil { - return err - } - defer k.Close() + if cfg.Level != ConfigEnv { + k, err := registry.OpenKey(cfg.Level.key(), regKeyADBC, registry.WRITE) + if err != nil { + return err + } + defer k.Close() - if err := registry.DeleteKey(k, info.ID); err != nil { - return fmt.Errorf("failed to delete driver registry key: %w", err) + if err := registry.DeleteKey(k, info.ID); err != nil { + return fmt.Errorf("failed to delete driver registry key: %w", err) + } } - if err = UninstallDriverShared(cfg, info); err != nil { + if err := UninstallDriverShared(cfg, info); err != nil { return fmt.Errorf("failed to delete driver shared object: %w", err) } From 0fe482e48bebbae94b8c2701676d1c0cc2e18821 Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Tue, 16 Sep 2025 12:58:23 -0700 Subject: [PATCH 8/9] Fix invalid code in prev commit --- cmd/dbc/uninstall_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/dbc/uninstall_test.go b/cmd/dbc/uninstall_test.go index f45f6af4..7b5f70b0 100644 --- a/cmd/dbc/uninstall_test.go +++ b/cmd/dbc/uninstall_test.go @@ -140,7 +140,7 @@ func (suite *SubcommandTestSuite) TestUninstallManifestOnlyDriver() { new_sidecar_path := fmt.Sprintf("test-driver-manifest-only_%s_v1.0.0", config.PlatformTuple()) err := os.Rename(filepath.Join(suite.tempdir, "test-driver-manifest-only"), filepath.Join(suite.tempdir, new_sidecar_path)) if err != nil { - suite.Fail("Failed to rename sidecar folder. Something is wrong with this test: %v", err) + suite.Fail(fmt.Sprintf("Failed to rename sidecar folder. Something is wrong with this test: %v", err)) } suite.DirExists(filepath.Join(suite.tempdir, new_sidecar_path)) From b5a943fe1d52076b39b7cc65e80b29f41387c9a5 Mon Sep 17 00:00:00 2001 From: Bryce Mecum Date: Tue, 16 Sep 2025 13:27:16 -0700 Subject: [PATCH 9/9] Fix installation behavior on Windows path.base isn't for file paths, see https://pkg.go.dev/path. --- config/config.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index b51115da..a5b6a2b8 100644 --- a/config/config.go +++ b/config/config.go @@ -11,7 +11,6 @@ import ( "io/fs" "maps" "os" - "path" "path/filepath" "runtime" "slices" @@ -201,7 +200,7 @@ func InstallDriver(cfg Config, shortName string, downloaded *os.File) (Manifest, if loc, err = EnsureLocation(cfg); err != nil { return Manifest{}, fmt.Errorf("could not ensure config location: %w", err) } - base := strings.TrimSuffix(path.Base(downloaded.Name()), ".tar.gz") + base := strings.TrimSuffix(filepath.Base(downloaded.Name()), ".tar.gz") finalDir := filepath.Join(loc, base) if err := os.MkdirAll(finalDir, 0o755); err != nil {