fix(native): send the request path and query in the SETUP on every URI-less transport - #2572
Conversation
A `moqt://` / `moql://` dial dropped the URL path and query. moq-native only derived a SETUP path for the `tcp://` and `unix://` schemes, so a raw QUIC client reached the server's default path with no `?jwt=`. `setup_path` now keys off the URL scheme and covers `moqt`/`moql` alongside `tcp`/`unix`, carrying the query as well: path-abempty plus `?` and the query, per draft-ietf-moq-transport-19 section 10.3.1.2. That also fixes `tcp://`, which dropped the query the same way. Schemes whose transport carries its own request URI still send nothing, where a SETUP path is a protocol violation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe change extends SETUP 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ 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.
Actionable comments posted: 2
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/tests/backend.rs (1)
52-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover the complete URI-less transport matrix.
rs/moq-native/src/client.rspreservespath?queryformoqt,moql, andtcp, but this helper expects only/roomfortcp, and every new raw-QUIC caller exercises onlymoqt. Addtcpto the SETUP expectation and addmoqlcases, plus TCP coverage where supported, so these changed branches cannot regress untested.As per coding guidelines, bug fixes require regression tests that fail without the fix.
Suggested expectation fix
- "moqt" | "moql" => Some("/room?jwt=abc"), + "moqt" | "moql" | "tcp" => Some("/room?jwt=abc"),Also applies to: 371-384, 408-414, 599-605
🤖 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/tests/backend.rs` around lines 52 - 77, Update path_test so the SETUP-based expectation preserves /room?jwt=abc for moqt, moql, and tcp, while WebTransport continues expecting /room. Expand the raw-QUIC test cases to cover moql, and add TCP-backed cases wherever the backend is supported, including the related callers near the other referenced sections. Ensure these regression tests fail if client.rs stops preserving the query.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 `@rs/moq-native/src/client.rs`:
- Around line 488-491: Update the Unix URL path extraction in the client setup
logic to avoid query_pairs() form-decoding embedded path content, preserving
literal '+' characters in values such as nested JWT query strings. Decode only
the outer path key/value as needed, and add the requested regression case
covering a Unix SETUP path containing '+'.
In `@rs/moq-relay/tests/smoke.rs`:
- Around line 781-782: Extend the raw-QUIC regression test around the existing
pub_url and sub_url parses to also exercise a moql://.../room URL, performing
the applicable round trip and asserting the room/test result. Keep the existing
moqt:// coverage intact and ensure the added case validates scheme-specific
SETUP-path derivation.
---
Outside diff comments:
In `@rs/moq-native/tests/backend.rs`:
- Around line 52-77: Update path_test so the SETUP-based expectation preserves
/room?jwt=abc for moqt, moql, and tcp, while WebTransport continues expecting
/room. Expand the raw-QUIC test cases to cover moql, and add TCP-backed cases
wherever the backend is supported, including the related callers near the other
referenced sections. Ensure these regression tests fail if client.rs stops
preserving the query.
🪄 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 Plus
Run ID: 8cc2ea4a-3be3-4a1d-8f52-03f22f597a3f
📒 Files selected for processing (9)
doc/bin/cli.mddoc/lib/rs/env/native.mddrafts/draft-lcurley-moq-lite.mdjs/net/src/lite/setup.tsrs/moq-native/src/client.rsrs/moq-native/tests/backend.rsrs/moq-net/src/client.rsrs/moq-net/src/lite/setup.rsrs/moq-relay/tests/smoke.rs
| "unix" => url | ||
| .query_pairs() | ||
| .find(|(k, _)| k == "path") | ||
| .map(|(_, v)| v.into_owned()) | ||
| } else { | ||
| Some(url.path().to_string()) | ||
| .map(|(_, v)| v.into_owned()), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate client.rs and inspect relevant section =="
git ls-files | rg '(^|/)client\.rs$' | sed -n '1,20p'
if [ -f rs/moq-native/src/client.rs ]; then
wc -l rs/moq-native/src/client.rs
sed -n '440,520p' rs/moq-native/src/client.rs
fi
echo
echo "== search for Unix path parsing/usages =="
rg -n 'query_pairs\(\)|Unipath|unix|path|setup|SETU' rs/moq-native/src/client.rs rs/moq-native/src -S | sed -n '1,160p'
echo
echo "== Rust Url::query_pairs form decode behavior probe if tooling present =="
cat > /tmp/query_pairs_probe.rs <<'RS'
fn main() {
use url::Url;
let cases = vec![
"unix:///run/moq.sock?path=/room",
"unix:///run/moq.sock?path=/room?jwt=a+b",
"unix:///run/moq.sock?path=/room?jwt=a%2Bb",
"unix:///run/moq.sock?path=hello+world",
];
for input in cases {
let url = Url::parse(input).unwrap();
let raw = url.query().unwrap_or("");
let found = url.query_pairs()
.find(|(k, _)| k == "path")
.map(|(_, v)| v.to_string());
println!("{:?}\nraw={:?} paths={:?}", input, raw, found);
}
}
RS
if command -v rustc >/dev/null 2>&1 && command -v cargo >/dev/null 2>&1; then
mkdir -p /tmp/urlprobe
cd /tmp/urlprobe
cargo init --name urlprobe --quiet >/dev/null || true
cat >> Cargo.toml <<'TOML'
[dependencies]
url = "2"
TOML
cp /tmp/query_pairs_probe.rs src/main.rs
cargo run --quiet
else
echo "rust tooling not available"
fiRepository: moq-dev/moq
Length of output: 17166
🌐 Web query:
Rust url QuerySerializer query_pairs plus space decoding documentation
💡 Result:
In the Rust url crate, the query_pairs and query_pairs_mut methods implement the application/x-www-form-urlencoded specification [1][2]. Regarding space characters and their handling: Encoding: When using Serializer::append_pair (or query_pairs_mut().append_pair()), space characters in values are percent-encoded to %20 [3][4]. Historically, the url crate encoded spaces as +, but it was updated to follow the modern URL Standard, which specifies %20 as the default encoding [3]. Decoding: The query_pairs() iterator automatically parses and percent-decodes the query string [5]. During this decoding process, both + and %20 are typically interpreted as spaces, consistent with application/x-www-form-urlencoded parsing rules [6][2]. Documentation Notes: The documentation for query_pairs() specifies that it parses the query string as application/x-www-form-urlencoded and returns an iterator of key-value pairs [7]. While some users have noted ambiguity regarding whether decoding occurs [5], the implementation explicitly performs this decoding [5][6]. The percent-encoding crate, which underlies these operations, provides the low-level logic where spaces are handled by defining specific AsciiSet configurations [8]. If you require specific, non-standard encoding behavior (e.g., forcing + instead of %20), you may need to use the form_urlencoded module's lower-level APIs or manual string manipulation, though standard usage will default to %20 for encoding and handle both + and %20 as spaces during decoding [3][4].
Citations:
- 1: https://docs.rs/url/2.1.1/url/
- 2: Query parameter serialization for arbitrary bytes (not just UTF-8) servo/rust-url#219
- 3: encode space to '%20' as per url standard servo/rust-url#928
- 4: https://docs.rs/form_urlencoded/latest/form_urlencoded/struct.Serializer.html
- 5: Ambiguity in documentation of
Url::query_pairs()servo/rust-url#1078 - 6: '+' in query part of urls servo/rust-url#482
- 7: https://github.com/servo/rust-url/blob/48fcbe1c543a8350a74b226c17c1ec06c6e19a68/url/src/lib.rs
- 8: https://docs.rs/percent-encoding/latest/percent_encoding/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== current setup_path tests =="
sed -n '580,620p' rs/moq-native/src/client.rs
echo
echo "== parse form_urlencoded with plus handling =="
python3 - <<'PY'
import urllib.parse
cases = [
"path=/room?jwt=a+b",
"path=/room?jwt=a+b",
"path=/room?jwt=a%2Bb",
]
for query in cases:
print(query, "->", dict(urllib.parse.parse_qsl(query, separator="&")))
PY
echo
echo "== inspect form_urlencoded behavior if available =="
python3 - <<'PY'
try:
import form_urlencoded
query = "path=/room?jwt=a+b"
pairs = list(form_urlencoded.parse(query.encode('ascii')))
print("form_urlencoded.parse result:", [(k.decode(), v.decode()) for k, v in pairs])
except Exception as e:
print("form_urlencoded unavailable:", type(e).__name__, e)
PYRepository: moq-dev/moq
Length of output: 2028
Preserve literal + in Unix SETUP paths.
query_pairs() decodes + as a space under form-encoding rules, so ?path=/room?jwt=a+b becomes /room?jwt=a b. Decode only the outer path key/value without decoding the embedded +, and add the regression case below.
Regression case
let cases = [
("unix:///run/moq.sock?path=/room", Some("/room")),
+ ("unix:///run/moq.sock?path=/room?jwt=a+b", Some("/room?jwt=a+b")),
("unix:///run/moq.sock?path=", None),
];🤖 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/client.rs` around lines 488 - 491, Update the Unix URL path
extraction in the client setup logic to avoid query_pairs() form-decoding
embedded path content, preserving literal '+' characters in values such as
nested JWT query strings. Decode only the outer path key/value as needed, and
add the requested regression case covering a Unix SETUP path containing '+'.
Source: Coding guidelines
| let pub_url: url::Url = format!("moqt://{addr}/room").parse().expect("parse url"); | ||
| let sub_url: url::Url = format!("moqt://{addr}").parse().expect("parse url"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover moql:// in the raw-QUIC regression.
This test only dials moqt://, while the PR objective also fixes moql://. Add an applicable moql://.../room round trip asserting room/test so scheme-specific SETUP-path derivation remains covered. As per coding guidelines, “bug fixes require regression tests.”
🤖 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 781 - 782, Extend the raw-QUIC
regression test around the existing pub_url and sub_url parses to also exercise
a moql://.../room URL, performing the applicable round trip and asserting the
room/test result. Keep the existing moqt:// coverage intact and ensure the added
case validates scheme-specific SETUP-path derivation.
Source: Coding guidelines
`moql://` derives its SETUP path through the same arm as `moqt://`, so pin it end-to-end rather than only in the unit table. The unix `?path=` value is one form-encoded value, so document that a resource path carrying its own query percent-encodes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
iroh offers the moq ALPNs ahead of H3, so two moq endpoints normally land on raw QUIC. That binding carries no request URI, and the client sent no SETUP path either, so `iroh://peer/room?jwt=` reached the server's default path with no credential. The H3 fallback scheme-swapped the whole URL into its CONNECT, so the same URL addressed two different targets depending on which ALPN won. The scheme can't decide this the way `moqt://` can, so `iroh::connect` now reports the negotiated `Binding` and only the raw one advertises the target. `setup_path` splits into the scheme dispatch and the `request_target` join the iroh dial reuses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kixelated
changed the title
`Url::query` reports an empty query for a trailing `?`, so `moqt://host/room?` spelled the same target as `/room?` rather than `/room`, and `moqt://host?` produced a bare `?`: neither empty nor beginning with `/`, which the Path Parameter requires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Fixes #2570.
Summary
moqt:///moql://dial dropped the URL path and query. The server saw its default path and no?jwt=, so a raw QUIC client could not address a room or authenticate.setup_pathnow keys off the URL scheme and coversmoqt/moqlalongsidetcp/unix, and it carries the query: path-abempty plus?and the query, per draft-ietf-moq-transport-19 section 10.3.1.2. That also fixestcp://host/room?jwt=, which dropped the query the same way.iroh://had the same hole and a worse symptom: it offers the moq ALPNs ahead of H3, so two moq endpoints normally land on raw QUIC (no request URI, no SETUP path), while the H3 fallback scheme-swaps the whole URL into its CONNECT. The same URL addressed two different targets depending on which ALPN won. The scheme can't decide this, soiroh::connectnow reports the negotiatedBindingand only the raw one advertises the target.https,ws/wss) still send nothing, where a SETUP path is a protocol violation.?jwt=off a SETUP path (AuthParams::from_path), but the draft only allowed "path syntax", so the spec was behind the implementation.Root cause
moq_net::Client::with_pathsends the parameter and the server side already prefers it (moq_native::Request::pathfalls back to the dial URL only when the SETUP carries nothing). Nothing calledwith_pathfor QUIC:setup_pathwas gated#[cfg(any(feature = "tcp", feature = "uds"))]and keyed off apath_is_addressbool, andconnect_inner's QUIC branches handedmoq_net::Clientno path at all.#2413 proposed a broader fix and was closed on the day #2414 merged; #2414 carried the PATH-semantics half of it (empty path accepted and defaulted across protocols) and left
setup_pathscoped to the two stream transports, so the raw QUIC client gap survived.Public API changes
None.
setup_path,request_target,race_moq_connect, andiroh::Bindingare private; the moq-net and js/net doc comments only clarify the query suffix on the existing field.Test plan
just fix,just check,cargo nextest run --all-targets(2297 passed).cargo nextest run -p moq-native --all-features --test backend: new{quinn,quiche,noq}_raw_quic_pathandquinn_raw_quic_moql_pathassert the server observes/room?jwt=abc;quinn_webtransport_pathpins the WebTransport case to/room;iroh_connectnow dialsiroh://peer/room?jwt=abcand asserts the same. Verified every raw-binding test fails without the fix (the server sees"") while the WebTransport one still passes.cargo nextest run -p moq-relay --test smoke: newraw_quic_path_reaches_serverdrives a real relay over QUIC and asserts the SETUP path scopes the publisher's grant, across moq-lite-05 and moq-transport 14-18. It dials a bare IP, so there is no SNI and the SETUP is the only thing the server has.just drafts check.iroh::connectalways offers the moq ALPNs first and the server accepts them, so an H3 iroh session isn't reachable from a test today.js/netneeds no code change (browsers have no URI-less binding; qmux over WebSocket carries the path in its request URI), only the matching doc note onSetup.path.(Written by Opus 5)