From dc7dd1954350ce3e8790d101469bbdf57a99b5e3 Mon Sep 17 00:00:00 2001 From: Joe Julian Date: Sat, 11 Apr 2026 23:56:48 -0700 Subject: [PATCH] Fix browser authorization edge cases --- internal/api/host.go | 3 ++ internal/api/server.go | 16 +++--- internal/api/server_test.go | 102 ++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 7 deletions(-) diff --git a/internal/api/host.go b/internal/api/host.go index 8149144..04e1b96 100644 --- a/internal/api/host.go +++ b/internal/api/host.go @@ -101,6 +101,9 @@ func (h *Host) Stop() error { h.started = false h.grpcServer.Stop() err := h.listener.Close() + if errors.Is(err, net.ErrClosed) { + err = nil + } if h.socketPath != "" { if removeErr := os.Remove(h.socketPath); removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) && err == nil { err = removeErr diff --git a/internal/api/server.go b/internal/api/server.go index 2d42c54..e05d87d 100644 --- a/internal/api/server.go +++ b/internal/api/server.go @@ -348,22 +348,26 @@ func (s *Server) authorizedBrowserMatches(ctx context.Context, token apitokens.T if _, err := s.authorizeResourceRequest(ctx, token, apitokens.OperationListEntries, match.resource); err != nil { return nil, err } - return browserMatchesWithinPath(matches, match.resource.Path), nil + return s.authorizedBrowserMatchesWithinPath(ctx, token, matches, match.resource.Path) } return out, nil } -func browserMatchesWithinPath(matches []rankedBrowserMatch, path []string) []*keepassgov1.BrowserLoginMatch { +func (s *Server) authorizedBrowserMatchesWithinPath(_ context.Context, _ apitokens.Token, matches []rankedBrowserMatch, path []string) ([]*keepassgov1.BrowserLoginMatch, error) { out := make([]*keepassgov1.BrowserLoginMatch, 0, len(matches)) for _, match := range matches { if len(path) > len(match.resource.Path) { continue } - if slices.Equal(path, match.resource.Path[:len(path)]) { - out = append(out, match.match) + if !slices.Equal(path, match.resource.Path[:len(path)]) { + continue } + if match.decision == apitokens.DecisionDeny { + continue + } + out = append(out, match.match) } - return out + return out, nil } func (s *Server) GetBrowserCredential(ctx context.Context, req *keepassgov1.GetBrowserCredentialRequest) (*keepassgov1.GetBrowserCredentialResponse, error) { @@ -1068,8 +1072,6 @@ func classifyBrowserEntryMatch(pageHost, rawEntryURL string) (string, int) { return "exact-host", 3 case strings.HasSuffix(pageHost, "."+entryHost): return "subdomain", 2 - case strings.HasSuffix(entryHost, "."+pageHost): - return "parent-domain", 1 default: return "", 0 } diff --git a/internal/api/server_test.go b/internal/api/server_test.go index fe5fa28..4d55fce 100644 --- a/internal/api/server_test.go +++ b/internal/api/server_test.go @@ -336,6 +336,108 @@ func TestVaultServiceFindsBrowserLoginsWithinAuthorizedGroupScope(t *testing.T) } } +func TestVaultServiceFindsBrowserLoginsRechecksChildPoliciesAfterPrompt(t *testing.T) { + t.Parallel() + + model := vault.Model{ + Entries: []vault.Entry{ + { + ID: "rusty-casino", + Title: "Rusty Casino", + Username: "rustyryan", + Password: "bellagio-1", + URL: "https://vault.heist.example.invalid", + Path: []string{"Crews", "Bellagio"}, + }, + { + ID: "benedict-vault", + Title: "Benedict Vault", + Username: "terrybenedict", + Password: "bellagio-2", + URL: "https://vault.heist.example.invalid", + Path: []string{"Crews", "Bellagio", "Denied"}, + }, + testAPITokenEntry(t, + apitokens.PolicyRule{Effect: apitokens.EffectDeny, Operation: apitokens.OperationListEntries, Resource: apitokens.Resource{Kind: apitokens.ResourceGroup, Path: []string{"Crews", "Bellagio", "Denied"}}}, + ), + }, + } + client, _, service, cleanup := newTestHarnessForModel(t, model) + defer cleanup() + service.approvals = apiapproval.NewBroker(time.Minute) + + respCh := make(chan *keepassgov1.FindBrowserLoginsResponse, 1) + errCh := make(chan error, 1) + go func() { + resp, err := client.FindBrowserLogins(tokenContext(defaultTestTokenSecret), &keepassgov1.FindBrowserLoginsRequest{ + PageUrl: "https://vault.heist.example.invalid/login", + }) + respCh <- resp + errCh <- err + }() + + pending := waitForServerPendingApproval(t, service, 1)[0] + if got := pending.Resource.Path; !slices.Equal(got, []string{"Crews", "Bellagio"}) { + t.Fatalf("pending.Resource.Path = %v, want [Crews Bellagio]", got) + } + if _, _, err := service.ResolveApproval(pending.ID, apiapproval.OutcomeAllowOnce); err != nil { + t.Fatalf("ResolveApproval(allow once) error = %v", err) + } + + resp := <-respCh + if err := <-errCh; err != nil { + t.Fatalf("FindBrowserLogins() error = %v", err) + } + if len(resp.Matches) != 1 { + t.Fatalf("len(FindBrowserLogins().Matches) = %d, want 1", len(resp.Matches)) + } + if got := resp.Matches[0].Id; got != "rusty-casino" { + t.Fatalf("FindBrowserLogins().Matches[0].Id = %q, want rusty-casino", got) + } +} + +func TestVaultServiceDoesNotMatchSpecificBrowserEntryToParentDomain(t *testing.T) { + t.Parallel() + + client, _, cleanup := newTestClientForModel(t, vault.Model{ + Entries: []vault.Entry{ + { + ID: "inside-man-accounts", + Title: "Inside Man Accounts", + Username: "daltonrussell", + Password: "diamond-1", + URL: "https://accounts.heist.example.invalid", + Path: []string{"Crews", "Bank"}, + }, + testAPITokenEntry(t, + apitokens.PolicyRule{Effect: apitokens.EffectAllow, Operation: apitokens.OperationListEntries, Resource: apitokens.Resource{Kind: apitokens.ResourceGroup, Path: []string{"Crews"}}}, + apitokens.PolicyRule{Effect: apitokens.EffectAllow, Operation: apitokens.OperationCopyUsername, Resource: apitokens.Resource{Kind: apitokens.ResourceEntry, EntryID: "inside-man-accounts", Path: []string{"Crews", "Bank"}}}, + apitokens.PolicyRule{Effect: apitokens.EffectAllow, Operation: apitokens.OperationCopyPassword, Resource: apitokens.Resource{Kind: apitokens.ResourceEntry, EntryID: "inside-man-accounts", Path: []string{"Crews", "Bank"}}}, + apitokens.PolicyRule{Effect: apitokens.EffectAllow, Operation: apitokens.OperationCopyURL, Resource: apitokens.Resource{Kind: apitokens.ResourceEntry, EntryID: "inside-man-accounts", Path: []string{"Crews", "Bank"}}}, + ), + }, + }) + defer cleanup() + + resp, err := client.FindBrowserLogins(tokenContext(defaultTestTokenSecret), &keepassgov1.FindBrowserLoginsRequest{ + PageUrl: "https://heist.example.invalid/login", + }) + if err != nil { + t.Fatalf("FindBrowserLogins() error = %v", err) + } + if len(resp.Matches) != 0 { + t.Fatalf("len(FindBrowserLogins().Matches) = %d, want 0", len(resp.Matches)) + } + + _, err = client.GetBrowserCredential(tokenContext(defaultTestTokenSecret), &keepassgov1.GetBrowserCredentialRequest{ + Id: "inside-man-accounts", + PageUrl: "https://heist.example.invalid/login", + }) + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("GetBrowserCredential() code = %v, want %v", status.Code(err), codes.InvalidArgument) + } +} + func TestVaultServiceListEntriesHidesSingleInternalVaultRoot(t *testing.T) { t.Parallel()