Repository navigation
feat(cli): reach a server through a Unix socket and refuse non-server schemes - #988
Conversation
… schemes `--server unix:///absolute/path.sock`, or a configured server `url:` of that form, carries the CLI's HTTP/1.1 requests over a Unix-domain socket, so a sandboxed client allowed one socket path can reach a local gateway in front of omnigraph-server. Discovery, the contract header, the bearer token and the no-redirect and no-retry rules are the same as over TCP. `ServerTransport::parse` is the one parser of a server address. It runs where every literal `--server`, configured url, profile and alias server resolves, so a server scope with any scheme other than http, https or unix now fails as an invalid server address instead of falling through to storage resolution. A unix root's whole path names the socket: `GraphHttpClient::request` sends what a composed URL carries after the root to http://localhost and refuses a URL outside the root before sending anything.
ragnorc
left a comment
There was a problem hiding this comment.
Recommendation: fix the platform-dependent address test before merge. I found one P2 test defect, described inline. I found no other blocking defect in the Unix runtime path.
This PR lets a sandboxed CLI reach OmniGraph through an allowed Unix-socket path. A local relay forwards HTTP to the existing TCP server. The CLI keeps the socket address for configuration and token lookup, then maps each request to a root-relative HTTP target. It also rejects storage and unknown schemes in --server, so an explicit server scope cannot become embedded storage access.
--server must never select embedded storage. The fix enforces that rule at server-address resolution. resolve_server_flag validates literal and configured roots. The shared classifier then covers reads, writes, registry commands, aliases, and cluster administration. GraphHttpClient retains discovery, contract checks, bearer handling, no redirects, and no retries. Blob delivery also retains the selected socket when it rebuilds its client.
The tradeoff is a new local relay process and socket-permission setup. Discovery still precedes each data request; this is an access feature, with no measured speed claim. The internal transport enum, normalized root, and decoded socket path separate the filesystem address from the HTTP target. They stay behind one request boundary. This removes the server-to-storage fallthrough and a duplicate registry scheme check. The new unix:// address contract needs platform and path-encoding coverage. It adds no durable state or storage format. Reusing reqwest and one shared test relay keeps the maintenance cost contained. Five more changes of this kind should extend this boundary and its tests, while still sharing credential and contract logic.
I checked reqwest 0.12.28's ClientBuilder::unix_socket and connector implementation: the fixed socket bypasses DNS and proxies. This agrees with the versioned upstream API. I also checked OmniGraph's storage classifier and the pinned Lance 11.0.0 object-store registry. Unix here is an HTTP transport, with storage still owned by the server. No private Lance API or future Lance feature is needed. The design fits the open profile work (#675), managed-client direction (#897), and request/outcome work (#981); these branches were inspected for direction, not tested as a combined merge.
The CLI Rust tests are the correct owner. GQT cannot choose CLI schemes, operator configuration, Unix sockets, or wire headers. The existing integration owners cover the user-visible contract without a second graph-test setup.
Local before/after proof on macOS:
- Before:
d8d2e3a8ddaa614acb6aa8fc1371f81c5bcf2897, with only the PR's two integration regressions and shared fixture carried back. All four test/fixture files matched the reviewed head byte for byte. 2 tests ran; 2 failed (exit 101). The scheme test gotserver discovery transport failedforftp://without a graph. The socket journey started its server and relay, then gotstorage_root_invalid/ unsupportedunixscheme on its first query. These were the predicted behavioral failures. - After: exact head
15586e2678e69378dec8fd58ca7d1b2351a972b3. 2 tests ran; 2 passed (exit 0), both in the clean baseline and again after restoring the checkout. Query/export matched TCP, Blob bytes matched, and mutation/load rows appeared in the export.
Both runs used this command from the checkout root:
env RUSTUP_TOOLCHAIN=1.97.1 \
CARGO_TARGET_DIR=/tmp/review-988/target \
RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
RUST_MIN_STACK=16777216 RUSTC_WRAPPER= \
CARGO_BIN_EXE_omnigraph-server=/tmp/review-988/target/debug/omnigraph-server \
cargo test --locked -p omnigraph-cli -p omnigraph-server \
--test integration -j3 issue_979 -- --nocaptureI prebuilt with cargo build --locked -p omnigraph-cli -p omnigraph-server --bin omnigraph-server --test integration -j3 under the same toolchain/flags. This avoided the fixture's initial 30-second server-build timeout. Selecting both packages kept build features aligned; the server test target matched zero cases and is not counted as proof.
Other local checks: the two new unit tests passed. The head-built unit binary, run with graph_http --nocapture, passed six cases; its normally ignored five-second timeout case also passed when run by its full name with --exact --ignored --nocapture. cargo fmt --all --check, git diff --check, bash scripts/check-agents-md.sh, /tmp/review-916-docs/bin/python scripts/check-docs.py, and python3 scripts/check-dependency-sources.py passed. All temporary test edits are restored, and the checkout is clean. Nothing was pushed.
GitHub CI is green. Its workspace log confirms all four new tests passed. That job checked out synthetic merge 4ed8800144a24214486afcf151edd1140b48b65c (this head into ff80af2fbca81ceb396faae8f1d58931442e6c01); it is merge evidence. The local runs above provide the exact-head evidence.
The Windows finding follows directly from the unconditional test and the cfg(not(unix)) refusal; I did not execute a Windows binary. I did not test the documented socat command or a real agent sandbox allowlist. The socket journey uses the PR's byte relay and the real server. No performance benchmark was run. No optional improvements are requested.
`unix_transport` refuses every `unix://` address on a non-Unix platform, so the address test's Unix acceptance and malformed-path cases failed wherever the CLI builds without Unix sockets (the Windows release target). The scheme classification and the refusal of storage schemes stay unconditional; the socket path cases run under `cfg(unix)`, and other platforms assert the platform refusal instead.
What & why
Sandboxed agents often cannot open a TCP connection to a loopback server, but a sandbox can allow one Unix-socket path.
--server unix:///absolute/path.sock(or a serverurl:of that form in~/.omnigraph/config.yaml) now carries the CLI's HTTP/1.1 requests over that socket, for example to a local gateway in front ofomnigraph-server. A--servervalue or configuredurl:with any scheme other thanhttp,httpsorunixnow fails as an invalid server address instead of being resolved as graph storage ([storage_root_invalid] … unsupported storage URI scheme).Backing issue / RFC
--server, and refuse unknown server schemes #979 (labelledneeds-triage, not yetaccepted)Checklist
Design
graph_http::ServerTransport::parseis the one parser of a server address.resolve_server_flagcalls it for every literal--server, configuredurl:, profile and alias server, so a server scope can no longer resolve to an embedded client.GraphHttpClientbuilds its transport from the same parser, andhelpers::is_remote_uriuses the same scheme list (now case-insensitive, as URL schemes are).unix://root's whole path names the socket, so a request's HTTP target is whatever its composed URL carries after the root.GraphHttpClient::request, now fallible, is the one place that mapsunix://…/graphs/g/…tohttp://localhost/graphs/g/…. It refuses a URL outside the root (another socket path, or a..graph id) before sending anything. URL composition (remote_url), user-visible URIs, keyed-token matching and write-target echoes keep theunix://spelling.ClientBuilder::unix_socket(already a dependency). Proxies and DNS do not apply, and requests carryHost: localhost, which the server does not read. Discovery, the contract header, the bearer token, no redirects and no retries are unchanged, because every client of a root is built by the sameGraphHttpClient::build.percent-encoding, already in the lockfile throughurl), so~/Library/Application Support/…works written raw or as%20. A host, query, fragment, a path naming a directory (…/,…/..) and a NUL byte are refused. Non-Unix platforms refuseunix://.GraphClient::resolve_registrydrops its own scheme check, whichresolve_server_flagnow guarantees.Local verification
cargo test --locked -p omnigraph-cli(withRUST_MIN_STACK=16777216): 145 unit tests passed (6 ignored), 296 integration tests passed (8 ignored)cargo clippy -p omnigraph-cli --all-targets --locked -- -D warnings -W clippy::dbg_macro: cleancargo fmt --all --check,typos: cleanpython3 scripts/check-docs.py,bash scripts/check-agents-md.sh,python3 scripts/release_notes.py preview --working-tree: passpython3 scripts/check-dependency-sources.py,cargo deny --locked check: passpython3 scripts/check-fix-regression.pyagainst this body:ok: issue #979 has a matching regression additionquery --server unix:///tmp/x.sock --graph gand--server ftp://hwith[storage_root_invalid]. This branch attempts the socket (server discovery connection failed (Unix socket); … this data request was not sent) and refusesftp://as an invalid server address.Tests
helpers::tests::server_flag_resolves_unix_sockets_and_refuses_other_schemes_issue_979: the accepted and refused address table.client::tests::graph_http_over_a_unix_socket_sends_root_relative_targets_issue_979: over a real socket, discovery isHEAD /healthzwith no bearer. The data request's target is/graphs/knowledge/branches/etl%2Frun%201?branch=a+b, withHost: localhost, the bearer and the contract header. URLs outside the root are refused unsent, and a removed socket fails discovery asconnection failed (Unix socket), not dispatched.system_remote::remote_unix_socket_serves_query_mutate_load_export_and_blob_get_issue_979: the real CLI and server throughtests/support/unix_gateway.rs, a byte relay whose socket sits under a directory with a space. Query and export output over the socket equal the same reads over TCP, the Blob bytes are exact, mutate and load land, and every command used the socket.cli_data::server_scope_refuses_a_non_server_scheme_issue_979: query, mutate, load, export andblob get(with and without--graph),graphs listandcluster statusrefuseftp://,s3://,file://and a configuredurl: ftp://…as an invalid server address, never as storage.--store unix://…is refused like anhttp://store.Notes for reviewers
docs/user/deployment.mdshows asocat UNIX-LISTEN:…,fork TCP:127.0.0.1:8080relay.socatwas not available locally, so that example command is untested.reference.mdandserver.mdadditions fit into existing lines.docs/user/cli/index.md(new section),docs/user/cli/reference.md,docs/user/operations/server.md,docs/user/deployment.md,docs/dev/testing.md, and twochangelog.dnotes.