fix: add test to cover behavior for invalid manifests #48
Conversation
|
Okay good. I rebased this and now it's failing like I expect. The current code is exposing a bad bug we need to patch. If Driver.shared isn't a file like it's supposed to be, we actually delete the entirety of |
amoeba
changed the title
|
I ended up just explicitly detecting the weird state that causes this bug, see 43bd381. It might be cleaner just entirely fail the install when we get a MANIFEST that has this issue but until then this catches it. |
config/config.go
Outdated
| shared_path_is_subdir, err := IsImmediateSubDir(cfg.Location, sharedPath) | ||
| if err != nil { | ||
| return fmt.Errorf("error removing driver %s: %w", info.ID, err) | ||
| } |
There was a problem hiding this comment.
Could we simplify this logic by using filepath.Ext(sharedPath) and assuming that it should have a non-empty extension? (so, dll, dylib, etc.)
We could also leverage https://pkg.go.dev/os#Root.FS to ensure that we don't have any issues with malicious symbolic links. Maybe do os.OpenRoot(cfg.Location) and then work in terms of the returned root FS?
There was a problem hiding this comment.
Could we simplify this logic by using filepath.Ext(sharedPath) and assuming that it should have a non-empty extension? (so, dll, dylib, etc.)
I don't know if that would quite cover it. We basically just want to know whether calling filepath.Dir doesn't give us a subfolder of cfg.Location. It should almost every time, just not in this edge case.
https://pkg.go.dev/os#Root.FS looks cool. That could be a nice improvement (followup?).
There was a problem hiding this comment.
I don't know if that would quite cover it. We basically just want to know whether calling filepath.Dir doesn't give us a subfolder of cfg.Location. It should almost every time, just not in this edge case.
Should we just do info, _ := os.Stat() and info.IsDir() then? would be simpler
There was a problem hiding this comment.
I'm not sure I see how that'd catch the issue. If you want to take a shot at the idea I can take a look. Knowing the path is a folder is part of detecting that we're in the edge case but we also need to ensure it's a folder that, when we call filepath.Dir, we don't get cfg.Location and then delete all the user's drivers.
There was a problem hiding this comment.
Could we just do filepath.Clean(dir) != filepath.Clean(cfg.Location) then?
There was a problem hiding this comment.
Yeah I guess so. Maybe we don't need to also remove the folder in the case of the edge case.
There was a problem hiding this comment.
i like that better in the edge case, so we can reduce the complexity of the logic
2 participants
Fixes an edge case where we get confused about what fields to put in a manifest. When we package a driver, we include an all-caps
MANIFESTfile which has aFilestable that points to the shared library living in the same package. When dbc installs a package, it converts what's inFilesinto what goes inDriver.shared. If we goof up and put aDriver.sharedtable inMANIFEST,dbc uninstallwas mistakenly deleting all drivers in the config level.This patch catches this case and handles it. It's good to be defensive here because we'll be asking people to crate
MANIFESTfiles who aren't us so mistakes will happnen.Closes #29