Skip to content

feat(relay): unauthenticated internal listener over qmux (tcp:// + unix://)#1810

Merged
kixelated merged 6 commits into
mainfrom
claude/eloquent-hamilton-db8452
Jun 20, 2026
Merged

feat(relay): unauthenticated internal listener over qmux (tcp:// + unix://)#1810
kixelated merged 6 commits into
mainfrom
claude/eloquent-hamilton-db8452

Conversation

@kixelated

@kixelated kixelated commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Why

We want to run localhost (and trusted same-host / private-VPC) workers against the relay over qmux without paying for TLS and UDP. They should be treated as fully privileged (internal), with no JWT or certificate ceremony.

This adds dedicated, opt-in listeners for exactly that. They're isolated from the public QUIC/HTTPS paths, so the privilege can't leak: the only thing that grants unrestricted access is connecting to one of these sockets.

What changed

Relay (rs/moq-relay)

  • New [internal] config with two independent, opt-in listeners:
    • [internal.tcp] listen = "host:port" → plain-TCP qmux (tcp://), flag --internal-listen / MOQ_INTERNAL_LISTEN.
    • [internal.uds] listen = "/path" → Unix-socket qmux (unix://), flag --internal-uds-listen / MOQ_INTERNAL_UDS_LISTEN.
    • Both can run at once.
  • Every accepted connection gets AuthToken::unrestricted("") on the internal tier (full publish/subscribe under the empty root) — the local-worker analogue of an mTLS cluster peer dialing /.
  • TCP has no peer identity, so a non-loopback bind logs a warning (operator firewalls it); it is not refused.
  • Unix sockets support [internal.uds.allow] with uid / gid / pid lists, checked against kernel-reported peer credentials (SO_PEERCRED / LOCAL_PEERCRED) at accept. Empty = no check; AND across fields, OR within a field; a required pid fails closed if the platform doesn't report one. This is how you restrict access to a specific worker user rather than any local process (loopback TCP can't do that). The allowlist lives inside [internal.uds] because it's Unix-only.

moq-native (rs/moq-native)

  • New tcp:// and unix:// schemes (qmux directly over a raw stream — no TLS, no WebSocket framing) with client connect + server Listeners. The Unix Listener surfaces PeerCred and sets socket perms.
  • Both are always-on for native via default features and drop out of --no-default-features (wasm) builds. uds is unix-only.
  • The application protocol (moq ALPN) is negotiated in-band (qmux 0.2 transport parameter), so the exact moq version is agreed up front instead of defaulting via SETUP.

qmux 0.0.8 → 0.2.0 (breaking dep bump)

  • tcp/uds/ws builder API replaces the old free functions.
  • ws::Barews::Upgraded.
  • qmux::PREFIXES was removed; the WebSocket subprotocol builders now reconstruct the {prefix}{alpn} cross-product from qmux::Version::prefix(). The existing WS negotiation regression test was updated and still passes.

Docs

  • doc/bin/relay/auth.md (new "Internal Listener" section) and doc/bin/relay/config.md ([internal.tcp] / [internal.uds]).

Reviewer notes

  • Public API: new additive surface — moq_native::{tcp, unix} modules (Listener, PeerCred, Error) and moq_relay::{InternalConfig, InternalTcp, InternalUds, InternalAllow, run_internal}. No existing public signatures changed; qmux is not re-exported, so its major bump isn't a breaking change to moq-native's surface. Hence targeting main.
  • Security: the listeners are unauthenticated by design. TCP relies on the bind interface + firewall; Unix relies on filesystem perms + the allow peercred check. Root on the host can always bypass either (it can read the socket / ptrace the worker) — the threat model is other non-root users on a shared host.
  • wasm: tcp:///unix:// are native-only (browsers can't open raw TCP/Unix sockets); the JS client is unaffected.
  • Not wired: moq-cli doesn't yet expose the tcp/uds features, so the moq publish tcp://… doc examples assume a CLI built with them. Happy to add that wiring in a follow-up if wanted.

Test plan

  • cargo check --workspace --all-targets (qmux 0.2 bump clean across all crates)
  • cargo fmt --check + clippy -D warnings (moq-native, moq-relay)
  • moq-native + moq-relay test suites pass, including the reworked WS subprotocol regression test
  • Smoke tests internal_tcp_round_trip and internal_unix_round_trip round-trip a frame end-to-end and assert the newest moq-lite version is negotiated in-band
  • moq-native --no-default-features (wasm-style) build is clean with tcp/uds compiled out

🤖 Generated with Claude Code

(Written by Claude)

kixelated and others added 4 commits June 19, 2026 12:43
…ix://)

Add a second relay listener for trusted same-host workers that want qmux
without the overhead of TLS and UDP. Every accepted connection is granted
unrestricted, internal-tier access (full publish/subscribe under the empty
root), with no JWT or client certificate. This is the local-worker analogue
of an mTLS cluster peer dialing "/".

A single `--internal-listen` value picks the transport: a `host:port` binds a
plain-TCP listener, a filesystem path (optionally `unix:`-prefixed) binds a
Unix socket. TCP carries no peer identity, so a non-loopback bind logs a
warning (operator firewalls it). A Unix socket can additionally restrict
callers by kernel-reported credentials via `[internal.allow]` (uid/gid/pid;
empty = no check, AND across fields, OR within), which is how you ensure only
a specific worker user connects rather than any local process.

moq-native gains `tcp://` and `unix://` schemes (qmux over a raw stream) plus
server `Listener`s; both are always-on for native via default features and
drop out of `--no-default-features` (wasm) builds. The application protocol is
negotiated in-band (qmux 0.2 transport parameter), so the exact moq version is
agreed up front instead of falling back to a SETUP default.

Adopts the breaking qmux 0.0.8 -> 0.2.0 bump: the tcp/uds/ws builder API
replaces the old free functions, `ws::Bare` becomes `ws::Upgraded`, and the
removed `qmux::PREFIXES` is reconstructed locally via `Version::prefix()` in
the WebSocket subprotocol builders.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The peer-credential allowlist only applies to Unix sockets, so a single
`listen` field (TCP-or-Unix) with a sibling `allow` was awkward: `allow` was
silently ignored for TCP. Split into `[internal.tcp]` and `[internal.uds]`,
each with its own `listen`, and move `allow` inside `[internal.uds]` where it
belongs.

Drops the `InternalListen` enum (and its custom FromStr/Display/serde): the TCP
listener is a plain `Option<SocketAddr>` and the Unix listener a plain
`Option<PathBuf>`, both parsed natively. The Unix flag is now
`--internal-uds-listen` (`MOQ_INTERNAL_UDS_LISTEN`); `--internal-listen` stays
the TCP one. Both listeners can run concurrently.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`uds` implies `tcp`, so it was covered transitively, but add it explicitly to
the `Client::new`/`connect` cfg(any) feature lists (and the NoBackend
messages) so the gates stay correct if uds ever stops implying tcp.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
moq-transport-18 requires the qmux-01 wire format, so pairing it with qmux-00
is illegal. The qmux 0.0.8 -> 0.2.0 bump introduced qmux-01, so the WebSocket
subprotocol cross-product started emitting `qmux-00.moqt-18`. Exclude it in
both the relay's `supported_subprotocols` and moq-native's
`websocket_subprotocols`, matching the policy already on `dev` (and js/net's
connect.ts).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2394e929-1a5c-40b7-abf1-50705f80f87b

📥 Commits

Reviewing files that changed from the base of the PR and between 510d42f and 3ac1a2f.

📒 Files selected for processing (1)
  • rs/moq-native/src/tcp.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • rs/moq-native/src/tcp.rs

Walkthrough

The pull request bumps the qmux workspace dependency from 0.0.8 to 0.2, adapts WebSocket session creation from qmux::ws::Bare to qmux::ws::Upgraded with conditional in-band ALPN wiring and a revised subprotocol generation algorithm, and adds two new transport modules (tcp and unix) to moq-native for plain-TCP and Unix-domain-socket QMux sessions. A new internal module in moq-relay introduces an unauthenticated listener for trusted local workers, configurable via [internal.tcp] and [internal.uds] TOML sections, with peer-credential allowlist filtering for Unix sockets; run_internal is wired into main via tokio::select!. Smoke tests cover TCP and Unix internal listener round-trips, a config regression test guards TOML field survival through CLI re-parse, and documentation describes the new internal listener semantics and configuration.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main feature: adding an unauthenticated internal listener to the relay using qmux over TCP and Unix sockets, which is the primary objective of the changeset.
Description check ✅ Passed The description provides comprehensive coverage of why the change was made, what was changed across multiple components, security considerations, and test results. It clearly relates to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/eloquent-hamilton-db8452

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (1)
rs/moq-relay/src/internal.rs (1)

1-8: ⚡ Quick win

Add module-level //! docs for the internal module.

This file exports public API, but there is no module-root //! documentation block.
As per coding guidelines: “Document every exported Rust symbol with doc comments (///), including every pub item and module-level docs (//! block at module root)”.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-relay/src/internal.rs` around lines 1 - 8, Add a module-level
documentation block at the very beginning of the internal.rs file before the use
statements and imports. Insert a `//!` comment block that describes the purpose
and contents of the internal module, following Rust documentation conventions.
This documentation should explain what this module does and what public API it
exports.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@doc/bin/relay/auth.md`:
- Around line 259-262: The sentence "any local process of any user can connect"
in the TCP authentication section understates the actual exposure when the relay
is bound to non-loopback addresses. Revise this wording to clarify that while
loopback restricts access to local processes only, non-loopback binds allow
remote hosts on reachable networks to connect as well. This will help operators
correctly understand the true security exposure of their configuration choices.

In `@doc/bin/relay/config.md`:
- Around line 101-103: The opening description of the `[internal]` section
currently describes the listeners as being for "trusted local workers," which is
narrower than the actual behavior. When `internal.tcp.listen` is configured with
a non-loopback address, connections are accepted from any trusted workers on
reachable networks, not just local ones. Update the intro text in the
`[internal]` section (around lines 101-103) to use broader wording such as
"trusted workers on trusted reachable networks" to accurately reflect the actual
access behavior and risk model. Also apply the same terminology update to the
corresponding description at lines 121-122.

In `@rs/moq-native/src/error.rs`:
- Around line 75-81: The Error enum is currently externally exhaustive, which
breaks forward compatibility as new variants like Tcp and Unix are being added.
Add the #[non_exhaustive] attribute to the Error enum declaration to signal to
external consumers that additional variants may be introduced in future
releases, preventing breaking changes when the enum grows.

In `@rs/moq-native/src/lib.rs`:
- Around line 30-34: The newly exported public modules tcp and unix lack
item-level documentation comments. Add a /// doc comment block above each of the
pub mod tcp; and pub mod unix; declarations to document their purpose and
functionality according to the coding guidelines that require every exported
Rust symbol to have doc comments. Ensure the doc comments clearly describe what
each module provides to users of the library.

In `@rs/moq-native/src/unix.rs`:
- Around line 106-108: The code unconditionally removes socket files at two
locations without verifying ownership, risking deletion of active listeners or
files recreated by other processes. First, in the fs::symlink_metadata match
block around line 107, before calling fs::remove_file on a socket path, add
verification that the socket is not actively in use or owned by another process.
Second, in the Drop implementation around lines 170-174, before unconditionally
removing the socket file, add a check to verify that the current path still
refers to the socket this listener created (such as by comparing inode numbers
or other identifying metadata), and only remove it if ownership can be proven.
This ensures bind-time preemptive unlink and drop-time cleanup only delete
sockets that are definitely safe to remove.

In `@rs/moq-relay/src/config.rs`:
- Around line 343-346: The test isolation in the unsafe block is incomplete as
it only clears MOQ_INTERNAL_LISTEN and MOQ_INTERNAL_UDS_LISTEN environment
variables. To prevent test environment-dependency and ensure proper isolation,
add three additional std::env::remove_var calls within the same unsafe block to
clear MOQ_INTERNAL_ALLOW_UID, MOQ_INTERNAL_ALLOW_GID, and MOQ_INTERNAL_ALLOW_PID
environment variables alongside the existing removal calls.

In `@rs/moq-relay/src/internal.rs`:
- Around line 190-195: The error message in the run_uds function currently
doesn't distinguish between two different failure scenarios: running on a
non-Unix target versus running on Unix without the uds feature enabled. Split
the run_uds function into two separate implementations using distinct cfg
attributes: one for non-Unix targets (cfg(not(unix))) with an error message
indicating Unix socket support is only available on Unix platforms, and another
for Unix systems without the uds feature (cfg(all(unix, not(feature = "uds"))))
with an error message indicating the relay was built without the uds feature.
This way, operators will see the correct error message for their specific
situation during debugging.

In `@rs/moq-relay/tests/smoke.rs`:
- Around line 364-365: The spawn_internal_unix_relay() function is gated only on
the unix platform with #[cfg(unix)], but it should also be gated on the uds
feature since the test path relies on internal.uds.listen support. Update the
conditional compilation attribute to gate the function on both unix and the uds
feature (using #[cfg(all(unix, feature = "uds"))]). Apply the same fix to the
other related test function mentioned in the also applies to section around
lines 399-401.

---

Nitpick comments:
In `@rs/moq-relay/src/internal.rs`:
- Around line 1-8: Add a module-level documentation block at the very beginning
of the internal.rs file before the use statements and imports. Insert a `//!`
comment block that describes the purpose and contents of the internal module,
following Rust documentation conventions. This documentation should explain what
this module does and what public API it exports.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0fb8e893-f3f6-48e0-beff-b88dab60fc3e

📥 Commits

Reviewing files that changed from the base of the PR and between 6ce3708 and 510d42f.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (17)
  • Cargo.toml
  • doc/bin/relay/auth.md
  • doc/bin/relay/config.md
  • rs/moq-native/Cargo.toml
  • rs/moq-native/src/client.rs
  • rs/moq-native/src/error.rs
  • rs/moq-native/src/lib.rs
  • rs/moq-native/src/tcp.rs
  • rs/moq-native/src/unix.rs
  • rs/moq-native/src/websocket.rs
  • rs/moq-relay/Cargo.toml
  • rs/moq-relay/src/config.rs
  • rs/moq-relay/src/internal.rs
  • rs/moq-relay/src/lib.rs
  • rs/moq-relay/src/main.rs
  • rs/moq-relay/src/websocket.rs
  • rs/moq-relay/tests/smoke.rs

Comment thread doc/bin/relay/auth.md
Comment on lines +259 to +262
TCP carries no peer identity, so **any local process of any user** can connect.
Loopback is the safest bind; a private VPC interface is also valid. The relay
logs a warning when `listen` is not a loopback address but does not refuse to
start, so firewalling the port is your responsibility.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify TCP reachability wording to include non-local clients.

This section currently says “any local process,” but non-loopback binds can also be reached by remote hosts on reachable networks. Please tighten the wording so operators don’t underestimate exposure.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@doc/bin/relay/auth.md` around lines 259 - 262, The sentence "any local
process of any user can connect" in the TCP authentication section understates
the actual exposure when the relay is bound to non-loopback addresses. Revise
this wording to clarify that while loopback restricts access to local processes
only, non-loopback binds allow remote hosts on reachable networks to connect as
well. This will help operators correctly understand the true security exposure
of their configuration choices.

Comment thread doc/bin/relay/config.md
Comment on lines +101 to +103
Unauthenticated qmux listeners (no TLS) for trusted local workers. Every
connection is granted full, unrestricted access. A TCP and a Unix-socket
listener can each be enabled independently; both default to disabled.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Align [internal] intro wording with non-loopback behavior.

The opening “trusted local workers” phrasing is narrower than actual behavior when internal.tcp.listen is non-loopback. Consider “trusted workers on trusted reachable networks” (or equivalent) to match the risk model.

Also applies to: 121-122

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@doc/bin/relay/config.md` around lines 101 - 103, The opening description of
the `[internal]` section currently describes the listeners as being for "trusted
local workers," which is narrower than the actual behavior. When
`internal.tcp.listen` is configured with a non-loopback address, connections are
accepted from any trusted workers on reachable networks, not just local ones.
Update the intro text in the `[internal]` section (around lines 101-103) to use
broader wording such as "trusted workers on trusted reachable networks" to
accurately reflect the actual access behavior and risk model. Also apply the
same terminology update to the corresponding description at lines 121-122.

Comment on lines +75 to +81
#[cfg(feature = "tcp")]
#[error(transparent)]
Tcp(Arc<crate::tcp::Error>),

#[cfg(all(feature = "uds", unix))]
#[error(transparent)]
Unix(Arc<crate::unix::Error>),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Make Error forward-compatible before adding more variants.

This enum is still externally exhaustive while continuing to grow (new Tcp/Unix variants). Add #[non_exhaustive] on the enum declaration.

Suggested change
+#[non_exhaustive]
 pub enum Error {

As per coding guidelines: "Add #[non_exhaustive] to public enums that may gain variants in the future."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-native/src/error.rs` around lines 75 - 81, The Error enum is currently
externally exhaustive, which breaks forward compatibility as new variants like
Tcp and Unix are being added. Add the #[non_exhaustive] attribute to the Error
enum declaration to signal to external consumers that additional variants may be
introduced in future releases, preventing breaking changes when the enum grows.

Source: Coding guidelines

Comment thread rs/moq-native/src/lib.rs
Comment on lines +30 to +34
#[cfg(feature = "tcp")]
pub mod tcp;
pub mod tls;
#[cfg(all(feature = "uds", unix))]
pub mod unix;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add item docs for newly exported modules.

pub mod tcp; and pub mod unix; are new public exports but currently have no /// item-level docs.

As per coding guidelines: "Document every exported Rust symbol with doc comments (///), including every pub item and module-level docs."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-native/src/lib.rs` around lines 30 - 34, The newly exported public
modules tcp and unix lack item-level documentation comments. Add a /// doc
comment block above each of the pub mod tcp; and pub mod unix; declarations to
document their purpose and functionality according to the coding guidelines that
require every exported Rust symbol to have doc comments. Ensure the doc comments
clearly describe what each module provides to users of the library.

Source: Coding guidelines

Comment thread rs/moq-native/src/unix.rs
Comment on lines +106 to +108
match fs::symlink_metadata(&path) {
Ok(meta) if meta.file_type().is_socket() => fs::remove_file(&path)?,
Ok(_) => return Err(Error::NotASocket(path)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Avoid unconditional socket-path deletion; it can clobber active or replaced files.

On Line 107, bind-time preemptive unlink of any socket path can replace a live listener. On Lines 170-174, unconditional remove_file during Drop can delete a path that was recreated by another process.

Safer approach
- match fs::symlink_metadata(&path) {
-   Ok(meta) if meta.file_type().is_socket() => fs::remove_file(&path)?,
-   Ok(_) => return Err(Error::NotASocket(path)),
-   Err(err) if err.kind() == io::ErrorKind::NotFound => {}
-   Err(err) => return Err(err.into()),
- }
- let listener = tokio::net::UnixListener::bind(&path)?;
+ let listener = match tokio::net::UnixListener::bind(&path) {
+   Ok(l) => l,
+   Err(err) if err.kind() == io::ErrorKind::AddrInUse => {
+     // only reclaim after confirming stale socket, then retry bind
+     // (e.g., connect probe fails with ECONNREFUSED / ENOENT)
+     // ...
+   }
+   Err(err) => return Err(err.into()),
+ };

Also guard drop cleanup by verifying the current path still refers to the socket this listener created (or skip cleanup unless ownership can be proven).

Also applies to: 170-174

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-native/src/unix.rs` around lines 106 - 108, The code unconditionally
removes socket files at two locations without verifying ownership, risking
deletion of active listeners or files recreated by other processes. First, in
the fs::symlink_metadata match block around line 107, before calling
fs::remove_file on a socket path, add verification that the socket is not
actively in use or owned by another process. Second, in the Drop implementation
around lines 170-174, before unconditionally removing the socket file, add a
check to verify that the current path still refers to the socket this listener
created (such as by comparing inode numbers or other identifying metadata), and
only remove it if ownership can be proven. This ensures bind-time preemptive
unlink and drop-time cleanup only delete sockets that are definitely safe to
remove.

Comment on lines +343 to +346
unsafe {
std::env::remove_var("MOQ_INTERNAL_LISTEN");
std::env::remove_var("MOQ_INTERNAL_UDS_LISTEN");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Harden test isolation by clearing internal allowlist env vars too.

The test currently clears only MOQ_INTERNAL_LISTEN and MOQ_INTERNAL_UDS_LISTEN. If MOQ_INTERNAL_ALLOW_UID/GID/PID is set in the host environment, this test can become environment-dependent.

Suggested patch
 		unsafe {
 			std::env::remove_var("MOQ_INTERNAL_LISTEN");
 			std::env::remove_var("MOQ_INTERNAL_UDS_LISTEN");
+			std::env::remove_var("MOQ_INTERNAL_ALLOW_UID");
+			std::env::remove_var("MOQ_INTERNAL_ALLOW_GID");
+			std::env::remove_var("MOQ_INTERNAL_ALLOW_PID");
 		}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
unsafe {
std::env::remove_var("MOQ_INTERNAL_LISTEN");
std::env::remove_var("MOQ_INTERNAL_UDS_LISTEN");
}
unsafe {
std::env::remove_var("MOQ_INTERNAL_LISTEN");
std::env::remove_var("MOQ_INTERNAL_UDS_LISTEN");
std::env::remove_var("MOQ_INTERNAL_ALLOW_UID");
std::env::remove_var("MOQ_INTERNAL_ALLOW_GID");
std::env::remove_var("MOQ_INTERNAL_ALLOW_PID");
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-relay/src/config.rs` around lines 343 - 346, The test isolation in the
unsafe block is incomplete as it only clears MOQ_INTERNAL_LISTEN and
MOQ_INTERNAL_UDS_LISTEN environment variables. To prevent test
environment-dependency and ensure proper isolation, add three additional
std::env::remove_var calls within the same unsafe block to clear
MOQ_INTERNAL_ALLOW_UID, MOQ_INTERNAL_ALLOW_GID, and MOQ_INTERNAL_ALLOW_PID
environment variables alongside the existing removal calls.

Comment on lines +190 to +195
#[cfg(not(all(feature = "uds", unix)))]
async fn run_uds(path: PathBuf, _allow: InternalAllow, _cluster: Cluster) -> anyhow::Result<()> {
anyhow::bail!(
"internal.uds.listen requests a Unix socket ({}) but this relay was built without the `uds` feature",
path.display()
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Differentiate non-Unix vs missing-uds in the fallback error message.

Line 193 currently always says “built without the uds feature”, but this branch also covers non-Unix targets. That can mislead operators during debugging.

Suggested patch
 #[cfg(not(all(feature = "uds", unix)))]
 async fn run_uds(path: PathBuf, _allow: InternalAllow, _cluster: Cluster) -> anyhow::Result<()> {
-	anyhow::bail!(
-		"internal.uds.listen requests a Unix socket ({}) but this relay was built without the `uds` feature",
-		path.display()
-	)
+	if !cfg!(unix) {
+		anyhow::bail!(
+			"internal.uds.listen requests a Unix socket ({}) but this target is not Unix",
+			path.display()
+		);
+	}
+	anyhow::bail!(
+		"internal.uds.listen requests a Unix socket ({}) but this relay was built without the `uds` feature",
+		path.display()
+	)
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-relay/src/internal.rs` around lines 190 - 195, The error message in
the run_uds function currently doesn't distinguish between two different failure
scenarios: running on a non-Unix target versus running on Unix without the uds
feature enabled. Split the run_uds function into two separate implementations
using distinct cfg attributes: one for non-Unix targets (cfg(not(unix))) with an
error message indicating Unix socket support is only available on Unix
platforms, and another for Unix systems without the uds feature (cfg(all(unix,
not(feature = "uds")))) with an error message indicating the relay was built
without the uds feature. This way, operators will see the correct error message
for their specific situation during debugging.

Comment on lines +364 to +365
#[cfg(unix)]
async fn spawn_internal_unix_relay() -> (std::path::PathBuf, tokio::task::JoinHandle<()>) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate the Unix internal smoke test on uds feature too.

On Unix builds without uds, run_internal rejects internal.uds.listen, so this test path can fail by timeout instead of being skipped.

Suggested patch
-#[cfg(unix)]
+#[cfg(all(unix, feature = "uds"))]
 async fn spawn_internal_unix_relay() -> (std::path::PathBuf, tokio::task::JoinHandle<()>) {
@@
-#[cfg(unix)]
+#[cfg(all(unix, feature = "uds"))]
 #[tokio::test]
 async fn internal_unix_round_trip() {

Also applies to: 399-401

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rs/moq-relay/tests/smoke.rs` around lines 364 - 365, The
spawn_internal_unix_relay() function is gated only on the unix platform with
#[cfg(unix)], but it should also be gated on the uds feature since the test path
relies on internal.uds.listen support. Update the conditional compilation
attribute to gate the function on both unix and the uds feature (using
#[cfg(all(unix, feature = "uds"))]). Apply the same fix to the other related
test function mentioned in the also applies to section around lines 399-401.

kixelated and others added 2 commits June 19, 2026 13:52
`qmux::Session::protocol` is a trait method (not inherent), and qmux isn't
re-exported, so rustdoc's `-D broken-intra-doc-links` rejected the `[`...`]`
link in the tcp module doc. Make it plain inline code.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kixelated kixelated merged commit 3449f21 into main Jun 20, 2026
1 check passed
@kixelated kixelated deleted the claude/eloquent-hamilton-db8452 branch June 20, 2026 00:22
@moq-bot moq-bot Bot mentioned this pull request Jun 20, 2026
@kixelated kixelated mentioned this pull request Jun 21, 2026
@moq-bot moq-bot Bot mentioned this pull request Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant