Feat/keepass provider#3
Conversation
mikeboiko
left a comment
mikeboiko
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The core KeePass provider implementation is solid — clean architecture, good test coverage with stub scripts, secure password handling (stdin, not CLI args), and proper caching.
However, there are several issues that need to be addressed before this can be merged.
Must fix before merge
-
test-secrets.kdbxcommitted to the repo — A real KeePass database is tracked in git. The*.kdbxgitignore rule only prevents future untracked files — since this is already committed, it lives in history forever. The tests don't use it (they use shell script stubs), so pleasegit rmit. -
.envsync.yamland.env.examplecommitted to the project root — These are developer-local artifacts that would override config for every contributor. Please remove (or move totestdata/if needed for tests). -
go 1.22→go 1.25.0version bump —golang.org/x/termv0.43.0 supports Go 1.22+, so this bump appears unnecessary and will break users on older Go. -
ensureConfigerrors silently swallowed — Ininit.goandscaffold.go, errors returnnilinstead of the error, meaning failures are invisible to the user. -
ds pushsilently falls through to Bitwarden when configured withprovider: keepass— should error clearly instead. -
Missing trailing newlines at EOF in
root.go,doctor.go,init.go. -
Orphaned doc comment at
adapter.go:178.
See inline comments for details on each.
| provider: keepass | ||
| schema_file: .env.example | ||
| env_file: .env | ||
| keepass_database: test-secrets.kdbx |
There was a problem hiding this comment.
This config file should not be committed to the project root — it hardcodes provider: keepass and points at test-secrets.kdbx, which would override the config for every contributor who clones the repo.
If you need this for test fixtures, place it under testdata/. Otherwise please remove it.
| @@ -0,0 +1,4 @@ | |||
| # Database | |||
| DATABASE_URL= | |||
There was a problem hiding this comment.
Same concern — this appears to be a demo/test artifact rather than something that belongs in the project root. The project generates .env.example via ds init; committing a sample one here may confuse contributors or conflict with the actual project schema.
Please remove, or move to testdata/ if needed for tests.
|
|
||
| go 1.22 | ||
| go 1.25.0 | ||
|
|
There was a problem hiding this comment.
This bumps the minimum Go version from 1.22 to 1.25.0 — a significant breaking change for anyone on an older Go.
golang.org/x/term v0.43.0 and golang.org/x/sys v0.44.0 both support Go 1.22+, so this bump appears unnecessary. Please revert to go 1.22.
| } | ||
| return a.client.CreateEntry(ctx, a.cfg.KeePassDatabase, a.cfg.KeePassGroup, key, "") | ||
| } | ||
| // per process run. Subsequent calls return immediately because the password is |
There was a problem hiding this comment.
This doc comment fragment ("per process run. Subsequent calls return immediately...") is orphaned from its function — it's separated from ensurePassword by CreateEntry's closing brace on the line above.
Please merge this into the comment block at line 183, or restructure so the full doc comment is directly above func (a *Adapter) ensurePassword(...).
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| _, cfg, setupPassword, err := ensureConfig(s, opts) | ||
| if err != nil { | ||
| return nil |
There was a problem hiding this comment.
When ensureConfig returns an error, this silently returns nil — meaning the command reports success (exit 0) even when setup failed. The user gets no indication anything went wrong.
Should this be return err instead?
| setupRan, cfg, _, err := ensureConfig(s, opts) | ||
| if err != nil { | ||
| return err | ||
| return nil |
There was a problem hiding this comment.
Same silent-error issue: the original init.go on main correctly returned err from loadConfig. This change swallows the error. If ensureConfig already prints its own message, at minimum the exit code should still be non-zero — please return the error.
|
|
||
| var pushProviderFactory = func(cfg config.Config) provider.PushProvider { | ||
| // KeePass does not yet implement PushProvider (ds push is Bitwarden-only for now). | ||
| // All push operations fall through to Bitwarden regardless of config. |
There was a problem hiding this comment.
ds push with provider: keepass will silently use Bitwarden here, which is confusing and potentially dangerous — the user thinks they're pushing to their configured provider but they're actually interacting with a different one.
Please add a guard in the push command (or here) that returns a clear error like "push is not supported for the keepass provider" when cfg.Provider == "keepass".
|
Hi Mike, Test artifacts removed — test-secrets.kdbx, .envsync.yaml, and .env.example removed from git tracking via git rm --cached Let me know if anything else needs attention. |
|
Hey Greg — thanks for the fixes! I addressed the remaining items myself since I'll be squash-merging this shortly:
All tests pass. Merging now — nice work on the KeePass provider! 🎉 |
2 participants
Add KeePass provider support via keepassxc-cli
Adds KeePass as a second secret provider alongside Bitwarden. Default behaviour is unchanged — existing Bitwarden users see no difference.
What's new:
provider: keepass option in .envsync.yaml
Single master password prompt per ds invocation (no daemon required)
ds scaffold — seeds a KeePass vault with blank entries for all provider-managed keys in .env.example (dev lead tool)
ds init first-run setup — interactive provider selection with live vault validation before writing config
ds doctor updated to report keepass ready
Full test coverage for the new provider package
*.kdbx added to .gitignore
KeePass config:
yamlprovider: keepass
keepass_database: secrets.kdbx
keepass_group: dotenv
Requires: keepassxc-cli (ships with KeePassXC)