Reorganize moq-native API: feature-gated modules and builder pattern#1141
Open
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>
Contributor
WalkthroughThis PR renames and re-exports types and reorganizes modules across crates. In moq-native: backend-specific types are renamed to generic 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ 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}
Contributor
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>
Contributor
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
Use create_new(true) for atomic secret file creation to handle concurrent startup races. Pass ALPNs vec into Request::Quic so ALPN matching uses the configured list instead of the hardcoded constant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
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)
193-209:⚠️ Potential issue | 🟡 MinorUse the caller-supplied
alpnsin the raw-QUIC branch too.Line 205 still checks
moq_lite::ALPNS, so a caller that negotiates a narrower or future ALPN list can be rejected after the handshake even thoughconnect_with_opts()offered it. Matching againstalpnskeeps this symmetric withRequest::accept().🔧 Minimal fix
- alpn if moq_lite::ALPNS.contains(&alpn) => { + alpn if alpns.contains(&alpn) => {🤖 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 193 - 209, The raw-QUIC branch is still validating against moq_lite::ALPNS instead of the caller-supplied alpns, causing valid negotiated ALPNs to be rejected; change the match arm to check membership in the caller-provided alpns (e.g., replace moq_lite::ALPNS.contains(&alpn) with a check against alpns, converting types as needed such as alpns.contains(&alpn.to_string()) or alpns.iter().any(|p| p == alpn)) so the arm becomes the same pattern as the H3 branch (await connecting, then call web_transport_iroh::Session::raw(conn)).
🤖 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 63-70: The AlreadyExists branch is racy because a concurrent
reader can see a truncated/empty file; fix by either making write_secret_file
perform atomic publish (write to a temp file in the same directory and then
tokio::fs::rename to the final path) or by changing the AlreadyExists fallback
to retry reading until SecretKey::from_str succeeds (loop:
tokio::fs::read_to_string(&path) -> attempt SecretKey::from_str, sleep short
backoff on parse/empty result, and give up with a clear error after a
timeout/attempt limit). Update the code that calls write_secret_file and the
match arm using tokio::fs::read_to_string and SecretKey::from_str accordingly so
readers only parse fully-written files.
---
Outside diff comments:
In `@rs/moq-native/src/iroh.rs`:
- Around line 193-209: The raw-QUIC branch is still validating against
moq_lite::ALPNS instead of the caller-supplied alpns, causing valid negotiated
ALPNs to be rejected; change the match arm to check membership in the
caller-provided alpns (e.g., replace moq_lite::ALPNS.contains(&alpn) with a
check against alpns, converting types as needed such as
alpns.contains(&alpn.to_string()) or alpns.iter().any(|p| p == alpn)) so the arm
becomes the same pattern as the H3 branch (await connecting, then call
web_transport_iroh::Session::raw(conn)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9697dc34-4d18-467c-b45c-fb292218a03d
📒 Files selected for processing (1)
rs/moq-native/src/iroh.rs
Comment on lines
+63
to
+70
| match write_secret_file(&path, data.as_bytes()).await { | ||
| Ok(()) => secret, | ||
| Err(e) | ||
| if e.downcast_ref::<std::io::Error>() | ||
| .is_some_and(|io| io.kind() == std::io::ErrorKind::AlreadyExists) => | ||
| { | ||
| let key_str = tokio::fs::read_to_string(&path).await?; | ||
| SecretKey::from_str(&key_str)? |
Contributor
There was a problem hiding this comment.
The AlreadyExists fallback can still read an incomplete key file.
Another process can hit this branch after the winner has created the path but before it has finished writing the hex payload, so the one-shot read_to_string() can still see empty/truncated contents and fail SecretKey::from_str(). Please retry until a valid key is readable, or publish via a temp file + atomic rename, so concurrent startup doesn’t still fail intermittently.
🤖 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 63 - 70, The AlreadyExists branch is
racy because a concurrent reader can see a truncated/empty file; fix by either
making write_secret_file perform atomic publish (write to a temp file in the
same directory and then tokio::fs::rename to the final path) or by changing the
AlreadyExists fallback to retry reading until SecretKey::from_str succeeds
(loop: tokio::fs::read_to_string(&path) -> attempt SecretKey::from_str, sleep
short backoff on parse/empty result, and give up with a clear error after a
timeout/attempt limit). Update the code that calls write_secret_file and the
match arm using tokio::fs::read_to_string and SecretKey::from_str accordingly so
readers only parse fully-written files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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