From 2ec786187d630325d3c83806acec73aa72ace179 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Sun, 11 Oct 2026 00:01:27 -0400 Subject: [PATCH 1/2] browser: gate acting tools on a web_host slot and fill credentials without leaking them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the web_host session-only slot, the browser acting tools gated External on the live page host, query_logins and the toolbox prompt fragment, and the no-secret credential filler with an in-memory hold — blanking filled fields from observation (by node liveness) until real navigation. --- ...rimitives.authzed.com_agentidentities.yaml | 10 +- ...ves.authzed.com_sessionuseridentities.yaml | 10 +- ...primitives.authzed.com_useridentities.yaml | 10 +- .../cmd/operator/schema_conformance_test.go | 9 + internal/cmd/runner/main.go | 100 ++- pkg/agent/tool/browser/acting_test.go | 220 +++++++ pkg/agent/tool/browser/blanking_test.go | 279 +++++++++ pkg/agent/tool/browser/chrome_test.go | 2 +- pkg/agent/tool/browser/fill_test.go | 156 +++++ pkg/agent/tool/browser/filler.go | 226 +++++++ pkg/agent/tool/browser/filler_test.go | 233 +++++++ .../tool/browser/policy_precheck_test.go | 2 +- pkg/agent/tool/browser/prompt_test.go | 77 +++ pkg/agent/tool/browser/query_logins_test.go | 234 +++++++ pkg/agent/tool/browser/session.go | 190 +++++- pkg/agent/tool/browser/snapshot.go | 120 +++- pkg/agent/tool/browser/snapshot_test.go | 94 +++ pkg/agent/tool/browser/synthesize.go | 130 +++- pkg/agent/tool/browser/tools.go | 578 +++++++++++++++++- pkg/agent/tool/browser/tools_test.go | 51 +- pkg/apis/v1alpha1/directcredentials_types.go | 33 +- pkg/apis/v1alpha1/zz_generated.deepcopy.go | 15 + .../guardian/schema/web_host_slot_test.go | 61 ++ pkg/authz/plangate/card.go | 4 + .../plangate/card_resource_display_test.go | 9 + pkg/authz/plangate/card_slot_standing_test.go | 1 + pkg/authz/plangate/resource_display.go | 3 + .../toolcheck/web_host_external_test.go | 84 +++ .../spicedb/web_host_slot_integration_test.go | 101 +++ pkg/controllers/agentclass/browser_slot.go | 43 ++ .../agentclass/browser_slot_test.go | 77 +++ pkg/controllers/agentclass/controller.go | 7 + .../agentidentity/direct_credential_test.go | 4 +- ...cret_arrival_prodcache_integration_test.go | 162 +++++ .../browsercatalog_envtest_test.go | 2 +- ...browsercatalog_fingerprint_envtest_test.go | 2 +- pkg/controllers/agentsession/controller.go | 82 +++ pkg/controllers/agentsession/rbac.go | 36 ++ pkg/controllers/agentsession/rbac_test.go | 40 ++ .../browsertoolbox/controller_test.go | 2 +- .../watches_integration_test.go | 2 +- .../guardian/agentsessiongrants_controller.go | 14 +- .../credkind/directcredentials/kind_test.go | 16 +- ...credentials_secretref_optional_key_test.go | 138 +++++ pkg/platform/manifests/install.yaml | 30 +- pkg/tools/browserschema/schema.go | 116 ++++ pkg/tools/browserschema/schema_test.go | 64 ++ test/e2e/harness.go | 11 + test/e2e/inprocess_runner_factory.go | 49 +- 49 files changed, 3845 insertions(+), 94 deletions(-) create mode 100644 pkg/agent/tool/browser/acting_test.go create mode 100644 pkg/agent/tool/browser/blanking_test.go create mode 100644 pkg/agent/tool/browser/fill_test.go create mode 100644 pkg/agent/tool/browser/filler.go create mode 100644 pkg/agent/tool/browser/filler_test.go create mode 100644 pkg/agent/tool/browser/prompt_test.go create mode 100644 pkg/agent/tool/browser/query_logins_test.go create mode 100644 pkg/agent/tool/browser/snapshot_test.go create mode 100644 pkg/authz/guardian/schema/web_host_slot_test.go create mode 100644 pkg/authz/spicedb/toolcheck/web_host_external_test.go create mode 100644 pkg/authz/spicedb/web_host_slot_integration_test.go create mode 100644 pkg/controllers/agentclass/browser_slot.go create mode 100644 pkg/controllers/agentclass/browser_slot_test.go create mode 100644 pkg/controllers/agentidentity/secret_arrival_prodcache_integration_test.go create mode 100644 pkg/platform/manifests/directcredentials_secretref_optional_key_test.go create mode 100644 pkg/tools/browserschema/schema.go create mode 100644 pkg/tools/browserschema/schema_test.go diff --git a/config/crds/agentprimitives.authzed.com_agentidentities.yaml b/config/crds/agentprimitives.authzed.com_agentidentities.yaml index 2fc495a8..6d7cce49 100644 --- a/config/crds/agentprimitives.authzed.com_agentidentities.yaml +++ b/config/crds/agentprimitives.authzed.com_agentidentities.yaml @@ -103,15 +103,19 @@ spec: or the host-keyed login Secret (secret). properties: key: - description: Key is the data key within the Secret holding - the value. + description: |- + Key is the data key within the Secret holding the value. Optional: + "token" (or omitted) for source=onepassword; must be omitted for + source=secret, whose login Secret is a multi-key shape with no single + named value key. ValidateSpec enforces the exact rule per source; this + marker only ensures the apiserver never rejects an omitted key before + ValidateSpec gets to run. type: string name: description: Name is the Secret's name, in the referring object's own namespace. type: string required: - - key - name type: object source: diff --git a/config/crds/agentprimitives.authzed.com_sessionuseridentities.yaml b/config/crds/agentprimitives.authzed.com_sessionuseridentities.yaml index 19598136..66893e17 100644 --- a/config/crds/agentprimitives.authzed.com_sessionuseridentities.yaml +++ b/config/crds/agentprimitives.authzed.com_sessionuseridentities.yaml @@ -113,15 +113,19 @@ spec: or the host-keyed login Secret (secret). properties: key: - description: Key is the data key within the Secret holding - the value. + description: |- + Key is the data key within the Secret holding the value. Optional: + "token" (or omitted) for source=onepassword; must be omitted for + source=secret, whose login Secret is a multi-key shape with no single + named value key. ValidateSpec enforces the exact rule per source; this + marker only ensures the apiserver never rejects an omitted key before + ValidateSpec gets to run. type: string name: description: Name is the Secret's name, in the referring object's own namespace. type: string required: - - key - name type: object source: diff --git a/config/crds/agentprimitives.authzed.com_useridentities.yaml b/config/crds/agentprimitives.authzed.com_useridentities.yaml index f22de9cb..e50aed9e 100644 --- a/config/crds/agentprimitives.authzed.com_useridentities.yaml +++ b/config/crds/agentprimitives.authzed.com_useridentities.yaml @@ -109,15 +109,19 @@ spec: or the host-keyed login Secret (secret). properties: key: - description: Key is the data key within the Secret holding - the value. + description: |- + Key is the data key within the Secret holding the value. Optional: + "token" (or omitted) for source=onepassword; must be omitted for + source=secret, whose login Secret is a multi-key shape with no single + named value key. ValidateSpec enforces the exact rule per source; this + marker only ensures the apiserver never rejects an omitted key before + ValidateSpec gets to run. type: string name: description: Name is the Secret's name, in the referring object's own namespace. type: string required: - - key - name type: object source: diff --git a/internal/cmd/operator/schema_conformance_test.go b/internal/cmd/operator/schema_conformance_test.go index 8af40ebc..d26a8cac 100644 --- a/internal/cmd/operator/schema_conformance_test.go +++ b/internal/cmd/operator/schema_conformance_test.go @@ -11,6 +11,7 @@ import ( guardianschema "github.com/authzed/openagentprimitives/pkg/authz/guardian/schema" "github.com/authzed/openagentprimitives/pkg/channels/channelkinds" channelregistry "github.com/authzed/openagentprimitives/pkg/channels/channelkinds/registry" + "github.com/authzed/openagentprimitives/pkg/tools/browserschema" "github.com/authzed/openagentprimitives/toolkits" ) @@ -87,11 +88,19 @@ func TestCompileTimeFragmentsCompose(t *testing.T) { toolkitFrags := guardianschema.ToolkitFragments(toolkits.All()) compileTime = append(compileTime, toolkitFrags...) + // The browser toolbox's builtin web_host fragment is part of the same + // compile-time baseline the guardian controller composes (schemaBaseline), + // so it must compose here too — this is the only net before a bad builtin + // fragment reaches a cluster as a startup-time schema freeze. + compileTime = append(compileTime, browserschema.Fragment()) + out, err := guardianschema.ComposeBase(compileTime) require.NoError(t, err, "every compile-time fragment must compose into the base") assert.Contains(t, out, "definition agentsession") assert.Contains(t, out, "definition slack_channel", "the Slack fragment is load-bearing: audience resolution reads slack_channel#view") + assert.Contains(t, out, "definition web_host", + "the browser toolbox's builtin web_host fragment must reach the composed base") // Every definition a built-in toolkit fragment declares must actually // reach the composed base. Until this loop existed, nothing in this test diff --git a/internal/cmd/runner/main.go b/internal/cmd/runner/main.go index cb16e8ce..4e5b4dc2 100644 --- a/internal/cmd/runner/main.go +++ b/internal/cmd/runner/main.go @@ -153,7 +153,9 @@ import ( "github.com/authzed/openagentprimitives/pkg/platform/identity/broker/inproc" "github.com/authzed/openagentprimitives/pkg/platform/identity/credkind/githubapp" _ "github.com/authzed/openagentprimitives/pkg/platform/identity/credkind/imports" // register the static/oauth/federated/githubApp credkind.Kinds the broker dispatches to via registry.Get + credkindregistry "github.com/authzed/openagentprimitives/pkg/platform/identity/credkind/registry" "github.com/authzed/openagentprimitives/pkg/platform/identity/credresolve" + "github.com/authzed/openagentprimitives/pkg/platform/identity/directcred" _ "github.com/authzed/openagentprimitives/pkg/platform/identity/directcred/onepassword" // register the "onepassword" directcred.Source Phase C's browser form filler dispatches to via directcred.Get (Fetch at use time) _ "github.com/authzed/openagentprimitives/pkg/platform/identity/directcred/secretsource" // register the "secret" directcred.Source for the same Phase C Fetch consumer "github.com/authzed/openagentprimitives/pkg/platform/identity/externaltoken" @@ -1489,6 +1491,14 @@ func run(cfg *config) error { const browserPodIPPollInterval = 2 * time.Second browserTools := []tool.Tool{} + // browserSections carries each resolved toolbox's one-line prompt fragment + // (spec §5.1 — query_logins' catalog size, and the allowed-host count in + // allowlist mode). The browser toolbox is hand-wired here rather than + // through capability.Capability/SectionOfferer (see + // browsertoolboxsynth.PromptSection's doc comment), so this is a second, + // independent source of PromptSections alongside assembled.Sections below + // — merged at the WithPromptSections call rather than decided there. + var browserSections []capability.PromptSection for _, rt := range sess.Status.ResolvedBrowserToolboxes { if rt.BrowserPodIP == "" { slog.Info("browser toolbox IP not yet reflected; polling", @@ -1560,7 +1570,11 @@ func run(cfg *config) error { // offered to a model that cannot consume it. acceptsImages := provider.NativeInputMIMEs(sess.Status.EffectiveSettings.Model.Name).Has("image/png") gateURL := fmt.Sprintf("http://%s:%d", rt.BrowserPodIP, rt.GatePort) - built, serr := browsertoolboxsynth.Synthesize(rt, gateURL, token, acceptsImages) + // The credential-filler Fetch seam: resolves {{login...}} placeholders at + // use time against the bound identity's directcred source. nil when the + // toolbox binds no identity (no catalog → no placeholder resolves). + fetch := newBrowserCredentialFetcher(c, ns, rt) + built, serr := browsertoolboxsynth.Synthesize(rt, gateURL, token, acceptsImages, fetch) if serr != nil { if werr := failSession(spiceboxv1alpha1.ReasonAgentSessionBrowserBootFailed, fmt.Sprintf("browser toolbox %q: %v", rt.Name, serr)); werr != nil { @@ -1569,6 +1583,9 @@ func run(cfg *config) error { return nil } browserTools = append(browserTools, built...) + if sec := browsertoolboxsynth.PromptSection(rt); sec != nil { + browserSections = append(browserSections, *sec) + } } // Resolve opted-in skills for this agent class. The ceiling enforcement @@ -2168,7 +2185,13 @@ func run(cfg *config) error { // Capability-owned prompt sections ride with the tools they describe // (capability.SectionOfferer): the views capability's "Your page" exists // only when update_view does. Nothing here decides whether to include one. - composeOpts = append(composeOpts, runner.WithPromptSections(assembled.Sections)) + // + // browserSections is the browser toolbox's analog of the same rule, from + // its own hand-wired path (built alongside browserTools above, not through + // capability.Capability): PromptSection returns nil exactly when + // query_logins is withheld, so appending it here is still "the tool's own + // decision rides along", not a decision made in this file. + composeOpts = append(composeOpts, runner.WithPromptSections(append(assembled.Sections, browserSections...))) composedSystem := runner.ComposeSystem(systemPrompt, mergedTools, skillMeta, repoInstrs, assetKinds, modalityInstructions, composeOpts...) // Record WHAT the agent was asked, alongside the turn log's record of WHICH @@ -3964,3 +3987,76 @@ func sidecarNoticeSink(loop *runner.Loop, sessKey client.ObjectKey) func(context "session", sessKey.String()) } } + +// newBrowserCredentialFetcher builds the credential-filler Fetch seam for one +// resolved browser toolbox, mirroring the operator's resolveBrowserCatalog +// recipe (pkg/controllers/agentsession/browserpod.go): it resolves the bound +// identity's first directCredentials credential and its directcred source, then +// — lazily, on each Fetch — reads the backing Secret and dispatches to the +// source's Fetch. Reading the Secret per Fetch (rather than capturing its bytes +// for the session's lifetime) keeps the raw credential material in memory only +// for the moment of use plus the browser session's short §5.3 hold. +// +// Returns nil when the toolbox binds no identity: with no catalog, no +// {{login...}} placeholder can resolve (the filler's rule 2 denies first), so +// there is nothing to Fetch. Every error path returns the error to the filler, +// which renders it as a fail-closed tool result — never a credential value. +func newBrowserCredentialFetcher(c client.Client, ns string, rt spiceboxv1alpha1.ResolvedBrowserToolbox) browsertoolboxsynth.CredentialFetcher { + if rt.IdentityName == "" { + return nil + } + identityName := rt.IdentityName + return func(ctx context.Context, catalogID string) (directcred.Credential, error) { + var id spiceboxv1alpha1.AgentIdentity + if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: identityName}, &id); err != nil { + return directcred.Credential{}, fmt.Errorf("get AgentIdentity %q: %w", identityName, err) + } + var cred *spiceboxv1alpha1.AgentCredential + for i := range id.Spec.Credentials { + if id.Spec.Credentials[i].DirectCredentials != nil { + cred = &id.Spec.Credentials[i] + break + } + } + if cred == nil { + return directcred.Credential{}, fmt.Errorf("AgentIdentity %q has no directCredentials credential", identityName) + } + // Classify through the credkind registry (never a cred.Type switch) to + // locate the backing Secret and key, exactly as resolveBrowserCatalog does. + k, err := credkindregistry.Get(cred.Type) + if err != nil { + return directcred.Credential{}, fmt.Errorf("classify credential %q: %w", cred.Name, err) + } + secRef := k.SecretRef(*cred) + if secRef == nil || secRef.Name == "" { + return directcred.Credential{}, fmt.Errorf("credential %q has no backing Secret", cred.Name) + } + var sec corev1.Secret + if err := c.Get(ctx, client.ObjectKey{Namespace: ns, Name: secRef.Name}, &sec); err != nil { + return directcred.Credential{}, fmt.Errorf("read credential Secret %q: %w", secRef.Name, err) + } + src, ok := directcred.Get(cred.DirectCredentials.Source) + if !ok { + return directcred.Credential{}, fmt.Errorf( + "no directcred source registered for %q (the binary may be missing its blank import)", cred.DirectCredentials.Source) + } + cfg := directcred.Config{ + SecretData: sec.Data, + Vault: cred.DirectCredentials.Vault, + VaultID: pinnedBrowserVaultID(&id, cred.Name), + } + return src.Fetch(ctx, cfg, catalogID) + } +} + +// pinnedBrowserVaultID returns the vault id the identity's status pinned for +// credName (onepassword), or "" when none is recorded (the secret source has no +// vault concept). Mirrors the operator's pinnedVaultID, which is unexported. +func pinnedBrowserVaultID(id *spiceboxv1alpha1.AgentIdentity, credName string) string { + for _, pv := range id.Status.PinnedVaults { + if pv.Credential == credName { + return pv.VaultID + } + } + return "" +} diff --git a/pkg/agent/tool/browser/acting_test.go b/pkg/agent/tool/browser/acting_test.go new file mode 100644 index 00000000..b99e4eb4 --- /dev/null +++ b/pkg/agent/tool/browser/acting_test.go @@ -0,0 +1,220 @@ +package browser + +import ( + "context" + "net/http" + "testing" + + v1 "github.com/authzed/authzed-go/proto/authzed/api/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authzed/openagentprimitives/pkg/authz" + "github.com/authzed/openagentprimitives/pkg/authz/spicedb/toolcheck" + "github.com/authzed/openagentprimitives/pkg/tools/browserschema" +) + +// actingTool builds one acting browserTool whose Session reports host as its +// current page host, without any Chrome — enough to exercise Permission and +// NamedArgs in isolation. +func actingTool(op, host string) *browserTool { + return &browserTool{op: op, sess: &Session{currentHost: func() string { return host }}} +} + +// TestActingOpPermissionIsExternalWebHost proves each acting op authorizes +// External on the web_host slot with the act permission, and an observation op +// stays Passthrough. +func TestActingOpPermissionIsExternalWebHost(t *testing.T) { + for _, op := range []string{"click", "press", "select", "submit"} { + t.Run(op+": External on web_host#act", func(t *testing.T) { + p := actingTool(op, "aa.com").Permission() + assert.Equal(t, authz.External, p.StateImpact) + require.NotNil(t, p.Check, "an acting op must carry a Check") + assert.Equal(t, browserschema.WebHostType, p.Check.ResourceType) + assert.Equal(t, browserschema.ActPermission, p.Check.Permission) + assert.Equal(t, "{"+hostArgKey+"}", p.Check.ResourceIDTemplate) + assert.Empty(t, p.Check.ResourceIDTransforms, + "the canonical host is a valid object id as-is; no transforms needed") + }) + } + + t.Run("read: Passthrough with no Check", func(t *testing.T) { + p := actingTool("read", "aa.com").Permission() + assert.Equal(t, authz.Passthrough, p.StateImpact) + assert.Nil(t, p.Check, "an observation op maps to no SpiceDB resource") + }) +} + +// TestActingNamedArgsSurfacesPageHost proves the page host reaches the check's +// ResolveResourceID under hostArgKey, yielding the canonical host — and that it +// is PAGE STATE: a model-supplied "host" cannot override the real one. +func TestActingNamedArgsSurfacesPageHost(t *testing.T) { + bt := actingTool("click", "aa.com") + + named, ok := bt.NamedArgs(map[string]any{"ref": "e3", "host": "attacker.example"}) + require.True(t, ok) + assert.Equal(t, "aa.com", named[hostArgKey], + "the live page host must win over a host the model put in the call") + assert.Equal(t, "e3", named["ref"], "other args pass through untouched") + + id, err := authz.ResolveResourceID(*bt.Permission().Check, named) + require.NoError(t, err) + assert.Equal(t, "aa.com", id, "the check resolves the instance to the canonical page host") +} + +// TestObservationNamedArgsPassThrough proves an observation op does not touch the +// args (no host surfaced, no live-location round-trip on the authz path). +func TestObservationNamedArgsPassThrough(t *testing.T) { + bt := &browserTool{op: "read", sess: &Session{currentHost: func() string { + t.Fatal("CurrentHost must not be read for an observation op") + return "" + }}} + in := map[string]any{"page": 2} + out, ok := bt.NamedArgs(in) + require.True(t, ok) + assert.Equal(t, in, out) + _, hasHost := out[hostArgKey] + assert.False(t, hasHost, "an observation op surfaces no host") +} + +// recordingCli is a toolcheck.Client that records whether SpiceDB was queried, +// so the empty-host test can prove the denial happens BEFORE any round trip. +type recordingCli struct{ called bool } + +func (r *recordingCli) CheckPermission(context.Context, *v1.CheckPermissionRequest) (*v1.CheckPermissionResponse, error) { + r.called = true + return &v1.CheckPermissionResponse{Permissionship: v1.CheckPermissionResponse_PERMISSIONSHIP_NO_PERMISSION}, nil +} + +// TestActingEmptyHostFailsClosed proves an empty current host (about:blank / no +// page) is an UnresolvedResource denial routed to a human — never a silent allow, +// and never a wasted SpiceDB query. +func TestActingEmptyHostFailsClosed(t *testing.T) { + bt := actingTool("click", "") // CurrentHost resolves to "" + + named, ok := bt.NamedArgs(map[string]any{"ref": "e3"}) + require.True(t, ok) + assert.Equal(t, "", named[hostArgKey]) + + cli := &recordingCli{} + res := toolcheck.Checker{Cli: cli}.CheckToolCall(context.Background(), bt.Permission(), authz.Inputs{ + Subject: "u-1", + Args: named, + AgentSessionRef: "ns/s1", + SlotResourceTypes: []string{browserschema.WebHostType}, + }) + assert.Equal(t, authz.OutcomeDenied, res.Outcome, "empty host must deny") + assert.True(t, res.UnresolvedResource, "empty host must be an UnresolvedResource denial, never a silent allow") + assert.False(t, cli.called, "an unresolved host must deny before querying SpiceDB") +} + +// --- chrome-backed DOM-mutation tests (local Chrome via startFixture) --- +// +// These drive a real headless Chrome through the in-process gate and assert the +// acting op actually changed the page. The testbrowser C10 login site is not +// built yet, so each test serves its own minimal form/handler page from the +// httptest site the fixture allowlists. + +func TestClickMutatesDOM(t *testing.T) { + fx := startFixture(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`` + + `` + + `

idle

`)) + })) + fx.rt.Spec.Interaction = "act" + tools := fx.synthTools(t, false) + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": fx.siteBase + "/"}).IsError) + + read := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, read.IsError, "read: %s", read.Content) + btnRef := refForRole(t, read.Content, "button") + + clicked := run(t, toolBy(t, tools, "click"), map[string]any{"ref": btnRef}) + assert.False(t, clicked.IsError, "click: %s", clicked.Content) + + after := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, after.IsError, "read after click: %s", after.Content) + assert.Contains(t, after.Content, "clicked", "click must fire the element's onclick handler") +} + +func TestSelectMutatesDOM(t *testing.T) { + fx := startFixture(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`` + + `` + + `

idle

`)) + })) + fx.rt.Spec.Interaction = "act" + tools := fx.synthTools(t, false) + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": fx.siteBase + "/"}).IsError) + + read := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, read.IsError, "read: %s", read.Content) + selRef := refForRole(t, read.Content, "textbox") // ` + + `

idle

`)) + })) + fx.rt.Spec.Interaction = "act" + tools := fx.synthTools(t, false) + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": fx.siteBase + "/"}).IsError) + + read := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, read.IsError, "read: %s", read.Content) + btnRef := refForRole(t, read.Content, "button") + + sub := run(t, toolBy(t, tools, "submit"), map[string]any{"ref": btnRef}) + assert.False(t, sub.IsError, "submit: %s", sub.Content) + + after := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, after.IsError, "read after submit: %s", after.Content) + assert.Contains(t, after.Content, "submitted", "submit must trigger the form's onsubmit") +} + +func TestPressDispatchesKey(t *testing.T) { + fx := startFixture(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`` + + `` + + `

idle

