Repository navigation
fix(rpc): sign in a page reached through a port forward - #939
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the full diff and surrounding RPC/CLI/Docker callers, checked the relevant history, and traced the browser security boundary: forwarded requests require Origin to equal Host on a literal loopback name, while the existing bound-port exception preserves the nginx path. Cookie naming remains backward-compatible for direct and nginx access and separates forwarded ports as intended.
I also checked AGENTS.md/CLAUDE.md and the runtime context map, backward compatibility, architecture boundaries, and whether the tests were weakened; the change adds coverage without relaxing existing assertions. The duplicated helper definition and damaged module-docstring sentence are cleanup nits with no runtime effect, so I am not holding the change for them.
Verification: uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py -x passed (221 tests). The first attempt did not reach tests because the fresh environment lacked the declared dev extra; after uv sync --extra dev, the same command passed. uv run ruff check raven/rpc/transports/ws.py raven/cli/serve_commands.py tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py also passed.
|
Windows compatibility review: no newly introduced Windows issue found in the inspected scope. Broader validation encountered inherited platform constraints. Reviewed all seven changed files at Native Windows / Python 3.13.12, separate run-owned uv environment and isolated user/Raven/temp directories; $R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\40676d5dc0d6427594f56d74f56a4232'
uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py::test_container_bootstraps_the_browser_after_the_engine_is_ready -n 0 -x --tb=short --basetemp "$R\checks\939-auth\pytest" --junitxml "$R\checks\939-auth\junit.xml"
# 27 passed, 1 failed: symlink creation requires a Windows privilege this host lacks (WinError 1314).
uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py::test_container_bootstraps_the_browser_after_the_engine_is_ready -k 'not symlink' -n 0 -x --tb=short --basetemp "$R\checks\939-auth-portable\pytest" --junitxml "$R\checks\939-auth-portable\junit.xml"
# 116 passed, 10 skipped, 1 deselected, 1 failed: the existing POSIX 0600 assertion sees 0666 mode bits on Windows.
uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py::test_container_bootstraps_the_browser_after_the_engine_is_ready -k 'not symlink and not test_the_state_file_stays_owner_only' -n 0 -x --tb=short --basetemp "$R\checks\939-auth-filtered\pytest" --junitxml "$R\checks\939-auth-filtered\junit.xml"
# 129 passed, 10 skipped, 2 deselected, 1 failed: the supervisor fixture cannot launch its `sleep 600` command (WinError 2).
uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py::TestTheSignInLink tests/test_docker_runtime.py::test_container_bootstraps_the_browser_after_the_engine_is_ready -k 'not symlink' -n 0 -x --tb=short --basetemp "$R\checks\939-auth-focused\pytest" --junitxml "$R\checks\939-auth-focused\junit.xml"
# 50 passed, 1 deselected.The three failing test paths and their relevant setup/assertions also exist in the recorded base; source comparison attributes none to this PR. They limit the broad suite here. The final selection covers the changed authentication behavior on real local aiohttp servers, including forwarded sign-in and WebSocket upgrades, cross-port/rebinding refusals, cookie separation, proxy compatibility, refusal messages and printing the link before browser launch. Browser opening itself is mocked. No live browser/VS Code forward, Docker container, Windows ACL audit or full repository suite was exercised. The skips require POSIX process groups. This is a scoped Windows result, not full compatibility or an ACL-security conclusion. |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the new commit and the resulting full change. The delta removes the duplicate helper, restores the module documentation, and strengthens coverage without changing the forwarded-port admission or cookie behavior previously reviewed. In particular, the standalone serve --open path is now executed rather than only inspected, and malformed Host plus localhost/IPv6 origin cases are covered.
I rechecked the project rules/context, affected callers, compatibility and security boundary, architecture placement, and that no assertions were weakened. No new issue changes the prior clean assessment.
Verification: uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py -x passed (233 tests); the focused Ruff check passed. There are no review threads from me to resolve.
|
Thanks, both nits were real and are fixed in 5dbb767.
Closing the coverage gap turned up a vacuous test. The localhost case sent the bound port, which the old allowlist already accepted, so the new rule's At this head: diff coverage 100% (26/26) locally; 11 single-construct mutants each killed, and a base control fails all 18 new tests. |
|
Addressed the native Windows test constraints and a shared-cache race that can produce the Windows self-upgrade failure. The existing branch now points to
Native Windows build 10.0.26300, Python 3.13.12: 195 passed, 30 skipped. Four skips identify the newly handled privilege/mode/signal preconditions; 26 existing POSIX skips remain. No ACL or symlink-confinement success is claimed for the skipped cases. With 3 passed: both real uv upgrade variants and the workflow contract. Whole-PR pre-commit, Ruff, import contracts, changed-production type checks, commitlint and repository gates passed. The independent Linux CI failure in the new fake Application ( |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the four-commit delta and the resulting full change. The Windows guards are limited to unavailable POSIX mode, SIGTERM-delivery, and symlink-privilege behavior; they retain each underlying assertion on platforms where its setup is meaningful. The upgrade fixture isolation and replacement decoding are test-only and do not alter product behavior.
I rechecked the repository rules/context, affected test callers and history, compatibility, architecture boundaries, and assertion strength. No tests were weakened to manufacture a pass, and this delta does not change the previously reviewed forwarded-port implementation.
Verification: uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py -x passed (233 tests). The modified real-uv upgrade case passed both parameters (2 tests); pytest emitted its existing warning that these unmarked cases exceed the advisory idle ceiling. Focused Ruff checks passed. There are no review threads from me to resolve.
|
Windows compatibility re-review: native auth/serve checks pass, but the already-disclosed built-page test fixture failure also reproduces on Windows. Scope: all eight changed files at P2 - Complete the new Application test double for a built page (reproduced). test_cli_serve_commands.py:473-478 supplies Native Windows / Python 3.13.12; reused uv environment with identical $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/0d8ab4cfce454f15b5d09028fe345dbb'
uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py::test_container_bootstraps_the_browser_after_the_engine_is_ready -n 0 -x --tb=short --basetemp "$R/checks/939-auth-updated/pytest" --junitxml "$R/checks/939-auth-updated/junit.xml"
# 195 passed, 30 skipped; exit 0 (snapshot without built UI assets).
uv run pytest "$R/test_windows_built_page_fixture.py" -n 0 -x --tb=short --basetemp "$R/checks/939-built-page/pytest" --junitxml "$R/checks/939-built-page/junit.xml"
# 1 failed: AttributeError, _Application has no on_response_prepare.This broader selection now completes on Windows without filtering tests by name. It covers forwarded authentication/WebSocket requests, malformed Host rejection, localhost/IPv6 admission, cookie separation, the standalone serve/open path and printing the sign-in link before browser launch. Browser launching is mocked. The symlink setup skips specifically for Windows privilege error 1314; other filesystem failures still propagate. The mode-bit and SIGTERM checks now declare their POSIX requirement. Code-reading only: the real uv-upgrade fixture now supplies its own spaced Limits: no live browser/VS Code forward, Docker container, real uv self-upgrade, Windows ACL audit or full repository suite. Skipping POSIX permissions does not establish equivalent Windows ACL enforcement. |
The gateway accepted a browser Origin only when it named the port the gateway bound, so any port forward that picked another local port was refused: every /auth/exchange answered 403 and every /rpc upgrade with it. The page reported an expired sign-in link over a refusal it cannot see, and re-running `raven web` minted one more link into the same refusal, 13 times on 2026-10-13. The same check already forced the container's nginx to rewrite Origin for every request it proxies. Origin now also passes when it asks the request's own origin on a loopback name. That is what a browser sends through a forward, since the forwarder relays Host unchanged, and it still refuses a page another local server shows (cookies and SameSite ignore the port) and DNS rebinding (the name is not loopback). The bound-port allowlist stays for nginx, which does not forward a port in Host. The session cookie is now named for the port the browser reached, so two gateways that both bound 18792 do not share one jar entry when one browser reaches them through different forwards. The exchange answers with the reason it refused, and the sign-in page shows that text instead of calling every failure an expired link. `_open` prints the sign-in link every time, before handing it to a browser: webbrowser.open returns True whenever it ran something, so the fallback was unreachable, and a console browser holds the terminal.
The previous commit defined _loopback_origin twice and replaced one sentence of the module docstring with an unrelated one. The first copy was shadowed at import, so behaviour was unchanged, but its ten lines could never run and took the diff-coverage gate to 60% (26/43). ruff's F811 does not report a redefined underscore name, so lint stayed clean. Both are restored, and tests now reach each branch the gate listed: - the localhost spelling through a forward. The old test used the bound port, which the previous allowlist already accepted, so the new branch never ran; - a Host whose port does not parse (out of range, not a number, an open IPv6 bracket), answered 401 rather than a 500; - an Origin equal to its Host that does not parse, answered 403; - the loopback rule asked directly, over the spellings a request-level test would each need a forward for; - `raven serve --open` run end to end, rather than pinned by source. Diff coverage at this head is 100% locally (26/26).
Give each real uv upgrade case its own marker and wheel cache so parallel Windows workers cannot replace or delete cached wheels from a sibling. Preserve localized failure diagnostics when capturing subprocess output.
Skip only the unsupported symlink setup when Windows reports privilege error 1314. Keep the path-confinement assertion active wherever symlink creation succeeds and propagate other filesystem errors.
Windows reports writable DOS mode bits rather than POSIX owner-only permissions. Keep the existing mode assertion on POSIX and identify Windows permission coverage as unsupported by this test.
Popen.terminate uses TerminateProcess on Windows and cannot exercise Python SIGTERM cleanup handlers. Keep these signal-delivery regressions on POSIX, alongside the existing process-group tests.
Use the real aiohttp Application while replacing the runner and TCP site, so build_app can register its response hook without a partial fake. Resolve the UI from temporary candidates, cover both page states, and assert that revalidation is registered only when a built page is served. Correct the startup comment now that the coroutine is exercised in tests.
230a498 to
4aed82d
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the rebased revision with git range-diff so upstream main changes were not attributed to this PR. The six previously reviewed commits are patch-equivalent; the only new patch replaces the incomplete aiohttp application fake with a real web.Application, exercises both built-page states, and corrects the now-stale startup comment. It changes no authentication behavior.
I rechecked the security boundary, repository rules/context, affected caller, history, backward compatibility, architecture placement, and assertion strength. The real application makes the test cover the response hook that failed under the partial fake, without weakening the sign-in assertions.
Verification: uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py -x passed (234 tests), and the focused Ruff check passed. There are no review threads from me to resolve.
|
Windows compatibility re-review: no newly introduced Windows issue found in the inspected scope; the previously reported built-page test fixture issue is fixed. Reviewed all eight changed files at Native Windows / Python 3.13.12: 196 passed, 30 skipped. Both Exact commands from the isolated detached head checkout; separate uv environment and home/temp/token directories, uv sync --frozen --python 3.13 --all-packages --extra dev
$R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/7d6eb1a69a25421f93a87e4aa3303329'
uv run pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py::test_container_bootstraps_the_browser_after_the_engine_is_ready -n 0 -x --tb=short --basetemp "$R/checks/939-auth/pytest" --junitxml "$R/checks/939-auth/junit.xml"
# 196 passed, 30 skipped; exit 0The selection exercises local HTTP/WebSocket authentication, forwarded cookies, malformed hosts, cross-port/rebinding refusals and link output. Browser launching and the standalone test's socket runner are mocked. Skips cover unavailable Windows symlink privilege and POSIX mode/signal/process-group requirements; they do not establish equivalent Windows ACL enforcement. Code-reading only: per-case |
|
Not a blocker. The port-forward sign-in fences the origin correctly -- I confirmed Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. (nit, one-definition) |
… cache (#942) ## Summary A long-running Raven host held on to three kinds of memory it no longer used. Observed on one dev box on 2026-10-12: - Two ACP agents, started on 2026-09-20 by a host that exited without closing them, were still running 22 days later. They had been reparented to init and held 523 MiB PSS between them. - A gateway's glibc thread arenas had grown from 85 to 214 MiB over 18 days of light use. - `SessionManager` kept every session it had ever loaded. This PR cleans up all three automatically. **1. ACP agents that outlive their host (Linux).** An ACP agent starts in a session of its own, so it gets none of the signals sent to its host's process group or terminal, and while the host lives only `AcpClient.close` ends it. A host that exited without closing its agents, or was killed, crashed or OOM-killed before its teardown, left them running. - The agent now starts through a one-line exec wrapper (`raven/acp_client/leftovers.py`). It runs the host's own interpreter with `-I -S`, arms `PR_SET_PDEATHSIG(SIGKILL)`, checks that its parent is still the host, restores SIGPIPE and SIGXFSZ to their defaults and execs the agent in place, with the same pid and group and the agent's own argv. The agent's process then ends with the host. - The signal reaches only the direct child, and an `npx` launcher's real server is a grandchild. So every host also records the process groups it launched, one file per host under `acp-launches/` in the config file's directory. The first ACP launch in any process ends the groups recorded by hosts that have since died. That sweep runs once per process on a daemon thread. Every launch waits for it for at most 5 s, and once one launch has waited that long the rest stop waiting for it. - A group is ended only when all of the following hold: - the record is of this version and comes from this boot and PID namespace; - its host is gone, judged by pid and start time; - the group is not group 0, 1 or the sweeper's own; - its leader, if still running, has the start time on record; - a current member still carries the dead host's mark, `RAVEN_ACP_HOST=<host pid>-<host start ticks>`, which every launched agent gets last in its environment. A read that fails, of a record or of `/proc`, keeps the record and ends nothing; a record that does not parse is deleted. **2. Native heap trimming (Linux with glibc).** glibc keeps the memory native code frees, such as PyMuPDF rasters and tokenizer tables, in its arenas. `raven/utils/heap.py` checks every ten minutes and, once anonymous memory stands 32 MiB above its lowest reading since the last trim, runs `malloc_trim(0)` in a worker thread. The five long-running hosts start it: `raven gateway`, `raven serve`, the TUI's RPC host, `raven acp` and `raven a2a serve`. **3. A bounded session cache.** `SessionManager._cache` becomes an LRU with a soft limit of 64 sessions. - A session leaves the cache only when nothing about it is unsaved. Its message count must match what is on disk, and a fingerprint of its metadata, consolidation mark and pending clarification must match the one taken when it was last read or written. A metadata patch counts its own keys as saved even when the caller set them on its copy first. - A session that a caller still holds comes back as the same object. - A reloaded session keeps the `updated_at` it was saved with. `CONTEXT.md` gains two terms, **Leftover agent** and **Launch mark**. Measured on this box on 2026-10-13: - **PyMuPDF.** 112 pages rasterised three times in a 3-thread pool. In two runs, anonymous memory was 279.8 and 315.3 MiB after `store_shrink(100)`, and 93.5 and 84.6 MiB after `heap.trim()`. - **Gateway.** An isolated gateway with a key-less config, run for 100 s. With the interval shortened to 45 s, one trim fired at t+47.7 s and took anonymous memory from 293 to 273 MiB; with the default interval, none fired. Final PSS was 321.6 against 328.6 MiB. Its boot check launched 16 ACP agents through the wrapper: the five discovered agents under `agents/`, which that config cannot start, and eleven presets, eight of which completed the handshake (Claude Code, Codex, OpenCode, Hermes Agent, OpenClaw, CodeBuddy, Qoder and Pi). Afterwards no marked process and no launch record remained. - **Killed host.** A stand-in host was SIGKILLed after launching through `AcpClient`. On main, a direct agent, an `npx`-shaped launcher and its grandchild all survive with ppid 1. On this branch, the direct agent and the launcher end with the host, and the next host's first launch ends the grandchild. - **Discovered agents, with and without the wrapper.** Each of the five under `agents/` was verified twice in a home holding only dummy keys (a handshake calls no model): Raven-Design completed the handshake both ways, and the other four stopped on the same missing key both ways. The Windows regression fixtures use explicit executable filenames and native project-path strings. The unreadable-transcript symlink fixture skips only when Windows reports missing symlink privilege (WinError 1314); other creation errors still fail. These test corrections do not extend the Linux-only cleanup mechanisms. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification The original author's Linux measurements below precede the Windows test-fixture follow-up. The native Windows results that follow were run against the final follow-up tree. - **Author's Linux full suite before the Windows test-fixture follow-up:** `uv run --frozen --python 3.12 --all-extras pytest -q -p no:cacheprovider` (repo addopts: `-n 4`, integration and e2e deselected). Result: 3 failed, 30261 passed, 158 skipped, 2 xfailed. The failures are the ones this box gives on main too: `test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for`, `test_knowledge_parser_excel.py::test_a_legacy_workbook_is_converted_and_read` (no LibreOffice) and `test_subagent_node_runtime.py::test_what_cannot_be_read_names_nothing` (run as root). - **Directly affected files:** `tests/test_acp_leftovers.py`, `tests/test_utils_heap.py`, `tests/test_session_manager.py`, `tests/test_cli_{a2a,acp,gateway,serve,tui}_commands.py` and `tests/test_subagent_host_env.py`, under `--idle-ceiling-strict` as CI runs them. Result: 620 passed. - **Neighbours:** `tests/test_rpc_session.py`, `tests/test_session_{export,title,project_grouping}.py`, `tests/test_defer_manager.py`, `tests/test_subagent_acp.py` and `tests/test_acp_{ask_user,elicitation,remote}.py`. Result: 683 passed. - **Lint and gates:** `make lint-python lint-imports lint-deps lint-types check-large-files check-source-language check-commits`. Result: all seven exit 0; import contracts 10 kept, 0 broken. - **Mutation:** guards were deleted or inverted one at a time, out of tree, and each such mutant failed the test written for it. They covered the sweep's wait, bound, daemon flag and start failure; the version, boot-time, PID namespace, liveness, leader and mark checks; the `/proc/<pid>/stat` parse; the trimmer's reader and threshold; and the cache's eviction order, fingerprint, `updated_at` restore and patch rule. - **Diff coverage:** 96.27% (387 of 402 executable changed lines) from the directly affected and neighbouring files alone, against CI's 90% gate. What stays uncovered is the arms for a `/proc` read or a `killpg` that fails, and the import inside `raven a2a serve`, which only a running server reaches. **Native Windows test-fixture follow-up (2026-10-14):** Windows 10.0.26300, Python 3.13.12, `PYTHONUTF8=1`, a dedicated worktree and `uv sync --frozen --python 3.13 --all-packages --extra dev`. The imported Raven package was verified to come from that worktree. Pytest's isolated homes and a separate caller `RAVEN_HOME` were used. JUnit output paths are omitted from the command below. ```powershell uv run --frozen --no-sync pytest tests/test_acp_leftovers.py tests/test_utils_heap.py tests/test_session_manager.py tests/test_session_project_grouping.py tests/test_subagent_host_env.py tests/test_cli_a2a_commands.py::test_serve_starts_the_heap_trimmer tests/test_cli_acp_commands.py::TestServe::test_the_heap_trimmer_starts_before_the_channel_is_served tests/test_cli_gateway_commands.py::test_run_starts_the_heap_trimmer_at_boot tests/test_cli_serve_commands.py::test_serve_starts_the_heap_trimmer_at_boot tests/test_cli_tui_commands.py::test_the_terminal_host_starts_the_heap_trimmer -k 'not test_acp_child_inherits_custom_raven_home and not test_acp_child_is_told_it_serves_as_a_subagent' -n 0 --idle-ceiling-strict --tb=short ``` Result: **229 passed, 73 skipped, 2 deselected**, exit 0 with the idle ceiling enforced. Of the skips, 72 are existing Linux/glibc cases and one is the measured lack of Windows symlink privilege. The three corrected discovery/path cases failed before the corrections and pass in this selection. Before the symlink guard, the same broader selection had 229 passes, 72 skips, two deselections and one WinError 1314 failure. The two deselected real ACP child tests still depend on the native close fix in #908. The broader CLI permission/signal limitations reported in the Windows review are covered separately by #939. This follow-up claims no full Windows suite pass, real ACP lifecycle validation, privileged symlink run, Windows ACL audit, or new Linux run. - `uv run --frozen --no-sync ruff check raven evolver agents plugins-dist tests scripts docs-site` and the matching `ruff format --check`: passed (2,272 files formatted); the final symlink edit also passed both file-scoped checks. - `uv run --frozen --no-sync lint-imports`: 10 contracts kept. `uv run --frozen --no-sync ty check raven/acp_client/client.py raven/acp_client/leftovers.py raven/cli/a2a_commands.py raven/cli/acp_commands.py raven/cli/gateway_commands.py raven/cli/serve_commands.py raven/cli/tui_commands.py raven/session/manager.py raven/utils/heap.py`: passed. - `uv run --frozen --no-sync pre-commit run --from-ref origin/main --to-ref HEAD`: all applicable hooks passed. - `node node_modules/@commitlint/cli/cli.js --from origin/main --to HEAD --config commitlint.config.cjs`, `uv run --frozen --no-sync python scripts/check_commit_messages.py origin/main..HEAD`, `uv run --frozen --no-sync python scripts/check_large_files.py origin/main...HEAD`, `uv run --frozen --no-sync python scripts/check_source_language.py origin/main...HEAD`, and `git diff --check origin/main...HEAD`: passed. The relevant-tests checkbox remains unchecked because the broader Windows files still contain the disclosed limitations above. - [ ] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No user-facing docs change, because no command, flag or setting does. The two new terms are glossary entries in `CONTEXT.md`. ## Risk User-visible behaviour changes: - **An ACP agent that exists but cannot be executed now fails after the spawn.** Examples are a bad interpreter line or a binary for the wrong architecture. The wrapper writes `raven: cannot start <path>: <error>` to the agent's stderr and exits 127, so the failure surfaces as a connection that ends with that line in its stderr, where `AcpClient.launch` used to raise `cannot start` at once. A missing program still fails at launch, as before. - **Agent environment.** Every ACP agent's environment carries `RAVEN_ACP_HOST`. It is an identifier, not a secret; its parts are world-readable in `/proc/<pid>/stat`. - **Locale.** An agent started under a C or POSIX locale gets `LC_CTYPE=C.UTF-8`, because the wrapper interpreter's PEP 538 coercion carries through the exec. - **Launch records.** Every ACP launch and close writes a small JSON file under `acp-launches/` beside the config file, synchronously and with an fsync. - **First launches.** While dead hosts' records are swept, a process's ACP launches wait for the sweep, 5 s at most in all: once one launch has waited that long, the rest start without it. Normally the sweep takes milliseconds. - **Session timestamps.** A reloaded session's `updated_at` is the one it was saved with, where it used to be the time of the load, so a save that adds no message no longer moves it. Sentinel's defer timing and the export header read it in memory, and the session list orders by the saved value. Left as today: - Anything but Linux. #928 tracks macOS and Windows. - A group whose surviving processes carry no readable mark: one started with a fresh environment, another user's, or an undumpable one. - An agent binary that is set-id or carries file capabilities: its exec clears the parent-death signal, so only the sweep can end it, and only where its environment is readable. - Spawns other than ACP agents: the CLI sub-agent backend, background exec and the sandbox executor. - Leftovers created during one long-lived host's life. They wait for another process's first ACP launch. - A host started with another `--config` sweeps that directory's records only. Edge cases of the records directory: - **Another PID namespace.** Its records are kept until a reboot, about 300 bytes per container start. - **A directory shared between machines.** Another machine's record older than this machine's boot is removed by this machine's sweep. That host's agents would then not be swept if it died before its next launch or close rewrote the file. Nothing is ever signalled across machines. Edge cases of the session cache: - A change made through a reference to a session that had already left the cache, and then released unsaved, is lost, as it would be in a fresh process. - An in-memory undo whose save failed, followed by new messages back to the same count, reads as clean. `save()` already mishandles that state, so the residue predates this PR. A group is signalled only when it passes all five checks above. A crafted record cannot point a sweep at a live host's agents, because the mark names the identity the liveness check reads. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes Rollback is a revert. Nothing persisted changes shape: session files are written exactly as before, and `acp-launches/` only holds what the next sweep reads, so it can be deleted at any time. ## Related Issues #928 (macOS and Windows; stays open)
## Summary RPC browser-origin checks and URL plaintext checks each implemented the same loopback IP classification. Both now use `security.hosts.is_loopback_ip`, which reuses the existing `as_ip` parser. The callers retain their name policies: RPC accepts `localhost` alongside loopback IPs; URL plaintext checks also allow `localhost.localdomain` and trailing-dot aliases. Origin/Host matching and malformed-origin refusals remain in the RPC layer. Regression cases exercise both accepted and refused hosts through the real sign-in handler. ## Type - [ ] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [x] Refactor - [ ] Other ## Verification Linux, Python 3.12.13, with existing dependencies matching the lockfile for the affected modules and check tools: ```bash uv run --no-sync pytest tests/test_security_hosts.py tests/test_security_urls.py tests/test_rpc_transport.py tests/test_security_network.py tests/test_security_web_ssrf.py tests/test_browser_policy.py tests/test_market.py tests/test_rpc_skillhub.py tests/test_skill_hub_policy.py tests/test_skill_hub_client.py tests/test_cli_serve_commands.py -q ``` 726 passed; one supervisor test timed out waiting for its child to write the state file. Its isolated rerun passed: ```bash uv run --no-sync pytest tests/test_cli_serve_commands.py::TestTheSupervisorCleansUpOnTheSignalThatStopsIt::test_sigterm_removes_the_state_file -q -n 0 ``` The expanded URL/RPC boundary cases passed before the production refactor (60 passed). A separate comparison of the original and shared predicates found no differences across 6,535 host strings and 39,214 origins, including legacy addresses, aliases, mapped IPv6 and malformed brackets. These checks passed: ```bash uv run --no-sync ruff check raven/security/hosts.py raven/security/urls.py raven/rpc/transports/ws.py tests/test_security_hosts.py tests/test_security_urls.py tests/test_rpc_transport.py uv run --no-sync ruff format --check raven/security/hosts.py raven/security/urls.py raven/rpc/transports/ws.py tests/test_security_hosts.py tests/test_security_urls.py tests/test_rpc_transport.py uv run --no-sync ty check raven/security raven/rpc/transports uv run --no-sync lint-imports uv run --no-sync pre-commit run --files raven/security/hosts.py raven/security/urls.py raven/rpc/transports/ws.py tests/test_security_hosts.py tests/test_security_urls.py tests/test_rpc_transport.py ``` - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No user-facing documentation change is needed for this refactor. The full repository suite and native Windows/macOS runs were not performed. The supervisor timeout was not compared against an unmodified main checkout. ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes The change touches authentication and URL trust predicates. It retains each caller's local-name rules and the standard library's IP classification, including acceptance of IPv4-mapped loopback on Python 3.12.13. No DNS resolution, listeners, credentials or configuration are added. Reverting this refactor restores the previous separate checks without a data migration. ## Related Issues #939
3 participants
Summary
A page reached through a port forward could not sign in. The gateway
accepted a browser
Originonly when it named the port the gatewaybound, and VS Code and
ssh -Lpick another local port whenever theremote one is taken on the browser's machine, so every
POST /auth/exchangeanswered403 bad originand every/rpcupgrade was refused with it. The sign-in page reported an expired link
over a refusal it cannot see, and re-running
raven webminted onemore link into the same refusal. Measured on 2026-10-13: 13 refusals
in six minutes, each from a page at
127.0.0.1:18793against agateway on 18792, all within a minute of their nonce being minted. The
same check had already forced the container's nginx to rewrite
Originfor every request it proxies.
_origin_oknow also accepts anOriginthat asks the request's ownorigin (
OriginequalsHost) on a loopback name. That is what abrowser sends through a forward, since a forwarder relays
Hostunchanged. It still refuses a page another local server shows, because
cookies and SameSite ignore the port and that origin names a port this
page does not ask; and it still refuses DNS rebinding, where a name
resolving to 127.0.0.1 asks its own origin and the two agree. The
bound-port allowlist stays for the container's nginx, which forwards
Hostwithout a port.Three smaller changes ride with it:
than the port bound, so two gateways that both bound 18792 no longer
share one jar entry when one browser reaches them through different
forwards. A
Hostwithout a port keeps the bound port, which is whatthe container's auto-login sets.
handle_auth_exchangeanswers with the reason it refused, and thesign-in page shows that text rather than calling every failure an
expired link.
_openprints the sign-in link every time, and before handing it toa browser.
webbrowser.openreturns True whenever it ran something:VS Code's
browser.shexits 0 whatever happened, and a machine withno display hands the URL to a console browser that has no JavaScript
and never redeems the nonce. The old fallback was unreachable, so the
link was never printed at all.
The standalone serve test now uses a real aiohttp
Applicationwhile replacing onlyAppRunnerandTCPSite. It selects temporary UI locations for both an absent page and a built page, and checks that the response revalidation hook is registered only for the built page. This removes dependence on whether the checkout contains a UI build.This PR also retains the reviewed Windows test guards and separate
RAVEN_HOMEdirectories for real-upgrade fixtures.Type
Verification
Linux fixture follow-up (Python 3.12.13; the three-file suite and diff coverage were rechecked after rebasing onto the current main):
TestTheSignInLinkover an absent page and a built page produced 4 passed / 1 failed. Only the built-page case failed, with the same missingon_response_prepareas CI.uv run --no-sync pytest -q -n 0 tests/test_cli_serve_commands.py::TestTheSignInLink: 5 passed after the fix.uv run --no-sync pytest -q tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py --cov=raven.rpc.transports.ws --cov=raven.cli.serve_commands --cov-report=json:<coverage-json>: 234 passed.<coverage-json>denotes the local coverage output file.uv run --no-sync python scripts/coverage_gate.py --coverage-json <coverage-json> diff --base-ref origin/main: 100.00% (32/32 executable changed lines), passing the 90% threshold. This measures diff coverage, not the full-suite ratchet.uv run --no-sync ruff check tests/test_cli_serve_commands.py raven/cli/serve_commands.py,uv run --no-sync ruff format --check tests/test_cli_serve_commands.py raven/cli/serve_commands.py, anduv run --no-sync ty check raven/cli/serve_commands.py tests/test_cli_serve_commands.py: passed.UV_NO_SYNC=1 make lint-python lint-imports lint-deps lint-types: passed after rebase (2268 Python files formatted; 10 import contracts kept; no dependency issues; type checks passed). Both commit-message validators and the repository source-language and large-file gates also passed.Native Windows follow-up (build 10.0.26300):
RAVEN_HOMEdirectories, isolating both upgrade markers and downloaded wheels. A scratch barrier forced both helpers to finish writing the same partial-wheel name before rename: the original fixture produced 1 failed / 1 passed withDistribution not foundin the shared cache; the identical probe with isolated homes produced 2 passed on Python 3.13.12. The ordinary run had passed before the forced overlap, so this result identifies a race rather than a consistently failing upgrade.PYTHONUTF8=1;uv run --frozen --no-sync pytest tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py::test_container_bootstraps_the_browser_after_the_engine_is_ready -n 0 -x --tb=short: 195 passed, 30 skipped on Python 3.13.12. Twenty-six skips were already POSIX-specific; four now identify the reported mode/signal/symlink preconditions. The skipped checks are not Windows equivalents.PYTHONUTF8=1,UV_PYTHON=3.12, and an isolated callerRAVEN_HOME;uv run --no-project --python 3.12 --with 'pytest>=8,<10' --with pytest-asyncio --with pytest-xdist pytest --noconftest tests/integration/test_cli_upgrade_real_uv.py -q --tb=short -n 4: 3 passed, covering two real uv upgrade variants and the workflow contract.Earlier author verification before this Windows follow-up:
uv run --frozen --python 3.12 --all-extras pytest -q tests/test_rpc_transport.py tests/test_cli_serve_commands.py tests/test_docker_runtime.py->233 passed.tests/test_rpc_*.pyandtests/test_cli_smoke.py->3121 passedon the first revision, before the fixture follow-up.--cov=raven --cov-branch->8 failed, 29791 passed, 158 skipped, thenscripts/coverage_gate.py diff --base-ref origin/main --threshold 90->100.00% (32/32 executable changed lines), passed. All eight failures are local baselines: five proxy tests (tests/test_config_update_providers.py) that fail whenever the shell carries this box's proxy;test_install_script.py::test_resolve_node_dir_answers_each_case_it_exists_for(node/npm resolution on this box);test_subagent_node_runtime.py::test_what_cannot_be_read_names_nothing(fails as root); andtest_knowledge_parser_excel.py::test_a_legacy_workbook_is_converted_and_read. The last three were replayed in an unmodified baseline worktree and fail there too (3 failed).403: bad origin, the port-forward cookie case astwo sign-ins wrote one jar entry: ['raven_session_18792'], the launcher cases on empty output. Againstmain's two source files the three test files now fail 18 tests.localhostbranch removed, either parse left unguarded, the cookie keyed to the bound port, the two refusals given one text, the link not printed, the link printed after the browser, andraven serve --openbypassing_open.ruff checkandruff format --checkclean;lint-importsreports10 kept, 0 broken.scripts/check_source_language.py origin/main...HEADandscripts/check_large_files.py origin/main...HEADboth exit 0.raven serveon its ownRAVEN_HOMEreached through a TCP forwarder: the tab signed in and landed on/, the cookie it kept wasraven_session_<forwarded port>, and an RPC call over a socket opened from that page was answered (server log:serve: ws client connected,system.hello). A direct tab on the bound port signed in too. Withfetchstubbed, the page rendered each of the four answers it can receive (refused address, used or expired link, other status, no answer) with the matching text.Risk
Originthat equals the request'sHoston a loopback name. The cross-origin local-page and DNS-rebinding cases remain refused, and both are covered by tests. What it accepts that the old rule did not is a page whose origin is exactly the address it asked for -- which only something serving that address can show, and a file the agent produced arrives with the CSPsandboxdirective and so has an opaque origin.cookie_nameso the two cannot drift.Hostthat does not parse (127.0.0.1:99999,127.0.0.1:abc,[::1) is answered 401 on/rpc, as onmain, and an Origin equal to such a Host is answered 403. Without the guard both would be a 500, since the cookie's name is read before any credential is checked.gui.auth.stale) still names the remedy correctly but not the cause, so a tab that reaches the gateway by a non-loopback address still reports a sign-in failure it cannot act on. Also unmeasured: a real VS Code forward and a real container (both request shapes were reproduced instead).Related Issues
N/A