Repository navigation
fix(devices): refuse a pane's command rows to a watching device (#2132) - #2246
Conversation
GET /api/pane/commands gave a watch-only device the same scanned rows /api/agent/commands refuses it: a custom command's description (its file's first line) and its path. It is now refused with 403 read_only on this PC, its /api/machines/local alias and another PC's route, at the access check (server/index.ts) and in the PC proxy (server/machine-api.ts). A driving device keeps the rows. The chat and terminal views still mount usePaneCommands for a watcher, so the hook takes a read_only refusal as a final empty list instead of asking again every 30 s. Closes #2132
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CodeRabbit's review limit was reached, so no CodeRabbit review ran on this PR. This PR touches observe permissions and an HTTP contract, so here are the code-review agents' findings and how each was resolved (head omo-native-code-reviewer (adversarial, read-only): APPROVE
Codex Sol 6.1 fast xhigh (adversarial, read-only): APPROVE
No CRITICAL, HIGH or MEDIUM finding is in scope, so nothing was changed after the reviews. |
|
Merging faab4fe. An access change, so it got a strict review; Codex was at capacity, so an omo reviewer on gpt-6.1-sol and an independent second reviewer reviewed it; both pass with no HIGH. The watch refusal is checked on the parsed pathname after the noncanonical-path rejection and before alias rewriting or forwarding, for |
… a watching device (#2244) (#2252) Closes #2244. Follows #2246 (merged as af13338), which refused a pane's command rows the same way: refused, not hidden. ## Change A watch-only device was refused a directory listing and file reads, but `GET /api/pane/files?pane_id=…&q=…` gave it up to 100 relative paths under the pane's folder. Sweeping the other `/api/pane/*`, file and plugin routes turned up one more of the same kind: `GET /api/plugin/action?plugin_id=…&log_id=…` answers where a plugin action's run stands, including the tail of what its command printed when it failed, to anyone holding a log id. Only the device that started the run asks it (after its own POST, which a watcher cannot send). - **Access check** (`server/index.ts`): the watch refusal covers `pane/files` and `plugin/action` on this PC, on its `/api/machines/local/` alias and on another PC's `/api/machines/<id>/` route, answering 403 `read_only`. - **PC proxy** (`server/machine-api.ts`): a read-only connection is never forwarded `pane/files` or `plugin/action` with the PC's credential, just as `pane/commands`, `agent/commands` and `sessions/history` are not forwarded. - **Client:** no change. The composer (the only caller of `fetchPaneFiles`, for @-mentions) is never mounted for an observer (`PaneTerminal`'s `!observing`) and does not retry on a backoff. `pluginRun` polls the GET only after its own POST. A driving device and the connection server's bridge token keep both routes on every path. ## Sweep: what stays open to a watcher, and why | route | why it stays | | --- | --- | | `pane/read`, `pane/scroll`, `pane/selection` | the terminal screen, which the watcher already watches | | `pane/conversation*`, `pane/plan`, `pane/omo-tasks`, `pane/background-agent` | the chat view and its panels, which a watcher sees | | `pane/prompt` | a watcher already gets the screen's card without Claude's plan file (`plan=0`) | | `pane/find` | a POST, already refused | | `worktree/list` | the sidebar asks it on every device for branch names; trimming its answer is #2250 | | `agents`, `integrations`, `plugins/actions`, `session` | no paths, file contents or commands | ## Tests Each new test failed on the code before the fix (#2246's head) and passes after it: - `server/device-access.contract.test.ts`: a watching device gets 403 `read_only` for both routes on the local, local-alias and remote paths. A driving device reaches them (`pane_not_found` and `plugin_log_not_found` for absent ones locally and through the alias, 503 for an unconnected PC). Before the fix the local path answered 404 instead of 403. - `server/machine-proxy.test.ts`: the proxy never asks a PC for `pane/files` or `plugin/action` for a read-only connection, and passes a driving device the PC's answers. `bun run check fast` passes. ## Manual QA An isolated Ovis server (mkdtemp state dir, port 0, the check run's own herdr session) has a pane in a folder holding `.commandcode/commands/deploy.md`. Each path was requested with `curl -i`: | device | path | answer | | --- | --- | --- | | watch | `/api/pane/files`, `/api/machines/local/pane/files`, `/api/machines/pc1/pane/files` | 403 `read_only` | | watch | `/api/plugin/action`, `/api/machines/local/plugin/action`, `/api/machines/pc1/plugin/action` | 403 `read_only` | | drive | `/api/pane/files`, `/api/machines/local/pane/files` | 200 `{"files":[".commandcode/commands/deploy.md"]}` | | drive | `/api/plugin/action`, `/api/machines/local/plugin/action` | 404 `plugin_log_not_found` (no such run) | | drive | `/api/machines/pc1/…` (both) | 503 `machine_offline` (no such PC connected) | ## Reviews This touches observe permissions and an HTTP contract, so it was reviewed adversarially before opening: - **omo-native-code-reviewer: APPROVE.** On a real server (port 0, no herdr) it tried GET, HEAD and OPTIONS, odd path spellings (`//`, a trailing slash, `%66iles`, case, `;x`, `./`, `../`), WS frames and the relay, and found no way to these answers. LOWs: 1. The driving-device check on `plugin/action` only asserted "not a 403". **Fixed:** it now asserts 404 `plugin_log_not_found`. 2. The proxy's refusal list repeats part of the access check's. **Accepted:** it is the existing pattern (#2132), and the access check always runs first. 3. (pre-existing) `/api/pane/conversation/tool-output` gives a watcher a tool call's complete output, while the chat shows it cut at 4000 characters. **Left as is:** that output is already in the chat view a watcher sees. - **Codex Sol 6.1 fast xhigh: APPROVE.** 450 path/query probes and 48 method/prefix checks found no bypass. Driving devices, the owner bearer and the bridge bearer keep access. Pre-existing, outside this diff: - MEDIUM: `/api/worktree/list` shows a watcher the paths and branches of checkouts not open in herdr. **Filed as #2250:** it needs the answer trimmed rather than refused, because the sidebar uses the route on watching devices. - LOW: a watcher's terminal Find bar submits a POST that is refused (403), so it shows an error. **Left as is:** there is no retry loop. Disabling it for observers is a UI choice outside this fix. No `REMOTE_BUNDLE_VERSION` change: only a release PR raises it.
…2250) (#2304) Closes #2250. Follows #2246 and #2252 (merged), which refused a watching device a pane's command rows, the paths under a pane's folder and a plugin run's output the same way: refused, not trimmed. ## Change A watch-only device was refused a directory listing, but `GET /api/worktree/list?workspace_id=…` gave it herdr's whole worktree inventory for that workspace's repository: every checkout's absolute path, branch and label, including checkouts not open in herdr. The issue suggested trimming the answer, because the sidebar reads it on every device for branch names. Per the owner's decision for #2246/#2252 (refuse instead of hiding), the route is now refused to a watching device, and the client stops asking. - **Access check** (`server/index.ts`): the watch refusal covers `worktree/list` on this PC, on its `/api/machines/local/` alias and on another PC's `/api/machines/<id>/` route, answering 403 `read_only`. - **PC proxy** (`server/machine-api.ts`): a read-only connection is never forwarded `worktree/list` with the PC's credential, just as `pane/files`, `pane/commands`, `agent/commands`, `plugin/action` and `sessions/history` are not. - **Client:** - `useWorktreeBranches` takes `watching`: while watching it asks nothing, and a `read_only` answer stops its 30 s poll instead of retrying it forever. Its cache is keyed on the PC and the role, so a device signed in to drive reads branches again, and a downgrade drops the branches it had learned. - The sidebar passes `watching`, skips its per-workspace "is this a repository" probe while watching, and hides New worktree / Open worktree (drive features whose dialog lists the checkouts), as it already hides Earlier sessions. - The command palette gets `watching` from `App` and mounts no branch readers while watching. - **Visible tradeoff:** a watching device no longer shows branch names in the sidebar or the palette. A driving device, the owner's token and the connection server's bridge token keep the route on every path. ## Sweep of `server/workspace-routes.ts` | route | state | | --- | --- | | `workspace/directories` (GET) | already refused (directory listing) | | `workspace/diff/files`, `diff/file`, `diff/refs` (GET) | already refused (repository diff) | | `worktree/list` (GET) | refused by this PR | | every other route (`workspace/*`, `tab/*`, `worktree/create|open|remove`) | POST, already refused as a mutation | ## Tests Each new test failed on the code before the fix and passes after it: - `server/device-access.contract.test.ts`: a watching device gets 403 `read_only` on the local, local-alias and remote paths. A driving device reaches the route (`workspace_not_found` locally and through the alias for an absent workspace, 503 for an unconnected PC). Before the fix the local and alias paths answered 404, not 403. - `server/machine-proxy.test.ts`: the proxy never asks a PC for `worktree/list` for a read-only connection, and passes a driving device the PC's answer. - `src/lib/useWorktreeBranches.hook.test.ts` (new): a driving device polls again after 30 s. After a `read_only` answer the hook asks once and never again (it asked again before the fix). While watching it asks nothing, and after a switch to drive it reads and polls again (this failed with the refusal kept per PC only). `bun run check fast` passes. ## Manual QA An isolated Ovis server (mkdtemp state dir, port 0, the `ovis-test` herdr session) has a workspace on a repository with one linked checkout, `secret-branch`, not open in herdr. Each path was requested with `curl -i`: | device | path | answer | | --- | --- | --- | | watch | `/api/worktree/list`, `/api/machines/local/worktree/list`, `/api/machines/pc1/worktree/list` | 403 `read_only` | | drive | `/api/worktree/list`, `/api/machines/local/worktree/list` | 200, the inventory | | drive | `/api/machines/pc1/worktree/list` | 503 `machine_offline` (no such PC connected) | ## Reviews This touches observe permissions and an HTTP contract, so it was reviewed adversarially before opening: - **omo-native-code-reviewer: APPROVE** (after one fix round). It probed `./`, `%2e`, `../`, `\`, `%6cist`, `;x`, case, `//` and a trailing `/`, plus GET, HEAD and OPTIONS. It also checked the WS and relay surfaces and the check order (the refusal runs before the alias rewrite and the proxy). It found no way to the answer. - MEDIUM: the `read_only` refusal was kept per PC, so a device signed in to drive later never read branches again until reload. **Fixed:** the hook's cache is keyed on the role, and there is a test for watch → drive. - LOW: the palette sent one refused request per open on a watching device. **Fixed:** it mounts no branch readers while watching. - LOW: branches learned before a downgrade stayed shown. **Fixed** by the same role-keyed cache. - LOW: the sidebar gating has no component test. **Left:** it matches the existing Earlier sessions gate, and the hook carries the behaviour. - LOW (after the fix): a refusal that arrives, followed by a downgrade and an upgrade, all between two health polls, keeps the refusal until reload. **Left:** it needs two role changes inside one poll interval. - LOW (both reviewers, and CodeRabbit as Minor): the palette kept the branches it read while driving after a switch to watching. It stays mounted while closed, so they outlived closing it too. **Fixed** in a follow-up commit: while watching, its branches are empty and the cached inventories are cleared. - **Codex Sol 6.1 fast xhigh:** **APPROVE** (after one fix round, re-reviewed on the fix only). It ran 532 method/path combinations without finding a watch-device bypass, and confirmed the new server tests fail against the unfixed code. - MEDIUM: the same refusal-outlives-role-change bug. **Fixed** as above. - LOW: cached branches survive a downgrade. **Fixed** as above. - LOW (pre-existing credential precedence): a request carrying both a valid bridge bearer and a watch device cookie is treated as the watch device, so it is now refused this route. **Left:** the bridge proxy strips cookies, so real bridge traffic never carries one. Changing which credential wins is outside this fix. No `REMOTE_BUNDLE_VERSION` change: only a release PR raises it.
…ith main (#2253) Fixes the **Native Windows mirrored terminal** check (`windows-terminal-qa.yml`, from #2211) failing on #2243 and #2242 at `Timed out: new native output after wheel pan` (and an `EBUSY` that hid the error), while it passed on #2246. ## Root cause The workflow checked out `github.event.pull_request.head.sha`. A branch cut before #2211 (524273e) ran **its own old copy** of `scripts/windows-terminal-browser-qa.ts`, against an app that already folds a wide mirrored pane to a phone's width (#1961). #2211 fixed that script in four places: it chooses Original width (`ovis:mirror-wrap` = false), waits for the grid to overflow, places the IME caret by the measured cell width, and retries the EBUSY cleanup. The old copy still has all of them. #2243 (base b48c66a) and #2242 (base 615c92b) predate #2211; #2246 (base 84824f1) does not. The workflow file itself comes from the merge ref, so the job ran on those PRs anyway. Nothing in the product is wrong: the failing PRs tested a stale script. ## Change - `windows-terminal-qa.yml` checks out `${{ github.sha }}`: the pull request's merge with its base, as every job in `ci.yml` does. Schedule and dispatch runs are unchanged (they already fell back to `github.sha`). `windows-install.yml` keeps the head SHA for its own reason (`install.ps1 -Ref` must name the checked-out commit); this workflow builds and runs the checkout in place. - `scripts/ci-setup.test.ts`: the QA job's checkout ref must be `${{ github.sha }}`. It fails on main (`Received: "${{ github.event.pull_request.head.sha || github.sha }}"`) and passes with the change. - `docs/development.md` and a changelog fragment say so. ## Verification Native Windows 11 VM (`winvm`, herdr 0.9.3, Bun 1.4.2), fresh `%TEMP%` clones, through `dori heavy`: | Checkout | Script | Result | | --- | --- | --- | | #2243 head 457f187 | its own (pre-#2211), plus diagnostics | **0/3**: all time out at `new native output after wheel pan` (one hides it behind EBUSY, as on #2242). Diagnostics: herdr's pane shows `PPP…> Q`, while the host has `scrollWidth 390 == clientWidth 390`, so the grid is folded, not at Original width | | #2243 head 457f187 | its own plus only `ovis:mirror-wrap = false` | wheel-pan step **3/3 PASS**; then each run fails at `a visible cursor keeps its composition anchor`, the fixed-52-column bug #2211 also fixed | | `refs/pull/2243/merge` 3493d80 (what this workflow now checks out) | current | **3/3 PASS**, all four PASS lines | | main 5c20a88 | current | **10/10 PASS** in a row, all four PASS lines each | - GitHub runner, all green with four PASS lines: - this PR's own run, which checked out `Merge d1bea76 into adfdef6` (7e74dbd), i.e. the merge: https://github.com/devswha/ovis/actions/runs/40688443672 - a manual dispatch on this branch: https://github.com/devswha/ovis/actions/runs/40688514345 - a manual dispatch on main adfdef6: https://github.com/devswha/ovis/actions/runs/40688525315 - `bun run check fast` green (workflow syntax/actionlint, typecheck, build, unit tests: 8163 pass, 0 fail) on this head, d1bea76. ## Reviews - omo-native-code-reviewer: **PASS**, no CRITICAL/HIGH/MEDIUM. LOW notes, left as is: "Re-run jobs" reuses the original merge commit (as in `ci.yml`; a push or reopen picks up a newer base); "merge with main" is strictly the merge with the base branch (PRs here target main); the test's `!` would throw a TypeError instead of a clean failure if the checkout step were removed, as in its neighbours. - Codex Sol 6.1 Fast xhigh: **PASS**, no findings. Its note: the step still depends on echo and mirror delivery within 15 s, so the same timeout label on a current-base PR would be a new cause (a real regression or a starved runner), not this one.
Closes #2132.
Change
A watch-only device was refused
GET /api/agent/commands(a folder's command rows), butGET /api/pane/commands?pane_id=gave it the same scanned rows: each custom command's description (its Markdown file's first line or frontmatterdescription) and its path asid. As the issue recommends, the rows are now refused, not trimmed. The composer menu that lists them is a drive feature.server/index.ts): the watch refusal covers(agent|pane)/commandson this PC, its/api/machines/local/alias and another PC's/api/machines/<id>/route, answering 403read_only.server/machine-api.ts): a read-only connection is never forwardedpane/commandswith the PC's credential, asagent/commandsandsessions/historyare not.src/lib/usePaneCommands.ts): the chat and terminal views still mount the hook for a watcher, so it takes aread_onlyrefusal as a final empty list instead of asking again on its 2/5/15/30 s backoff. A watcher's chat therefore draws sent slash commands as plain bubbles, the cost of refusing the rows. A device's role cannot change while the app runs, so this cannot stick for a driving device.A driving device keeps the rows on every path.
Tests
Each new test failed on the code before the fix and passes after it:
server/device-access.contract.test.ts: a watching device gets 403read_onlyon the local, local-alias and remote paths; a driving device reaches the route (pane_not_foundfor an absent pane locally and through the alias, 503 for an unconnected PC). Before: the local path answered 404pane_not_foundinstead of 403. After: the whole file passes, 15 pass.server/machine-proxy.test.ts: the proxy never asks a PC forpane/commandsoragent/commandsfor a read-only connection, and passes a driving device the PC's rows.src/lib/usePaneCommands.hook.test.ts: one read and no retry after aread_onlyrefusal (before: a second fetch after 2 s).bun run check fastpasses.Manual QA
An isolated Ovis server (mkdtemp state dir, port 0, the check run's own herdr session) has a Claude pane in a folder holding
.claude/commands/deploy.md, whose first line isSECRET. Each path was requested withcurl -i:/api/pane/commandsread_only/api/machines/local/pane/commandsread_only/api/machines/pc1/pane/commandsread_only/api/pane/commandsSECRET/api/machines/local/pane/commandsSECRET/api/machines/pc1/pane/commandsmachine_offline(no such PC connected)Reviews
This touches observe permissions and an HTTP contract, so it was reviewed adversarially before opening:
agent/commands.GET /api/pane/filesstill lists up to 100 relative paths under a pane's folder to a watch device. It was reproduced in the QA above ({"files":[".claude/commands/deploy.md",".commandcode/commands/deploy.md"]}) and filed as Watch devices can list a pane folder's file paths through /api/pane/files #2244.No
REMOTE_BUNDLE_VERSIONchange: only a release PR raises it.