`)) + })) + fx.rt.Spec.Interaction = "act" + tools := fx.synthTools(t, false) + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": fx.siteBase + "/"}).IsError) + + pressed := run(t, toolBy(t, tools, "press"), map[string]any{"key": "a"}) + assert.False(t, pressed.IsError, "press: %s", pressed.Content) + + after := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, after.IsError, "read after press: %s", after.Content) + assert.Contains(t, after.Content, "key:a", "press must dispatch a keydown the page observes") +} + +// TestCurrentHostReadsLivePage proves CurrentHost reads and canonicalizes the +// real page host, and is "" before any navigation (about:blank) — the fail-closed +// instance the acting check denies on. +func TestCurrentHostReadsLivePage(t *testing.T) { + fx := startFixture(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`

Home

`)) + })) + fx.rt.Spec.Interaction = "act" + tools := fx.synthTools(t, false) + bt, ok := tools[0].(*browserTool) + require.True(t, ok) + + assert.Equal(t, "", bt.sess.CurrentHost(), "no page yet (about:blank) has no host") + + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": fx.siteBase + "/"}).IsError) + assert.Equal(t, "site.test", bt.sess.CurrentHost(), "CurrentHost returns the canonical live host") +} diff --git a/pkg/agent/tool/browser/blanking_test.go b/pkg/agent/tool/browser/blanking_test.go new file mode 100644 index 00000000..574642aa --- /dev/null +++ b/pkg/agent/tool/browser/blanking_test.go @@ -0,0 +1,279 @@ +package browser + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + agenttool "github.com/authzed/openagentprimitives/pkg/agent/tool" + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" +) + +// blankingLoginPage serves a form with a username and password input and a +// submit button whose default GET submission navigates to /landed. Unlike +// fill_test.go's loginPage, it carries NO script that mirrors a typed value +// elsewhere in the DOM: the only way a filled field's value could reach +// read/find is the input's OWN accessibility value — exactly the vector Task +// C5 closes. (A page that mirrors a typed value into some OTHER element is a +// separate, still-open gap — the spec's Phase D taint tracking, not this +// task; fill_test.go's mirror exists only to prove a value reached the page, +// not to exercise C5.) +func blankingLoginPage() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // no-store disables the back-forward cache, so chromedp.NavigateBack does + // a real reload that fires a load event (a bfcache restore does not, and + // NavigateBack's internal load-wait would hang on it). + w.Header().Set("Cache-Control", "no-store") + if r.URL.Path == "/landed" { + _, _ = w.Write([]byte(`

Welcome

`)) + return + } + _, _ = w.Write([]byte(`` + + `
` + + `` + + `` + + `` + + `
` + + ``)) + }) +} + +// blankingFixture boots the chrome fixture in act mode with a one-entry +// catalog for the fixture site and serves handler, returning the synthesized +// tools, the shared session (for direct blanked-set assertions), and the site +// base URL. Mirrors fill_test.go's browserFillFixture but takes its own +// handler. +func blankingFixture(t *testing.T, handler http.Handler) (tools []agenttool.Tool, sess *Session, siteBase string) { + t.Helper() + fx := startFixture(t, handler) + fx.rt.Spec.Interaction = "act" + fx.rt.IdentityName = "site-identity" + fx.rt.Catalog = []spiceboxv1alpha1.BrowserLoginRef{{ + ID: "login-site", + Label: "Site", + Hosts: []spiceboxv1alpha1.BrowserLoginHost{{Host: "site.test"}}, + }} + ff := &fakeFetcher{} + var err error + tools, err = Synthesize(fx.rt, fx.gateURL, chromeToken, true, ff.fetch()) + require.NoError(t, err) + require.NotEmpty(t, tools) + sess = tools[0].(*browserTool).sess + t.Cleanup(func() { sess.Close() }) + return tools, sess, fx.siteBase +} + +// fillUsernameAndPassword navigates to base and types the username/password +// credential placeholders into the page's two inputs, returning their refs. +func fillUsernameAndPassword(t *testing.T, tools []agenttool.Tool, base string) (userRef, passRef string) { + t.Helper() + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": base + "/"}).IsError) + + userRef = refByName(t, tools, "UsernameField") + passRef = refByName(t, tools, "PasswordField") + + uRes := run(t, toolBy(t, tools, "type"), map[string]any{"ref": userRef, "text": "{{login.site.test.username}}"}) + require.False(t, uRes.IsError, "type username: %s", uRes.Content) + pRes := run(t, toolBy(t, tools, "type"), map[string]any{"ref": passRef, "text": "{{login.site.test.password}}"}) + require.False(t, pRes.IsError, "type password: %s", pRes.Content) + return userRef, passRef +} + +func TestReadOmitsBlankedFieldValue(t *testing.T) { + tools, sess, base := blankingFixture(t, blankingLoginPage()) + fillUsernameAndPassword(t, tools, base) + require.Len(t, sess.blankedNodes(), 2, "typing a credential into both fields must mark both blanked") + + read := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, read.IsError, "read: %s", read.Content) + assert.NotContains(t, read.Content, stubUsername, "a blanked field's value must never reach read") + assert.NotContains(t, read.Content, stubPassword, "a blanked field's value must never reach read") + assert.Contains(t, read.Content, "UsernameField", "the element itself must still appear (role/name)") + assert.Contains(t, read.Content, "PasswordField", "the element itself must still appear (role/name)") + assert.Contains(t, read.Content, "redacted", "a blanked field must say so, not just render as empty") +} + +func TestFindDoesNotRevealBlankedFieldValue(t *testing.T) { + tools, sess, base := blankingFixture(t, blankingLoginPage()) + fillUsernameAndPassword(t, tools, base) + require.Len(t, sess.blankedNodes(), 2) + + found := run(t, toolBy(t, tools, "find"), map[string]any{"text": "UsernameField"}) + require.False(t, found.IsError, "find: %s", found.Content) + assert.NotContains(t, found.Content, stubUsername, "find must not reveal a blanked field's value") + + foundPw := run(t, toolBy(t, tools, "find"), map[string]any{"text": "PasswordField"}) + require.False(t, foundPw.IsError, "find: %s", foundPw.Content) + assert.NotContains(t, foundPw.Content, stubPassword, "find must not reveal a blanked field's value") +} + +func TestScreenshotRefusedWhileFieldBlanked(t *testing.T) { + tools, sess, base := blankingFixture(t, blankingLoginPage()) + fillUsernameAndPassword(t, tools, base) + require.NotEmpty(t, sess.blankedNodes()) + + res := run(t, toolBy(t, tools, "screenshot"), map[string]any{}) + assert.True(t, res.IsError, "screenshot must be refused while a filled field is unsubmitted") + assert.Contains(t, res.Content, "screenshot refused") +} + +func TestNavigateClearsBlankedAndAllowsScreenshot(t *testing.T) { + tools, sess, base := blankingFixture(t, blankingLoginPage()) + fillUsernameAndPassword(t, tools, base) + require.NotEmpty(t, sess.blankedNodes()) + + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": base + "/landed"}).IsError) + assert.Empty(t, sess.blankedNodes(), "navigation must clear the blanked set") + + shot := run(t, toolBy(t, tools, "screenshot"), map[string]any{}) + assert.False(t, shot.IsError, "screenshot must be allowed again after navigation: %s", shot.Content) + + read := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, read.IsError, "read: %s", read.Content) + assert.NotContains(t, read.Content, "redacted", "a fresh page has nothing blanked to redact") +} + +func TestBackClearsBlanked(t *testing.T) { + tools, sess, base := blankingFixture(t, blankingLoginPage()) + // Start on /landed, navigate to the login page, fill it, then go back. The + // back() must clear the blanked set just as navigate does — both replace the + // document. + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": base + "/landed"}).IsError) + fillUsernameAndPassword(t, tools, base) + require.NotEmpty(t, sess.blankedNodes()) + + require.False(t, run(t, toolBy(t, tools, "back"), map[string]any{}).IsError) + assert.Empty(t, sess.blankedNodes(), "back must clear the blanked set") + + shot := run(t, toolBy(t, tools, "screenshot"), map[string]any{}) + assert.False(t, shot.IsError, "screenshot must be allowed again after back: %s", shot.Content) +} + +// TestSubmitThatNavigatesDoesNotLeakClearsOnNextNavigate proves the Option-A +// semantics: a submit that really navigates does NOT itself clear the blanked +// set (submit is not a reliable "fields gone" signal), but it also cannot leak +// — the old fields are detached, so read of the new document carries none of +// their values. The stale blanked ids clear on the next genuine navigation. +func TestSubmitThatNavigatesDoesNotLeakClearsOnNextNavigate(t *testing.T) { + tools, sess, base := blankingFixture(t, blankingLoginPage()) + userRef, _ := fillUsernameAndPassword(t, tools, base) + require.NotEmpty(t, sess.blankedNodes()) + + // The form's default GET submission navigates to /landed, detaching the inputs. + sub := run(t, toolBy(t, tools, "submit"), map[string]any{"ref": userRef}) + require.False(t, sub.IsError, "submit: %s", sub.Content) + + // The new document holds none of the filled values — no leak, regardless of + // whether the stale blanked ids have been cleared yet. + read := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, read.IsError, "read after submit+navigate: %s", read.Content) + assert.Contains(t, read.Content, "Welcome", "the page must have actually navigated to /landed") + assert.NotContains(t, read.Content, stubUsername, "a navigated-away document holds no filled value") + assert.NotContains(t, read.Content, stubPassword, "a navigated-away document holds no filled value") + + // An explicit navigation clears the stale set and re-enables screenshot. + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": base + "/landed"}).IsError) + assert.Empty(t, sess.blankedNodes(), "a genuine navigation clears the stale blanked ids") + shot := run(t, toolBy(t, tools, "screenshot"), map[string]any{}) + assert.False(t, shot.IsError, "screenshot must be allowed after a real navigation: %s", shot.Content) +} + +// hideShowPage serves a form plus two buttons that hide / re-show the USERNAME +// input via style.display, WITHOUT removing it from the DOM or clearing its +// value. This is the Critical the AX-presence liveness prune introduced: +// Chromium omits a display:none subtree from the accessibility tree, so a pruned +// field re-renders its real value on re-show. The username (a text input) is the +// vector — its value appears in plaintext in the AX tree, unlike a masked +// password. +func hideShowPage() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`` + + `
` + + `` + + `` + + `
` + + `` + + `` + + ``)) + }) +} + +// TestHiddenThenReshownFieldStaysBlanked is the re-review regression test: a +// filled field hidden via display:none (omitted from the AX tree, still in the +// DOM with its value) and then re-shown must STILL be blanked — read must not +// render its value, and screenshot must stay refused. An AX-presence liveness +// prune would drop it while hidden and leak it on re-show. +func TestHiddenThenReshownFieldStaysBlanked(t *testing.T) { + tools, sess, base := blankingFixture(t, hideShowPage()) + fillUsernameAndPassword(t, tools, base) + require.NotEmpty(t, sess.blankedNodes()) + + // Baseline: the filled username is redacted, not leaked. + base0 := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, base0.IsError, "read: %s", base0.Content) + assert.NotContains(t, base0.Content, stubUsername) + + // Hide the username field (display:none → omitted from the AX tree). The read + // while hidden is exactly where an AX-presence prune would drop the id. + hideRef := refByName(t, tools, "HideBtn") + require.False(t, run(t, toolBy(t, tools, "click"), map[string]any{"ref": hideRef}).IsError) + hidden := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, hidden.IsError, "read while hidden: %s", hidden.Content) + assert.NotContains(t, hidden.Content, stubUsername, "a hidden field must not render its value") + + // Re-show the username field WITHOUT retyping; its value is still in the DOM. + showRef := refByName(t, tools, "ShowBtn") + require.False(t, run(t, toolBy(t, tools, "click"), map[string]any{"ref": showRef}).IsError) + + reshown := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, reshown.IsError, "read after re-show: %s", reshown.Content) + assert.NotContains(t, reshown.Content, stubUsername, "a re-shown filled field must STILL be blanked (not re-exposed)") + assert.Contains(t, reshown.Content, "redacted", "the re-shown field must still carry the redaction marker") + assert.NotEmpty(t, sess.blankedNodes(), "hide/show must not drop the field from the blanked set") + + shot := run(t, toolBy(t, tools, "screenshot"), map[string]any{}) + assert.True(t, shot.IsError, "screenshot must stay refused while a filled field (even hidden/re-shown) is present") +} + +// spaLoginPage serves a form whose onsubmit changes the URL via history.pushState +// and cancels the real submission (return false), so the password input stays +// MOUNTED after "submit". This is the §3.6 vector a URL-change heuristic would +// get wrong: the URL moves to /dashboard but the credential-filled field is +// still on the page, so it must remain blanked. +func spaLoginPage() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`` + + `
` + + `` + + `` + + `` + + `
`)) + }) +} + +// TestSPASubmitKeepsMountedFieldBlanked is the Finding-1 regression test: a +// submit that only pushState's a new route (URL changes, field stays mounted) +// must NOT un-blank the field. A URL-based heuristic would wrongly clear here. +func TestSPASubmitKeepsMountedFieldBlanked(t *testing.T) { + tools, sess, base := blankingFixture(t, spaLoginPage()) + userRef, _ := fillUsernameAndPassword(t, tools, base) + require.NotEmpty(t, sess.blankedNodes()) + + // requestSubmit() fires onsubmit → pushState('/dashboard') → return false + // cancels the real submit, so the inputs stay mounted while the URL changes. + sub := run(t, toolBy(t, tools, "submit"), map[string]any{"ref": userRef}) + require.False(t, sub.IsError, "submit: %s", sub.Content) + + read := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, read.IsError, "read after SPA submit: %s", read.Content) + assert.NotContains(t, read.Content, stubPassword, "a still-mounted filled field must stay blanked despite the URL change") + assert.NotContains(t, read.Content, stubUsername, "a still-mounted filled field must stay blanked despite the URL change") + assert.Contains(t, read.Content, "redacted", "the mounted field must still carry the redaction marker") + assert.NotEmpty(t, sess.blankedNodes(), "a submit that did not navigate must leave the blanked set intact") + + shot := run(t, toolBy(t, tools, "screenshot"), map[string]any{}) + assert.True(t, shot.IsError, "screenshot must stay refused while the filled field is still mounted") + assert.Contains(t, shot.Content, "screenshot refused") +} diff --git a/pkg/agent/tool/browser/chrome_test.go b/pkg/agent/tool/browser/chrome_test.go index 9fa70b94..9ffbf6e5 100644 --- a/pkg/agent/tool/browser/chrome_test.go +++ b/pkg/agent/tool/browser/chrome_test.go @@ -189,7 +189,7 @@ func waitForDevtools(t *testing.T, debugPort int) { // shared session for teardown. func (fx *fixture) synthTools(t *testing.T, acceptsImages bool) []agenttool.Tool { t.Helper() - tools, err := Synthesize(fx.rt, fx.gateURL, chromeToken, acceptsImages) + tools, err := Synthesize(fx.rt, fx.gateURL, chromeToken, acceptsImages, nil) require.NoError(t, err) require.NotEmpty(t, tools) if bt, ok := tools[0].(*browserTool); ok { diff --git a/pkg/agent/tool/browser/fill_test.go b/pkg/agent/tool/browser/fill_test.go new file mode 100644 index 00000000..671af350 --- /dev/null +++ b/pkg/agent/tool/browser/fill_test.go @@ -0,0 +1,156 @@ +package browser + +import ( + "context" + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + agenttool "github.com/authzed/openagentprimitives/pkg/agent/tool" + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" +) + +// loginPage serves a form with a text username input and a password input, each +// mirroring its live value into a visible

via oninput — so a test can prove +// the typed value actually reached the page WITHOUT the browser tool echoing it. +func loginPage() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`` + + `

` + + `` + + `` + + `
` + + `

uidle

pidle

