Reorganize moq-native API: feature-gated modules and builder pattern#1141
Reorganize moq-native API: feature-gated modules and builder pattern#1141
Conversation
- Add with_* builder methods to ClientConfig and ServerConfig - Move websocket types into public `ws` module (ClientWebSocket → ws::ClientConfig, WebSocketListener → ws::Listener) - Move iroh types into public `iroh` module (IrohEndpointConfig → iroh::EndpointConfig, IrohEndpoint → iroh::Endpoint) - Remove prefixes from internal quinn/noq/quiche types (e.g. QuinnClient → quinn::Client) - Use explicit re-exports in lib.rs instead of glob re-exports - Add #[group(id = "...")] to all clap::Args structs to prevent name collisions Closes #1097 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThis PR renames and reorganizes types and modules across crates. In moq-native, backend-specific types are renamed to generic names (e.g., 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-native/src/iroh.rs (2)
47-61:⚠️ Potential issue | 🟠 MajorWrite generated iroh secrets with restricted permissions.
Line 61 uses a plain
tokio::fs::write, so the freshly generated private key can inherit default filesystem permissions and end up readable by other local users. Since this key defines the endpoint identity, create it with owner-only permissions (create_new+0600, or the platform equivalent) instead of a default-permissions write.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-native/src/iroh.rs` around lines 47 - 61, The generated secret is written with tokio::fs::write in EndpointConfig::bind (when SecretKey::generate is used), which can create a file with default permissive permissions; change this to create the file with owner-only permissions instead: open the target PathBuf with tokio::fs::OpenOptions (create_new = true, write = true) and set mode to 0o600 (use std::os::unix::fs::OpenOptionsExt::mode on Unix) before writing the hex-encoded secret bytes, falling back to a safe cross-platform alternative on Windows; ensure you still handle and propagate the same errors as the original tokio::fs::write usage.
124-133:⚠️ Potential issue | 🟠 MajorRespect configured MoQ versions on the iroh path.
Lines 128-131 accept the first client-offered protocol, and Lines 172-178 always advertise the full
moq_lite::ALPNSset. That makesClientConfig::versionandServerConfig::versionineffective foriroh://sessions, unlike the noq/quinn/quiche paths that negotiate againstversions.alpns(). Thread the allowed ALPN list through these helpers and reject when there is no overlap.Also applies to: 156-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-native/src/iroh.rs` around lines 124 - 133, The iroh path currently accepts the first client-offered protocol and advertises the hard-coded moq_lite::ALPNS, ignoring configured ClientConfig::version/ServerConfig::version; change the logic in the Request::WebTransport branch of ok (and the similar helper used around the 156-178 region) to accept only ALPNs that intersect with the configured allowed list (use versions.alpns() or the server/client version-derived ALPN list), pick the first matching ALPN from request.protocols that is also in the allowed set, call response.with_protocol(matching) when present, and return a reject/error when there is no overlap; also replace any use of moq_lite::ALPNS for advertised ALPNs with the negotiated/allowed ALPN list so negotiation respects ClientConfig::version/ServerConfig::version.
🧹 Nitpick comments (1)
rs/moq-native/src/server.rs (1)
122-155: Add builders for the QUIC-LB fields too.Lines 122-155 cover most of
ServerConfig, butquic_lb_idandquic_lb_noncestill require direct field mutation. If this PR is meant to move callers toward a fluent API, those two knobs should getwith_*helpers as well.♻️ Minimal API completion
+ pub fn with_quic_lb_id(mut self, quic_lb_id: ServerId) -> Self { + self.quic_lb_id = Some(quic_lb_id); + self + } + + pub fn with_quic_lb_nonce(mut self, quic_lb_nonce: usize) -> Self { + self.quic_lb_nonce = Some(quic_lb_nonce); + self + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-native/src/server.rs` around lines 122 - 155, ServerConfig's fluent API is incomplete because quic_lb_id and quic_lb_nonce still require direct mutation; add builder methods (e.g., with_quic_lb_id and with_quic_lb_nonce) mirroring the style of with_bind/with_backend/etc. that set the corresponding fields on self (accepting the appropriate types used by quic_lb_id and quic_lb_nonce), return Self, and follow the same mut self, set field, return self pattern so callers can use the same fluent API as with_bind, with_backend, with_tls_cert, etc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@rs/moq-native/src/iroh.rs`:
- Around line 47-61: The generated secret is written with tokio::fs::write in
EndpointConfig::bind (when SecretKey::generate is used), which can create a file
with default permissive permissions; change this to create the file with
owner-only permissions instead: open the target PathBuf with
tokio::fs::OpenOptions (create_new = true, write = true) and set mode to 0o600
(use std::os::unix::fs::OpenOptionsExt::mode on Unix) before writing the
hex-encoded secret bytes, falling back to a safe cross-platform alternative on
Windows; ensure you still handle and propagate the same errors as the original
tokio::fs::write usage.
- Around line 124-133: The iroh path currently accepts the first client-offered
protocol and advertises the hard-coded moq_lite::ALPNS, ignoring configured
ClientConfig::version/ServerConfig::version; change the logic in the
Request::WebTransport branch of ok (and the similar helper used around the
156-178 region) to accept only ALPNs that intersect with the configured allowed
list (use versions.alpns() or the server/client version-derived ALPN list), pick
the first matching ALPN from request.protocols that is also in the allowed set,
call response.with_protocol(matching) when present, and return a reject/error
when there is no overlap; also replace any use of moq_lite::ALPNS for advertised
ALPNs with the negotiated/allowed ALPN list so negotiation respects
ClientConfig::version/ServerConfig::version.
---
Nitpick comments:
In `@rs/moq-native/src/server.rs`:
- Around line 122-155: ServerConfig's fluent API is incomplete because
quic_lb_id and quic_lb_nonce still require direct mutation; add builder methods
(e.g., with_quic_lb_id and with_quic_lb_nonce) mirroring the style of
with_bind/with_backend/etc. that set the corresponding fields on self (accepting
the appropriate types used by quic_lb_id and quic_lb_nonce), return Self, and
follow the same mut self, set field, return self pattern so callers can use the
same fluent API as with_bind, with_backend, with_tls_cert, etc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bcb380a-8abd-45c4-96f5-590bebdb46e7
📒 Files selected for processing (12)
rs/moq-cli/src/main.rsrs/moq-native/src/client.rsrs/moq-native/src/iroh.rsrs/moq-native/src/lib.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/server.rsrs/moq-native/src/ws.rsrs/moq-native/tests/backend.rsrs/moq-native/tests/broadcast.rsrs/moq-relay/src/config.rs
…uilder methods - Write iroh secret key files with 0o600 permissions on Unix - Pass configured ALPN list to iroh Request::accept/ok and connect(), matching the pattern used by quinn/noq/quiche backends - Add with_quic_lb_id and with_quic_lb_nonce builder methods to ServerConfig Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-native/src/iroh.rs (1)
104-123:⚠️ Potential issue | 🟠 MajorUse the new
alpnsparameter on the raw QUIC branch too.Line 119 still checks
moq_lite::ALPNS, so the configured version subset is ignored for non-H3 iroh sessions.Targeted fix
- alpn if moq_lite::ALPNS.contains(&alpn) => Ok(Self::Quic { + alpn if alpns.contains(&alpn) => Ok(Self::Quic { request: web_transport_iroh::QuicRequest::accept(conn), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-native/src/iroh.rs` around lines 104 - 123, The accept function ignores the passed-in alpns for non-H3 sessions; replace the moq_lite::ALPNS check with the local alpns parameter (use alpns.contains(&alpn)) in the Quic match arm and ensure the Self::Quic construction carries the alpns value (similar to Self::WebTransport) and uses web_transport_iroh::QuicRequest::accept(conn) for the request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-native/src/iroh.rs`:
- Around line 56-63: The current bootstrap probes the path with
PathBuf::exists() then calls write_secret_file(path, ...) which opens with
create_new(true), creating a TOCTOU race; change the logic in the
secret-bootstrapping code that uses self.secret (and the duplicate block around
the same logic at the later location) to avoid the separate exists() check:
attempt to create/write the file via write_secret_file and if it returns an
AlreadyExists error, fall back to reading the file (e.g., call read_secret_file
or the same code path used when the file exists) instead of propagating the
error; keep using SecretKey::generate and rand::rng() for the generation path
and ensure write_secret_file remains the create_new(true) call so the race is
safely handled by the AlreadyExists branch.
---
Outside diff comments:
In `@rs/moq-native/src/iroh.rs`:
- Around line 104-123: The accept function ignores the passed-in alpns for
non-H3 sessions; replace the moq_lite::ALPNS check with the local alpns
parameter (use alpns.contains(&alpn)) in the Quic match arm and ensure the
Self::Quic construction carries the alpns value (similar to Self::WebTransport)
and uses web_transport_iroh::QuicRequest::accept(conn) for the request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b8aef8c-67e8-4a0e-8515-1559f50ddfb7
📒 Files selected for processing (3)
rs/moq-native/src/client.rsrs/moq-native/src/iroh.rsrs/moq-native/src/server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- rs/moq-native/src/server.rs
| } else if let Some(path) = self.secret { | ||
| let path = PathBuf::from(path); | ||
| if !path.exists() { | ||
| // Generate a new random secret and write it to the file. | ||
| // Generate a new random secret and write it to the file with restricted permissions. | ||
| let secret = SecretKey::generate(&mut rand::rng()); | ||
| tokio::fs::write(path, hex::encode(secret.to_bytes())).await?; | ||
| let data = hex::encode(secret.to_bytes()); | ||
| write_secret_file(&path, data.as_bytes()).await?; | ||
| secret |
There was a problem hiding this comment.
Avoid the exists() / create_new() TOCTOU on secret-file bootstrap.
Lines 58-63 do a separate existence probe before write_secret_file() opens the path with create_new(true). If another process wins that race, bind() fails even though the key file is now valid; please fall back to reading the file on AlreadyExists instead of bubbling the error.
Also applies to: 225-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-native/src/iroh.rs` around lines 56 - 63, The current bootstrap probes
the path with PathBuf::exists() then calls write_secret_file(path, ...) which
opens with create_new(true), creating a TOCTOU race; change the logic in the
secret-bootstrapping code that uses self.secret (and the duplicate block around
the same logic at the later location) to avoid the separate exists() check:
attempt to create/write the file via write_secret_file and if it returns an
AlreadyExists error, fall back to reading the file (e.g., call read_secret_file
or the same code path used when the file exists) instead of propagating the
error; keep using SecretKey::generate and rand::rng() for the generation path
and ensure write_secret_file remains the create_new(true) call so the race is
safely handled by the AlreadyExists branch.
1 participant
Summary
with_*builder methods toClientConfigandServerConfigfor fluent configurationwsmodule (ClientWebSocket→ws::ClientConfig,WebSocketListener→ws::Listener)irohmodule (IrohEndpointConfig→iroh::EndpointConfig, etc.)lib.rsinstead of glob re-exports#[group(id = "...")]to allclap::Argsstructs to prevent name collisionsCloses #1097
Test plan
just fix— no formatting issuescargo check -p moq-native --all-features— compilescargo check -p moq-cli -p moq-relay --all-features— downstream crates compilecargo test -p moq-native --lib— all 8 unit tests pass🤖 Generated with Claude Code