From e171f49287b984c83b6a49ce2703f15286c681a2 Mon Sep 17 00:00:00 2001 From: Joe Julian Date: Tue, 28 Apr 2026 21:10:44 -0700 Subject: [PATCH 1/2] Document simplifying refactor preference --- AGENTS.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index b064fbf..def94a4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -116,6 +116,10 @@ These features are product requirements, not “nice to have” ideas. - UI state should not be the source of truth for vault structure or search behavior. - Domain packages must be test-driven where practical. - Prefer behavior-oriented tests that describe expected product behavior rather than implementation details. +- Prefer simplifying refactors that extract shared behavior into smaller named + functions. When a new path needs most of an existing function, factor the + common behavior out and let the specific functions call it instead of adding + flags or branches that make the original function larger. - Provide a secure gRPC API as a first-class programmatic surface, not as a thin wrapper around UI state. - Design browser-extension and automation integrations against the gRPC API, not against ad hoc local protocols. - Treat the vault model as local-first across all platforms: -- 2.52.0 From 72006aa4b188213ae729e7b5b6c5a07a5a10c481 Mon Sep 17 00:00:00 2001 From: Joe Julian Date: Tue, 28 Apr 2026 21:15:15 -0700 Subject: [PATCH 2/2] Allow explicit browser search fill --- browser/extension/background.js | 50 ++++++++++++++++++++++----- browser/extension/background.test.cjs | 21 +++++++++++ browser/extension/popup.js | 17 +++++++-- internal/api/server.go | 4 +++ internal/api/server_test.go | 34 ++++++++++++++++++ 5 files changed, 116 insertions(+), 10 deletions(-) diff --git a/browser/extension/background.js b/browser/extension/background.js index 4572290..1d77052 100644 --- a/browser/extension/background.js +++ b/browser/extension/background.js @@ -700,7 +700,34 @@ async function statusForPage(options = {}) { return refreshPageState(page.tabId, page.url, options); } -async function fillLogin(tabId, entryId) { +function matchedLoginCredentialRequest(settings, entryId, pageUrl) { + return { + action: "get-login", + bearerToken: settings.bearerToken, + entryId, + url: pageUrl + }; +} + +function selectedLoginCredentialRequest(settings, entryId) { + return { + action: "get-login", + bearerToken: settings.bearerToken, + entryId + }; +} + +async function fillMatchedLogin(tabId, entryId) { + const page = await loginFillPage(tabId); + return fillLoginOnPage(tabId, entryId, page.url, matchedLoginCredentialRequest); +} + +async function fillSelectedLogin(tabId, entryId) { + const page = await loginFillPage(tabId); + return fillLoginOnPage(tabId, entryId, page.url, selectedLoginCredentialRequest); +} + +async function loginFillPage(tabId) { if (!Number.isInteger(tabId)) { throw new Error("No active tab is available."); } @@ -709,7 +736,10 @@ async function fillLogin(tabId, entryId) { if (!supportsPageStateURL(pageUrl)) { throw new Error("This page cannot be filled."); } + return { url: pageUrl }; +} +async function fillLoginOnPage(tabId, entryId, pageUrl, credentialRequest) { let state = await getPageState(tabId, pageUrl); state = await setPageState(tabId, { ...state, @@ -729,12 +759,7 @@ async function fillLogin(tabId, entryId) { throw new Error("API token is not configured."); } - const response = await connectNative({ - action: "get-login", - bearerToken: settings.bearerToken, - entryId, - url: pageUrl - }); + const response = await connectNative(credentialRequest(settings, entryId, pageUrl)); if (!response?.success || !response.credential) { throw new Error(response?.error || "KeePassGO did not return a credential."); } @@ -846,6 +871,8 @@ const backgroundTestExports = { shouldContinueWatchingState, tokenPendingApprovalCount, savePlanForObservedLogin, + matchedLoginCredentialRequest, + selectedLoginCredentialRequest, defaultSettings }; @@ -872,7 +899,14 @@ if (isNodeTestEnv) { focusTarget: cloneTarget(message.target) }); } - sendResponse({ success: true, ...(await fillLogin(targetTabID, message.entryId)) }); + sendResponse({ success: true, ...(await fillMatchedLogin(targetTabID, message.entryId)) }); + return; + } + case "keepassgo-fill-selected-entry": { + const targetTabID = Number.isInteger(message?.tabId) + ? message.tabId + : (Number.isInteger(sender?.tab?.id) ? sender.tab.id : (await activePageContext()).tabId); + sendResponse({ success: true, ...(await fillSelectedLogin(targetTabID, message.entryId)) }); return; } case "keepassgo-load-settings": diff --git a/browser/extension/background.test.cjs b/browser/extension/background.test.cjs index 3bae90e..620ee88 100644 --- a/browser/extension/background.test.cjs +++ b/browser/extension/background.test.cjs @@ -149,3 +149,24 @@ test("applyBestMatchOnly preserves all matches when disabled", () => { assert.deepEqual(filtered.map((match) => match.id), ["livingston", "rusty"]); }); + +test("matched login credential requests include the page URL for URL validation", () => { + assert.deepEqual(background.matchedLoginCredentialRequest({ + bearerToken: "token-1" + }, "vault-console", "https://bellagio.example.invalid/login"), { + action: "get-login", + bearerToken: "token-1", + entryId: "vault-console", + url: "https://bellagio.example.invalid/login" + }); +}); + +test("explicit selected credential requests omit the page URL", () => { + assert.deepEqual(background.selectedLoginCredentialRequest({ + bearerToken: "token-1" + }, "no-url-entry"), { + action: "get-login", + bearerToken: "token-1", + entryId: "no-url-entry" + }); +}); diff --git a/browser/extension/popup.js b/browser/extension/popup.js index de456e3..ba6e720 100644 --- a/browser/extension/popup.js +++ b/browser/extension/popup.js @@ -97,7 +97,7 @@ function renderMatchList(root, matches, options = {}) { setStatus("Filled", `${match.title} was sent to the current page.`, "ready"); } } catch (error) { - setStatus(options.onSelect ? "Save failed" : "Fill failed", error instanceof Error ? error.message : String(error), "error"); + setStatus(options.errorTitle || (options.onSelect ? "Save failed" : "Fill failed"), error instanceof Error ? error.message : String(error), "error"); } finally { row.disabled = false; } @@ -147,7 +147,20 @@ function renderSearchResults(results, query) { return; } renderMatchList(root, results, { - emptyMessage: `No entries matched "${query}".` + emptyMessage: `No entries matched "${query}".`, + errorTitle: "Fill failed", + onSelect: async (match, targetTabID) => { + setStatus("Approval may be required", "KeePassGO will prompt if this token needs approval before fill.", "warning"); + const result = await runtimeSend({ + type: "keepassgo-fill-selected-entry", + entryId: match.id, + tabId: targetTabID + }); + if (!result?.success) { + throw new Error(result?.error || "Fill failed."); + } + setStatus("Filled", `${match.title} was sent to the current page.`, "ready"); + } }); } diff --git a/internal/api/server.go b/internal/api/server.go index 9001593..0607df6 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -394,6 +394,10 @@ func (s *Server) GetBrowserCredential(ctx context.Context, req *keepassgov1.GetB return nil, status.Error(codes.InvalidArgument, "entry url does not match requested page") } } + return s.browserCredential(ctx, token, entry) +} + +func (s *Server) browserCredential(ctx context.Context, token apitokens.Token, entry vault.Entry) (*keepassgov1.GetBrowserCredentialResponse, error) { if strings.TrimSpace(entry.Username) != "" { if _, err := s.authorizeResourceRequest(ctx, token, apitokens.OperationCopyUsername, apitokens.Resource{Kind: apitokens.ResourceEntry, EntryID: entry.ID, Path: entry.Path}); err != nil { return nil, err diff --git a/internal/api/server_test.go b/internal/api/server_test.go index 3668952..eb00a6c 100644 --- a/internal/api/server_test.go +++ b/internal/api/server_test.go @@ -693,6 +693,40 @@ func TestVaultServiceGetsBrowserCredentialForAuthorizedClients(t *testing.T) { } } +func TestVaultServiceGetsExplicitBrowserCredentialWithoutURLMatch(t *testing.T) { + t.Parallel() + + client, _, cleanup := newTestClientForModel(t, vault.Model{ + Entries: []vault.Entry{ + { + ID: "no-url-entry", + Title: "Livingston Console", + Username: "livingstondell", + Password: "demo-loop", + Path: []string{"Root", "Heist Crew"}, + }, + testAPITokenEntry(t, + apitokens.PolicyRule{Effect: apitokens.EffectAllow, Operation: apitokens.OperationCopyUsername, Resource: apitokens.Resource{Kind: apitokens.ResourceEntry, EntryID: "no-url-entry", Path: []string{"Root", "Heist Crew"}}}, + apitokens.PolicyRule{Effect: apitokens.EffectAllow, Operation: apitokens.OperationCopyPassword, Resource: apitokens.Resource{Kind: apitokens.ResourceEntry, EntryID: "no-url-entry", Path: []string{"Root", "Heist Crew"}}}, + ), + }, + }) + defer cleanup() + + resp, err := client.GetBrowserCredential(tokenContext(defaultTestTokenSecret), &keepassgov1.GetBrowserCredentialRequest{ + Id: "no-url-entry", + }) + if err != nil { + t.Fatalf("GetBrowserCredential(no-url-entry without page URL) error = %v", err) + } + if resp.GetId() != "no-url-entry" { + t.Fatalf("GetBrowserCredential(no-url-entry without page URL).Id = %q, want no-url-entry", resp.GetId()) + } + if resp.GetPassword() != "demo-loop" { + t.Fatalf("GetBrowserCredential(no-url-entry without page URL).Password = %q, want demo-loop", resp.GetPassword()) + } +} + func TestVaultServiceRejectsUnauthorizedBrowserCredentialAccess(t *testing.T) { t.Parallel() -- 2.52.0