` + + ``)) + }) +} + +// browserFillFixture boots the chrome fixture in act mode with a one-entry +// catalog for the fixture site, returning the synthesized tools, the fake +// fetcher (for its call counter), the shared session (for the blanked set), and +// the site base URL. +func browserFillFixture(t *testing.T) (tools []agenttool.Tool, ff *fakeFetcher, sess *Session, siteBase string) { + t.Helper() + fx := startFixture(t, loginPage()) + fx.rt.Spec.Interaction = "act" + fx.rt.IdentityName = "site-identity" + fx.rt.Catalog = []spiceboxv1alpha1.BrowserLoginRef{{ + ID: "login-site", + Label: "Site", + Hosts: []spiceboxv1alpha1.BrowserLoginHost{{Host: "site.test"}}, + }} + ff = &fakeFetcher{} + var err error + tools, err = Synthesize(fx.rt, fx.gateURL, chromeToken, false, ff.fetch()) + require.NoError(t, err) + require.NotEmpty(t, tools) + sess = tools[0].(*browserTool).sess + t.Cleanup(func() { sess.Close() }) + return tools, ff, sess, fx.siteBase +} + +// refByName finds the first [eN] ref whose outline line contains name. +func refByName(t *testing.T, tools []agenttool.Tool, name string) string { + t.Helper() + res := run(t, toolBy(t, tools, "find"), map[string]any{"text": name}) + require.False(t, res.IsError, "find %q: %s", name, res.Content) + for _, line := range strings.Split(res.Content, "\n") { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "[e") { + continue + } + if end := strings.IndexByte(line, ']'); end > 1 { + return line[1:end] + } + } + t.Fatalf("no ref found for %q in:\n%s", name, res.Content) + return "" +} + +func TestTypeFillsCredentialsWithoutLeaking(t *testing.T) { + tools, ff, sess, siteBase := browserFillFixture(t) + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": siteBase + "/"}).IsError) + + userRef := refByName(t, tools, "UsernameField") + passRef := refByName(t, tools, "PasswordField") + + typeTool := toolBy(t, tools, "type") + + // Type the username placeholder into the text input. Capture the RAW args the + // runner loop would record for the transcript, and prove Execute never rewrites + // them to the substituted value (the fill runs strictly inside Execute). + userArgs, err := json.Marshal(map[string]any{"ref": userRef, "text": "{{login.site.test.username}}"}) + require.NoError(t, err) + userSnapshot := append(json.RawMessage(nil), userArgs...) + uRes, err := typeTool.Execute(context.Background(), userArgs, &agenttool.SessionContext{}) + require.NoError(t, err) + assert.False(t, uRes.IsError, "type username: %s", uRes.Content) + assert.NotContains(t, uRes.Content, stubUsername, "the tool result must NOT echo the typed value") + assert.Equal(t, userSnapshot, json.RawMessage(userArgs), "the recorded args must keep the placeholder, unchanged by Execute") + assert.Contains(t, string(userArgs), "{{login.site.test.username}}") + assert.NotContains(t, string(userArgs), stubUsername) + + // Type the password placeholder into the password input. + pRes := run(t, typeTool, map[string]any{"ref": passRef, "text": "{{login.site.test.password}}"}) + assert.False(t, pRes.IsError, "type password: %s", pRes.Content) + assert.NotContains(t, pRes.Content, stubPassword, "the tool result must NOT echo the typed value") + + // Both values actually reached the page (the oninput mirrors prove it). + after := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, after.IsError, "read after type: %s", after.Content) + assert.Contains(t, after.Content, "u:"+stubUsername, "the username value must have reached the page") + assert.Contains(t, after.Content, "p:"+stubPassword, "the password value must have reached the page") + + // One Fetch for the host, despite two placeholders across two calls (the hold). + assert.Equal(t, 1, ff.fetches, "the hold must Fetch once per host") + + // Both fields were recorded for C5 blanking. + assert.Len(t, sess.blankedNodes(), 2, "both typed-into fields must be marked for blanking") +} + +func TestTypeRule3PasswordIntoTextInputFailsClosed(t *testing.T) { + tools, ff, _, siteBase := browserFillFixture(t) + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": siteBase + "/"}).IsError) + + userRef := refByName(t, tools, "UsernameField") // a type=text input + + res := run(t, toolBy(t, tools, "type"), map[string]any{"ref": userRef, "text": "{{login.site.test.password}}"}) + assert.True(t, res.IsError, "a password placeholder into a non-password input must fail") + assert.Contains(t, res.Content, "password input field") + assert.NotContains(t, res.Content, stubPassword) + assert.Equal(t, 0, ff.fetches, "rule 3 must deny before any Fetch") + + after := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, after.IsError) + assert.NotContains(t, after.Content, "u:"+stubPassword, "nothing may have been typed on a refused fill") +} + +// TestTypeNeverMutatesRecordedArgs is the no-Chrome guard on the transcript +// property: loop_dispatch records the RAW tool args before Execute (and only +// stripArgTags touches them — no expansion), so if Execute leaves the raw bytes +// untouched the transcript keeps the {{login...}} placeholder, never the value. +// Pointed at a dead gate so no Chrome is needed; the tool errors, but the point +// is that the recorded bytes are never rewritten. +func TestTypeNeverMutatesRecordedArgs(t *testing.T) { + sess := newSession("http://127.0.0.1:1", "tok") + // Fail the connect instantly: a pre-cancelled allocator context makes + // ensureConnected's chromedp.Run return at once rather than dialing. + sess.newAllocator = func(parent context.Context) (context.Context, context.CancelFunc) { + ctx, cancel := context.WithCancel(parent) + cancel() + return ctx, cancel + } + bt := &browserTool{op: "type", sess: sess, filler: newCredFiller(sess, nil, nil)} + + raw := json.RawMessage(`{"ref":"e3","text":"before {{login.aa.example.password}} after"}`) + snapshot := append(json.RawMessage(nil), raw...) + + _, err := bt.Execute(context.Background(), raw, &agenttool.SessionContext{}) + require.NoError(t, err, "a tool-level failure is an IsError result, not a Go error") + assert.Equal(t, snapshot, raw, "Execute must not rewrite the recorded args") + assert.Contains(t, string(raw), "{{login.aa.example.password}}", "the placeholder must survive in the recorded args") +} diff --git a/pkg/agent/tool/browser/filler.go b/pkg/agent/tool/browser/filler.go new file mode 100644 index 00000000..cfe30793 --- /dev/null +++ b/pkg/agent/tool/browser/filler.go @@ -0,0 +1,226 @@ +package browser + +import ( + "context" + "fmt" + "regexp" + "strings" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/platform/identity/directcred" + "github.com/authzed/openagentprimitives/pkg/tools/browserpolicy" +) + +// filledKinds records which credential field kinds a Fill substituted, so the +// type tool can decide whether a field needs blanking afterward (Phase C Task +// C5). It carries only booleans — never the values. +type filledKinds struct { + username bool + password bool + otp bool +} + +// any reports whether Fill substituted at least one credential value. +func (f filledKinds) any() bool { return f.username || f.password || f.otp } + +// CredentialFetcher resolves one catalog entry's credential, by its opaque +// catalog ID, for IMMEDIATE USE. The production implementation (built in +// internal/cmd/runner) resolves through directcred against the identity's +// backing Secret; tests and the e2e harness supply a fake that returns a stub +// and counts calls, so no test path touches a real vault. The returned +// Credential must never be logged or persisted — see directcred.Credential. +type CredentialFetcher func(ctx context.Context, catalogID string) (directcred.Credential, error) + +// CredentialFiller resolves {{login..username|password|otp}} placeholders +// in a to-be-typed string, FAIL-CLOSED per the three §3.5 rules, substituting +// values that never reach the model, the transcript, or any log (it runs strictly +// inside the type tool's Execute, after the loop has already recorded the raw +// placeholder args). An unresolvable placeholder returns a stable §3.4 error — +// never a silent empty, and never the raw secret in the error text. +type CredentialFiller interface { + // Fill substitutes every login placeholder in text against pageHost, the + // host the browser is actually on. inputIsPassword reports whether the DOM + // element the text will be typed into is a type=password input (rule 3). It + // returns the substituted text and which kinds it filled. + Fill(ctx context.Context, pageHost string, inputIsPassword bool, text string) (string, filledKinds, error) +} + +// placeholderRe matches one credential placeholder: {{login..}}. +// The host group is any run of non-brace characters (hosts never contain braces); +// the field is anchored to the closed set, so the greedy host group backtracks +// to the last "." before a valid field. CanonicalHost rejects a host the regexp +// over-accepts (spaces, empty labels, a scheme), so the grammar stays permissive +// and the three rules do the real validation. +var placeholderRe = regexp.MustCompile(`\{\{login\.([^{}]+)\.(username|password|otp)\}\}`) + +// malformedLoginMarker is the substring that, if it survives in the NON-matched +// gaps of the input, means the model wrote something login-placeholder-SHAPED +// that the strict grammar did not accept (a missing or misspelled field). Typing +// it literally could leak a half-formed credential reference, so Fill fails +// closed on it rather than passing it through. +const malformedLoginMarker = "{{login" + +// credFiller is the default CredentialFiller. It owns the catalog and the Fetch +// seam, and reaches the §5.3 hold through its Session so a host's credential is +// Fetched at most once and later placeholders (including .otp's live code) +// resolve from the hold. +type credFiller struct { + sess *Session + catalog []spiceboxv1alpha1.BrowserLoginRef + fetch CredentialFetcher +} + +var _ CredentialFiller = (*credFiller)(nil) + +// newCredFiller builds the default filler. fetch may be nil (a toolbox with no +// bound identity, or a test that never resolves a placeholder) — in that case +// any placeholder that reaches the Fetch step fails closed rather than panicking. +func newCredFiller(sess *Session, catalog []spiceboxv1alpha1.BrowserLoginRef, fetch CredentialFetcher) *credFiller { + return &credFiller{sess: sess, catalog: catalog, fetch: fetch} +} + +// Fill implements CredentialFiller. See the interface doc for the contract. +func (f *credFiller) Fill(ctx context.Context, pageHost string, inputIsPassword bool, text string) (string, filledKinds, error) { + var kinds filledKinds + matches := placeholderRe.FindAllStringSubmatchIndex(text, -1) + if len(matches) == 0 { + if strings.Contains(text, malformedLoginMarker) { + return "", filledKinds{}, fmt.Errorf("malformed login placeholder; use {{login..username|password|otp}}") + } + return text, kinds, nil + } + + var out strings.Builder + var gaps strings.Builder // the ORIGINAL text between matches, for the malformed-leftover check + last := 0 + for _, m := range matches { + start, end := m[0], m[1] + host := text[m[2]:m[3]] + field := text[m[4]:m[5]] + + value, kind, err := f.resolve(ctx, pageHost, inputIsPassword, host, field) + if err != nil { + // Fail the WHOLE fill closed: never a partially-substituted string, + // never the raw placeholder typed alongside a real secret. + return "", filledKinds{}, err + } + + gap := text[last:start] + out.WriteString(gap) + out.WriteString(value) + gaps.WriteString(gap) + last = end + + switch kind { + case "username": + kinds.username = true + case "password": + kinds.password = true + case "otp": + kinds.otp = true + } + } + tail := text[last:] + out.WriteString(tail) + gaps.WriteString(tail) + + // A login-shaped leftover in the non-matched gaps is a malformed placeholder + // (checked over the ORIGINAL gap text, so a substituted value that happens to + // contain the marker can never trip it). Fail closed. + if strings.Contains(gaps.String(), malformedLoginMarker) { + return "", filledKinds{}, fmt.Errorf("malformed login placeholder; use {{login..username|password|otp}}") + } + return out.String(), kinds, nil +} + +// resolve enforces the three §3.5 rules for one placeholder and returns its +// value and kind, all fail-closed. Rules 1-3 are checked BEFORE any Fetch, so a +// placeholder that violates one never reaches the vault. +func (f *credFiller) resolve(ctx context.Context, pageHost string, inputIsPassword bool, rawHost, field string) (string, string, error) { + host, err := browserpolicy.CanonicalHost(rawHost) + if err != nil { + return "", "", fmt.Errorf("login placeholder host %q is not a valid hostname", rawHost) + } + + // Rule 1: the placeholder host must be the host the page is actually on + // (equality or a subdomain). A placeholder for aa.example typed on a lookalike + // — or on no page at all — matches nothing and resolves to an error, never the + // secret. + if pageHost == "" || !browserpolicy.Match(browserpolicy.Site{Host: host}, pageHost) { + return "", "", fmt.Errorf("login placeholder for %q does not match the current page host; open the site with navigate first", host) + } + + // Rule 2: the host must have a catalog entry in THIS toolbox's identity. + entry, ok := f.catalogEntryFor(host) + if !ok { + return "", "", fmt.Errorf("no stored login for host %q in this browser toolbox", host) + } + + // Rule 3: a password may only land in a type=password input. + if field == "password" && !inputIsPassword { + return "", "", fmt.Errorf("a password can only be typed into a password input field") + } + + cred, err := f.credentialFor(ctx, host, entry.ID) + if err != nil { + // The directcred sources (first-party) do not put credential values in + // their errors; wrapping is safe and keeps the no-silent-errors contract. + return "", "", fmt.Errorf("resolving the stored login for host %q: %w", host, err) + } + + switch field { + case "username": + return cred.Username, "username", nil + case "password": + return cred.Password, "password", nil + case "otp": + if cred.OTP == nil { + return "", "", fmt.Errorf("the stored login for host %q has no one-time code configured", host) + } + code, oerr := cred.OTP() + if oerr != nil { + // Deliberately NOT wrapped: a TOTP/HOTP generation error is about the + // seed or clock, but we refuse to risk echoing anything value-adjacent. + return "", "", fmt.Errorf("could not generate the one-time code for host %q", host) + } + return code, "otp", nil + default: + // Unreachable: the grammar's field group is the closed set above. + return "", "", fmt.Errorf("unknown login field %q", field) + } +} + +// catalogEntryFor returns the catalog entry whose hosts cover host (via the same +// suffix-by-label match the egress policy uses), and whether one was found. +func (f *credFiller) catalogEntryFor(host string) (spiceboxv1alpha1.BrowserLoginRef, bool) { + for _, e := range f.catalog { + for _, h := range e.Hosts { + ch, err := browserpolicy.CanonicalHost(h.Host) + if err != nil { + continue + } + if browserpolicy.Match(browserpolicy.Site{Host: ch, Exact: h.Exact}, host) { + return e, true + } + } + } + return spiceboxv1alpha1.BrowserLoginRef{}, false +} + +// credentialFor returns host's credential from the §5.3 hold, Fetching it (once) +// and placing it in the hold on a miss. The held Credential carries the live OTP +// closure, so repeated .otp placeholders produce fresh codes without re-Fetching. +func (f *credFiller) credentialFor(ctx context.Context, host, catalogID string) (directcred.Credential, error) { + if cred, ok := f.sess.heldCredential(host); ok { + return cred, nil + } + if f.fetch == nil { + return directcred.Credential{}, fmt.Errorf("no credential source is configured for this browser toolbox") + } + cred, err := f.fetch(ctx, catalogID) + if err != nil { + return directcred.Credential{}, err + } + f.sess.putHeldCredential(host, cred) + return cred, nil +} diff --git a/pkg/agent/tool/browser/filler_test.go b/pkg/agent/tool/browser/filler_test.go new file mode 100644 index 00000000..984eaea6 --- /dev/null +++ b/pkg/agent/tool/browser/filler_test.go @@ -0,0 +1,233 @@ +package browser + +import ( + "context" + "fmt" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/platform/identity/directcred" +) + +// timeNowMinus returns a timestamp safely in the past, for planting an expired +// hold entry. +func timeNowMinus(t *testing.T) time.Time { + t.Helper() + return time.Now().Add(-time.Minute) +} + +// The stub values a fake fetcher returns. No test uses a real vault — these +// are the only "secret" bytes any filler test ever handles, so an assertion +// that an error/result does NOT contain them proves nothing leaked. +const ( + stubUsername = "alice@aa.example" + stubPassword = "s3cr3t-pw-value" +) + +// fakeFetcher returns a stub credential and counts calls, so a test can prove +// the §5.3 hold Fetches at most once per host. Each OTP() call returns a FRESH +// code, so a test can prove .otp is resolved live per placeholder. +type fakeFetcher struct { + fetches int + otps int +} + +func (f *fakeFetcher) fetch() CredentialFetcher { + return func(context.Context, string) (directcred.Credential, error) { + f.fetches++ + return directcred.Credential{ + Username: stubUsername, + Password: stubPassword, + OTP: func() (string, error) { + f.otps++ + return fmt.Sprintf("%06d", 100000+f.otps), nil + }, + }, nil + } +} + +// aaCatalog is a one-entry catalog for aa.example (non-exact: subdomains match). +func aaCatalog() []spiceboxv1alpha1.BrowserLoginRef { + return []spiceboxv1alpha1.BrowserLoginRef{{ + ID: "login-aa", + Label: "AA Example", + Hosts: []spiceboxv1alpha1.BrowserLoginHost{{Host: "aa.example"}}, + }} +} + +// newTestFiller builds a real credFiller over a bare Session (its hold is +// lazily initialized) and a fake fetcher. +func newTestFiller(t *testing.T) (*credFiller, *fakeFetcher) { + t.Helper() + ff := &fakeFetcher{} + return newCredFiller(&Session{}, aaCatalog(), ff.fetch()), ff +} + +func TestFillSubstitutesAndFetchesOncePerHost(t *testing.T) { + f, ff := newTestFiller(t) + ctx := context.Background() + + // Two placeholders for the same host in one call → exactly one Fetch. + got, kinds, err := f.Fill(ctx, "aa.example", true, + "u={{login.aa.example.username}} p={{login.aa.example.password}}") + require.NoError(t, err) + assert.Equal(t, "u="+stubUsername+" p="+stubPassword, got) + assert.True(t, kinds.username && kinds.password, "both kinds marked filled") + assert.Equal(t, 1, ff.fetches, "two placeholders for one host must Fetch once") + + // A later Fill for the same host resolves from the hold — still one Fetch. + got2, _, err := f.Fill(ctx, "aa.example", false, "{{login.aa.example.username}}") + require.NoError(t, err) + assert.Equal(t, stubUsername, got2) + assert.Equal(t, 1, ff.fetches, "a second Fill for the held host must not Fetch again") +} + +func TestFillSubtleSubdomainMatchesCatalogEntry(t *testing.T) { + f, ff := newTestFiller(t) + // Page host is a subdomain of the catalog entry's non-exact host. + got, _, err := f.Fill(context.Background(), "login.aa.example", false, "{{login.aa.example.username}}") + require.NoError(t, err) + assert.Equal(t, stubUsername, got) + assert.Equal(t, 1, ff.fetches) +} + +func TestFillRule1OriginMismatchFailsClosed(t *testing.T) { + f, ff := newTestFiller(t) + // The page is a lookalike; the placeholder names aa.example. + got, kinds, err := f.Fill(context.Background(), "aa.example.phish.test", true, + "{{login.aa.example.password}}") + require.Error(t, err, "a placeholder whose host is not the current page host must fail closed") + assert.Empty(t, got, "no substituted text on a failed fill") + assert.False(t, kinds.any()) + assert.Equal(t, 0, ff.fetches, "rule 1 must deny BEFORE any Fetch") + assert.NotContains(t, err.Error(), stubPassword, "the error must never carry the secret") +} + +func TestFillRule1EmptyPageHostFailsClosed(t *testing.T) { + f, _ := newTestFiller(t) + _, _, err := f.Fill(context.Background(), "", true, "{{login.aa.example.password}}") + require.Error(t, err, "no current page (about:blank) must fail closed") +} + +func TestFillRule2HostNotInCatalogFailsClosed(t *testing.T) { + f, ff := newTestFiller(t) + // Page host == placeholder host so rule 1 passes; the host is absent from + // the catalog, so rule 2 must deny. + _, _, err := f.Fill(context.Background(), "bb.example", true, "{{login.bb.example.password}}") + require.Error(t, err, "a host with no catalog entry must fail closed") + assert.Equal(t, 0, ff.fetches, "rule 2 must deny BEFORE any Fetch") + assert.NotContains(t, err.Error(), stubPassword) +} + +func TestFillRule3PasswordIntoNonPasswordInputFailsClosed(t *testing.T) { + f, ff := newTestFiller(t) + // inputIsPassword=false: a password placeholder must be refused. + _, _, err := f.Fill(context.Background(), "aa.example", false, "{{login.aa.example.password}}") + require.Error(t, err, "a password may only land in a type=password input") + assert.Equal(t, 0, ff.fetches, "rule 3 must deny BEFORE any Fetch") + assert.NotContains(t, err.Error(), stubPassword) +} + +func TestFillUsernameIntoNonPasswordInputAllowed(t *testing.T) { + f, _ := newTestFiller(t) + // Rule 3 restricts only .password; a username into a text input is fine. + got, kinds, err := f.Fill(context.Background(), "aa.example", false, "{{login.aa.example.username}}") + require.NoError(t, err) + assert.Equal(t, stubUsername, got) + assert.True(t, kinds.username) + assert.False(t, kinds.password) +} + +func TestFillOTPCallsOTPFuncFreshPerPlaceholder(t *testing.T) { + f, ff := newTestFiller(t) + ctx := context.Background() + + first, kinds, err := f.Fill(ctx, "aa.example", false, "{{login.aa.example.otp}}") + require.NoError(t, err) + assert.True(t, kinds.otp) + assert.Equal(t, 1, ff.otps, "first .otp calls the held OTP func once") + assert.Equal(t, 1, ff.fetches) + + second, _, err := f.Fill(ctx, "aa.example", false, "{{login.aa.example.otp}}") + require.NoError(t, err) + assert.Equal(t, 2, ff.otps, "a later .otp calls the held OTP func again (fresh code)") + assert.Equal(t, 1, ff.fetches, "the OTP func comes from the held credential — no re-Fetch") + assert.NotEqual(t, first, second, "each .otp resolution is a fresh code") +} + +func TestFillOTPWithNoConfiguredCodeFailsClosed(t *testing.T) { + // A credential whose OTP func is nil (no one-time code configured). + fetch := func(context.Context, string) (directcred.Credential, error) { + return directcred.Credential{Username: stubUsername, Password: stubPassword, OTP: nil}, nil + } + f := newCredFiller(&Session{}, aaCatalog(), fetch) + _, _, err := f.Fill(context.Background(), "aa.example", false, "{{login.aa.example.otp}}") + require.Error(t, err, "requesting .otp on a login with no OTP must fail closed") +} + +func TestFillMalformedPlaceholderFailsClosed(t *testing.T) { + f, _ := newTestFiller(t) + for _, text := range []string{ + "{{login.aa.example}}", // no field + "{{login.aa.example.pasword}}", // typo'd field + "before {{login.aa.example.username}} and {{login.bad}}", // one good, one malformed + } { + t.Run(text, func(t *testing.T) { + _, _, err := f.Fill(context.Background(), "aa.example", true, text) + require.Error(t, err, "a malformed login placeholder must fail closed, never be typed literally") + }) + } +} + +func TestFillNoPlaceholdersPassesThrough(t *testing.T) { + f, ff := newTestFiller(t) + got, kinds, err := f.Fill(context.Background(), "aa.example", true, "just some search text") + require.NoError(t, err) + assert.Equal(t, "just some search text", got) + assert.False(t, kinds.any()) + assert.Equal(t, 0, ff.fetches, "no placeholders means no Fetch") +} + +func TestSessionHoldClearedOnClose(t *testing.T) { + s := newSession("http://127.0.0.1:1", "tok") + s.putHeldCredential("aa.example", directcred.Credential{Username: stubUsername}) + s.markBlanked(42) + + _, ok := s.heldCredential("aa.example") + require.True(t, ok, "a just-held credential resolves from the hold") + require.Equal(t, []int64{42}, s.blankedNodes()) + + s.Close() + + _, ok = s.heldCredential("aa.example") + assert.False(t, ok, "Close must clear the credential hold") + assert.Empty(t, s.blankedNodes(), "Close must clear the blanked set") +} + +func TestSessionHoldExpires(t *testing.T) { + s := newSession("http://127.0.0.1:1", "tok") + // Plant an already-expired entry. + s.hold = map[string]heldCred{"aa.example": { + cred: directcred.Credential{Username: stubUsername}, + expiresAt: timeNowMinus(t), + }} + _, ok := s.heldCredential("aa.example") + assert.False(t, ok, "an expired hold entry must not resolve") +} + +func TestFillGrammarParsesMixedText(t *testing.T) { + f, _ := newTestFiller(t) + got, kinds, err := f.Fill(context.Background(), "aa.example", true, + "login: {{login.aa.example.username}} / {{login.aa.example.password}} !") + require.NoError(t, err) + assert.Equal(t, "login: "+stubUsername+" / "+stubPassword+" !", got) + assert.True(t, kinds.username && kinds.password) + // The literal surrounding text survives verbatim. + assert.True(t, strings.HasPrefix(got, "login: ")) + assert.True(t, strings.HasSuffix(got, " !")) +} diff --git a/pkg/agent/tool/browser/policy_precheck_test.go b/pkg/agent/tool/browser/policy_precheck_test.go index 19efb316..078c4a13 100644 --- a/pkg/agent/tool/browser/policy_precheck_test.go +++ b/pkg/agent/tool/browser/policy_precheck_test.go @@ -53,7 +53,7 @@ func toolNames(tools []agenttool.Tool) []string { // must be refused BEFORE any Chrome connection is attempted. func execNavigate(t *testing.T, rt spiceboxv1alpha1.ResolvedBrowserToolbox, url string) agenttool.Result { t.Helper() - tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false) + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) require.NoError(t, err) nav := toolBySuffix(t, tools, "navigate") args, err := json.Marshal(map[string]any{"url": url}) diff --git a/pkg/agent/tool/browser/prompt_test.go b/pkg/agent/tool/browser/prompt_test.go new file mode 100644 index 00000000..dc17ea2c --- /dev/null +++ b/pkg/agent/tool/browser/prompt_test.go @@ -0,0 +1,77 @@ +package browser + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" +) + +func TestPromptSectionNilWithoutIdentity(t *testing.T) { + rt := buildToolboxWithCatalog("", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "blocklist") + assert.Nil(t, PromptSection(rt), "a toolbox with no bound identity offers no query_logins, so it must contribute no fragment") +} + +func TestPromptSectionReflectsCatalogSize(t *testing.T) { + cases := []struct { + name string + catalog []spiceboxv1alpha1.BrowserLoginRef + want string + }{ + {name: "zero logins", catalog: nil, want: "0 stored logins"}, + {name: "one login (singular)", catalog: []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, want: "1 stored login;"}, + {name: "two logins (plural)", catalog: []spiceboxv1alpha1.BrowserLoginRef{aaLogin(), unitedLogin()}, want: "2 stored logins"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", tc.catalog, "blocklist") + sec := PromptSection(rt) + require.NotNil(t, sec) + assert.Contains(t, sec.Body, tc.want) + assert.Contains(t, sec.Body, "query_logins finds them by host or label") + }) + } +} + +func TestPromptSectionAllowlistCountsAllowedHosts(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "allowlist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "aa.com", Source: "explicit"}, + spiceboxv1alpha1.BrowserEffectiveSite{Host: "united.com", Source: "explicit"}, + ) + sec := PromptSection(rt) + require.NotNil(t, sec) + assert.Contains(t, sec.Body, "In allowlist mode, 2 hosts are allowed") + assert.Contains(t, sec.Body, "query_logins also answers whether a given host is one of them") +} + +func TestPromptSectionAllowlistSingularHost(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", nil, "allowlist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "aa.com", Source: "explicit"}, + ) + sec := PromptSection(rt) + require.NotNil(t, sec) + assert.Contains(t, sec.Body, "In allowlist mode, 1 host is allowed") +} + +func TestPromptSectionBlocklistOmitsHostCount(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "blocklist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "blocked.test", Source: "explicit"}, + ) + sec := PromptSection(rt) + require.NotNil(t, sec) + assert.NotContains(t, sec.Body, "allowlist") + assert.NotContains(t, sec.Body, "allowed") +} + +// TestPromptSectionIsOneLine pins the spec's "one line" requirement: no +// embedded newline in the body. +func TestPromptSectionIsOneLine(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "allowlist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "aa.com", Source: "explicit"}, + ) + sec := PromptSection(rt) + require.NotNil(t, sec) + assert.NotContains(t, sec.Body, "\n") +} diff --git a/pkg/agent/tool/browser/query_logins_test.go b/pkg/agent/tool/browser/query_logins_test.go new file mode 100644 index 00000000..1507c084 --- /dev/null +++ b/pkg/agent/tool/browser/query_logins_test.go @@ -0,0 +1,234 @@ +package browser + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + agenttool "github.com/authzed/openagentprimitives/pkg/agent/tool" + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" +) + +// buildToolboxWithCatalog returns a ResolvedBrowserToolbox named "demo" with +// the given identity/catalog/mode/effective-sites, mirroring buildToolbox +// (policy_precheck_test.go) but adding the identity + catalog fields +// query_logins needs. +func buildToolboxWithCatalog(identity string, catalog []spiceboxv1alpha1.BrowserLoginRef, mode string, sites ...spiceboxv1alpha1.BrowserEffectiveSite) spiceboxv1alpha1.ResolvedBrowserToolbox { + rt := buildToolbox(mode, sites...) + rt.IdentityName = identity + rt.Catalog = catalog + return rt +} + +// execQueryLogins synthesizes rt and runs its query_logins tool with the +// given args, returning the Result. No live Chrome connection is ever +// attempted: query_logins reads only rt.Catalog/EffectiveSites, so the gate +// URL is unreachable on purpose, same discipline as execNavigate. +func execQueryLogins(t *testing.T, rt spiceboxv1alpha1.ResolvedBrowserToolbox, args map[string]any) agenttool.Result { + t.Helper() + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) + require.NoError(t, err) + ql := toolBySuffix(t, tools, "query_logins") + raw, err := json.Marshal(args) + require.NoError(t, err) + res, err := ql.Execute(context.Background(), raw, &agenttool.SessionContext{}) + require.NoError(t, err, "Execute must not return a Go error; refusals are tool errors") + return res +} + +func aaLogin() spiceboxv1alpha1.BrowserLoginRef { + return spiceboxv1alpha1.BrowserLoginRef{ + ID: "item-1", + Label: "AA Advantage", + Hosts: []spiceboxv1alpha1.BrowserLoginHost{{Host: "aa.com"}}, + } +} + +func unitedLogin() spiceboxv1alpha1.BrowserLoginRef { + return spiceboxv1alpha1.BrowserLoginRef{ + ID: "item-2", + Label: "United MileagePlus", + Hosts: []spiceboxv1alpha1.BrowserLoginHost{{Host: "united.com"}, {Host: "mileageplus.united.com", Exact: true}}, + } +} + +func TestQueryLoginsOfferedOnlyWithIdentity(t *testing.T) { + cases := []struct { + name string + identity string + want bool + }{ + {name: "no identity: not offered", identity: "", want: false}, + {name: "identity bound: offered", identity: "my-identity", want: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := buildToolboxWithCatalog(tc.identity, []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "blocklist") + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) + require.NoError(t, err) + got := false + for _, tl := range tools { + if tl.Name() == "demo_query_logins" { + got = true + } + } + assert.Equal(t, tc.want, got) + }) + } +} + +// TestQueryLoginsOfferedInBothModes proves the gate is identity-only, +// independent of Interaction (act vs readonly) — unlike the acting tools +// Phase C adds elsewhere, this observation tool is symmetric across modes. +func TestQueryLoginsOfferedInBothModes(t *testing.T) { + for _, mode := range []string{"readonly", "act"} { + t.Run(mode, func(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "blocklist") + rt.Spec.Interaction = mode + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) + require.NoError(t, err) + found := false + for _, tl := range tools { + if tl.Name() == "demo_query_logins" { + found = true + } + } + assert.True(t, found, "query_logins must be offered in %s mode", mode) + }) + } +} + +func TestQueryLoginsFilterMatchesHostOrLabel(t *testing.T) { + catalog := []spiceboxv1alpha1.BrowserLoginRef{aaLogin(), unitedLogin()} + cases := []struct { + name string + filter string + wantLabels []string + }{ + {name: "empty filter returns all", filter: "", wantLabels: []string{"AA Advantage", "United MileagePlus"}}, + {name: "matches host substring, case-insensitive", filter: "AA.COM", wantLabels: []string{"AA Advantage"}}, + {name: "matches a secondary host", filter: "mileageplus", wantLabels: []string{"United MileagePlus"}}, + {name: "matches label substring, case-insensitive", filter: "advantage", wantLabels: []string{"AA Advantage"}}, + {name: "no match", filter: "nonexistent", wantLabels: nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", catalog, "blocklist") + res := execQueryLogins(t, rt, map[string]any{"filter": tc.filter}) + require.False(t, res.IsError, "result: %s", res.Content) + + var entries []queryLoginsEntry + require.NoError(t, json.Unmarshal([]byte(firstJSONLine(t, res.Content)), &entries)) + var gotLabels []string + for _, e := range entries { + gotLabels = append(gotLabels, e.Label) + } + assert.Equal(t, tc.wantLabels, gotLabels) + }) + } +} + +// TestQueryLoginsEntryShapeIsHostsAndLabelOnly pins the output shape to +// exactly {hosts, label} — nothing else about the catalog item, in +// particular never its opaque id. +func TestQueryLoginsEntryShapeIsHostsAndLabelOnly(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "blocklist") + res := execQueryLogins(t, rt, map[string]any{}) + require.False(t, res.IsError) + + var raw []map[string]any + require.NoError(t, json.Unmarshal([]byte(firstJSONLine(t, res.Content)), &raw)) + require.Len(t, raw, 1) + keys := make(map[string]bool) + for k := range raw[0] { + keys[k] = true + } + assert.True(t, keys["hosts"]) + assert.True(t, keys["label"]) + assert.Len(t, raw[0], 2, "entry must carry nothing beyond hosts+label; got keys %v", raw[0]) + assert.NotContains(t, res.Content, "item-1", "the catalog's opaque id must never be exposed") +} + +func TestQueryLoginsPagination(t *testing.T) { + // Build enough entries to span 3 pages at loginsPerPage=20: 45 entries. + var catalog []spiceboxv1alpha1.BrowserLoginRef + for i := 0; i < 45; i++ { + catalog = append(catalog, spiceboxv1alpha1.BrowserLoginRef{ + ID: "item", + Label: "site", + Hosts: []spiceboxv1alpha1.BrowserLoginHost{{Host: "site.test"}}, + }) + } + rt := buildToolboxWithCatalog("my-identity", catalog, "blocklist") + + res1 := execQueryLogins(t, rt, map[string]any{}) + require.False(t, res1.IsError) + assert.Contains(t, res1.Content, "page 1 of 3") + var page1 []queryLoginsEntry + require.NoError(t, json.Unmarshal([]byte(firstJSONLine(t, res1.Content)), &page1)) + assert.Len(t, page1, 20) + + res2 := execQueryLogins(t, rt, map[string]any{"page": 2}) + require.False(t, res2.IsError) + assert.Contains(t, res2.Content, "page 2 of 3") + var page2 []queryLoginsEntry + require.NoError(t, json.Unmarshal([]byte(firstJSONLine(t, res2.Content)), &page2)) + assert.Len(t, page2, 20) + + res3 := execQueryLogins(t, rt, map[string]any{"page": 3}) + require.False(t, res3.IsError) + assert.Contains(t, res3.Content, "page 3 of 3") + var page3 []queryLoginsEntry + require.NoError(t, json.Unmarshal([]byte(firstJSONLine(t, res3.Content)), &page3)) + assert.Len(t, page3, 5) + + res4 := execQueryLogins(t, rt, map[string]any{"page": 4}) + assert.True(t, res4.IsError) + assert.Contains(t, res4.Content, "out of range") +} + +func TestQueryLoginsHostPolicyAnswer(t *testing.T) { + t.Run("allowed host, allowlist mode", func(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", nil, "allowlist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "good.test", Source: "explicit"}) + res := execQueryLogins(t, rt, map[string]any{"host": "good.test"}) + require.False(t, res.IsError) + assert.Contains(t, res.Content, "host good.test is allowed by this toolbox's policy") + }) + t.Run("denied host, allowlist mode: rule 'not in allowlist'", func(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", nil, "allowlist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "good.test", Source: "explicit"}) + res := execQueryLogins(t, rt, map[string]any{"host": "evil.com"}) + require.False(t, res.IsError) + assert.Contains(t, res.Content, "host evil.com is NOT allowed by this toolbox's policy (rule: not in allowlist)") + }) + t.Run("denied host, blocklist mode: rule 'explicit denylist'", func(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", nil, "blocklist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "blocked.test", Source: "explicit"}) + res := execQueryLogins(t, rt, map[string]any{"host": "blocked.test"}) + require.False(t, res.IsError) + assert.Contains(t, res.Content, "host blocked.test is NOT allowed by this toolbox's policy (rule: explicit denylist)") + }) + t.Run("host check and filter compose in one call", func(t *testing.T) { + rt := buildToolboxWithCatalog("my-identity", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "allowlist", + spiceboxv1alpha1.BrowserEffectiveSite{Host: "aa.com", Source: "explicit"}) + res := execQueryLogins(t, rt, map[string]any{"host": "aa.com", "filter": "advantage"}) + require.False(t, res.IsError) + assert.Contains(t, res.Content, "host aa.com is allowed by this toolbox's policy") + assert.Contains(t, res.Content, "AA Advantage") + }) +} + +// firstJSONLine extracts the JSON-array block from a queryLogins result body +// (the block before the trailing "page N of M" footer, and after an optional +// leading host-policy line) — blocks are separated by a blank line. +func firstJSONLine(t *testing.T, content string) string { + t.Helper() + parts := strings.Split(content, "\n\n") + require.GreaterOrEqual(t, len(parts), 2, "expected at least a JSON block and a page footer in %q", content) + return parts[len(parts)-2] +} diff --git a/pkg/agent/tool/browser/session.go b/pkg/agent/tool/browser/session.go index 2d74256d..146ee551 100644 --- a/pkg/agent/tool/browser/session.go +++ b/pkg/agent/tool/browser/session.go @@ -15,11 +15,15 @@ package browser import ( "context" "fmt" + "net/url" "sync" "time" "github.com/chromedp/cdproto/network" "github.com/chromedp/chromedp" + + "github.com/authzed/openagentprimitives/pkg/platform/identity/directcred" + "github.com/authzed/openagentprimitives/pkg/tools/browserpolicy" ) // connectRetryDelay is the pause before the single reconnect attempt. The pod @@ -27,6 +31,13 @@ import ( // a wake race harmless. const connectRetryDelay = 500 * time.Millisecond +// credHoldTTL bounds how long a Fetched website credential lingers in the +// in-memory hold (spec §5.3). A login flow (type username, type password, +// type OTP, submit) completes in seconds; a few minutes covers a slow page +// without keeping the plaintext around for the session's lifetime. The hold is +// ALSO dropped on Close, so this is only the upper bound for an idle session. +const credHoldTTL = 3 * time.Minute + // Session owns the lazily-established chromedp connection to one per-session // browser, reached through the gate. It is shared by every tool of a toolbox // and is safe for concurrent use: all browser operations serialize under mu, so @@ -40,6 +51,13 @@ type Session struct { // for tests; production always dials the gate's /json/version with the token. newAllocator func(parent context.Context) (context.Context, context.CancelFunc) + // currentHost reads the canonical host of the live page. Overridable for + // tests (production reads chromedp.Location through the gate and + // canonicalizes it); newSession wires the live implementation. It backs + // CurrentHost, which the acting tools' web_host authorization check reads as + // the per-call resource instance. + currentHost func() string + mu sync.Mutex rootCtx context.Context rootStop context.CancelFunc @@ -58,6 +76,29 @@ type Session struct { netMu sync.Mutex reqURLs map[network.RequestID]string // per-navigation request URLs blockedURL string // the last URL a gate/proxy denial failed on + + // credMu guards the credential hold and the blanked-field set below. It is + // SEPARATE from mu: the filler reads/writes the hold OUTSIDE do() (the Fetch + // is network I/O that must not hold the single browser lock), so sharing mu + // would serialize a vault round-trip against every browser op. Close acquires + // mu then credMu; nothing acquires them in the other order, so there is no + // deadlock. + credMu sync.Mutex + // hold is the short-lived, time-bounded cache of Fetched website credentials + // (spec §5.3), keyed by canonical host. A value here is for IMMEDIATE USE + // ONLY — never logged, never persisted, dropped on expiry and on Close. + hold map[string]heldCred + // blanked is the set of DOM backend node IDs a credential was typed into, + // recorded by the type tool for Phase C Task C5's blanking pass to consume. + blanked map[int64]bool +} + +// heldCred is one Fetched credential kept briefly in the §5.3 hold so repeated +// placeholders for a host (username, password, otp, …) resolve from a single +// Fetch. Carries an expiry so a stale entry is never reused. +type heldCred struct { + cred directcred.Credential + expiresAt time.Time } // refEntry is what a per-snapshot "e" ref resolves to: the DOM backend node @@ -72,7 +113,13 @@ type refEntry struct { // "http://:"; the connection is not made until the // first tool call. func newSession(gateURL, token string) *Session { - s := &Session{gateURL: gateURL, token: token, refs: map[string]refEntry{}} + s := &Session{ + gateURL: gateURL, + token: token, + refs: map[string]refEntry{}, + hold: map[string]heldCred{}, + blanked: map[int64]bool{}, + } s.newAllocator = func(parent context.Context) (context.Context, context.CancelFunc) { // The gate rewrites /json/version's webSocketDebuggerUrl back to itself // WITH the token, so chromedp stays on the authenticated gate rather than @@ -80,9 +127,41 @@ func newSession(gateURL, token string) *Session { // query, so the token reaches the gate's auth check. return chromedp.NewRemoteAllocator(parent, s.gateURL+"/json/version?t="+s.token) } + s.currentHost = s.liveCurrentHost return s } +// CurrentHost returns the canonical host (browserpolicy.CanonicalHost) of the +// current page, or "" when there is no live page (about:blank), the location +// cannot be read, or the host fails canonicalization. +// +// Empty is the fail-closed answer by design: an acting tool exposes this as the +// web_host resource instance its External check resolves against, and an empty +// id resolves to an UnresolvedResource denial — routed to a human, never a +// silent allow. +func (s *Session) CurrentHost() string { return s.currentHost() } + +// liveCurrentHost reads chromedp.Location through the gate and canonicalizes its +// host. Every failure collapses to "" (fail-closed); nothing is surfaced to the +// caller, because an acting call with no resolvable host must deny, not error. +func (s *Session) liveCurrentHost() string { + var loc string + if err := s.do(context.Background(), shortTimeout, func(c context.Context) error { + return runAction(c, func(ac context.Context) error { return chromedp.Location(&loc).Do(ac) }) + }); err != nil { + return "" + } + u, err := url.Parse(loc) + if err != nil || u.Hostname() == "" { + return "" + } + host, err := browserpolicy.CanonicalHost(u.Hostname()) + if err != nil { + return "" + } + return host +} + // ensureConnected establishes the chromedp connection on first use, with one // retry. It must be called with s.mu held. Every failure is returned with // context; nothing is swallowed. @@ -216,8 +295,105 @@ func (s *Session) lookupRef(ref string) (refEntry, bool) { return e, ok } -// Close tears down the browser connection and its goroutines. Safe to call more -// than once. After Close a subsequent tool call reconnects lazily. +// heldCredential returns the hold's entry for host, and whether it is present +// and unexpired. An expired entry is dropped and reported absent. Safe for +// concurrent use; takes credMu, never mu. +func (s *Session) heldCredential(host string) (directcred.Credential, bool) { + s.credMu.Lock() + defer s.credMu.Unlock() + hc, ok := s.hold[host] + if !ok { + return directcred.Credential{}, false + } + if time.Now().After(hc.expiresAt) { + delete(s.hold, host) + return directcred.Credential{}, false + } + return hc.cred, true +} + +// putHeldCredential places cred in the §5.3 hold under host, time-bounded by +// credHoldTTL. The value is for immediate use only and is dropped on expiry and +// on Close — it is never logged or persisted. +func (s *Session) putHeldCredential(host string, cred directcred.Credential) { + s.credMu.Lock() + defer s.credMu.Unlock() + if s.hold == nil { + s.hold = map[string]heldCred{} + } + s.hold[host] = heldCred{cred: cred, expiresAt: time.Now().Add(credHoldTTL)} +} + +// markBlanked records that a credential was typed into backendNodeID, so Phase +// C Task C5's blanking pass can clear the field's value afterward. +func (s *Session) markBlanked(backendNodeID int64) { + s.credMu.Lock() + defer s.credMu.Unlock() + if s.blanked == nil { + s.blanked = map[int64]bool{} + } + s.blanked[backendNodeID] = true +} + +// blankedNodes returns a snapshot of the backend node IDs a credential was +// typed into (for C5). The returned slice is a copy; the caller may mutate it. +func (s *Session) blankedNodes() []int64 { + s.credMu.Lock() + defer s.credMu.Unlock() + out := make([]int64, 0, len(s.blanked)) + for id := range s.blanked { + out = append(out, id) + } + return out +} + +// blankedSet returns a copy of the blanked backend-node-ID set, for +// buildSnapshot to redact by backendNodeID (spec §3.6). A copy, not the live +// map, so the snapshot build never races a concurrent markBlanked/clearBlanked. +func (s *Session) blankedSet() map[int64]bool { + s.credMu.Lock() + defer s.credMu.Unlock() + out := make(map[int64]bool, len(s.blanked)) + for id := range s.blanked { + out[id] = true + } + return out +} + +// anyBlanked reports whether at least one field is currently blanked — a +// credential placeholder was typed into it and the page has not navigated +// since. screenshot refuses while this is true (spec §3.6). +func (s *Session) anyBlanked() bool { + s.credMu.Lock() + defer s.credMu.Unlock() + return len(s.blanked) > 0 +} + +// clearBlanked drops the whole blanked set: every element it named was on a +// document that is now gone. Called alongside clearRefs ONLY on a genuine +// document replacement — navigate / back / a successful follow (which is a +// navigate) — because that is the one event that reliably detaches every prior +// backend node. It is deliberately the ONLY thing that un-blanks mid-session: +// +// - submit does NOT call it: a submit may be intercepted by the page (an SPA +// onsubmit that pushState's a new route while keeping the password input +// mounted), so it is not a reliable "the fields are gone" signal. +// - observation (read/find) does NOT prune by AX-tree presence either: +// Chromium omits a display:none subtree from the accessibility tree, so a +// field hidden by a tab/accordion/"show password" toggle — still in the DOM +// with its typed value — would look gone and be re-exposed verbatim on +// re-show. Over-blanking a field that really was removed mid-document (it +// stays blanked until the next nav) is a harmless usability wart; this is +// strictly fail-closed. +func (s *Session) clearBlanked() { + s.credMu.Lock() + defer s.credMu.Unlock() + s.blanked = map[int64]bool{} +} + +// Close tears down the browser connection and its goroutines, and clears the +// credential hold and blanked-field set. Safe to call more than once. After +// Close a subsequent tool call reconnects lazily. func (s *Session) Close() { s.mu.Lock() defer s.mu.Unlock() @@ -235,4 +411,12 @@ func (s *Session) Close() { } s.rootCtx = nil s.connected = false + + // Drop every held credential and the blanked set: a closed session must + // leave no plaintext secret in memory. credMu is taken after mu here; no + // other path takes them in the opposite order. + s.credMu.Lock() + s.hold = map[string]heldCred{} + s.blanked = map[int64]bool{} + s.credMu.Unlock() } diff --git a/pkg/agent/tool/browser/snapshot.go b/pkg/agent/tool/browser/snapshot.go index c995547a..8bdbeeb5 100644 --- a/pkg/agent/tool/browser/snapshot.go +++ b/pkg/agent/tool/browser/snapshot.go @@ -11,21 +11,39 @@ import ( ) // snapNode is one rendered line of an accessibility outline: a per-snapshot ref, -// the display role, the accessible name, the indentation depth (by rendered -// ancestors), and the DOM backend node the ref resolves to. +// the display role, the accessible name, the element's current value (form +// controls only; "" when it has none), whether that value is blanked (spec +// §3.6 — a credential placeholder was typed into it and the page has not +// navigated since), the indentation depth (by rendered ancestors), and the DOM +// backend node the ref resolves to. type snapNode struct { ref string role string name string + value string + blanked bool depth int backendNodeID int64 } // snapshot fetches a fresh full accessibility tree and builds the ordered // interesting-node list plus the per-snapshot ref map. startBackendID scopes the -// walk to that node's subtree (0 = whole document). ctx must be bound to the -// chromedp browser context (i.e. called from within Session.do). -func snapshot(ctx context.Context, startBackendID int64) ([]snapNode, map[string]refEntry, error) { +// walk to that node's subtree (0 = whole document). blanked is the session's +// current blanked-backend-node-ID set (spec §3.6); a node in it (and its whole +// subtree) has its value redacted rather than rendered, by backendNodeID, so +// the redaction survives any re-snapshot until the page genuinely navigates +// (navigate/back/follow clear the set; see Session.clearBlanked). ctx must be +// bound to the chromedp browser context (i.e. called from within Session.do). +// +// The blanked set is NOT pruned by AX-tree presence: Chromium OMITS a +// display:none subtree from the accessibility tree entirely, so a filled field +// hidden by a tab/accordion/"show password" toggle — still in the DOM, still +// carrying its typed value — would look "gone" to the AX tree and, if pruned, +// re-expose its value verbatim when re-shown without retyping. Blanking +// therefore persists until a real navigation, which is strictly fail-closed +// (at worst a field removed mid-document stays over-blanked until the next nav, +// a usability wart, never a leak). +func snapshot(ctx context.Context, startBackendID int64, blanked map[int64]bool) ([]snapNode, map[string]refEntry, error) { var nodes []*accessibility.Node err := runAction(ctx, func(c context.Context) error { var e error @@ -35,15 +53,19 @@ func snapshot(ctx context.Context, startBackendID int64) ([]snapNode, map[string if err != nil { return nil, nil, fmt.Errorf("reading accessibility tree: %w", err) } - lines, refs := buildSnapshot(nodes, startBackendID) + lines, refs := buildSnapshot(nodes, startBackendID, blanked) return lines, refs, nil } // buildSnapshot is the pure half of snapshot: it turns a flat AX node list into // the rendered-node list and ref map. Refs are assigned in document pre-order // over interesting, non-ignored nodes. It never panics on a nil node, a missing -// child id, or a detached subtree. -func buildSnapshot(nodes []*accessibility.Node, startBackendID int64) ([]snapNode, map[string]refEntry) { +// child id, or a detached subtree. A node whose backend ID is in blanked never +// has its value populated, and its whole subtree is suppressed — the redaction +// happens here, once, rather than at render time, so every consumer of the +// returned []snapNode (read's outline, find's matched lines) is blind to it by +// construction. +func buildSnapshot(nodes []*accessibility.Node, startBackendID int64, blanked map[int64]bool) ([]snapNode, map[string]refEntry) { byID := make(map[accessibility.NodeID]*accessibility.Node, len(nodes)) for _, n := range nodes { if n != nil { @@ -73,36 +95,66 @@ func buildSnapshot(nodes []*accessibility.Node, startBackendID int64) ([]snapNod seen := make(map[accessibility.NodeID]bool, len(nodes)) counter := 0 - var walk func(n *accessibility.Node, depth int) - walk = func(n *accessibility.Node, depth int) { + // ancestorBlanked carries down the recursion: Chrome represents a text + // input's live rendered content as a CHILD static-text AX node (its own + // backendNodeID, distinct from the input's), the same way it represents any + // other visible text on the page — it is not a quirk of form controls + // specifically, it is how the generic "visible text becomes a text/ + // statictext AX node" rendering behaves for an input's displayed value. + // That child is the actual vector a typed credential surfaces through: the + // input's OWN node rarely carries the text in its "value" property, but its + // child does, in its NAME. So blanking by backendNodeID must suppress the + // WHOLE subtree under a blanked node, not just that one node's value, or + // the exact bytes redacted on the parent line reappear one line below it. + var walk func(n *accessibility.Node, depth int, ancestorBlanked bool) + walk = func(n *accessibility.Node, depth int, ancestorBlanked bool) { if n == nil || seen[n.NodeID] { return // nil child id or a cycle: never panic, never loop forever } seen[n.NodeID] = true + backendID := int64(n.BackendDOMNodeID) + selfBlanked := blanked[backendID] + effectiveBlanked := ancestorBlanked || selfBlanked + childDepth := depth if !n.Ignored { if disp, ok := interestingRole(axRole(n)); ok { - counter++ - ref := "e" + strconv.Itoa(counter) - name := axName(n) - lines = append(lines, snapNode{ - ref: ref, - role: disp, - name: name, - depth: depth, - backendNodeID: int64(n.BackendDOMNodeID), - }) - refs[ref] = refEntry{backendNodeID: int64(n.BackendDOMNodeID), role: disp} + if !ancestorBlanked { + // Either an ordinary node, or the exact element a placeholder was + // typed into (selfBlanked) — render it; render it here, but with + // its value redacted rather than shown when selfBlanked. + counter++ + ref := "e" + strconv.Itoa(counter) + name := axName(n) + var value string + if !selfBlanked { + value = axValueString(n.Value) + } + lines = append(lines, snapNode{ + ref: ref, + role: disp, + name: name, + value: value, + blanked: selfBlanked, + depth: depth, + backendNodeID: backendID, + }) + refs[ref] = refEntry{backendNodeID: backendID, role: disp} + } + // else: a descendant of a blanked element — e.g. the rendered-value + // text child described above — never gets a ref or a line. The + // typed value is redacted on the blanked ancestor's own line; it must + // never reappear via a descendant's name. childDepth = depth + 1 } } for _, cid := range n.ChildIDs { - walk(byID[cid], childDepth) + walk(byID[cid], childDepth, effectiveBlanked) } } for _, r := range roots { - walk(r, 0) + walk(r, 0, false) } return lines, refs } @@ -110,15 +162,35 @@ func buildSnapshot(nodes []*accessibility.Node, startBackendID int64) ([]snapNod // renderOutline renders the snapshot as an indented outline, one node per line: // // [e3] link "Book a flight" +// [e4] textbox "Username" value="alice" +// [e5] textbox "Password" value=[redacted: filled via a credential placeholder] func renderOutline(lines []snapNode) string { var b strings.Builder for _, ln := range lines { b.WriteString(strings.Repeat(" ", ln.depth)) - fmt.Fprintf(&b, "[%s] %s %q\n", ln.ref, ln.role, ln.name) + b.WriteString(renderLine(ln)) + b.WriteByte('\n') } return strings.TrimRight(b.String(), "\n") } +// renderLine renders one node's "[ref] role "name"" line, appending its value +// when it has one. A blanked node (spec §3.6 — a credential placeholder was +// typed into it and the page has not navigated since) never has its value +// shown: the element itself still appears (ref, role, name), but the value +// is replaced with a fixed redaction marker, used by BOTH read's outline and +// find's matched lines, so neither can reveal what was typed. +func renderLine(ln snapNode) string { + line := fmt.Sprintf("[%s] %s %q", ln.ref, ln.role, ln.name) + switch { + case ln.blanked: + line += " value=[redacted: filled via a credential placeholder]" + case ln.value != "": + line += fmt.Sprintf(" value=%q", ln.value) + } + return line +} + // interestingRole maps an AX role to its display form, reporting whether the // role is one the outline renders. Only "interesting" roles surface; everything // else is traversed for its children but not rendered. diff --git a/pkg/agent/tool/browser/snapshot_test.go b/pkg/agent/tool/browser/snapshot_test.go new file mode 100644 index 00000000..55bbca9e --- /dev/null +++ b/pkg/agent/tool/browser/snapshot_test.go @@ -0,0 +1,94 @@ +package browser + +import ( + stdjson "encoding/json" + "strings" + "testing" + + "github.com/chromedp/cdproto/accessibility" + "github.com/chromedp/cdproto/cdp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// axVal builds an AXValue carrying the JSON-encoded string axValueString decodes +// — the shape GetFullAXTree returns a role/name/value in. +func axVal(s string) *accessibility.Value { + q, _ := stdjson.Marshal(s) + return &accessibility.Value{Type: accessibility.ValueTypeString, Value: q} +} + +// blankingTree is the minimal shape the §3.6 vector lives in: a form-control +// node (the input) whose live rendered content Chrome exposes as a CHILD +// static-text node carrying the actual typed value in its NAME. The input's own +// node also carries a value, so a correct suppression hides BOTH. Backend IDs: +// root=100, input=200, value-child=300. +func blankingTree() []*accessibility.Node { + return []*accessibility.Node{ + { + NodeID: "1", + Role: axVal("rootwebarea"), + BackendDOMNodeID: cdp.BackendNodeID(100), + ChildIDs: []accessibility.NodeID{"2"}, + }, + { + NodeID: "2", + Role: axVal("textbox"), + Name: axVal("Password"), + Value: axVal("ownsecret"), + ParentID: "1", + BackendDOMNodeID: cdp.BackendNodeID(200), + ChildIDs: []accessibility.NodeID{"3"}, + }, + { + NodeID: "3", + Role: axVal("statictext"), + Name: axVal("childsecret"), + ParentID: "2", + BackendDOMNodeID: cdp.BackendNodeID(300), + }, + } +} + +// TestBuildSnapshotSuppressesBlankedSubtree locks the suppression MECHANISM +// (not just the plaintext-absent outcome): when the input's backend ID is +// blanked, buildSnapshot must drop the entire subtree under it — so the +// value-carrying child node produces NO line at all. This still guards if a +// future Chrome moves the rendered value onto the input's own node, and the +// converse: it must not over-suppress when nothing is blanked. +func TestBuildSnapshotSuppressesBlankedSubtree(t *testing.T) { + nodes := blankingTree() + + // Blanked: the child's value-bearing line is gone; the input line stays but + // redacted; neither secret appears. + lines, refs := buildSnapshot(nodes, 0, map[int64]bool{200: true}) + out := renderOutline(lines) + assert.NotContains(t, out, "childsecret", "a blanked node's value-child must be suppressed entirely") + assert.NotContains(t, out, "ownsecret", "a blanked node's own value must be redacted") + assert.Contains(t, out, "redacted", "the blanked node must still render, carrying the redaction marker") + assert.Contains(t, out, "Password", "the element itself (role/name) still appears") + + require.Len(t, lines, 2, "root + the (redacted) input only — the value-child line is gone") + assert.True(t, lines[1].blanked) + assert.Empty(t, lines[1].value, "a blanked node carries no value") + _, hasChildRef := refs["e3"] + assert.False(t, hasChildRef, "a suppressed descendant gets no ref") + + // Not blanked: everything renders, both values present. + lines2, _ := buildSnapshot(nodes, 0, map[int64]bool{}) + out2 := renderOutline(lines2) + assert.Contains(t, out2, "ownsecret") + assert.Contains(t, out2, "childsecret") + assert.Len(t, lines2, 3, "root + input + value-child all render when nothing is blanked") +} + +// TestRenderLineValueFormatting documents the three render forms on one line. +func TestRenderLineValueFormatting(t *testing.T) { + assert.Equal(t, `[e1] link "Home"`, renderLine(snapNode{ref: "e1", role: "link", name: "Home"})) + assert.Equal(t, `[e2] textbox "User" value="alice"`, + renderLine(snapNode{ref: "e2", role: "textbox", name: "User", value: "alice"})) + blanked := renderLine(snapNode{ref: "e3", role: "textbox", name: "Pass", value: "ignored", blanked: true}) + assert.Contains(t, blanked, "redacted") + assert.NotContains(t, blanked, "ignored", "a blanked line never shows the value, even if one is set") + assert.True(t, strings.HasPrefix(blanked, `[e3] textbox "Pass"`)) +} diff --git a/pkg/agent/tool/browser/synthesize.go b/pkg/agent/tool/browser/synthesize.go index ca4c157f..7d5693e0 100644 --- a/pkg/agent/tool/browser/synthesize.go +++ b/pkg/agent/tool/browser/synthesize.go @@ -2,8 +2,10 @@ package browser import ( "encoding/json" + "fmt" agenttool "github.com/authzed/openagentprimitives/pkg/agent/tool" + "github.com/authzed/openagentprimitives/pkg/agent/tool/meta/capability" "github.com/authzed/openagentprimitives/pkg/agent/tool/originfmt" "github.com/authzed/openagentprimitives/pkg/agent/tool/synthesize" spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" @@ -57,6 +59,43 @@ var observationOps = []opSpec{ }, } +// actingOps are the Phase C acting tools (spec §3.2), offered ONLY when the +// toolbox's Interaction is "act". Each reaches and mutates the live page, so each +// authorizes External on the web_host slot for the current page host (see +// browserTool.Permission). +var actingOps = []opSpec{ + { + op: "type", + description: "Type text into the input named by a ref from a prior read (focuses it first). " + + "text may embed a stored login WITHOUT you ever seeing its value, using placeholders: " + + "{{login..username}}, {{login..password}}, {{login..otp}} — where is " + + "a login's host from query_logins and must be the host of the page you are currently on. " + + "A {{login...password}} placeholder may only be typed into a password field. Refs from before " + + "a type remain valid for the next field, so you can fill a username, password and submit from one read.", + schema: `{"type":"object","properties":{"ref":{"type":"string","description":"A ref (e.g. e3) from a prior read identifying the input to type into."},"text":{"type":"string","description":"The text to type. May contain {{login..username|password|otp}} placeholders resolved against the current page's stored login."}},"required":["ref","text"]}`, + }, + { + op: "click", + description: "Click the element named by a ref from a prior read (a button, link, checkbox, etc.), firing its handlers and any form submission. Acts on the live page; refs from before the click are stale afterward — read again.", + schema: `{"type":"object","properties":{"ref":{"type":"string","description":"A ref (e.g. e3) from a prior read identifying the element to click."}},"required":["ref"]}`, + }, + { + op: "press", + description: "Press one key on the page. Use a single character to type it, or a named key: Enter, Tab, Escape, Backspace, Delete, ArrowUp, ArrowDown, ArrowLeft, ArrowRight, Home, End, PageUp, PageDown, Space.", + schema: `{"type":"object","properties":{"key":{"type":"string","description":"A single character, or a named key such as Enter or Tab."}},"required":["key"]}`, + }, + { + op: "select", + description: "Choose an option in a ."},"option":{"type":"string","description":"The option to choose, by its value, label, or visible text."}},"required":["ref","option"]}`, + }, + { + op: "submit", + description: "Submit the form that the element named by a ref belongs to (its own form, or the nearest enclosing one), running the form's submit handler. Refs from before the submit are stale afterward.", + schema: `{"type":"object","properties":{"ref":{"type":"string","description":"A ref (e.g. e3) from a prior read for an element inside the form to submit."}},"required":["ref"]}`, + }, +} + // screenshotOp is offered only when the model accepts images. var screenshotOp = opSpec{ op: "screenshot", @@ -64,12 +103,29 @@ var screenshotOp = opSpec{ schema: `{"type":"object","properties":{"ref":{"type":"string","description":"A ref to clip the screenshot to."}}}`, } +// queryLoginsOp is offered only when the toolbox binds an identity (spec +// §3.2) — in BOTH act and readonly modes, independent of the +// acceptsImages/Interaction gates above. It is the one tool that reads the +// identity's login catalog and the toolbox's egress policy together: a model +// planning a login finds which hosts it can sign into, and separately checks +// whether an arbitrary host is in policy, without a round-trip through +// navigate just to find out. +var queryLoginsOp = opSpec{ + op: "query_logins", + description: "List the bound identity's stored logins. filter (optional) is matched case-insensitively against each login's hosts and label; omit it to list everything. Each result is {hosts, label} — nothing else about the login is exposed. Paginated. Pass host (optional) to additionally learn whether that hostname is within this toolbox's egress policy, independent of filter.", + schema: `{"type":"object","properties":{"filter":{"type":"string","description":"Case-insensitive substring matched against each login's hosts and label."},"host":{"type":"string","description":"A hostname to check against the toolbox's egress policy; the result states whether it is allowed."},"page":{"type":"integer","description":"1-based page number (default 1)."}}}`, +} + // Synthesize returns the observation tools for one resolved toolbox. // gateURL is "http://:"; token is the per-session -// browser token; acceptsImages gates the screenshot tool (spec §3.2). All -// returned tools share one chromedp session (lazily connected on first use) and -// one origin. -func Synthesize(rt spiceboxv1alpha1.ResolvedBrowserToolbox, gateURL, token string, acceptsImages bool) ([]agenttool.Tool, error) { +// browser token; acceptsImages gates the screenshot tool (spec §3.2). fetch is +// the credential-filler's Fetch seam: the production runner builds it from the +// bound identity's directcred source (so a {{login...}} placeholder resolves to +// a real website login at use time), tests and the e2e harness pass a fake, and +// a toolbox with no bound identity passes nil (its catalog is empty, so no +// placeholder can resolve anyway). All returned tools share one chromedp session +// (lazily connected on first use), one origin, and one credential filler. +func Synthesize(rt spiceboxv1alpha1.ResolvedBrowserToolbox, gateURL, token string, acceptsImages bool, fetch CredentialFetcher) ([]agenttool.Tool, error) { sites := make([]browserpolicy.Site, 0, len(rt.EffectiveSites)) for _, s := range rt.EffectiveSites { sites = append(sites, browserpolicy.Site{Host: s.Host, Exact: s.Exact}) @@ -77,19 +133,28 @@ func Synthesize(rt spiceboxv1alpha1.ResolvedBrowserToolbox, gateURL, token strin sess := newSession(gateURL, token) origin := originfmt.ForBrowser(rt) + // One filler per toolbox, sharing the session's §5.3 hold. Built here, after + // the session, because the default filler reaches the hold through it. + filler := newCredFiller(sess, rt.Catalog, fetch) ops := make([]opSpec, len(observationOps)) copy(ops, observationOps) if acceptsImages { ops = append(ops, screenshotOp) } + // Offered iff the toolbox binds an identity — independent of + // acceptsImages and of the act/readonly Interaction gate below. A toolbox + // with no identity has no catalog, so there is nothing for this tool to + // find. + if rt.IdentityName != "" { + ops = append(ops, queryLoginsOp) + } - // Phase A offers the identical observation set regardless of Interaction. - // Acting tools (click/type/etc.) arrive in Phase C; the act branch exists - // so the shape is explicit, but today it returns the same set. + // Acting tools (click/press/select/submit) are offered ONLY in act mode; + // readonly offers the observation set alone. Each acting tool authorizes + // External on the web_host slot for the live page host (browserTool.Permission). if rt.Spec.Interaction == "act" { - // Phase C: append acting tools here. For now, same observation set. - _ = rt.Spec.Interaction + ops = append(ops, actingOps...) } tools := make([]agenttool.Tool, 0, len(ops)) @@ -104,7 +169,54 @@ func Synthesize(rt spiceboxv1alpha1.ResolvedBrowserToolbox, gateURL, token strin rtName: rt.Name, mode: rt.Spec.Access.Mode, sites: sites, + catalog: rt.Catalog, + filler: filler, }) } return tools, nil } + +// PromptSection returns the toolbox's one-line system-prompt fragment (spec +// §5.1), or nil when the toolbox binds no identity — mirroring +// queryLoginsOp's own offered-iff-identity gate, so the prompt never +// advertises a tool the tool list does not actually contain. +// +// The browser toolbox is hand-wired directly into internal/cmd/runner's tool +// list, not through the capability.Capability/SectionOfferer registry (see +// pkg/agent/tool/sidecartoolbox's doc comment for why execution-side toolbox +// synthesizers are wired this way); this still returns capability.PromptSection +// so the one caller renders it through the exact same "## Title\n\nBody" +// path every other capability-contributed section uses, and so a future move +// of the browser toolbox onto the capability registry costs nothing here. +// +// unattended is part of the catalog shape the spec table describes +// (§3.2), but it is a property of a SITE (BrowserSite.Unattended, added by +// Phase C Task C8), not of a BrowserLoginRef — and BrowserEffectiveSite +// carries no Unattended field yet either, so there is no data to derive a +// per-login unattended flag from today. This fragment states counts only and +// omits any mention of unattended; once C8 threads Unattended onto +// BrowserEffectiveSite, this is the place to add the per-entry/per-host +// derivation. +func PromptSection(rt spiceboxv1alpha1.ResolvedBrowserToolbox) *capability.PromptSection { + if rt.IdentityName == "" { + return nil + } + n := len(rt.Catalog) + logins := "logins" + if n == 1 { + logins = "login" + } + line := fmt.Sprintf("%s has %d stored %s; query_logins finds them by host or label.", rt.Name, n, logins) + if rt.Spec.Access.Mode == browserpolicy.ModeAllowlist { + h := len(rt.EffectiveSites) + hosts, verb := "hosts", "are" + if h == 1 { + hosts, verb = "host", "is" + } + line += fmt.Sprintf(" In allowlist mode, %d %s %s allowed; query_logins also answers whether a given host is one of them.", h, hosts, verb) + } + return &capability.PromptSection{ + Title: fmt.Sprintf("Browser toolbox: %s", rt.Name), + Body: line, + } +} diff --git a/pkg/agent/tool/browser/tools.go b/pkg/agent/tool/browser/tools.go index 8a3e2451..1db3b8a0 100644 --- a/pkg/agent/tool/browser/tools.go +++ b/pkg/agent/tool/browser/tools.go @@ -7,15 +7,21 @@ import ( "net/url" "strings" "time" + "unicode/utf8" "github.com/chromedp/cdproto/cdp" "github.com/chromedp/cdproto/dom" + "github.com/chromedp/cdproto/input" "github.com/chromedp/cdproto/page" + "github.com/chromedp/cdproto/runtime" "github.com/chromedp/chromedp" + "github.com/chromedp/chromedp/kb" agenttool "github.com/authzed/openagentprimitives/pkg/agent/tool" + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" "github.com/authzed/openagentprimitives/pkg/authz" "github.com/authzed/openagentprimitives/pkg/tools/browserpolicy" + "github.com/authzed/openagentprimitives/pkg/tools/browserschema" ) const ( @@ -33,8 +39,56 @@ const ( waitMinTimeout = 1 * time.Second waitMaxTimeout = 30 * time.Second waitDefaultTimeout = 30 * time.Second + + // hostArgKey is the named-argument key the acting tools surface the current + // page host under (via NamedArgs) and that the web_host check's + // ResourceIDTemplate reads it back from. One constant keeps the two halves — + // what the tool writes and what the check reads — from drifting apart. + hostArgKey = "host" ) +// actingOpNames is the set of ops that reach and mutate external page state +// (spec §3.2). It is the single source of truth for which ops are "acting": +// synthesize.go offers them only in interaction=act, Permission() gates them +// External on the web_host slot, and NamedArgs surfaces the page host for them. +// Every other op is an observation (Passthrough). +var actingOpNames = map[string]bool{ + "type": true, + "click": true, + "press": true, + "select": true, + "submit": true, +} + +// isActingOp reports whether op reaches external page state and therefore +// authorizes External against the current page host. +func isActingOp(op string) bool { return actingOpNames[op] } + +// namedKeys maps the key names an acting press call may use to the single +// control rune chromedp.KeyEvent encodes into the right DevTools key event. +// KeyEvent otherwise types its argument CHARACTER BY CHARACTER, so without this +// map press("Enter") would type the five letters E-n-t-e-r rather than pressing +// Return. +var namedKeys = map[string]string{ + "Enter": kb.Enter, + "Return": kb.Enter, + "Tab": kb.Tab, + "Escape": kb.Escape, + "Esc": kb.Escape, + "Backspace": kb.Backspace, + "Delete": kb.Delete, + "Del": kb.Delete, + "ArrowUp": kb.ArrowUp, + "ArrowDown": kb.ArrowDown, + "ArrowLeft": kb.ArrowLeft, + "ArrowRight": kb.ArrowRight, + "Home": kb.Home, + "End": kb.End, + "PageUp": kb.PageUp, + "PageDown": kb.PageDown, + "Space": " ", +} + // browserTool is one observation tool for a resolved BrowserToolbox. Every tool // of a toolbox shares one *Session and one origin; op selects the behavior. type browserTool struct { @@ -48,12 +102,23 @@ type browserTool struct { rtName string // the LLM-facing toolbox Name, embedded in the policy error text mode string sites []browserpolicy.Site + + // catalog backs query_logins only; nil/empty for every other op (and for + // query_logins itself when the toolbox binds no identity, in which case + // the op is never offered — see synthesize.go's queryLoginsOp gate). + catalog []spiceboxv1alpha1.BrowserLoginRef + + // filler resolves {{login...}} placeholders for the type op, strictly inside + // Execute so the transcript only ever records the placeholder. Shared across + // a toolbox's tools (one per session, over the session's §5.3 hold). + filler CredentialFiller } var ( - _ agenttool.Tool = (*browserTool)(nil) - _ agenttool.OriginTool = (*browserTool)(nil) - _ agenttool.Cancellable = (*browserTool)(nil) + _ agenttool.Tool = (*browserTool)(nil) + _ agenttool.OriginTool = (*browserTool)(nil) + _ agenttool.Cancellable = (*browserTool)(nil) + _ agenttool.NamedCallArgs = (*browserTool)(nil) ) func (t *browserTool) Name() string { return t.name } @@ -61,14 +126,68 @@ func (t *browserTool) Kind() agenttool.Kind { return agenttool.KindMCP } func (t *browserTool) Description() string { return t.description } func (t *browserTool) InputSchema() json.RawMessage { return t.schema } -// Permission is Passthrough: a browser observation reaches external state but -// maps to no SpiceDB resource, so no per-tool Check is required — the gate's -// egress policy, not SpiceDB, authorizes which hosts the agent may reach. +// Permission for an OBSERVATION op is Passthrough: a browser read reaches +// external state but maps to no SpiceDB resource, so no per-tool Check is +// required — the gate's egress policy, not SpiceDB, authorizes which hosts the +// agent may reach. +// +// Permission for an ACTING op (click/press/select/submit) is External on the +// web_host slot: every one of these leaves the session and mutates the real +// site, so it must be satisfied by a human-approved slot grant for the current +// page host and nothing else. The instance is the page host, surfaced by +// NamedArgs under hostArgKey — it is PAGE STATE, not a model argument, so the +// model cannot name a host it is not actually on. An empty host (about:blank / +// no page) resolves the template to "", which the External check treats as an +// UnresolvedResource denial (fail-closed), never a silent allow. +// +// Shaped byte-for-byte like git push's external check (toolkits/git.yaml): the +// resource id is already the canonical host (Session.CurrentHost runs +// browserpolicy.CanonicalHost), so no resourceIDTransforms are needed — the host +// is an acceptable SpiceDB object id as-is, exactly as the C1 web_host slot +// tests and the real-SpiceDB round-trip fixture assert. func (t *browserTool) Permission() authz.Permission { - return authz.Permission{StateImpact: authz.Passthrough} + if !isActingOp(t.op) { + return authz.Permission{StateImpact: authz.Passthrough} + } + return authz.Permission{ + StateImpact: authz.External, + Check: &authz.PermissionCheck{ + ResourceType: browserschema.WebHostType, + Permission: browserschema.ActPermission, + ResourceIDTemplate: "{" + hostArgKey + "}", + ResourceIDHint: "this acts on the page the browser is currently on; open an http(s) page with navigate first — " + + "about:blank or no page has no host to authorize against", + }, + } } func (t *browserTool) PermissionVariants() []authz.PermissionVariant { return nil } +// NamedArgs surfaces the current page host under hostArgKey so an acting op's +// web_host check resolves its resource instance from PAGE STATE rather than from +// a model argument. The host is written LAST so it wins over any "host" key the +// model may have put in the call — the page the browser is on is a fact, not +// something the caller gets to assert. +// +// For an observation op the args pass through unchanged (ok=true): those tools +// are Passthrough and resolve no resource, so there is nothing to surface and no +// reason to spend a round-trip reading the live location on the authz path. +// +// ok is always true: a browser call's arguments are plain JSON that always +// parse, unlike a sandbox argv. CurrentHost returning "" is NOT a parse failure +// — it is the fail-closed empty instance the External check denies on — so it is +// reported as a resolved (empty) host, not as ok=false. +func (t *browserTool) NamedArgs(args map[string]any) (map[string]any, bool) { + if !isActingOp(t.op) { + return args, true + } + out := make(map[string]any, len(args)+1) + for k, v := range args { + out[k] = v + } + out[hostArgKey] = t.sess.CurrentHost() + return out, true +} + // Origin is shared across one toolbox's tools so toolguard's origin-level // circuit breaker treats them as a single upstream. func (t *browserTool) Origin() string { return t.origin } @@ -92,6 +211,10 @@ func (t *browserTool) Execute(ctx context.Context, raw json.RawMessage, _ *agent Text string `json:"text"` Direction string `json:"direction"` TimeoutSeconds int `json:"timeout_seconds"` + Filter string `json:"filter"` + Host string `json:"host"` + Key string `json:"key"` + Option string `json:"option"` } if len(strings.TrimSpace(string(raw))) > 0 { if err := json.Unmarshal(raw, &a); err != nil { @@ -118,6 +241,18 @@ func (t *browserTool) Execute(ctx context.Context, raw json.RawMessage, _ *agent return t.back(ctx), nil case "wait_for": return t.waitFor(ctx, a.Text, a.Ref, a.TimeoutSeconds), nil + case "query_logins": + return t.queryLogins(a.Filter, a.Host, a.Page), nil + case "type": + return t.typeInto(ctx, a.Ref, a.Text), nil + case "click": + return t.click(ctx, a.Ref), nil + case "press": + return t.press(ctx, a.Key), nil + case "select": + return t.selectOption(ctx, a.Ref, a.Option), nil + case "submit": + return t.submit(ctx, a.Ref), nil default: return errResult("unknown browser tool %q", t.op), nil } @@ -178,6 +313,10 @@ func (t *browserTool) navigateTo(ctx context.Context, rawURL string) agenttool.R } finalURL, title := t.locationTitle(c) t.sess.clearRefs() + // A navigate/back leaves the old page behind, and with it any field a + // credential placeholder was typed into — clear the blanking alongside + // refs so a fresh page is read/screenshotted normally (spec §3.6). + t.sess.clearBlanked() out = okResult(fmt.Sprintf("loaded %s — title: %s", finalURL, title)) return nil }) @@ -222,7 +361,7 @@ func (t *browserTool) read(ctx context.Context, pageNum int, ref string) agentto } startBackend = e.backendNodeID } - lines, refs, e := snapshot(c, startBackend) + lines, refs, e := snapshot(c, startBackend, t.sess.blankedSet()) if e != nil { return e } @@ -251,7 +390,7 @@ func (t *browserTool) find(ctx context.Context, text string) agenttool.Result { } var out agenttool.Result runErr := t.sess.do(ctx, snapshotTimeout, func(c context.Context) error { - lines, refs, e := snapshot(c, 0) + lines, refs, e := snapshot(c, 0, t.sess.blankedSet()) if e != nil { return e } @@ -260,7 +399,10 @@ func (t *browserTool) find(ctx context.Context, text string) agenttool.Result { matched := make([]string, 0, 50) for _, ln := range lines { if strings.Contains(strings.ToLower(ln.name), needle) { - matched = append(matched, fmt.Sprintf("[%s] %s %q", ln.ref, ln.role, ln.name)) + // renderLine (not a bare role/name format) so a blanked field's + // matched line carries the same redaction marker read's outline + // does — find must not reveal what read would not (spec §3.6). + matched = append(matched, renderLine(ln)) if len(matched) >= 50 { break } @@ -329,6 +471,10 @@ func (t *browserTool) back(ctx context.Context) agenttool.Result { } finalURL, title := t.locationTitle(c) t.sess.clearRefs() + // A navigate/back leaves the old page behind, and with it any field a + // credential placeholder was typed into — clear the blanking alongside + // refs so a fresh page is read/screenshotted normally (spec §3.6). + t.sess.clearBlanked() if lu, e := url.Parse(finalURL); e == nil && lu.Hostname() != "" { if dec := t.decide(lu.Hostname()); !dec.Allowed { out = t.policyError(lu.Hostname(), dec.Rule) @@ -393,6 +539,19 @@ func (t *browserTool) scroll(ctx context.Context, direction, ref string) agentto // and is exactly what should flip this stub over to real image delivery once // agenttool.Result grows an image field. func (t *browserTool) screenshot(ctx context.Context, ref string) agenttool.Result { + // Refused while ANY credential-filled field is still blanked (spec §3.6) — + // global, not scoped to ref: a screenshot of one element would still render + // the rest of the page's pixels including the filled field, so the refusal + // does not depend on which element ref names. Checked before touching Chrome + // at all, matching navigateTo's policy pre-check. The blanked set clears only + // on a real navigation (navigate/back/follow), so after a submit that + // navigated the agent re-enables screenshot by navigating; until then this may + // over-refuse — deliberately fail-safe, refusing a screenshot is always the + // safe direction. + if t.sess.anyBlanked() { + return errResult("screenshot refused: a credential-filled field is still on the page and the form " + + "has not been submitted; submit the form or navigate away before capturing a screenshot") + } var out agenttool.Result runErr := t.sess.do(ctx, snapshotTimeout, func(c context.Context) error { var data []byte @@ -471,7 +630,7 @@ func (t *browserTool) waitFor(ctx context.Context, text, ref string, seconds int for { var found bool if e := t.sess.do(ctx, snapshotTimeout, func(c context.Context) error { - lines, refs, se := snapshot(c, 0) + lines, refs, se := snapshot(c, 0, t.sess.blankedSet()) if se != nil { return se } @@ -504,6 +663,403 @@ func (t *browserTool) waitFor(ctx context.Context, text, ref string, seconds int } } +// typeInto types text into the input a prior read's ref names. text may embed +// {{login..username|password|otp}} placeholders, which the credential +// filler substitutes IN PLACE, strictly inside this Execute — so the transcript +// (which records the model's raw args before dispatch) only ever sees the +// placeholder, never the resolved secret. +// +// The substitution is fail-closed on the three §3.5 rules: the placeholder host +// must be the host the page is actually on, must have a catalog entry in this +// toolbox's identity, and a password may only land in a type=password input. The +// password-input check reads the live element type via dom.DescribeNode BEFORE +// the filler runs, so a misdirected password never even Fetches. +// +// Refs are DELIBERATELY not dropped afterward (unlike click/submit): typing does +// not navigate, and a login flow fills a username, a password and then submits +// from one read — clearing refs after each type would force a re-read between +// fields. A type that DID rebuild the DOM is caught by the next op's stale-ref +// check. +// +// The tool result never echoes text (filled or not): it names the ref only, so a +// successful fill leaves no secret in the result, the transcript, or any log. +func (t *browserTool) typeInto(ctx context.Context, ref, text string) agenttool.Result { + if ref == "" { + return errResult("type requires a ref") + } + if text == "" { + return errResult("type requires text") + } + + // The host the page is actually on — the ONLY host a placeholder may name + // (rule 1). Read before the ref resolve; empty (about:blank) fails rule 1. + pageHost := t.sess.CurrentHost() + + // Resolve the ref to a backend node and read whether it is a password input, + // in one browser round-trip. + var backendNodeID int64 + var inputIsPassword bool + var handled *agenttool.Result + if runErr := t.sess.do(ctx, shortTimeout, func(c context.Context) error { + e, ok := t.sess.lookupRef(ref) + if !ok { + r := errResult("ref %s is stale; call read again", ref) + handled = &r + return nil + } + backendNodeID = e.backendNodeID + var node *cdp.Node + if ae := runAction(c, func(ac context.Context) error { + var e2 error + node, e2 = dom.DescribeNode().WithBackendNodeID(cdp.BackendNodeID(backendNodeID)).Do(ac) + return e2 + }); ae != nil { + return ae + } + inputIsPassword = isPasswordInput(node) + return nil + }); runErr != nil { + return errResult("type failed: %v", runErr) + } + if handled != nil { + return *handled + } + + // Substitute placeholders. Runs OUTSIDE do() (no browser lock held across the + // vault Fetch) and strictly inside Execute. filler is nil only if a tool was + // hand-built without one; guard rather than panic. + if t.filler == nil { + return errResult("type failed: no credential filler is configured") + } + filled, kinds, ferr := t.filler.Fill(ctx, pageHost, inputIsPassword, text) + if ferr != nil { + // ferr carries only host/field names, never a credential value. + return errResult("type failed: %v", ferr) + } + + // Focus the element and insert the (possibly substituted) text. InsertText + // fires the page's input events, so onchange/oninput handlers run. + if runErr := t.sess.do(ctx, shortTimeout, func(c context.Context) error { + if ae := runAction(c, func(ac context.Context) error { + return dom.Focus().WithBackendNodeID(cdp.BackendNodeID(backendNodeID)).Do(ac) + }); ae != nil { + return ae + } + return runAction(c, func(ac context.Context) error { + return input.InsertText(filled).Do(ac) + }) + }); runErr != nil { + return errResult("type failed: %v", runErr) + } + + // Record the field for C5's blanking pass IFF a credential actually landed in + // it. A plain-text type (no placeholder) leaves nothing to blank. + if kinds.any() { + t.sess.markBlanked(backendNodeID) + } + return okResult(fmt.Sprintf("typed into ref %s", ref)) +} + +// isPasswordInput reports whether node is an . Anything +// else (a text input, a textarea, a non-input element, or a nil node) is false, +// which fail-closes rule 3: a password placeholder is refused unless the target +// is unmistakably a password field. +func isPasswordInput(node *cdp.Node) bool { + if node == nil || !strings.EqualFold(node.NodeName, "input") { + return false + } + return strings.EqualFold(attrValue(node, "type"), "password") +} + +// click dispatches a native click on the element a prior read's ref names, +// firing its onclick handler and any form submission a submit button triggers. +// The snapshot's refs are dropped afterward: a click may navigate or rebuild the +// DOM, so every ref it described must be re-minted by a fresh read. +func (t *browserTool) click(ctx context.Context, ref string) agenttool.Result { + if ref == "" { + return errResult("click requires a ref") + } + var out agenttool.Result + runErr := t.sess.do(ctx, shortTimeout, func(c context.Context) error { + e, ok := t.sess.lookupRef(ref) + if !ok { + out = errResult("ref %s is stale; call read again", ref) + return nil + } + threw, err := callOnNode(c, e.backendNodeID, `function(){ this.click(); }`) + if err != nil { + return err + } + if threw { + out = errResult("click on ref %s failed", ref) + return nil + } + t.sess.clearRefs() + out = okResult(fmt.Sprintf("clicked ref %s", ref)) + return nil + }) + if runErr != nil { + return errResult("click failed: %v", runErr) + } + return out +} + +// press dispatches one key to the page. A single character types that character; +// a recognized named key (Enter, Tab, Escape, the arrows, …) presses that key. +// Anything else is refused rather than typed letter-by-letter — chromedp.KeyEvent +// would otherwise send "Enter" as five keystrokes. Refs are dropped afterward: a +// keypress (Enter submitting a form, say) may change the page. +func (t *browserTool) press(ctx context.Context, key string) agenttool.Result { + if strings.TrimSpace(key) == "" { + return errResult("press requires a key") + } + seq, ok := namedKeys[key] + if !ok { + if utf8.RuneCountInString(key) != 1 { + return errResult("press: unknown key %q; use a single character or a named key "+ + "(Enter, Tab, Escape, Backspace, Delete, ArrowUp/Down/Left/Right, Home, End, PageUp, PageDown, Space)", key) + } + seq = key + } + var out agenttool.Result + runErr := t.sess.do(ctx, shortTimeout, func(c context.Context) error { + if err := runAction(c, func(ac context.Context) error { return chromedp.KeyEvent(seq).Do(ac) }); err != nil { + return err + } + t.sess.clearRefs() + out = okResult(fmt.Sprintf("pressed %s", key)) + return nil + }) + if runErr != nil { + return errResult("press failed: %v", runErr) + } + return out +} + +// selectOption sets a , or an option that matches nothing, is a tool error. Refs are +// dropped afterward since a change handler may rebuild the DOM. +func (t *browserTool) selectOption(ctx context.Context, ref, option string) agenttool.Result { + if ref == "" { + return errResult("select requires a ref") + } + if option == "" { + return errResult("select requires an option") + } + // JSON-encode the option INTO the function body rather than passing it as a + // call argument: a JSON string literal is also a valid JS string literal, so + // this is injection-safe, and it sidesteps the CallArgument value encoding. + optJSON, err := json.Marshal(option) + if err != nil { + return errResult("select: encode option: %v", err) + } + funcDecl := fmt.Sprintf(`function(){ + var want = %s; + if (this.options === undefined || this.options === null) { throw new Error("not a select"); } + for (var i = 0; i < this.options.length; i++) { + var o = this.options[i]; + if (o.value === want || o.label === want || o.text === want) { + this.value = o.value; + this.dispatchEvent(new Event("input", {bubbles:true})); + this.dispatchEvent(new Event("change", {bubbles:true})); + return; + } + } + throw new Error("no matching option"); +}`, optJSON) + var out agenttool.Result + runErr := t.sess.do(ctx, shortTimeout, func(c context.Context) error { + e, ok := t.sess.lookupRef(ref) + if !ok { + out = errResult("ref %s is stale; call read again", ref) + return nil + } + threw, err := callOnNode(c, e.backendNodeID, funcDecl) + if err != nil { + return err + } + if threw { + out = errResult("select failed: ref %s is not a ` + + `` + + `

