fix(berdctl): authenticate local control requests - #272
Conversation
Require a fresh per-process bearer capability on every loopback route and secure its discovery transport and restart behavior. Co-authored-by: Kalvin Chau <kalvin@block.xyz> Signed-off-by: Kalvin Chau <kalvin@block.xyz> Co-authored-by: Goose <opensource@block.xyz>
Co-authored-by: Goose <opensource@block.xyz>
Co-authored-by: Goose <opensource@block.xyz>
Co-authored-by: Goose <opensource@block.xyz>
Co-authored-by: Goose <opensource@block.xyz>
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
APPROVE. Fresh static review of the exact three-dot comparison 8e7e350...a0bc89f found no publishable findings. The review covered the authenticated berdctl discovery, handshake, request, retry, broker lifecycle, permission-hardening, stale-file cleanup, protocol-version, documentation, and test changes. Accessibility, i18n, navigation, and consent surfaces are not changed by this PR. Supplied GitHub evidence was inspected: all ten reported check runs completed successfully; required checks still independently govern merge readiness.
Deterministic publication result: 0 blocking and 0 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
Pending checks: 1 check(s) are not complete.
This approval reflects the completed code review only; merge readiness remains governed by the repository's required checks.
morgmart
left a comment
There was a problem hiding this comment.
🤖 Automated code review
COMMENT. The completed four-lens review found one new non-blocking issue: newly added privacy and credential rejection states are surfaced through generic reachability guidance, which can send people toward ineffective recovery steps. Atlas, Spar, and Engineering otherwise found the bounded transport hardening coherent and sound. Supplied evidence was inspected: all ten reported check runs completed successfully, while the combined commit status was pending; required checks still independently govern merge readiness.
Deterministic publication result: 0 blocking and 1 non-blocking finding(s) publishable; 0 duplicate(s) suppressed.
| || parent_metadata.uid() != current_uid | ||
| || parent_metadata.mode() & 0o077 != 0 | ||
| { | ||
| return Err(format!( |
There was a problem hiding this comment.
🤖 P2 · Security failures misdiagnose reachability (non-blocking)
The new private-file validation can reject a running Berd instance for ownership or permission reasons, but the existing final error wrapper describes every rejection as an unavailable control endpoint and says Berd may not be running or app control may be disabled. The new exhausted authentication-retry path likewise reports that Berd is not reachable after it returned 403. These newly introduced security states therefore lose their actionable diagnosis.
User effect: People whose app-control file has unsafe permissions or whose credentials remain stale will be told to check whether Berd is running, so they can repeat ineffective steps without learning how to restore the connection safely.
Recommended fix: Preserve the failure category and provide recovery matched to it: describe privacy validation as an app-control file that is not private to the user account, describe exhausted authentication as changed app-control credentials, and reserve running/enabled guidance for missing or unreachable endpoints. Keep OS-specific mode or ACL details secondary to the plain-language cause and next action.
Test: Add discriminating error-mapping tests showing that an unsafe-permission discovery record produces privacy-specific recovery, an exhausted authenticated 403 produces credential-specific recovery, and a genuinely missing or unreachable endpoint retains the running/enabled guidance.
Category: fix
User Impact: Berd app control now rejects commands from other local OS users while continuing to support same-user companion tools that invoke
berdctl.Problem: The release broker trusted loopback reachability and browser-oriented headers as if they identified the owning OS user. Another local account could scan for the endpoint and read, inject, or destructively modify sessions.
Solution: Generate a fresh capability for every broker start, publish it only in the owner-private discovery record, and require it on ping and call requests before consuming request bodies. The bundled CLI handles authentication and bounded restart recovery transparently, preserving existing
BERDCTL_LOCKintegrations.File changes
docs/berdctl-architecture.md
Documents capability authentication, its same-user trust boundary, and protocol versioning.
src-tauri/README.md
Updates the broker and CLI lifecycle documentation for authenticated requests.
src-tauri/Cargo.lock
Locks the capability, constant-time comparison, and Windows security dependencies.
src-tauri/crates/berdctl/Cargo.toml
Adds platform support needed to validate private discovery files.
src-tauri/crates/berdctl/api-surface-feedback.json
Records the authenticated wire protocol version for feedback-enabled builds.
src-tauri/crates/berdctl/api-surface.json
Records the authenticated wire protocol version for standard builds.
src-tauri/crates/berdctl/src/client.rs
Reads and redacts the capability, authenticates ping and call requests without proxies or redirects, and safely handles one broker restart without duplicate dispatch.
src-tauri/crates/berdctl/src/discovery.rs
Strictly validates capability records, ownership, permissions, object type, symlinks or reparse points, ACLs, and size on Unix and Windows.
src-tauri/crates/berdctl/src/main.rs
Passes discovery context through the authenticated call lifecycle.
src-tauri/crates/berdctl/src/validate.rs
Updates validation fixtures for protocol version 5.
src-tauri/plugins/berdctl/Cargo.toml
Adds random generation, constant-time comparison, and platform security dependencies.
src-tauri/plugins/berdctl/src/discovery.rs
Publishes capabilities atomically into private records, including protected Windows ACLs and replacement behavior.
src-tauri/plugins/berdctl/src/lib.rs
Generates and rotates a capability for every actual broker start.
src-tauri/plugins/berdctl/src/server.rs
Requires constant-time bearer authentication before body consumption and enforces a bounded call envelope.
src-tauri/src/services/berdctl_discovery.rs
Recognizes and cleans randomized temporary discovery-file names.
src/features/berdctl/commands/contract.ts
Bumps the renderer’s mirrored wire protocol version.
Validation
just fmt-checkjust checkjust tauri-checkjust tauri-testjust clippyjust testjust buildjust ciadditionally reached one unrelated macOS release-script fixture requiringditto, which is unavailable on the Linux workstation. Every platform-relevant stage above passed independently.