From c5670a9a3649a2dbe491503457653f9c6e32cec9 Mon Sep 17 00:00:00 2001 From: Joe Julian Date: Sun, 29 Mar 2026 13:32:21 -0700 Subject: [PATCH] Add secret-safe copy and reveal UX coverage --- clipboard/service.go | 20 +++++- clipboard/service_test.go | 27 +++++++ main.go | 22 ++++-- main_test.go | 143 ++++++++++++++++++++++++++++++++++++++ ui_editor.go | 2 +- 5 files changed, 204 insertions(+), 10 deletions(-) diff --git a/clipboard/service.go b/clipboard/service.go index 730fe9a..c44aa98 100644 --- a/clipboard/service.go +++ b/clipboard/service.go @@ -2,7 +2,6 @@ package clipboard import ( "errors" - "fmt" systemclipboard "github.com/atotto/clipboard" @@ -10,6 +9,7 @@ import ( ) var ErrUnsupportedTarget = errors.New("unsupported clipboard target") +var ErrWriteFailed = errors.New("clipboard write failed") type Target string @@ -39,7 +39,7 @@ func (s Service) Copy(model vault.Model, entryID string, target Target) error { } if err := s.writer().WriteText(content); err != nil { - return fmt.Errorf("write clipboard text: %w", err) + return writeError{err: err} } return nil @@ -79,3 +79,19 @@ type systemWriter struct{} func (systemWriter) WriteText(text string) error { return systemclipboard.WriteAll(text) } + +type writeError struct { + err error +} + +func (e writeError) Error() string { + return ErrWriteFailed.Error() +} + +func (e writeError) Unwrap() error { + return e.err +} + +func (e writeError) Is(target error) bool { + return target == ErrWriteFailed +} diff --git a/clipboard/service_test.go b/clipboard/service_test.go index 9d769a0..c11730a 100644 --- a/clipboard/service_test.go +++ b/clipboard/service_test.go @@ -68,6 +68,25 @@ func TestServiceRejectsUnknownEntryAndUnsupportedTarget(t *testing.T) { } } +func TestServiceSanitizesClipboardWriteErrors(t *testing.T) { + t.Parallel() + + service := Service{Writer: failingWriter{err: errors.New("backend refused token-1")}} + model := vault.Model{ + Entries: []vault.Entry{ + {ID: "vault-console", Password: "token-1"}, + }, + } + + err := service.Copy(model, "vault-console", TargetPassword) + if !errors.Is(err, ErrWriteFailed) { + t.Fatalf("Copy() write error = %v, want ErrWriteFailed", err) + } + if err.Error() != ErrWriteFailed.Error() { + t.Fatalf("Copy() write error string = %q, want %q", err.Error(), ErrWriteFailed.Error()) + } +} + type memoryWriter struct { content string } @@ -76,3 +95,11 @@ func (w *memoryWriter) WriteText(text string) error { w.content = text return nil } + +type failingWriter struct { + err error +} + +func (w failingWriter) WriteText(string) error { + return w.err +} diff --git a/main.go b/main.go index ec00a42..45fd4a9 100644 --- a/main.go +++ b/main.go @@ -86,7 +86,6 @@ type ui struct { copyUser widget.Clickable copyPass widget.Clickable copyURL widget.Clickable - openURL widget.Clickable lockVault widget.Clickable unlockVault widget.Clickable createVault widget.Clickable @@ -135,6 +134,7 @@ type ui struct { eyeIcon *widget.Icon eyeOffIcon *widget.Icon copyIcon *widget.Icon + clipboardWriter clipboard.Writer loadingMessage string statusMessage string errorMessage string @@ -975,10 +975,7 @@ func (u *ui) detailPanel(gtx layout.Context) layout.Dimensions { layout.Rigid(u.entryEditorPanel), ) } - password := strings.Repeat("•", max(8, len(item.Password))) - if u.showPassword { - password = item.Password - } + password := u.detailPasswordValue() titleSize := unit.Sp(26) titlePad := unit.Dp(10) sectionGap := unit.Dp(8) @@ -1016,7 +1013,7 @@ func (u *ui) detailPanel(gtx layout.Context) layout.Dimensions { }), layout.Rigid(layout.Spacer{Height: unit.Dp(8)}.Layout), layout.Rigid(func(gtx layout.Context) layout.Dimensions { - return tonedButton(gtx, u.theme, &u.openURL, "Open URL") + return tonedButton(gtx, u.theme, &u.copyURL, "Copy URL") }), ) } @@ -1032,7 +1029,7 @@ func (u *ui) detailPanel(gtx layout.Context) layout.Dimensions { }), layout.Rigid(layout.Spacer{Width: unit.Dp(8)}.Layout), layout.Rigid(func(gtx layout.Context) layout.Dimensions { - btn := material.Button(u.theme, &u.openURL, "Open URL") + btn := material.Button(u.theme, &u.copyURL, "Copy URL") return btn.Layout(gtx) }), ) @@ -1201,6 +1198,17 @@ func (u *ui) passwordLine(label, value string) layout.Widget { } } +func (u *ui) detailPasswordValue() string { + item, ok := u.selectedEntry() + if !ok { + return "" + } + if u.showPassword { + return item.Password + } + return strings.Repeat("•", max(8, len(item.Password))) +} + func card(gtx layout.Context, w layout.Widget) layout.Dimensions { return layout.Background{}.Layout(gtx, fill(panelColor), func(gtx layout.Context) layout.Dimensions { return layout.UniformInset(unit.Dp(16)).Layout(gtx, w) diff --git a/main_test.go b/main_test.go index d2a5b55..870a283 100644 --- a/main_test.go +++ b/main_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "slices" + "strings" "testing" "git.julianfamily.org/keepassgo/clipboard" @@ -891,3 +892,145 @@ func TestUIUsesKeePassGOProductCopy(t *testing.T) { t.Fatalf("desktopSubtitle = %q, want updated product subtitle", desktopSubtitle) } } + +func TestUICopyActionsWriteExpectedClipboardContentsAndSanitizedFeedback(t *testing.T) { + t.Parallel() + + model := vault.Model{ + Entries: []vault.Entry{ + { + ID: "vault-console", + Title: "Vault Console", + Username: "dannyocean", + Password: "token-1", + URL: "https://vault.crew.example.invalid", + Path: []string{"Root", "Internet"}, + }, + }, + } + + tests := []struct { + name string + target clipboard.Target + label string + want string + }{ + {name: "username", target: clipboard.TargetUsername, label: "copy username", want: "dannyocean"}, + {name: "password", target: clipboard.TargetPassword, label: "copy password", want: "token-1"}, + {name: "url", target: clipboard.TargetURL, label: "copy URL", want: "https://vault.crew.example.invalid"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + u := newUIWithModel("desktop", model) + writer := &memoryClipboardWriter{} + u.clipboardWriter = writer + u.showEntriesSection() + u.currentPath = []string{"Root", "Internet"} + u.filter() + u.state.SelectedEntryID = "vault-console" + + u.runAction(tt.label, func() error { return u.copySelectedFieldAction(tt.target) }) + + if writer.content != tt.want { + t.Fatalf("clipboard content = %q, want %q", writer.content, tt.want) + } + if u.statusMessage != tt.label+" complete" { + t.Fatalf("statusMessage = %q, want %q", u.statusMessage, tt.label+" complete") + } + if u.errorMessage != "" { + t.Fatalf("errorMessage = %q, want empty", u.errorMessage) + } + if strings.Contains(u.statusMessage, tt.want) { + t.Fatalf("statusMessage = %q, must not contain copied secret or field value %q", u.statusMessage, tt.want) + } + }) + } +} + +func TestUICopyActionSanitizesClipboardBackendErrors(t *testing.T) { + t.Parallel() + + u := newUIWithModel("desktop", vault.Model{ + Entries: []vault.Entry{ + { + ID: "vault-console", + Title: "Vault Console", + Username: "dannyocean", + Password: "token-1", + URL: "https://vault.crew.example.invalid", + Path: []string{"Root", "Internet"}, + }, + }, + }) + u.clipboardWriter = failingClipboardWriter{err: os.ErrPermission} + u.showEntriesSection() + u.currentPath = []string{"Root", "Internet"} + u.filter() + u.state.SelectedEntryID = "vault-console" + + u.runAction("copy password", func() error { return u.copySelectedFieldAction(clipboard.TargetPassword) }) + + if u.errorMessage != clipboard.ErrWriteFailed.Error() { + t.Fatalf("errorMessage = %q, want %q", u.errorMessage, clipboard.ErrWriteFailed.Error()) + } + if strings.Contains(u.errorMessage, "token-1") { + t.Fatalf("errorMessage = %q, must not contain copied password", u.errorMessage) + } + if u.statusMessage != "" { + t.Fatalf("statusMessage = %q, want empty on copy failure", u.statusMessage) + } +} + +func TestUIPasswordRevealTogglesDisplayedPasswordAndLockResetsIt(t *testing.T) { + t.Parallel() + + u := newUIWithModel("desktop", vault.Model{ + Entries: []vault.Entry{ + { + ID: "vault-console", + Title: "Vault Console", + Username: "dannyocean", + Password: "token-1", + Path: []string{"Root", "Internet"}, + }, + }, + }) + u.showEntriesSection() + u.currentPath = []string{"Root", "Internet"} + u.filter() + u.state.SelectedEntryID = "vault-console" + + if got := u.detailPasswordValue(); got != "••••••••" { + t.Fatalf("detailPasswordValue() hidden = %q, want %q", got, "••••••••") + } + + u.showPassword = true + if got := u.detailPasswordValue(); got != "token-1" { + t.Fatalf("detailPasswordValue() revealed = %q, want %q", got, "token-1") + } + + if err := u.lockAction(); err != nil { + t.Fatalf("lockAction() error = %v", err) + } + if u.showPassword { + t.Fatal("showPassword = true after lockAction(), want false") + } +} + +type memoryClipboardWriter struct { + content string +} + +func (w *memoryClipboardWriter) WriteText(text string) error { + w.content = text + return nil +} + +type failingClipboardWriter struct { + err error +} + +func (w failingClipboardWriter) WriteText(string) error { + return w.err +} diff --git a/ui_editor.go b/ui_editor.go index b855402..111b264 100644 --- a/ui_editor.go +++ b/ui_editor.go @@ -241,7 +241,7 @@ func (u *ui) copySelectedFieldAction(target clipboard.Target) error { return err } - service := clipboard.Service{} + service := clipboard.Service{Writer: u.clipboardWriter} return service.Copy(model, u.state.SelectedEntryID, target) }