AWAITING-CODE

` + + ``)) + }) +} + +// browserCodeFixture boots the chrome fixture in act mode with a scripted +// HumanStepRequester that answers the code card, returning the synthesized tools, +// the fake (for its call counter), the shared session (for the blanked set), and +// the site base URL. +func browserCodeFixture(t *testing.T, hs *fakeHumanStep) (tools []agenttool.Tool, sess *Session, siteBase string) { + t.Helper() + fx := startFixture(t, codePage()) + fx.rt.Spec.Interaction = "act" + var err error + tools, err = Synthesize(fx.rt, fx.gateURL, chromeToken, false, nil, hs, nil) + require.NoError(t, err) + require.NotEmpty(t, tools) + sess = tools[0].(*browserTool).sess + t.Cleanup(func() { sess.Close() }) + return tools, sess, fx.siteBase +} + +// TestRequestHumanStepCode_RoundTripNoLeak is the C7 keystone (Review Focus 3): a +// human provides "123456"; the RUNNER types it into the code field (not a model +// tool call); the page advances; and the code "123456" appears NOWHERE the model +// can see it — not in the tool result, not in the request_human_step call args, and +// not in a later read of the page (the field is blanked). +func TestRequestHumanStepCode_RoundTripNoLeak(t *testing.T) { + const code = "123456" + hs := &fakeHumanStep{codeOutcome: HumanStepCodeOutcome{Approved: true, Code: code, DecidedBy: "alice"}} + tools, sess, siteBase := browserCodeFixture(t, hs) + + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": siteBase + "/"}).IsError) + + codeRef := refByName(t, tools, "CodeField") + + // The model's request_human_step call carries ONLY kind/prompt/ref — never the + // code (the model does not have it; the human supplies it out of band). Capture + // the raw args the transcript would record and prove the code is absent. + humanStepTool := toolBy(t, tools, "request_human_step") + rawArgs, err := json.Marshal(map[string]any{"kind": "code", "prompt": "enter the 6-digit code we emailed you", "ref": codeRef}) + require.NoError(t, err) + argsSnapshot := append(json.RawMessage(nil), rawArgs...) + + res, err := humanStepTool.Execute(context.Background(), rawArgs, &agenttool.SessionContext{}) + require.NoError(t, err, "a tool-level outcome is never a Go error") + require.False(t, res.IsError, "the code round-trip must succeed: %s", res.Content) + + assert.Equal(t, 1, hs.codeCalls, "the code card must have blocked through the requester once") + assert.Equal(t, 0, hs.confirmCalls, "the code path must not use the confirm path") + + // 1) The tool RESULT is a status only — never the code. + assert.NotContains(t, res.Content, code, "the tool result must never contain the typed code") + assert.Contains(t, res.Content, codeRef, "the result names the ref it entered the code into") + + // 2) The recorded tool-call args are unchanged and never held the code. + assert.Equal(t, argsSnapshot, json.RawMessage(rawArgs), "Execute must not rewrite the recorded args") + assert.NotContains(t, string(rawArgs), code, "the model's tool-call args never carry the code") + + // 3) The page actually ADVANCED — the code reached the field (the sentinel only + // appears when the exact code was typed), proving the runner typed it for real. + after := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, after.IsError, "read after code: %s", after.Content) + assert.Contains(t, after.Content, "CODE-ACCEPTED-SIGNED-IN", "the page must have advanced, proving the code landed") + + // 4) …and that later read does NOT surface the code: the field was blanked. + assert.NotContains(t, after.Content, code, "a read after the code must never reveal it (the field is blanked)") + + // The code field was marked for C5 blanking, exactly like a filled credential. + assert.Len(t, sess.blankedNodes(), 1, "the code field must be marked for blanking") +} + +// TestRequestHumanStepCode_TimeoutFailsClosedNoType is the fail-closed half with a +// real Chrome: a lapsed code card types NOTHING into the page and reports an error. +func TestRequestHumanStepCode_TimeoutFailsClosedNoType(t *testing.T) { + hs := &fakeHumanStep{codeOutcome: HumanStepCodeOutcome{TimedOut: true}} + tools, sess, siteBase := browserCodeFixture(t, hs) + + require.False(t, run(t, toolBy(t, tools, "navigate"), map[string]any{"url": siteBase + "/"}).IsError) + codeRef := refByName(t, tools, "CodeField") + + res := run(t, toolBy(t, tools, "request_human_step"), map[string]any{"kind": "code", "prompt": "enter the code", "ref": codeRef}) + assert.True(t, res.IsError, "a lapsed code card must fail closed") + assert.Contains(t, res.Content, "timed out") + + // Nothing was typed: the page never advanced and no field was blanked. + after := run(t, toolBy(t, tools, "read"), map[string]any{}) + require.False(t, after.IsError) + assert.NotContains(t, after.Content, "CODE-ACCEPTED-SIGNED-IN", "a timed-out code must type nothing") + assert.Empty(t, sess.blankedNodes(), "a timed-out code must blank no field") +} diff --git a/pkg/agent/tool/browser/human_step_test.go b/pkg/agent/tool/browser/human_step_test.go new file mode 100644 index 00000000..e454459a --- /dev/null +++ b/pkg/agent/tool/browser/human_step_test.go @@ -0,0 +1,198 @@ +package browser + +import ( + "context" + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authzed/openagentprimitives/pkg/authz" +) + +// fakeHumanStep records what request_human_step asked and returns a scripted +// outcome, with no runner/host/orchestrator involved. +type fakeHumanStep struct { + confirmCalls int + confirmPrompt string + outcome HumanStepOutcome + confirmErr error + + codeCalls int + codePrompt string + codeOutcome HumanStepCodeOutcome + codeErr error + + captchaCalls int + captchaPrompt string + captchaErr error +} + +func (f *fakeHumanStep) Confirm(_ context.Context, prompt string) (HumanStepOutcome, error) { + f.confirmCalls++ + f.confirmPrompt = prompt + return f.outcome, f.confirmErr +} + +func (f *fakeHumanStep) RequestCode(_ context.Context, prompt string) (HumanStepCodeOutcome, error) { + f.codeCalls++ + f.codePrompt = prompt + return f.codeOutcome, f.codeErr +} + +func (f *fakeHumanStep) Captcha(_ context.Context, prompt string) error { + f.captchaCalls++ + f.captchaPrompt = prompt + return f.captchaErr +} + +func humanStepTool(hs HumanStepRequester) *browserTool { + return &browserTool{op: requestHumanStepOp, humanStep: hs} +} + +func runHumanStep(t *testing.T, bt *browserTool, args map[string]any) agentResult { + t.Helper() + raw, err := json.Marshal(args) + require.NoError(t, err) + res, err := bt.Execute(context.Background(), raw, nil) + require.NoError(t, err, "Execute must never return a Go error for a tool-level outcome") + return agentResult{content: res.Content, isError: res.IsError} +} + +type agentResult struct { + content string + isError bool +} + +// TestRequestHumanStepPermissionIsStateless proves request_human_step is NOT +// web_host-gated: its authorization is the human card it posts, so it carries no +// External/web_host Check — unlike the mutating acting ops (click/type/...). +func TestRequestHumanStepPermissionIsStateless(t *testing.T) { + p := humanStepTool(nil).Permission() + assert.Equal(t, authz.Stateless, p.StateImpact, + "request_human_step reaches no page state; the human card IS the gate") + assert.Nil(t, p.Check, "no web_host slot check — layering one would be a redundant second human gate") +} + +// TestRequestHumanStepConfirm maps each confirm outcome to the tool result. The +// approve result is a STATUS only (success, no secret); deny and timeout are +// fail-closed error results — a timeout never silently succeeds. +func TestRequestHumanStepConfirm(t *testing.T) { + cases := []struct { + name string + outcome HumanStepOutcome + wantIsError bool + wantContains string + }{ + {name: "approved → success status", outcome: HumanStepOutcome{Approved: true}, wantIsError: false, wantContains: "confirmed"}, + {name: "denied → fail-closed", outcome: HumanStepOutcome{Denied: true}, wantIsError: true, wantContains: "declined"}, + {name: "timed out → fail-closed (not a silent success)", outcome: HumanStepOutcome{TimedOut: true}, wantIsError: true, wantContains: "timed out"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hs := &fakeHumanStep{outcome: tc.outcome} + bt := humanStepTool(hs) + res := runHumanStep(t, bt, map[string]any{"kind": "confirm", "prompt": "approve the push on your phone"}) + + assert.Equal(t, 1, hs.confirmCalls, "confirm must block through the requester exactly once") + assert.Equal(t, 0, hs.captchaCalls, "confirm must not surface a captcha notice") + assert.Equal(t, "approve the push on your phone", hs.confirmPrompt) + assert.Equal(t, tc.wantIsError, res.isError) + assert.Contains(t, res.content, tc.wantContains) + }) + } +} + +// TestRequestHumanStepCaptcha proves the no-kind path surfaces a notice and +// reports that human intervention is needed (fail: it never blocks on a decision). +func TestRequestHumanStepCaptcha(t *testing.T) { + for _, kind := range []string{"", "captcha"} { + t.Run("kind="+kind, func(t *testing.T) { + hs := &fakeHumanStep{} + bt := humanStepTool(hs) + args := map[string]any{"prompt": "a reCAPTCHA is blocking the page"} + if kind != "" { + args["kind"] = kind + } + res := runHumanStep(t, bt, args) + + assert.Equal(t, 1, hs.captchaCalls, "a captcha must surface the notice exactly once") + assert.Equal(t, 0, hs.confirmCalls, "a captcha must not block on a decision") + assert.Equal(t, "a reCAPTCHA is blocking the page", hs.captchaPrompt) + assert.True(t, res.isError, "a captcha the agent cannot solve is an error result") + assert.Contains(t, res.content, "person", "the result must say a person is needed") + }) + } +} + +// TestRequestHumanStepCode_RefRequired proves kind=code refuses up front without a +// ref — the code has to be typed somewhere, so a missing ref fails BEFORE a human +// is ever asked (no RequestCode call). +func TestRequestHumanStepCode_RefRequired(t *testing.T) { + hs := &fakeHumanStep{} + bt := humanStepTool(hs) + res := runHumanStep(t, bt, map[string]any{"kind": "code", "prompt": "enter the emailed code"}) + + assert.True(t, res.isError) + assert.Contains(t, res.content, "ref") + assert.Equal(t, 0, hs.codeCalls, "a missing ref must fail before blocking a human") +} + +// TestRequestHumanStepCode_OutcomeMapping maps each RequestCode outcome to the +// tool result. Approve with a code is a STATUS only (and the code never appears in +// the result); deny and timeout are fail-closed errors; an approve with no code +// fails closed rather than typing nothing and claiming success. The actual +// type-into-ref needs Chrome and is covered by the fill-fixture round-trip below; +// here humanStep is set but sess is nil, so the approve+code case would nil-panic +// on the type — which is itself the proof the request is refused for the right +// reasons BEFORE the type on every non-approve path. The approve+code result path +// is asserted in TestRequestHumanStepCode_RoundTripNoLeak (Chrome). +func TestRequestHumanStepCode_OutcomeMapping(t *testing.T) { + cases := []struct { + name string + outcome HumanStepCodeOutcome + wantIsError bool + wantContains string + }{ + {name: "denied → fail-closed", outcome: HumanStepCodeOutcome{Denied: true}, wantIsError: true, wantContains: "declined"}, + {name: "timed out → fail-closed (not a silent success)", outcome: HumanStepCodeOutcome{TimedOut: true}, wantIsError: true, wantContains: "timed out"}, + {name: "approved but no code → fail-closed", outcome: HumanStepCodeOutcome{Approved: true, Code: ""}, wantIsError: true, wantContains: "no code"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + hs := &fakeHumanStep{codeOutcome: tc.outcome} + bt := humanStepTool(hs) + res := runHumanStep(t, bt, map[string]any{"kind": "code", "prompt": "enter the emailed code", "ref": "e3"}) + + assert.Equal(t, 1, hs.codeCalls, "code must block through the requester exactly once") + assert.Equal(t, 0, hs.confirmCalls, "code must not use the confirm path") + assert.Equal(t, "enter the emailed code", hs.codePrompt) + assert.Equal(t, tc.wantIsError, res.isError) + assert.Contains(t, res.content, tc.wantContains) + }) + } +} + +// TestRequestHumanStepValidation covers the local guards: empty prompt, a nil +// requester, and an unknown kind all fail as tool-level errors without panicking. +func TestRequestHumanStepValidation(t *testing.T) { + t.Run("empty prompt is refused", func(t *testing.T) { + res := runHumanStep(t, humanStepTool(&fakeHumanStep{}), map[string]any{"kind": "confirm"}) + assert.True(t, res.isError) + assert.Contains(t, res.content, "prompt") + }) + t.Run("nil requester fails loud, not nil-panic", func(t *testing.T) { + res := runHumanStep(t, humanStepTool(nil), map[string]any{"kind": "confirm", "prompt": "approve the push"}) + assert.True(t, res.isError) + assert.Contains(t, res.content, "unavailable") + }) + t.Run("unknown kind is refused", func(t *testing.T) { + hs := &fakeHumanStep{} + res := runHumanStep(t, humanStepTool(hs), map[string]any{"kind": "banana", "prompt": "do a thing"}) + assert.True(t, res.isError) + assert.Contains(t, res.content, "unknown kind") + assert.Equal(t, 0, hs.confirmCalls) + assert.Equal(t, 0, hs.captchaCalls) + }) +} diff --git a/pkg/agent/tool/browser/humanstep.go b/pkg/agent/tool/browser/humanstep.go new file mode 100644 index 00000000..87789e79 --- /dev/null +++ b/pkg/agent/tool/browser/humanstep.go @@ -0,0 +1,74 @@ +package browser + +import "context" + +// HumanStepRequester is request_human_step's seam to the runner's +// human-in-the-loop machinery, mirroring meta.PreferenceSaver exactly: the +// interface lives in this tool package (the consumer), the runner implements it +// over the approval host (pkg/agent/runner's humanStepRequester). A fake +// satisfies it in tests with no Loop/orchestrator. +// +// request_human_step never returns a secret TO THE MODEL. Confirm reports only +// WHETHER the human completed the step; Captcha reports only that a notice was +// sent. RequestCode returns the human's typed code so the TOOL (the runner) can +// type it into the page element the agent named — it is never handed to the model. +type HumanStepRequester interface { + // Confirm posts a login_human_step card with a Done button and blocks until a + // person presses it, declines, or the approval window lapses — a push approval + // the person completes on their phone, say. The outcome tells the tool which + // happened; it carries no secret. + Confirm(ctx context.Context, prompt string) (HumanStepOutcome, error) + + // RequestCode posts a login_human_step card asking the person to type an + // emailed/SMS code, and blocks until they answer, decline, or the window lapses. + // On an approval the outcome carries the person's typed Code, which the CALLER + // (request_human_step) types into the page via the CDP fill path — the same + // InsertText the type tool uses, but sourced from the human's code, not a vault + // credential. The code is NEVER returned as a tool result or shown to the model. + // A timeout is fail-closed: TimedOut is true, Code empty, and the caller reports + // a failure rather than silently succeeding. + RequestCode(ctx context.Context, prompt string) (HumanStepCodeOutcome, error) + + // Captcha surfaces a one-way notice that a person must take over in the browser + // (a CAPTCHA the agent cannot solve, or any step with no actionable kind). It + // does NOT block — there is no decision leg; the agent has reported it cannot + // proceed, and a human steps in out of band. + Captcha(ctx context.Context, prompt string) error +} + +// HumanStepCodeOutcome is the result of a RequestCode round-trip. Exactly one of +// Approved / Denied / TimedOut is true whenever RequestCode returns a nil error. +type HumanStepCodeOutcome struct { + // Approved is true when the person supplied the code (and Code holds it). + Approved bool + // Denied is true when the person explicitly declined to provide a code. + Denied bool + // TimedOut is true when the prompt lapsed with no answer — fail-closed, never a + // silent success; distinguished from Denied so the model learns which happened. + TimedOut bool + // DecidedBy is the canonical subject id of whoever answered (set for Approved + // and Denied), empty on a timeout. + DecidedBy string + // Code is the person's typed login code, set ONLY on Approved. It is a SECRET: + // the caller types it into the page and returns a status; it is never placed in + // a tool result, a log, the transcript, or any model-visible message. + Code string +} + +// HumanStepOutcome is the result of a Confirm round-trip. Exactly one of +// Approved / Denied / TimedOut is true whenever Confirm returns a nil error, so +// the tool can tell the model which happened rather than reporting every +// non-approval as a human "no". +type HumanStepOutcome struct { + // Approved is true when the person confirmed the step (pressed Done). + Approved bool + // Denied is true when the person explicitly declined. + Denied bool + // TimedOut is true when the confirm prompt lapsed with no decision — a + // fail-closed result, never a silent success. Distinguished from Denied so the + // model learns whether a person said no or nobody answered. + TimedOut bool + // DecidedBy is the canonical subject id of whoever decided (set for Approved + // and Denied), or empty on a timeout — nobody decided. + DecidedBy string +} diff --git a/pkg/agent/tool/browser/policy_precheck_test.go b/pkg/agent/tool/browser/policy_precheck_test.go index 078c4a13..4dc712e4 100644 --- a/pkg/agent/tool/browser/policy_precheck_test.go +++ b/pkg/agent/tool/browser/policy_precheck_test.go @@ -53,7 +53,7 @@ func toolNames(tools []agenttool.Tool) []string { // must be refused BEFORE any Chrome connection is attempted. func execNavigate(t *testing.T, rt spiceboxv1alpha1.ResolvedBrowserToolbox, url string) agenttool.Result { t.Helper() - tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil, nil, nil) require.NoError(t, err) nav := toolBySuffix(t, tools, "navigate") args, err := json.Marshal(map[string]any{"url": url}) diff --git a/pkg/agent/tool/browser/query_logins_test.go b/pkg/agent/tool/browser/query_logins_test.go index 1507c084..2ed5eaf1 100644 --- a/pkg/agent/tool/browser/query_logins_test.go +++ b/pkg/agent/tool/browser/query_logins_test.go @@ -30,7 +30,7 @@ func buildToolboxWithCatalog(identity string, catalog []spiceboxv1alpha1.Browser // URL is unreachable on purpose, same discipline as execNavigate. func execQueryLogins(t *testing.T, rt spiceboxv1alpha1.ResolvedBrowserToolbox, args map[string]any) agenttool.Result { t.Helper() - tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil, nil, nil) require.NoError(t, err) ql := toolBySuffix(t, tools, "query_logins") raw, err := json.Marshal(args) @@ -68,7 +68,7 @@ func TestQueryLoginsOfferedOnlyWithIdentity(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { rt := buildToolboxWithCatalog(tc.identity, []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "blocklist") - tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil, nil, nil) require.NoError(t, err) got := false for _, tl := range tools { @@ -89,7 +89,7 @@ func TestQueryLoginsOfferedInBothModes(t *testing.T) { t.Run(mode, func(t *testing.T) { rt := buildToolboxWithCatalog("my-identity", []spiceboxv1alpha1.BrowserLoginRef{aaLogin()}, "blocklist") rt.Spec.Interaction = mode - tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil) + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", false, nil, nil, nil) require.NoError(t, err) found := false for _, tl := range tools { diff --git a/pkg/agent/tool/browser/session.go b/pkg/agent/tool/browser/session.go index 146ee551..29312544 100644 --- a/pkg/agent/tool/browser/session.go +++ b/pkg/agent/tool/browser/session.go @@ -15,6 +15,7 @@ package browser import ( "context" "fmt" + "log/slog" "net/url" "sync" "time" @@ -91,6 +92,20 @@ type Session struct { // blanked is the set of DOM backend node IDs a credential was typed into, // recorded by the type tool for Phase C Task C5's blanking pass to consume. blanked map[int64]bool + + // store persists captured session state (cookies + localStorage) across reap + // and wake for a `persistent` toolbox (Task C9). nil for an `ephemeral` + // toolbox, which disables capture and inject entirely. Written by the runner's + // per-session state Secret, the sole writer of that Secret's content. + store StateStore + // logName is the LLM-facing toolbox name, used only as structured log context + // for best-effort capture/inject failures. + logName string + // injectAttempted guards the once-per-boot inject on first attach. Guarded by + // mu (set inside do(), which holds mu), so a concurrent first tool call injects + // exactly once. An inject is ATTEMPTED once even if it fails, so a persistently + // bad blob cannot wedge every subsequent call behind a retry. + injectAttempted bool } // heldCred is one Fetched credential kept briefly in the §5.3 hold so repeated @@ -214,6 +229,11 @@ func (s *Session) do(ctx context.Context, timeout time.Duration, fn func(ctx con if err := s.ensureConnected(); err != nil { return err } + // Inject persisted state on the FIRST attach after a pod boot, before fn runs — + // so a wake's restored cookies are in place before the first browser action. + // Once per boot, best-effort, under mu (the connection is live here). A nil + // store (ephemeral) or an empty Secret (first boot) makes this a no-op. + s.maybeInject(ctx) runCtx, cancel := context.WithTimeout(s.browseCtx, timeout) defer cancel() // Propagate the caller's cancellation (an interrupt or the LLM call ending) @@ -223,6 +243,79 @@ func (s *Session) do(ctx context.Context, timeout time.Duration, fn func(ctx con return fn(runCtx) } +// maybeInject performs the once-per-boot state injection on first attach. It is +// called with s.mu held (from do, right after ensureConnected) and runs directly +// on s.browseCtx — NOT through s.do, which would re-lock mu. A nil store disables +// it; an empty blob (first boot) injects nothing. Failures are LOGGED with origin +// context and swallowed (repo rule: never crash the turn on a best-effort restore +// — the login simply isn't present and the model re-logs-in, spec §5.5). The +// attempt flag is set unconditionally so a bad blob cannot wedge every later call. +func (s *Session) maybeInject(ctx context.Context) { + if s.store == nil || s.injectAttempted { + return + } + s.injectAttempted = true + data, err := s.store.Load(ctx) + if err != nil { + slog.Warn("browser: load persisted state failed; starting with a clean session", + "toolbox", s.logName, "gate", s.gateURL, "err", err.Error()) + return + } + blob, err := unmarshalBlob(data) + if err != nil { + slog.Warn("browser: persisted state is unreadable; starting with a clean session", + "toolbox", s.logName, "gate", s.gateURL, "err", err.Error()) + return + } + if len(blob.Cookies) == 0 && len(blob.Origins) == 0 { + return // first boot: nothing to inject. + } + ictx, cancel := context.WithTimeout(s.browseCtx, shortTimeout) + defer cancel() + if err := injectFrom(ictx, blob); err != nil { + slog.Warn("browser: inject persisted state failed; the model may need to log in again", + "toolbox", s.logName, "gate", s.gateURL, "err", err.Error()) + } +} + +// captureAndPersist captures the live cookie jar + current-origin localStorage, +// merges it with the prior saved blob, and writes it back through the StateStore. +// A no-op for an ephemeral toolbox (nil store). Every failure is RETURNED with +// context so the caller can log and continue (capture must never fail a turn — +// the next acting call or the SIGTERM capture retries). Not called with mu held: +// the capture goes through s.do (which takes mu) and the store I/O runs outside it. +func (s *Session) captureAndPersist(ctx context.Context) error { + if s.store == nil { + return nil + } + var captured stateBlob + if err := s.do(ctx, shortTimeout, func(c context.Context) error { + return captureInto(c, &captured) + }); err != nil { + return fmt.Errorf("capture browser state: %w", err) + } + prior, err := s.store.Load(ctx) + if err != nil { + return fmt.Errorf("load prior browser state: %w", err) + } + priorBlob, err := unmarshalBlob(prior) + if err != nil { + // A corrupt prior blob must not block a fresh capture: start from the + // capture alone rather than refusing to persist anything. + slog.Warn("browser: prior persisted state is unreadable; overwriting with a fresh capture", + "toolbox", s.logName, "err", err.Error()) + priorBlob = stateBlob{} + } + data, err := marshalBlob(mergeBlob(priorBlob, captured)) + if err != nil { + return fmt.Errorf("marshal browser state: %w", err) + } + if err := s.store.Save(ctx, data); err != nil { + return fmt.Errorf("persist browser state: %w", err) + } + return nil +} + // onNetworkEvent records request URLs and, on a gate/proxy-denied load failure, // the URL it failed at — so navigate can name the real blocked host (e.g. a // redirect target) instead of Chrome's chrome-error pseudo-host. Runs on diff --git a/pkg/agent/tool/browser/synthesize.go b/pkg/agent/tool/browser/synthesize.go index 7d5693e0..5e195e34 100644 --- a/pkg/agent/tool/browser/synthesize.go +++ b/pkg/agent/tool/browser/synthesize.go @@ -94,6 +94,15 @@ var actingOps = []opSpec{ description: "Submit the form that the element named by a ref belongs to (its own form, or the nearest enclosing one), running the form's submit handler. Refs from before the submit are stale afterward.", schema: `{"type":"object","properties":{"ref":{"type":"string","description":"A ref (e.g. e3) from a prior read for an element inside the form to submit."}},"required":["ref"]}`, }, + { + op: requestHumanStepOp, + description: "Ask a person to complete a sign-in step you can't do yourself; the result is a STATUS only and never contains a code or secret. " + + "With kind=\"confirm\" it posts a card with a Done button and BLOCKS until the person confirms (use this for a push notification they approve on their phone, or any \"do X then press Done\" step). " + + "With kind=\"code\" it asks the person for an emailed or texted code and, once they provide it, types it into the input you name with ref and BLOCKS until then — you never see the code, it is entered for you (read the page first to get the code field's ref). " + + "Omit kind to report a CAPTCHA or any other step a person must take over — a notice is sent and the call reports that human intervention is needed (it does not wait). " + + "The prompt should say plainly what the person needs to do.", + schema: `{"type":"object","properties":{"kind":{"type":"string","enum":["confirm","code"],"description":"\"confirm\" posts a Done button and blocks until a person confirms. \"code\" asks for an emailed/SMS code and types it into ref for you. Omit kind entirely to report a CAPTCHA or other step needing a person."},"prompt":{"type":"string","description":"A short message telling the person what to do (e.g. \"approve the push notification on your phone\", or \"enter the 6-digit code we emailed you\")."},"ref":{"type":"string","description":"Required for kind=\"code\": a ref (e.g. e3) from a prior read naming the input the code should be typed into."}},"required":["prompt"]}`, + }, } // screenshotOp is offered only when the model accepts images. @@ -123,15 +132,28 @@ var queryLoginsOp = opSpec{ // bound identity's directcred source (so a {{login...}} placeholder resolves to // a real website login at use time), tests and the e2e harness pass a fake, and // a toolbox with no bound identity passes nil (its catalog is empty, so no -// placeholder can resolve anyway). All returned tools share one chromedp session -// (lazily connected on first use), one origin, and one credential filler. -func Synthesize(rt spiceboxv1alpha1.ResolvedBrowserToolbox, gateURL, token string, acceptsImages bool, fetch CredentialFetcher) ([]agenttool.Tool, error) { +// placeholder can resolve anyway). humanStep backs request_human_step's blocking +// confirm + captcha notice (offered only in act mode); the production runner +// passes a late-bound requester over its approval host, tests pass a fake, and a +// nil requester leaves request_human_step offered but erroring at call time. +// store persists cookies + localStorage across reap/wake for a `persistent` +// toolbox (Task C9): the production runner passes a StateStore over the +// per-session state Secret, tests pass a fake, and an `ephemeral` toolbox (or any +// caller that does not persist) passes nil, which disables capture and inject +// entirely. All returned tools share one chromedp session (lazily connected on +// first use), one origin, one credential filler, one human-step requester, and +// one state store. +func Synthesize(rt spiceboxv1alpha1.ResolvedBrowserToolbox, gateURL, token string, acceptsImages bool, fetch CredentialFetcher, humanStep HumanStepRequester, store StateStore) ([]agenttool.Tool, error) { sites := make([]browserpolicy.Site, 0, len(rt.EffectiveSites)) for _, s := range rt.EffectiveSites { sites = append(sites, browserpolicy.Site{Host: s.Host, Exact: s.Exact}) } sess := newSession(gateURL, token) + // Persistent-state wiring (Task C9): a non-nil store enables capture-after-act, + // capture-on-stop, and inject-on-first-attach. nil (ephemeral) leaves them off. + sess.store = store + sess.logName = rt.Name origin := originfmt.ForBrowser(rt) // One filler per toolbox, sharing the session's §5.3 hold. Built here, after // the session, because the default filler reaches the hold through it. @@ -171,6 +193,7 @@ func Synthesize(rt spiceboxv1alpha1.ResolvedBrowserToolbox, gateURL, token strin sites: sites, catalog: rt.Catalog, filler: filler, + humanStep: humanStep, }) } return tools, nil diff --git a/pkg/agent/tool/browser/tools.go b/pkg/agent/tool/browser/tools.go index 1db3b8a0..8bf8d87b 100644 --- a/pkg/agent/tool/browser/tools.go +++ b/pkg/agent/tool/browser/tools.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "net/url" "strings" "time" @@ -45,6 +46,14 @@ const ( // ResourceIDTemplate reads it back from. One constant keeps the two halves — // what the tool writes and what the check reads — from drifting apart. hostArgKey = "host" + + // requestHumanStepOp is the human-handoff op: it asks a person to complete a + // sign-in step the agent cannot do itself. It is offered ONLY in act mode (it + // is part of the login flow), but it is NOT a mutating acting op — it writes + // nothing to the page — so it is deliberately absent from actingOpNames and + // carries StateImpact Stateless, not External on web_host. See + // browserTool.Permission. + requestHumanStepOp = "request_human_step" ) // actingOpNames is the set of ops that reach and mutate external page state @@ -112,6 +121,13 @@ type browserTool struct { // Execute so the transcript only ever records the placeholder. Shared across // a toolbox's tools (one per session, over the session's §5.3 hold). filler CredentialFiller + + // humanStep backs request_human_step only; nil for every other op (and for + // request_human_step itself when the toolbox is synthesized without a + // requester, e.g. a unit test — Execute guards nil rather than panicking). + // The runner implements it over the approval host (pkg/agent/runner); a fake + // satisfies it in tests. + humanStep HumanStepRequester } var ( @@ -140,12 +156,31 @@ func (t *browserTool) InputSchema() json.RawMessage { return t.schema } // no page) resolves the template to "", which the External check treats as an // UnresolvedResource denial (fail-closed), never a silent allow. // -// Shaped byte-for-byte like git push's external check (toolkits/git.yaml): the -// resource id is already the canonical host (Session.CurrentHost runs -// browserpolicy.CanonicalHost), so no resourceIDTransforms are needed — the host -// is an acceptable SpiceDB object id as-is, exactly as the C1 web_host slot -// tests and the real-SpiceDB round-trip fixture assert. +// Shaped like git push's external check (toolkits/git.yaml), with the one +// difference a host forces: the resource id is the canonical page host +// (Session.CurrentHost runs browserpolicy.CanonicalHost), which is NOT a legal +// SpiceDB object id as-is. SpiceDB's object-id charset forbids dots and every +// real hostname has them, so `web_host:example.com` is rejected with +// InvalidArgument — the acting feature could never authorize against real SpiceDB +// without escaping. ResourceIDTransforms therefore runs browserschema's canonical +// chain (spicedb_escape: "." → "=2E"), the SAME chain the AgentClass reconciler +// publishes onto web_host's resolvedSlot so the grant-write legs mint the +// identical id. spicedb_escape is INJECTIVE, so the plan-gate card can keep +// DISPLAYING the raw host (derivedInstanceID drops the escape for display) while +// the grant is written on the escaped id, and the card stays a truthful claim +// about exactly which host was approved. func (t *browserTool) Permission() authz.Permission { + // request_human_step is offered only in act mode but is NOT a page mutation: + // it posts a human-in-the-loop card (confirm) or a notice (captcha) and + // reaches no external page state. Its authorization IS the human login_human_step + // card it publishes (DecideApprovers, re-checked server-side), so it needs no + // web_host slot gate — layering one would be a redundant second human gate for + // one handoff. Stateless, exactly like set_preference, whose confirm is also its + // own gate. (The emitted-code half types into the page via the runner, after + // the human approves — never a model-driven acting call through this tool.) + if t.op == requestHumanStepOp { + return authz.Permission{StateImpact: authz.Stateless} + } if !isActingOp(t.op) { return authz.Permission{StateImpact: authz.Passthrough} } @@ -155,6 +190,10 @@ func (t *browserTool) Permission() authz.Permission { ResourceType: browserschema.WebHostType, Permission: browserschema.ActPermission, ResourceIDTemplate: "{" + hostArgKey + "}", + // Escape the host into a legal SpiceDB object id — dots are illegal in + // one. Sourced from browserschema so the check path and every + // grant-write leg derive the identical id. See Permission's doc. + ResourceIDTransforms: browserschema.WebHostValueTransforms(), ResourceIDHint: "this acts on the page the browser is currently on; open an http(s) page with navigate first — " + "about:blank or no page has no host to authorize against", }, @@ -215,6 +254,8 @@ func (t *browserTool) Execute(ctx context.Context, raw json.RawMessage, _ *agent Host string `json:"host"` Key string `json:"key"` Option string `json:"option"` + Kind string `json:"kind"` + Prompt string `json:"prompt"` } if len(strings.TrimSpace(string(raw))) > 0 { if err := json.Unmarshal(raw, &a); err != nil { @@ -244,20 +285,193 @@ func (t *browserTool) Execute(ctx context.Context, raw json.RawMessage, _ *agent case "query_logins": return t.queryLogins(a.Filter, a.Host, a.Page), nil case "type": - return t.typeInto(ctx, a.Ref, a.Text), nil + return t.afterAct(ctx, t.typeInto(ctx, a.Ref, a.Text)), nil case "click": - return t.click(ctx, a.Ref), nil + return t.afterAct(ctx, t.click(ctx, a.Ref)), nil case "press": - return t.press(ctx, a.Key), nil + return t.afterAct(ctx, t.press(ctx, a.Key)), nil case "select": - return t.selectOption(ctx, a.Ref, a.Option), nil + return t.afterAct(ctx, t.selectOption(ctx, a.Ref, a.Option)), nil case "submit": - return t.submit(ctx, a.Ref), nil + return t.afterAct(ctx, t.submit(ctx, a.Ref)), nil + case requestHumanStepOp: + return t.requestHumanStep(ctx, a.Kind, a.Prompt, a.Ref), nil default: return errResult("unknown browser tool %q", t.op), nil } } +// StateCapturer is implemented by a browser tool whose toolbox is `persistent`: +// it captures the live cookie jar + localStorage into the per-session state +// Secret. The runner calls it on graceful stop (SIGTERM) so the last login state +// precedes a reap (spec §2.3). Every tool of one toolbox shares one Session, so +// the runner dedupes by Origin and calls this once per toolbox. An `ephemeral` +// toolbox's tool implements it as a no-op (nil store), so a type assertion that +// matches is always safe to invoke. +type StateCapturer interface { + CaptureState(ctx context.Context) error +} + +var _ StateCapturer = (*browserTool)(nil) + +// CaptureState implements StateCapturer: it persists the shared Session's state. +// A no-op for an ephemeral toolbox. Returned errors let the runner log-and-continue. +func (t *browserTool) CaptureState(ctx context.Context) error { + return t.sess.captureAndPersist(ctx) +} + +// afterAct persists session state after a successful acting call for a +// `persistent` toolbox (spec §2.3 "after every acting tool call"). Best-effort: +// a capture failure is LOGGED with toolbox + host context and the acting result +// is returned UNCHANGED — a persistence blip must never turn a successful page +// action into a tool error (repo rule: log + continue; the next call retries). +// A no-op for an ephemeral toolbox (nil store), so the common case costs one +// nil-check. +func (t *browserTool) afterAct(ctx context.Context, res agenttool.Result) agenttool.Result { + if err := t.sess.captureAndPersist(ctx); err != nil { + slog.Warn("browser: capture after acting call failed; continuing", + "toolbox", t.rtName, "op", t.op, "host", t.sess.CurrentHost(), "err", err.Error()) + } + return res +} + +// requestHumanStep asks a person to complete a sign-in step the agent cannot do +// itself. It returns a STATUS only — never a code or secret. +// +// - kind=="confirm": post a login_human_step card with a Done button and BLOCK +// until the person confirms, declines, or the approval window lapses (a push +// approval, say). A timeout is fail-closed: the call reports "timed out", it +// does NOT silently succeed. +// - no kind (or "captcha"): the agent hit a CAPTCHA or other step it cannot do. +// Surface a one-way notice and report that human intervention is needed. This +// does not block — a person takes over out of band. +// +// kind=="code" (ref REQUIRED): post a login_human_step card asking the person to +// type an emailed/SMS code, BLOCK until they answer, then type the code into the +// element `ref` names via the CDP fill path — the same InsertText the type op uses, +// but sourced from the human's code, not a vault credential. The code is typed by +// THIS tool (the runner), never by a model-issued type call, and the result is a +// STATUS only: the code never enters the tool result, the transcript, a log, or the +// model's context. A timeout is fail-closed. +func (t *browserTool) requestHumanStep(ctx context.Context, kind, prompt, ref string) agenttool.Result { + prompt = strings.TrimSpace(prompt) + if prompt == "" { + return errResult("request_human_step requires a prompt describing what the person should do") + } + if t.humanStep == nil { + // Hand-built tool with no requester (a unit test), or a session with no + // human-step wiring. Fail loud rather than nil-panic. + return errResult("request_human_step is unavailable: no human-step requester is configured for this session") + } + + switch kind { + case "confirm": + outcome, err := t.humanStep.Confirm(ctx, prompt) + if err != nil { + return errResult("request_human_step failed: %v", err) + } + switch { + case outcome.Approved: + // Status only — the tool never reveals how the step was completed. + return okResult("the person confirmed the sign-in step; you can continue") + case outcome.Denied: + return errResult("request_human_step: the person declined to complete the sign-in step") + case outcome.TimedOut: + // Fail-closed: a lapsed deadline is NOT a success. + return errResult("request_human_step: timed out waiting for the person to complete the sign-in step") + default: + return errResult("request_human_step: no decision was recorded") + } + case "code": + return t.requestHumanStepCode(ctx, prompt, ref) + case "", "captcha": + // No actionable kind: a CAPTCHA, or any step the agent cannot do. Surface a + // notice and tell the model a person must step in. + if err := t.humanStep.Captcha(ctx, prompt); err != nil { + return errResult("request_human_step: could not notify anyone that human intervention is needed: %v", err) + } + return errResult("request_human_step: this step needs a person and cannot be completed automatically; a notice has been sent asking someone to take over in the browser") + default: + return errResult("request_human_step: unknown kind %q — use \"confirm\" or \"code\", or omit kind to report a CAPTCHA or other step needing a person", kind) + } +} + +// requestHumanStepCode drives the kind:code handoff: block for the person's typed +// code, then type it into the element `ref` names. ref is REQUIRED (the code has to +// go somewhere), checked before blocking a human. The result is a STATUS only; the +// code is never echoed into the result, and the field it was typed into is marked +// for C5 blanking so a later read/screenshot does not surface it either. +func (t *browserTool) requestHumanStepCode(ctx context.Context, prompt, ref string) agenttool.Result { + if ref == "" { + return errResult("request_human_step: kind=\"code\" requires a ref naming the input to type the code into — read the page first and pass the code field's ref") + } + outcome, err := t.humanStep.RequestCode(ctx, prompt) + if err != nil { + return errResult("request_human_step failed: %v", err) + } + switch { + case outcome.TimedOut: + // Fail-closed: a lapsed deadline is NOT a success, and nothing is typed. + return errResult("request_human_step: timed out waiting for the person to provide the code") + case outcome.Denied: + return errResult("request_human_step: the person declined to provide the code") + case outcome.Approved: + if outcome.Code == "" { + // Approved but no code supplied — fail closed rather than type nothing + // and report success. + return errResult("request_human_step: the person approved but provided no code") + } + // The runner types the human's code into the page here — NOT a model-issued + // type call. The code never enters the tool result below. + if res := t.typeHumanCode(ctx, ref, outcome.Code); res.IsError { + return res + } + return okResult(fmt.Sprintf("the person provided the code; it was entered into ref %s", ref)) + default: + return errResult("request_human_step: no decision was recorded") + } +} + +// typeHumanCode focuses the element `ref` names and inserts `code` via the same +// CDP InsertText path typeInto uses for a vault credential — but the source here is +// the human's typed code, and there is no {{login...}} placeholder or password-slot +// gate to apply (the human chose the field by reading the page, and the model never +// saw the value). The filled field is marked for C5 blanking: the code is a secret, +// so a subsequent read/screenshot must not reveal it. The returned Result never +// contains `code`. +func (t *browserTool) typeHumanCode(ctx context.Context, ref, code string) agenttool.Result { + var backendNodeID int64 + var handled *agenttool.Result + if runErr := t.sess.do(ctx, shortTimeout, func(c context.Context) error { + e, ok := t.sess.lookupRef(ref) + if !ok { + r := errResult("ref %s is stale; call read again", ref) + handled = &r + return nil + } + backendNodeID = e.backendNodeID + if ae := runAction(c, func(ac context.Context) error { + return dom.Focus().WithBackendNodeID(cdp.BackendNodeID(backendNodeID)).Do(ac) + }); ae != nil { + return ae + } + return runAction(c, func(ac context.Context) error { + return input.InsertText(code).Do(ac) + }) + }); runErr != nil { + // runErr carries only the CDP transport error, never the code value. + return errResult("entering the code failed: %v", runErr) + } + if handled != nil { + return *handled + } + // The code is a secret: blank the field so read/find/screenshot redact it until + // a real navigation clears the blanking (spec §3.6), exactly as a filled + // credential field is treated. + t.sess.markBlanked(backendNodeID) + return okResult(fmt.Sprintf("entered the code into ref %s", ref)) +} + // decide evaluates host against the toolbox's egress policy. func (t *browserTool) decide(host string) browserpolicy.Decision { return browserpolicy.Decide(t.mode, t.sites, host) diff --git a/pkg/agent/tool/browser/tools_test.go b/pkg/agent/tool/browser/tools_test.go index badca3d3..f0341ac6 100644 --- a/pkg/agent/tool/browser/tools_test.go +++ b/pkg/agent/tool/browser/tools_test.go @@ -47,7 +47,7 @@ func TestScreenshotOfferedOnlyWithImages(t *testing.T) { } hasScreenshot := func(acceptsImages bool) bool { - tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", acceptsImages, nil) + tools, err := Synthesize(rt, "http://127.0.0.1:1", "tok", acceptsImages, nil, nil, nil) if err != nil { t.Fatalf("Synthesize: %v", err) } @@ -80,12 +80,12 @@ func TestActingToolsOfferedOnlyInActMode(t *testing.T) { act := base act.Spec.Interaction = "act" - ro, err := Synthesize(readonly, "http://127.0.0.1:1", "tok", true, nil) + ro, err := Synthesize(readonly, "http://127.0.0.1:1", "tok", true, nil, nil, nil) assert.NoError(t, err) - ac, err := Synthesize(act, "http://127.0.0.1:1", "tok", true, nil) + ac, err := Synthesize(act, "http://127.0.0.1:1", "tok", true, nil, nil, nil) assert.NoError(t, err) - actingSuffixes := []string{"_type", "_click", "_press", "_select", "_submit"} + actingSuffixes := []string{"_type", "_click", "_press", "_select", "_submit", "_request_human_step"} for _, suf := range actingSuffixes { assert.False(t, hasSuffix(toolNames(ro), suf), "readonly must NOT offer %s", suf) assert.True(t, hasSuffix(toolNames(ac), suf), "act must offer %s", suf) diff --git a/pkg/apis/v1alpha1/agentsession_ownership.go b/pkg/apis/v1alpha1/agentsession_ownership.go index 30045854..93df6999 100644 --- a/pkg/apis/v1alpha1/agentsession_ownership.go +++ b/pkg/apis/v1alpha1/agentsession_ownership.go @@ -190,6 +190,7 @@ var ApprovalConditionTypes = map[string]bool{ AgentSessionConditionContentInspectionApprovalPending: true, AgentSessionConditionPreconditionWaiverPending: true, AgentSessionConditionPreferenceConfirmPending: true, + AgentSessionConditionLoginHumanStepPending: true, AgentSessionConditionCredentialRequestPublished: true, AgentSessionConditionInteractPolicyApplied: true, } @@ -212,6 +213,7 @@ var pendingApprovalConditionTypes = map[string]bool{ AgentSessionConditionContentInspectionApprovalPending: true, AgentSessionConditionPreconditionWaiverPending: true, AgentSessionConditionPreferenceConfirmPending: true, + AgentSessionConditionLoginHumanStepPending: true, } // IsPendingApprovalCondition reports whether t, when True, means the session is diff --git a/pkg/apis/v1alpha1/browsertoolbox_types.go b/pkg/apis/v1alpha1/browsertoolbox_types.go index aab4868b..8c6cc40b 100644 --- a/pkg/apis/v1alpha1/browsertoolbox_types.go +++ b/pkg/apis/v1alpha1/browsertoolbox_types.go @@ -42,10 +42,45 @@ type BrowserToolbox struct { // the field, not the type. // +kubebuilder:validation:XValidation:rule="self.access.mode != 'blocklist' || (!has(self.access.identity) && !self.access.identitySites)",message="a blocklist (general-access) browser cannot carry an identity or identitySites" // +kubebuilder:validation:XValidation:rule="!self.access.identitySites || has(self.access.identity)",message="identitySites requires identity" + // A Phase C rule, same reasoning as the two above: an `unattended` site's + // web_host#act grant is written unconditionally at session start (C8, no + // human card) for a bound identity's browsing session, so it needs both an + // allowlist (a blocklist's "every site but these" shape has no per-site + // standing to grant against) and a bound identity (there is no session + // approver to ask if the binding is missing, and the grant is meant to + // stand in for that identity's own sign-in). + // +kubebuilder:validation:XValidation:rule="!self.access.sites.exists(s, s.unattended) || (self.access.mode == 'allowlist' && has(self.access.identity))",message="an unattended site requires access.mode=allowlist and access.identity" + // A Phase C (Task C9) rule, same field-scoped reasoning as the four above: an + // explicit state=persistent captures cookies + localStorage into a per-session + // Secret that survives reap, which is only legitimate for a credentialed + // (identity-bound) browser -- a general browser has nothing to keep and a + // long-lived profile of arbitrary-site cookies is only a tracking surface + // (spec §1.2, §1.4). An UNSET state is left to the AgentSession snapshot to + // default (persistent iff an identity is bound, else ephemeral), so the rule + // short-circuits on absence; it fires only on an EXPLICIT persistent. + // +kubebuilder:validation:XValidation:rule="!has(self.state) || self.state != 'persistent' || has(self.access.identity)",message="a persistent browser requires access.identity" Spec BrowserToolboxSpec `json:"spec,omitempty"` Status BrowserToolboxStatus `json:"status,omitempty"` } +// Browser session-state kinds (BrowserToolboxSpec.State). The CRD states the +// legal set as a kubebuilder Enum marker on that field; these constants are the +// Go-side source of truth the AgentSession snapshot defaulting (controller.go), +// the state-Secret lifecycle (browserpod.go), and the runner's capture/inject +// wiring compare against, so a literal "persistent"/"ephemeral" never drifts. +// +// The field is INTENTIONALLY not defaulted at admission (no kubebuilder:default): +// the effective value is identity-driven and resolved in code at the snapshot +// (spec §1.4), so an unset field must stay distinguishably empty at the API -- +// a static default would mask the identity-driven one. +const ( + // BrowserStateEphemeral: a clean profile every pod boot; nothing captured. + BrowserStateEphemeral = "ephemeral" + // BrowserStatePersistent: cookies + localStorage for the effective sites are + // captured into a per-session Secret and re-injected across reap/wake. + BrowserStatePersistent = "persistent" +) + // +kubebuilder:object:root=true // // BrowserToolboxList is a list of BrowserToolbox. @@ -76,10 +111,18 @@ type BrowserToolboxSpec struct { Interaction string `json:"interaction,omitempty"` // State selects how the browser's session state (cookies, storage) is - // retained across turns. - // Phase A: ephemeral is the only state; persistent arrives with identities (Phase C). - // +kubebuilder:validation:Enum=ephemeral - // +kubebuilder:default=ephemeral + // retained across turns: "persistent" captures cookies + localStorage for the + // effective sites into a per-session Secret and re-injects them across reap and + // wake (spec §1.4); "ephemeral" keeps a clean profile every pod boot and + // captures nothing. An explicit "persistent" requires access.identity (the + // field-scoped XValidation rule on BrowserToolbox.Spec). + // + // DELIBERATELY NOT DEFAULTED at admission. An unset value is resolved in code + // at the AgentSession snapshot (controller.go) to "persistent" when an identity + // is bound and "ephemeral" otherwise -- a static kubebuilder default would mask + // that identity-driven default, so the field is left omitempty and + // enum-but-not-defaulted so an unset field stays distinguishably empty. + // +kubebuilder:validation:Enum=ephemeral;persistent State string `json:"state,omitempty"` } @@ -139,6 +182,26 @@ type BrowserSite struct { // suffix-by-label subdomain match. // +optional Exact bool `json:"exact,omitempty"` + + // Unattended marks this site's web_host#act grant as operator-approved: the + // AgentSession reconciler's runner writes the slot_grant_act relationship + // for this host at session cold start, unconditionally, with no human + // plan-gate card -- the operator's declaration of Unattended IS the + // authorization, exactly as an approved card would be. A Phase C addition; + // the third field-scoped XValidation rule on BrowserToolbox.Spec requires + // access.mode=allowlist and access.identity whenever any site sets this, so + // an unattended grant is always for a specific, bound-identity session and + // never for a general-access (blocklist) browser. + // + // The default marker below lands a structural default on every site, even + // one that omits the field entirely -- without it, the third XValidation + // rule's `s.unattended` on a plain site with no explicit value errors + // evaluation ("no such key: unattended") instead of reading false, and + // CEL's fail-closed-on-error semantics then refuse EVERY BrowserToolbox + // create, not just an unattended one. + // +optional + // +kubebuilder:default=false + Unattended bool `json:"unattended,omitempty"` } // BrowserToolboxStatus is intentionally minimal in Phase A: a condition set diff --git a/pkg/apis/v1alpha1/browsertoolbox_validation_test.go b/pkg/apis/v1alpha1/browsertoolbox_validation_test.go new file mode 100644 index 00000000..97f6ff70 --- /dev/null +++ b/pkg/apis/v1alpha1/browsertoolbox_validation_test.go @@ -0,0 +1,101 @@ +//go:build integration + +// pkg/apis/v1alpha1/browsertoolbox_validation_test.go +// +// Integration test for the BrowserToolbox CEL validation added in Phase C Task +// C9: an EXPLICIT state=persistent requires access.identity, while an UNSET state +// is accepted in every shape (it is defaulted in code at the AgentSession +// snapshot, not at admission — C-R5). Runs against a real envtest apiserver (via +// pkg/controllers/testenv) so the assertion exercises the generated CRD's +// XValidation rule and the enum-but-not-defaulted State field directly, not a +// Go-level pre-check that could drift from what is installed. +package v1alpha1_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/controllers/testenv" +) + +// newBrowserToolbox builds an allowlist BrowserToolbox over one host with the +// given identity (empty = none) and state (empty = unset), for exercising the +// persistent⇒identity CEL rule and the no-default State field in isolation. +func newBrowserToolbox(name, identity, state string) *v1alpha1.BrowserToolbox { + access := v1alpha1.BrowserAccess{ + Mode: "allowlist", + Sites: []v1alpha1.BrowserSite{{Host: "example.com"}}, + } + if identity != "" { + access.Identity = identity + } + return &v1alpha1.BrowserToolbox{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}, + Spec: v1alpha1.BrowserToolboxSpec{ + Access: access, + Interaction: "act", + State: state, + }, + } +} + +func TestBrowserToolboxStateCEL(t *testing.T) { + env := testenv.Start(t) + ctx := context.Background() + + // (c) EXPLICIT persistent without identity: rejected by the CEL rule. + t.Run("explicit persistent without identity: rejected", func(t *testing.T) { + tb := newBrowserToolbox("btb-persist-no-id", "", v1alpha1.BrowserStatePersistent) + err := env.Client.Create(ctx, tb) + require.Error(t, err) + assert.True(t, apierrors.IsInvalid(err), "expected an Invalid API error, got: %v", err) + assert.Contains(t, err.Error(), "a persistent browser requires access.identity") + }) + + // EXPLICIT persistent WITH identity: accepted. + t.Run("explicit persistent with identity: accepted", func(t *testing.T) { + tb := newBrowserToolbox("btb-persist-id", "joey-airlines", v1alpha1.BrowserStatePersistent) + require.NoError(t, env.Client.Create(ctx, tb), + "a persistent toolbox with a bound identity must be accepted") + }) + + // (d) EXPLICIT ephemeral WITH identity: honored as-is (no persistence rule). + t.Run("explicit ephemeral with identity: accepted", func(t *testing.T) { + tb := newBrowserToolbox("btb-ephemeral-id", "joey-airlines", v1alpha1.BrowserStateEphemeral) + require.NoError(t, env.Client.Create(ctx, tb), + "an explicit ephemeral toolbox must be accepted even with an identity") + }) + + // (e) UNSET state is accepted — enum-but-not-defaulted means an omitted field + // does not trip the enum, and the snapshot defaults it in code. + t.Run("unset state with identity: accepted", func(t *testing.T) { + tb := newBrowserToolbox("btb-unset-id", "joey-airlines", "") + require.NoError(t, env.Client.Create(ctx, tb), + "an unset state with an identity must be accepted (snapshot defaults it to persistent)") + // And the stored value stays empty — NOT defaulted at admission — so the + // snapshot can still tell "unset" from an explicit value. + var got v1alpha1.BrowserToolbox + require.NoError(t, env.Client.Get(ctx, client.ObjectKeyFromObject(tb), &got)) + assert.Empty(t, got.Spec.State, "state must stay unset at the API (no admission default)") + }) + + t.Run("unset state without identity: accepted", func(t *testing.T) { + tb := newBrowserToolbox("btb-unset-no-id", "", "") + require.NoError(t, env.Client.Create(ctx, tb), + "an unset state with no identity must be accepted (snapshot defaults it to ephemeral)") + }) + + t.Run("invalid state value: rejected by enum", func(t *testing.T) { + tb := newBrowserToolbox("btb-bad-state", "joey-airlines", "sticky") + err := env.Client.Create(ctx, tb) + require.Error(t, err) + assert.True(t, apierrors.IsInvalid(err), "expected an Invalid API error, got: %v", err) + }) +} diff --git a/pkg/apis/v1alpha1/conditions.go b/pkg/apis/v1alpha1/conditions.go index 86032a40..37856d02 100644 --- a/pkg/apis/v1alpha1/conditions.go +++ b/pkg/apis/v1alpha1/conditions.go @@ -72,7 +72,7 @@ const ( // message. Distinct from ReasonPodCrashed (PodFailed phase), since these pods // never reach PodFailed — they retry forever — so without this the bundle (and // its waiting AgentSession) would hang silently. - ReasonPodStartFailed = "PodStartFailed" + ReasonPodStartFailed = "PodStartFailed" // ReasonWorkspaceProvisioningFailed marks a session whose workspace (or // snapshot-store) PVC could not be provisioned by its StorageClass — an // undersized request the backend rejects (Filestore's minimum share size), @@ -84,12 +84,12 @@ const ( // generic "did not become Ready" text. ReasonWorkspaceProvisioningFailed = "WorkspaceProvisioningFailed" ReasonNodeEvicted = "NodeEvicted" - ReasonClassMissing = "ClassMissing" - ReasonClassInvalid = "ClassInvalid" - ReasonIdleTTL = "IdleTTL" - ReasonMaxDuration = "MaxDuration" - ReasonUserDelete = "UserDelete" - ReasonToolspecNotInClass = "ToolspecNotInClass" + ReasonClassMissing = "ClassMissing" + ReasonClassInvalid = "ClassInvalid" + ReasonIdleTTL = "IdleTTL" + ReasonMaxDuration = "MaxDuration" + ReasonUserDelete = "UserDelete" + ReasonToolspecNotInClass = "ToolspecNotInClass" ) // ToolCall condition types. @@ -868,6 +868,13 @@ const ( // AgentSessionConditionPreferenceConfirmPending is True while a user_preference_confirm // decision is outstanding: the addressee is being asked to confirm a preference update. AgentSessionConditionPreferenceConfirmPending = "PreferenceConfirmPending" + // AgentSessionConditionLoginHumanStepPending is True while a login_human_step + // decision is outstanding: a browser toolbox's request_human_step is blocked + // asking a person to complete a sign-in step the agent cannot do itself (a push + // approval today; an emailed/SMS code in a later slice). Same generic-park-handler + // machinery and observable-only status as the approval conditions above — the + // phase stays derived from the runner's signed lifecycle projection. + AgentSessionConditionLoginHumanStepPending = "LoginHumanStepPending" // AgentSessionConditionScopeReviewPending is True while a cold-start scope_review // decision is outstanding and turn-0 execution is blocked. Written by the operator // from the folded lifecycle projection; cleared when the decision resolves. diff --git a/pkg/authz/engine/engine.go b/pkg/authz/engine/engine.go index 539c0cc6..317b4982 100644 --- a/pkg/authz/engine/engine.go +++ b/pkg/authz/engine/engine.go @@ -211,6 +211,22 @@ type BindingLifecycle interface { // returns nil in BOTH cases: a timeout means the caller proceeds without // freshly-extracted bindings, never that it proceeds without a Check. WaitForExtraction(ctx context.Context, scope memory.Scope, inboxIdx int, deadline time.Duration) error + + // GrantUnattendedSlots writes a slot grant per binding UNCONDITIONALLY — + // no Check, no session-scope write, no human card. This is deliberately + // NOT BindClassDefaults' shape: BindClassDefaults Checks each default + // against the subject's EXISTING standing, which is exactly wrong for a + // slot whose own standing arm is inert (web_host's `owner` leg is never + // written — see browserschema.Resource) — the Check would always deny and + // admissibleCandidates would drop every candidate, binding nothing. + // + // GrantUnattendedSlots exists for the one case where the CALLER'S OWN + // DECLARATION is the authorization — an operator's BrowserSite.Unattended + // (C8) — exactly as an approved plan-gate card writes its grant directly + // with no re-Check (see authz.BindApproved). Mirrors authz.GrantSlots, + // with the same session-expiration-derived TTL BindClassDefaults uses. A + // nil RelWriter or empty bindings is a no-op. + GrantUnattendedSlots(ctx context.Context, sess authz.SessionRef, bindings []authz.SlotBinding) error } // Dependency interfaces — kept narrow so tests fake individual methods. @@ -499,3 +515,13 @@ func (i *impl) WaitForExtraction(ctx context.Context, scope memory.Scope, inboxI } return authz.WaitForExtraction(ctx, i.d.Memory, scope, inboxIdx, deadline) } + +// GrantUnattendedSlots writes bindings straight to authz.GrantSlots, with no +// Check and no session-scope write — see the interface doc for why this must +// NOT route through BindClassDefaults' admissibleCandidates gate. +func (i *impl) GrantUnattendedSlots(ctx context.Context, sess authz.SessionRef, bindings []authz.SlotBinding) error { + if i.d.RelWriter == nil || len(bindings) == 0 { + return nil + } + return authz.GrantSlots(ctx, i.d.RelWriter, sess, bindings, authz.SlotGrantExpiry(time.Now(), i.d.SessionExpiration)) +} diff --git a/pkg/authz/guardian/approval/applied.go b/pkg/authz/guardian/approval/applied.go index bf5ae9dd..ae48bfbb 100644 --- a/pkg/authz/guardian/approval/applied.go +++ b/pkg/authz/guardian/approval/applied.go @@ -57,11 +57,20 @@ func FromApplied(pl channelevents.InteractionAppliedPayload) (d Decision, delive // Denied and expired both resume as false; only an explicit approval is // true. A client-side timeout that already forgot the request makes the // false a no-op. + // + // ResponseText rides the SAME arm as the approve/deny boolean because the + // code path IS an approval (login_human_step kind:code): the human both + // approves AND supplies the free-text code. It is distinct from the + // ResumeChoice arm's OutcomeText, which carries a CLOSED action id, not a + // typed string. Empty for every confirm/tool/content/plan approval, so + // threading it here is a no-op for them. SECURITY: consumed only by the + // runner's human-step handler, never surfaced to the model. return Decision{ - Approved: pl.Outcome == channelevents.OutcomeApproved, - ApproverID: approverID, - Approver: approver, - Reason: pl.Reason, + Approved: pl.Outcome == channelevents.OutcomeApproved, + ApproverID: approverID, + Approver: approver, + Reason: pl.Reason, + ResponseText: pl.ResponseText, }, true, true default: diff --git a/pkg/authz/guardian/approval/applied_test.go b/pkg/authz/guardian/approval/applied_test.go index 41d87437..3b186acd 100644 --- a/pkg/authz/guardian/approval/applied_test.go +++ b/pkg/authz/guardian/approval/applied_test.go @@ -20,6 +20,39 @@ func applied(category, outcome, outcomeText string) channelevents.InteractionApp } } +// TestFromApplied_LoginHumanStepCarriesResponseText is the C7 carrier crux: a +// login_human_step approval that carries a free-text code (the human's typed +// login code) delivers it on Decision.ResponseText — a GENUINE text field, not +// smuggled through the closed OutcomeText/Action. This is the one thing the +// confirm half never needed and the code half cannot work without. +func TestFromApplied_LoginHumanStepCarriesResponseText(t *testing.T) { + pl := applied(categories.LoginHumanStep, channelevents.OutcomeApproved, "") + pl.ResponseText = "123456" + + d, deliver, registered := approval.FromApplied(pl) + + require.True(t, registered) + require.True(t, deliver) + assert.True(t, d.Approved, "the code path is an approval") + assert.Equal(t, "123456", d.ResponseText, "the typed code must ride back as free text") + assert.Empty(t, d.Action, "the code must NOT be smuggled through the closed Action field") +} + +// TestFromApplied_DeniedCodeCarriesNoText proves a denial drops the free text — +// a declined code must not resume as approved and must carry nothing to type. +func TestFromApplied_DeniedCodeCarriesNoText(t *testing.T) { + pl := applied(categories.LoginHumanStep, channelevents.OutcomeDenied, "") + pl.ResponseText = "should-not-matter" + + d, deliver, registered := approval.FromApplied(pl) + + require.True(t, registered) + require.True(t, deliver) + assert.False(t, d.Approved, "a denial is not an approval") + // The runner's RequestCode only reads the code on an approval, so even a + // (malformed) denial that carried text never reaches the page. +} + // The guard that was missing. Every category the runner blocks on must resume // off an applied envelope; a category that does not is a session that publishes // its prompt, accepts the click, and then hangs until its approval timeout with @@ -62,6 +95,15 @@ func TestFromApplied_EveryRunnerGateResumes(t *testing.T) { wantDeliver: true, check: func(t *testing.T, d approval.Decision) { assert.True(t, d.Approved) }, }, + { + // login_human_step (confirm half): a pure approve/deny the runner's + // request_human_step block resumes off. Registered here proves the + // browser human-step card can never hang the runner the way plan_phase + // once did — the whole reason FromApplied is registry-driven. + name: "login_human_step: approved → Approved=true", category: categories.LoginHumanStep, + wantDeliver: true, + check: func(t *testing.T, d approval.Decision) { assert.True(t, d.Approved) }, + }, { // The choice shape: a yes/no cannot say WHICH identity was picked, so // the answer has to survive as Action. diff --git a/pkg/authz/guardian/approval/orchestrator.go b/pkg/authz/guardian/approval/orchestrator.go index 824b9908..28a3780b 100644 --- a/pkg/authz/guardian/approval/orchestrator.go +++ b/pkg/authz/guardian/approval/orchestrator.go @@ -46,6 +46,17 @@ type Decision struct { // ColdStart action. Empty for mid-session approvals and tool-call // approvals, where Approved alone determines the outcome. Action string + + // ResponseText is a FREE-TEXT answer the human typed, distinct from the closed + // Action (which names a declared choice). Today only login_human_step kind:code + // sets it — the emailed/SMS code the person entered — which the runner types + // into the page element the agent named. + // + // SECURITY: a secret on the code path. It reaches the blocked gate ONLY through + // FromApplied's ResumeApproval arm and is consumed by the runner's human-step + // handler; it is NEVER returned as a tool result, logged, written to the + // transcript, or shown to the model. Empty for every approve/deny/timeout. + ResponseText string } // Request describes a single approval pause. RequestID is the unique diff --git a/pkg/authz/hooks/plangate.go b/pkg/authz/hooks/plangate.go index e47906c3..6671f6a9 100644 --- a/pkg/authz/hooks/plangate.go +++ b/pkg/authz/hooks/plangate.go @@ -144,6 +144,15 @@ type PlanGateDeps struct { // declared for that type. ResourceDisplays map[string]plangate.ResourceDisplay + // WebHostIdentityLabels maps a web_host slot's host to the bound + // identity login's display label (the SAME map the runner holds — + // Loop.PlanGateWebHostIdentityLabels, from + // runner.BrowserHostIdentityLabels). Display only: threaded into every + // card so the browser toolbox's acting line can name the signing + // identity ("act on aa.com, signing in as