Harden browser workbench control and guidance#139
Merged
glittercowboy merged 5 commits intomainfrom May 2, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThis PR extends the browser subsystem with pending-approval workflows for sensitive actions, richer frame/session metadata and staleness checks, tool policy classification/enforcement, a browser updater with digest verification and redaction, runtime capability flags, and updated PI tool descriptions/tests. ChangesBrowser Session Control & Sensitive Action Approval
Browser Update Module
Sequence DiagramsequenceDiagram
participant Client
participant Manager as Browser Manager
participant Service as Browser Service
participant Approver as Approval System
Client->>Manager: BrowserToolCall (sensitive)
Manager->>Manager: classify risk\nvalidate policy
alt Allowed by policy
Manager->>Service: Execute tool call
Service-->>Manager: ToolResult
Manager-->>Client: ToolResult
else Requires approval
Manager->>Manager: create pendingApproval\n(nonce, hash, expiry)
Manager->>Service: send BrowserSensitiveActionRequest
Service-->>Approver: forward request
Approver-->>Service: BrowserSensitiveActionResponse
Service-->>Manager: SensitiveActionResponse
Manager->>Manager: validate nonce/expiry/params
alt Approved
Manager->>Manager: restore owner if needed
Manager->>Service: execute stored ToolCall
Service-->>Manager: ToolResult
Manager-->>Client: ToolResult
else Denied
Manager->>Service: create BrowserEvidenceCreated
Manager-->>Client: ToolResult (denied/blocked)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 5/10 reviews remaining, refill in 26 minutes and 3 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 8: The go.mod currently pins github.com/gsd-build/protocol-go to a
non-existent tag v0.35.0 which breaks CI; update go.mod to reference a valid
existing revision by either replacing the version string
"github.com/gsd-build/protocol-go v0.35.0" with an existing tag or
pseudo-version (or a specific commit hash) or create and push the v0.35.0 tag in
the remote repo; ensure you run `go get`/`go mod tidy` locally to verify the
chosen replacement resolves and commits the updated go.mod and go.sum.
In `@internal/browser/manager.go`:
- Around line 821-835: The code marks pending.Consumed = true but never removes
entries from m.pendingApprovals, causing unbounded growth; update the handling
in the expiration branch and the successful validation branch in the same
critical section to delete the entry from m.pendingApprovals (e.g.,
delete(m.pendingApprovals, pending.ID)) before calling
m.restoreApprovalOwnerLocked(pending) and before releasing m.mu so cleanup
happens while locked; ensure both branches (the
time.Now().After(pending.ExpiresAt) path and the path after
validateApprovalResponse succeeds) perform the deletion under the same lock and
only then unlock.
- Around line 729-731: Replace predictable timestamp-based approvalID and nonce
generation in the approval flow (where approvalID and nonce are created) with
cryptographically secure random tokens: add a helper function like randomToken(n
int) that uses crypto/rand.Read and hex.EncodeToString to produce an n-byte
token and falls back to the existing timestamp string only on read error, then
call randomToken(...) to set approvalID and nonce instead of
fmt.Sprintf("approval_%d", ...) and fmt.Sprintf("nonce_%d", ...); keep expiresAt
logic unchanged.
- Around line 837-840: The denial branch only sends evidence and returns an
error, leaving the tool call unresolved; update the if !msg.Approved block to,
after calling m.sendBrowserEvidence(ctx, req, toolCall.BrowserID, "denied",
"approval", msg.DeniedReason), also emit the terminal events (e.g., call the
same/appropriate methods that publish BrowserToolCallUpdated and
BrowserToolResult for toolCall.BrowserID with a denied/failure status and
include msg.DeniedReason), then return the fmt.Errorf("browser action denied");
ensure you use the existing event-publishing helpers used elsewhere in this file
so consumers get a final state even on denial (reference symbols:
m.sendBrowserEvidence, BrowserToolCallUpdated, BrowserToolResult, msg.Approved,
msg.DeniedReason, toolCall.BrowserID).
In `@internal/browser/safety.go`:
- Around line 42-52: The current check in the upload_file handler just tests for
non-empty payload.FileToken or payload.SelectedToken allowing forged tokens;
replace the non-empty checks with real provenance validation: call a token
verification routine (e.g., VerifyLexSelectedToken(token) or lookup against the
server-side token store) for payload.FileToken and payload.SelectedToken and
only allow when that verifier returns true, and preserve any allowlistedPath
logic; update the branch that currently returns nil so it only returns nil after
successful VerifyLexSelectedToken/TokenStore.Get validation, otherwise return
ErrBrowserPolicy("upload_file requires a Lex-selected file token").
In `@internal/browser/service.go`:
- Around line 90-110: The Frame JSON struct in internal/browser/service.go is
missing the latencyMs and latestAcceptedFrameSeq fields so Frame.LatencyMS and
Frame.LatestAcceptedFrameSeq stay zero; add fields with the correct types and
JSON tags (e.g., LatencyMS int64 `json:"latencyMs"` and LatestAcceptedFrameSeq
int64 or int `json:"latestAcceptedFrameSeq"`) to the struct used by cloud_frame
and mirror the same change in the second identical struct block (lines ~115-135)
so both decoding paths populate Frame.LatencyMS and
Frame.LatestAcceptedFrameSeq.
- Around line 44-49: When req.Mode == "identity" ensure we fail closed if
req.IdentityKey is empty: add a validation branch in the service logic (where
args is built) that checks req.Mode, req.IdentityKey and returns an error (or
refuses to start) when IdentityKey == "" instead of silently omitting identity
args; keep the existing behavior of appending
"--identity-scope"/"--identity-key" and the "--identity-project" when
req.IdentityScope == "project" and req.IdentityProjectID is set, but enforce the
non-empty IdentityKey requirement before appending and abort with a clear error
if it's missing.
In `@internal/browser/updater.go`:
- Around line 58-67: The code currently skips digest verification when u.Fetch
is nil, allowing execution to continue to Runner without integrity checks;
update the update flow (the method using u.Fetch, req.Source, req.Digest and the
subsequent Runner call) to enforce integrity: if u.Fetch is nil or req.Digest is
empty, return an error instead of proceeding, and if u.Fetch returns bytes
verify sha256 matches strings.TrimPrefix(req.Digest, "sha256:") as already
implemented; in short, prevent running the Runner when no Fetch/digest
verification is available by returning a clear error from the same function.
- Around line 82-93: RedactBrowserUpdateLog currently only replaces the
token/approval prefixes and leaves the secret values intact; update
RedactBrowserUpdateLog to fully mask secret values by replacing the entire token
value and approval identifier, e.g., use a regexp to find `token=` followed by
any non-whitespace/non-& delimiters and replace the whole `token=...` with
`token=[redacted]`, and similarly replace `approval_[^_]+_` (the approval id
between the prefix and next underscore) with `approval_[redacted]_`; keep the
existing path check (HasPrefix "/") and ensure the function returns the fully
redacted string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c464cddb-5b7a-4be5-95f0-86c438bb50ac
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
go.modinternal/browser/manager.gointernal/browser/manager_test.gointernal/browser/runtime_probe.gointernal/browser/safety.gointernal/browser/safety_test.gointernal/browser/service.gointernal/browser/types.gointernal/browser/updater.gointernal/browser/updater_test.gointernal/loop/daemon.gointernal/pi/extension/browser-extension.tsinternal/pi/extension/browser-tool.test.mjs
| github.com/coder/websocket v1.8.14 | ||
| github.com/creack/pty v1.1.24 | ||
| github.com/gsd-build/protocol-go v0.34.0 | ||
| github.com/gsd-build/protocol-go v0.35.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify tag existence on remote.
git ls-remote --tags https://github.com/gsd-build/protocol-go.git | rg 'refs/tags/v0\.35\.0$' || trueRepository: gsd-build/daemon
Length of output: 42
Tag v0.35.0 does not exist; unpublished revision blocks CI and reproducible builds.
Line 8 pins github.com/gsd-build/protocol-go v0.35.0, which cannot be resolved during builds. The tag must be created and pushed to the remote repository, or the dependency must be pinned to an existing commit or tag.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 8, The go.mod currently pins github.com/gsd-build/protocol-go
to a non-existent tag v0.35.0 which breaks CI; update go.mod to reference a
valid existing revision by either replacing the version string
"github.com/gsd-build/protocol-go v0.35.0" with an existing tag or
pseudo-version (or a specific commit hash) or create and push the v0.35.0 tag in
the remote repo; ensure you run `go get`/`go mod tidy` locally to verify the
chosen replacement resolves and commits the updated go.mod and go.sum.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/browser/safety.go (1)
48-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFormat-only upload token checks are still forgeable.
Lines 48-49 now require a
lex_file_...-shaped token, butvalidLexSelectedFileTokenonly validates prefix/length/charset. Any caller can mint a string that matches that shape and bypass the policy, soupload_filestill is not tied to a server-issued selected-file token.Also applies to: 54-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/safety.go` around lines 48 - 49, The current check using validLexSelectedFileToken on payload.FileToken/payload.SelectedToken only verifies token shape and is forgeable; replace these shape-only checks with a server-backed verification that ties tokens to issued selected-file records (e.g., call a verifySelectedFileToken(token) that checks a server-side mapping/DB or validates an HMAC/signed token and ensures the token was issued for this user/upload), and apply the same replacement for the other occurrences in the block (the checks around lines 54-65); update callers that rely on validLexSelectedFileToken to use the new verify method so upload_file cannot be bypassed by a crafted lex_file_... string.internal/browser/service.go (1)
47-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject project-scoped identity launches that have no project ID.
Lines 49-50 append
--identity-projecteven whenreq.IdentityProjectIDis empty, so project-scoped identity mode still starts with an underspecified scope instead of failing closed.Suggested fix
func (s LocalService) Start(ctx context.Context, req OpenRequest) error { if req.Mode == "identity" && req.IdentityKey == "" { return fmt.Errorf("identity browser mode requires an identity key") } + if req.Mode == "identity" && req.IdentityScope == "project" && req.IdentityProjectID == "" { + return fmt.Errorf("identity browser mode requires a project id for project scope") + } args := []string{"--session", req.GrantID} if req.Mode == "identity" && req.IdentityKey != "" { args = append(args, "--identity-scope", req.IdentityScope, "--identity-key", req.IdentityKey) if req.IdentityScope == "project" { args = append(args, "--identity-project", req.IdentityProjectID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/browser/service.go` around lines 47 - 50, The code appends "--identity-project" even when req.IdentityProjectID is empty, allowing a project-scoped identity launch with an underspecified scope; update the logic that handles req.Mode/req.IdentityKey/req.IdentityScope so that when req.IdentityScope == "project" you validate req.IdentityProjectID is non-empty and return an error (or otherwise fail the request) instead of appending the flag, and only append "--identity-project" when req.IdentityProjectID is present; refer to the existing variables/expressions req.Mode, req.IdentityKey, req.IdentityScope, req.IdentityProjectID and the code that appends to args to locate where to add this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/browser/manager.go`:
- Around line 918-930: The handler Manager.sendBrowserEvidence currently
hardcodes RedactionStatus: "safe" while passing caller-controlled summary text
(e.g., msg.DeniedReason) into Summary; change this so you do not mark untrusted
text as safe: either add a redactionStatus parameter to
Manager.sendBrowserEvidence (or derive it from the OpenRequest/msg context) and
set RedactionStatus to that value, or ensure you sanitize/escape the summary
before marking it "safe"; update all callers (where msg.DeniedReason is passed)
to supply the correct redaction status (e.g., "unredacted" when raw denied
reasons are forwarded) and stop hardcoding RedactionStatus in
sendBrowserEvidence.
- Around line 274-277: The code sets current.lastFrameCapturedAt = time.Now(),
causing buffered/delayed frames to appear fresh; replace time.Now() with the
browser-reported timestamp from the incoming frame (e.g., frame.CapturedAt or
frame.Timestamp) when updating current.lastFrameCapturedAt in the
m.byID[browserID] update path, and add a safe fallback to time.Now() only if
that frame timestamp is zero/unset; keep the existing sequence check using
frame.Sequence and update current.lastFrameSeq as-is.
---
Duplicate comments:
In `@internal/browser/safety.go`:
- Around line 48-49: The current check using validLexSelectedFileToken on
payload.FileToken/payload.SelectedToken only verifies token shape and is
forgeable; replace these shape-only checks with a server-backed verification
that ties tokens to issued selected-file records (e.g., call a
verifySelectedFileToken(token) that checks a server-side mapping/DB or validates
an HMAC/signed token and ensures the token was issued for this user/upload), and
apply the same replacement for the other occurrences in the block (the checks
around lines 54-65); update callers that rely on validLexSelectedFileToken to
use the new verify method so upload_file cannot be bypassed by a crafted
lex_file_... string.
In `@internal/browser/service.go`:
- Around line 47-50: The code appends "--identity-project" even when
req.IdentityProjectID is empty, allowing a project-scoped identity launch with
an underspecified scope; update the logic that handles
req.Mode/req.IdentityKey/req.IdentityScope so that when req.IdentityScope ==
"project" you validate req.IdentityProjectID is non-empty and return an error
(or otherwise fail the request) instead of appending the flag, and only append
"--identity-project" when req.IdentityProjectID is present; refer to the
existing variables/expressions req.Mode, req.IdentityKey, req.IdentityScope,
req.IdentityProjectID and the code that appends to args to locate where to add
this validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d4103df8-e2b9-4947-9c4b-1ae32bfbb1c1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
internal/browser/manager.gointernal/browser/manager_test.gointernal/browser/safety.gointernal/browser/safety_test.gointernal/browser/service.gointernal/browser/updater.gointernal/browser/updater_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/browser/safety_test.go
- internal/browser/updater_test.go
Comment on lines
274
to
277
| if current := m.byID[browserID]; current == state && frame.Sequence > current.lastFrameSeq { | ||
| current.lastFrameSeq = frame.Sequence | ||
| current.lastFrameCapturedAt = time.Now() | ||
| } |
There was a problem hiding this comment.
Use the browser-reported capture time for freshness checks.
Line 276 stores time.Now() into lastFrameCapturedAt. That makes delayed or buffered frames look fresh, so the new staleByAge guard can accept inputs against an already-stale image.
Suggested fix
m.mu.Lock()
if current := m.byID[browserID]; current == state && frame.Sequence > current.lastFrameSeq {
current.lastFrameSeq = frame.Sequence
- current.lastFrameCapturedAt = time.Now()
+ capturedAt := time.Now()
+ if frame.CapturedAt != "" {
+ if ts, err := time.Parse(time.RFC3339Nano, frame.CapturedAt); err == nil {
+ capturedAt = ts
+ }
+ }
+ current.lastFrameCapturedAt = capturedAt
}
m.mu.Unlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager.go` around lines 274 - 277, The code sets
current.lastFrameCapturedAt = time.Now(), causing buffered/delayed frames to
appear fresh; replace time.Now() with the browser-reported timestamp from the
incoming frame (e.g., frame.CapturedAt or frame.Timestamp) when updating
current.lastFrameCapturedAt in the m.byID[browserID] update path, and add a safe
fallback to time.Now() only if that frame timestamp is zero/unset; keep the
existing sequence check using frame.Sequence and update current.lastFrameSeq
as-is.
Comment on lines
+918
to
+930
| func (m *Manager) sendBrowserEvidence(ctx context.Context, req OpenRequest, browserID string, status string, eventType string, summary string) error { | ||
| return m.sender.Send(ctx, &protocol.BrowserEvidenceCreated{ | ||
| Type: protocol.MsgTypeBrowserEvidenceCreated, | ||
| EvidenceID: fmt.Sprintf("evidence_%d", time.Now().UnixNano()), | ||
| BrowserID: browserID, | ||
| SessionID: req.SessionID, | ||
| ChannelID: req.ChannelID, | ||
| Actor: "daemon", | ||
| Status: status, | ||
| EventType: eventType, | ||
| Summary: summary, | ||
| RedactionStatus: "safe", | ||
| CreatedAt: time.Now().UTC().Format(time.RFC3339Nano), |
There was a problem hiding this comment.
Don't mark unredacted evidence text as safe.
Lines 918-930 always emit RedactionStatus: "safe", but the denial path passes msg.DeniedReason straight through as summary. That is caller-controlled text, so downstream consumers can treat unsanitized content as already scrubbed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/browser/manager.go` around lines 918 - 930, The handler
Manager.sendBrowserEvidence currently hardcodes RedactionStatus: "safe" while
passing caller-controlled summary text (e.g., msg.DeniedReason) into Summary;
change this so you do not mark untrusted text as safe: either add a
redactionStatus parameter to Manager.sendBrowserEvidence (or derive it from the
OpenRequest/msg context) and set RedactionStatus to that value, or ensure you
sanitize/escape the summary before marking it "safe"; update all callers (where
msg.DeniedReason is passed) to supply the correct redaction status (e.g.,
"unredacted" when raw denied reasons are forwarded) and stop hardcoding
RedactionStatus in sendBrowserEvidence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Dependency order
Post-merge actions
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores