feat(external): support macOS desktop credentials#983
Conversation
Pre-commit-gate: 5f9c0a5
|
Rebased this work onto current Current head: Verification run locally:
Security note: the desktop password value is never stored in config or passed on argv; only the environment variable name is configured. |
|
Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 7:30 AM ET / 11:30 UTC. Summary Reproducibility: yes. for the review finding from source inspection: the PR head passes real ARD credentials into the local bridge, the handoff fetches /credentials, and the handler returns username/password. I did not run a live External macOS WebVNC endpoint. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Land this only after ARD mode keeps desktop credentials inside the Go relay, live External macOS proof is added or explicitly waived, conflicts are resolved, and maintainers accept the new External desktop configuration surface. Do we have a high-confidence way to reproduce the issue? Yes for the review finding from source inspection: the PR head passes real ARD credentials into the local bridge, the handoff fetches /credentials, and the handler returns username/password. I did not run a live External macOS WebVNC endpoint. Is this the best way to solve the issue? No: the provider-hook direction fits the repository boundary, but the implementation must keep ARD credentials out of the browser/local viewer and still needs live proof plus maintainer acceptance. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0d768f6f5438. Label changesLabel justifications:
Evidence reviewedSecurity concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (9 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Maintainer repair pushed in 8b7ac021. Fixed the P1 credential-boundary finding: Verification:
Remaining merge gate: this repairs the source-bound credential bug, but does not claim the still-requested live macOS External/WebVNC behavior proof. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
P2
|
CI follow-up for 8b7ac021: all 9 checks are green, including the full Go race/modules/coverage gate, connector lifecycles, Apple VM, Worker, docs/scripts, release snapshot, and Socket checks. CI run: https://github.com/openclaw/crabbox/actions/runs/28842057791. The completed ClawSweeper re-review confirms no concrete security bug remains and rates patch quality platinum. The PR remains intentionally unmerged because real macOS External/WebVNC behavior proof and maintainer acceptance of the feature surface are still required. |
…et-provider # Conflicts: # internal/cli/coordinator.go # internal/cli/rfb_screenshot.go # internal/cli/webvnc.go # internal/cli/webvnc_test.go
…et-provider # Conflicts: # internal/cli/webvnc.go # internal/cli/webvnc_test.go
2 participants
Replaces #976 after its stack base merged into main.
Summary
externalprovider to preserve Linux, macOS, native Windows, and WSL2 targets, including ARM64 hostswebvnc --preflight, including RFB 3.3, 3.7, 3.8, and accepted alias-banner handlingMaintainer hardening
mainand resolved the WebVNC/RFB stack against the one-time portal handoff architecturewebvnc statususable without loading desktop secretsVerification
Exact head:
09c89fe190de5631b75f9757df0d6d4565926b2fgo vet ./...go test -race ./..../scripts/check-docs.sh.agents/skills/autoreview/scripts/autoreview --mode branch --base origin/main --parallel-tests "go test -race ./internal/providers/external ./internal/cli -run 'External|external|WebVNC|VNC|RFB|MacOSDesktop|ValidateProviderTarget' -count=1" --stream-engine-outputLive end-to-end External macOS proof remains credential-bound: no approved test endpoint currently exposes both SSH and Screen Sharing with a task-scoped credential. No endpoint security settings were changed. This PR is code-review and CI ready, but should retain the live-proof decision gate until such an endpoint is available or maintainers explicitly waive it.
Security and configuration
The desktop password value is never stored in Crabbox config, routing state, argv, or portal handoff. Config and daemon routing carry only the environment-variable name. The Crabbox process reads the value when an authenticated desktop operation starts, preserves it as opaque data, and fails closed if the configured variable is absent or empty.
For macOS WebVNC, Crabbox authenticates to ARD/VNC locally and gives the browser/coordinator only an already-authenticated RFB stream. Repository config cannot redirect an inherited desktop credential reference without a matching trusted or explicit override.