fix(cli): serve TCP and Unix listeners without QUIC - #2879
Conversation
Co-Authored-By: GPT-5 <noreply@openai.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ba3004f64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Whether a listener has to be bound at all. | ||
| pub fn serves(&self) -> bool { | ||
| self.server.resolved().bind.is_some() || self.lan() | ||
| self.server.has_explicit_listener() || self.lan() |
There was a problem hiding this comment.
When --cluster-lan is combined with only --listen-tcp-bind or --listen-unix-bind, this now starts the server, but spawn_server still passes moq.server.bind.is_some() as the public argument to cluster::serve. Because the QUIC bind was not explicitly set, cluster::serve considers the server mesh-only and closes every non-peer request with 404, including requests arriving on the explicitly configured TCP or Unix listener. The public-listener decision must include these stream binds as well.
Useful? React with 👍 / 👎.
| #[cfg(unix)] | ||
| let ignored = ignored.or_else(|| listen.unix.bind.is_some().then_some("--listen-unix-bind")); |
There was a problem hiding this comment.
When token or devices is invoked with --listen-unix-allow-uid, --listen-unix-allow-gid, or --listen-unix-allow-pid but no Unix bind, these newly exposed CLI flags populate listen.unix.allow, while this check examines only listen.unix.bind. The local command therefore succeeds while silently ignoring the supplied MoQ listener security configuration; inspect the allowlist fields here just as the bind is inspected.
Useful? React with 👍 / 👎.
Co-Authored-By: GPT-5 <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39b4c2f416
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - `--listen-unix-bind <path>` hosts qmux over a Unix socket. The socket's | ||
| filesystem permissions control access, and an optional peer-credential | ||
| allowlist can restrict it further. |
There was a problem hiding this comment.
When this listener is placed in a traversable directory such as /tmp or /run, the documentation implies that its socket permissions provide an access boundary, but StreamListeners::start unconditionally changes the socket to mode 0o666 and the allowlist defaults to None. Consequently, any local user who can traverse the parent directory can connect and publish or consume through this CLI; document that a restrictive parent directory or an explicit credential allowlist is required, or expose a configurable socket mode.
Useful? React with 👍 / 👎.
Co-Authored-By: GPT-5 <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca54c81a8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Whether a listener has to be bound at all. | ||
| pub fn serves(&self) -> bool { | ||
| self.server.resolved().bind.is_some() || self.lan() | ||
| self.server.has_explicit_bind() || self.lan() |
There was a problem hiding this comment.
With only --listen-tcp-bind or --listen-unix-bind, this now lets startup continue, but those sockets are bound later inside Server::listen() in the spawned serve task; spawn_moq calls notify_ready() immediately after spawning that task. As a result, systemd can receive READY=1 before the socket exists, and even receives it when the requested address or path cannot be bound, causing traffic to be routed to an unavailable process. Bind the stream listener before notifying readiness and add a regression test that verifies readiness is withheld on bind failure. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L123-L123
Useful? React with 👍 / 👎.
Co-Authored-By: GPT-5 <noreply@openai.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
moq-clirelease builds.Root cause
Both CLI validation and server startup use
MoqSide::serves(), but it only checked the QUIC--listenbind and LAN discovery. The native server already supports stream-only operation, so a TCP-only or Unix-only configuration was parsed and then treated as if no server had been requested. Also,moq-clidisabledmoq-nativedefault features without explicitly enablingtcpanduds, which made those flags depend on workspace feature unification instead of the released CLI feature graph.Stream sockets are bound asynchronously by
Server::listen(). The CLI previously movedServerinto a spawned serve task and immediately notified systemd, soREADY=1could precede the socket bind and could even be emitted when the bind later failed. The CLI now attaches ordinary-session routing before the stream accept loops capture it, awaitslisten(), and gives the spawned task an already-boundListener. A failed bind therefore aborts initialization before readiness.LAN dispatch used the presence of an explicit QUIC bind as a server-wide public flag. That rejected ordinary requests arriving on explicit TCP and Unix listeners, while changing the flag to cover every listener would also expose the mesh-created QUIC port. Public access is now decided per transport. Local-only command rejection also omitted Unix credential allowlist flags, and the CLI documentation overstated the protection from a socket that is deliberately created with mode
0666.Public API changes
moq_native::listen::Config::has_explicit_bind()query.tcpandudsin themoq-clidependency feature set.Test plan
nix develop --command just fixnix develop --command env CARGO_TARGET_DIR=/home/kixelated/work/moq/target/codex-cli-stream-only CARGO_INCREMENTAL=0 just checknix develop --command env CARGO_TARGET_DIR=/home/kixelated/work/moq/target/codex-cli-stream-only CARGO_INCREMENTAL=0 cargo test -p moq-cli a_stream_bind_failure_prevents_readiness -- --nocapturenix develop --command cargo test -p moq-cli --no-default-features --features quinnnix develop --command cargo test -p moq-native --lib --no-default-features --features aws-lc-rs,tcp,uds stream_nix develop --command cargo test -p moq-cli --no-default-features --features quinn,cluster-lan explicit_stream_listeners_are_public_without_exposing_mesh_quicnix develop --command cargo test -p moq-cli --no-default-features --features quinn token_verbnix develop --command bun remark doc/bin/cli.md rs/moq-cli/README.md --quiet --frailThe
rs/moq-clidocumentation sync is included. This does not change the wire protocol, FFI, gateways, catalog, or container format.Closes #2834
(Written by GPT-5)