diff --git a/build/deps/github_hashes/facebook/folly-rev.txt b/build/deps/github_hashes/facebook/folly-rev.txt index ee26efe7..5889f9ec 100644 --- a/build/deps/github_hashes/facebook/folly-rev.txt +++ b/build/deps/github_hashes/facebook/folly-rev.txt @@ -1 +1 @@ -Subproject commit ef07b7666e6a580a936987fc0f8727c1c51cc374 +Subproject commit 836dffb4ee21609b98b6ab0ba2f0fae43f9c0097 diff --git a/build/deps/github_hashes/facebook/mvfst-rev.txt b/build/deps/github_hashes/facebook/mvfst-rev.txt index 51b60012..c3488c04 100644 --- a/build/deps/github_hashes/facebook/mvfst-rev.txt +++ b/build/deps/github_hashes/facebook/mvfst-rev.txt @@ -1 +1 @@ -Subproject commit f98bd7649a1a604b2a8fa548e928ad046ec12ad4 +Subproject commit 36f0ef484140540a63c710c9336672dce4e37153 diff --git a/build/deps/github_hashes/facebook/proxygen-rev.txt b/build/deps/github_hashes/facebook/proxygen-rev.txt index af7e2516..832aa506 100644 --- a/build/deps/github_hashes/facebook/proxygen-rev.txt +++ b/build/deps/github_hashes/facebook/proxygen-rev.txt @@ -1 +1 @@ -Subproject commit 7558747901d08465619ed67f6dc896cc9c2117d1 +Subproject commit 61b53870bbb2194335ff81781243171733019d11 diff --git a/build/deps/github_hashes/facebook/wangle-rev.txt b/build/deps/github_hashes/facebook/wangle-rev.txt index 943daf78..d3b5bf9e 100644 --- a/build/deps/github_hashes/facebook/wangle-rev.txt +++ b/build/deps/github_hashes/facebook/wangle-rev.txt @@ -1 +1 @@ -Subproject commit dc6de26b8d5b34aa87ae1159f9e0c9a64c78386d +Subproject commit 47656df19271f1323351f787a9f03e203ae4d9d2 diff --git a/build/deps/github_hashes/facebookincubator/fizz-rev.txt b/build/deps/github_hashes/facebookincubator/fizz-rev.txt index b8b37ad1..6681272c 100644 --- a/build/deps/github_hashes/facebookincubator/fizz-rev.txt +++ b/build/deps/github_hashes/facebookincubator/fizz-rev.txt @@ -1 +1 @@ -Subproject commit 11216ba32d86478caedfcb73c6330f69052fecfd +Subproject commit 2703f674e45ef4b8c647c1e2e0de08be1c2f07c0 diff --git a/moxygen/MoQFramer.cpp b/moxygen/MoQFramer.cpp index fc85619b..e4def67c 100644 --- a/moxygen/MoQFramer.cpp +++ b/moxygen/MoQFramer.cpp @@ -3458,6 +3458,34 @@ folly::Expected MoQFrameParser::parseRequestError( } requestError.reasonPhrase = std::move(reasonPhrase.value()); + if (requestError.errorCode == RequestErrorCode::REDIRECT) { + if (getDraftMajorVersion(*version_) < 18) { + XLOG(DBG4) << "REDIRECT errorCode received on pre-v18 draft"; + return folly::makeUnexpected(ErrorCode::PROTOCOL_VIOLATION); + } + } + + if (getDraftMajorVersion(*version_) >= 18 && + requestError.errorCode == RequestErrorCode::REDIRECT) { + Redirect redirect; + + auto connectUri = parseFixedString(cursor, length); + if (!connectUri) { + XLOG(DBG4) << "parseRequestError: UNDERFLOW on Redirect Connect URI"; + return folly::makeUnexpected(connectUri.error()); + } + redirect.connectUri = std::move(connectUri.value()); + + auto fullTrackName = parseFullTrackName(cursor, length); + if (!fullTrackName) { + XLOG(DBG4) << "parseRequestError: error parsing Redirect Full Track Name"; + return folly::makeUnexpected(fullTrackName.error()); + } + redirect.fullTrackName = std::move(fullTrackName.value()); + + requestError.redirect = std::move(redirect); + } + // Check for leftover bytes if (length > 0) { return folly::makeUnexpected(ErrorCode::PROTOCOL_VIOLATION); @@ -5793,10 +5821,11 @@ WriteResult MoQFrameWriter::writeGoaway( writeFixedString(writeBuf, goaway.newSessionUri, size, error); if (getDraftMajorVersion(*version_) >= 18) { writeVarint(writeBuf, goaway.timeout, size, error); + // Per draft 18, Request ID is present only when GOAWAY is sent on the + // control stream. Callers signal request-stream GOAWAY by leaving + // requestID unset. if (goaway.requestID.has_value()) { writeVarint(writeBuf, goaway.requestID->value, size, error); - } else { - error = true; } } writeSize(sizePtr, size, error, *version_); @@ -6099,6 +6128,18 @@ WriteResult MoQFrameWriter::writeRequestError( << "Invalid frameType passed to writeRequestError: " << static_cast(frameType); + if (requestError.errorCode == RequestErrorCode::REDIRECT) { + if (getDraftMajorVersion(*version_) < 18) { + XLOG(ERR) << "REDIRECT errorCode is only valid for draft 18+, version=" + << *version_; + return folly::makeUnexpected(quic::TransportErrorCode::INTERNAL_ERROR); + } + if (!requestError.redirect) { + XLOG(ERR) << "REDIRECT errorCode without a Redirect struct"; + return folly::makeUnexpected(quic::TransportErrorCode::INTERNAL_ERROR); + } + } + size_t size = 0; bool error = false; if (getDraftMajorVersion(*version_) > 14) { @@ -6119,6 +6160,13 @@ WriteResult MoQFrameWriter::writeRequestError( } writeFixedString(writeBuf, requestError.reasonPhrase, size, error); + if (getDraftMajorVersion(*version_) >= 18 && + requestError.errorCode == RequestErrorCode::REDIRECT) { + const Redirect& redirect = *requestError.redirect; + writeFixedString(writeBuf, redirect.connectUri, size, error); + writeFullTrackName(writeBuf, redirect.fullTrackName, size, error); + } + writeSize(sizePtr, size, error, *version_); if (error) { return folly::makeUnexpected(quic::TransportErrorCode::INTERNAL_ERROR); diff --git a/moxygen/MoQSession.cpp b/moxygen/MoQSession.cpp index cab01240..6413df08 100644 --- a/moxygen/MoQSession.cpp +++ b/moxygen/MoQSession.cpp @@ -2154,9 +2154,7 @@ MoQSession::PendingRequestState::setError( return type_; } case FrameType::TRACK_STATUS: { - storage_.trackStatus_.setValue( - folly::makeUnexpected(TrackStatusError( - {error.requestID, error.errorCode, error.reasonPhrase}))); + storage_.trackStatus_.setValue(folly::makeUnexpected(std::move(error))); return type_; } case FrameType::FETCH_ERROR: { @@ -3876,6 +3874,43 @@ void MoQSession::onRequestError(RequestError error, FrameType frameType) { frameType = pendingState->getErrorFrameType(); } + if (error.errorCode == RequestErrorCode::REDIRECT) { + using PRStateType = PendingRequestState::Type; + const auto pendingType = pendingState->type(); + const bool isNamespaceScoped = + (pendingType == PRStateType::PUBLISH_NAMESPACE || + pendingType == PRStateType::SUBSCRIBE_NAMESPACE); + const bool isRedirectable = + (pendingType == PRStateType::SUBSCRIBE_TRACK || + pendingType == PRStateType::FETCH || + pendingType == PRStateType::TRACK_STATUS || isNamespaceScoped); + if (!isRedirectable) { + XLOG(ERR) << "REDIRECT not permitted for pending request type=" + << static_cast(pendingType) << " id=" << error.requestID + << " sess=" << this; + close(SessionCloseErrorCode::PROTOCOL_VIOLATION); + return; + } + if (error.redirect) { + if (dir_ == MoQControlCodec::Direction::SERVER && + !error.redirect->connectUri.empty()) { + XLOG(ERR) << "Server received REDIRECT with non-empty Connect URI" + << " id=" << error.requestID << " sess=" << this; + close(SessionCloseErrorCode::PROTOCOL_VIOLATION); + return; + } + if (isNamespaceScoped && + !error.redirect->fullTrackName.trackName.empty()) { + XLOG(ERR) << "Namespace-scoped REDIRECT has non-empty Track Name" + << " id=" << error.requestID + << " pendingType=" << static_cast(pendingType) + << " sess=" << this; + close(SessionCloseErrorCode::PROTOCOL_VIOLATION); + return; + } + } + } + auto setErrorRes = pendingState->setError(error, frameType); if (setErrorRes.hasError()) { XLOG(ERR) << "setError failure id=" << error.requestID @@ -6541,6 +6576,10 @@ WriteResult MessageReply::ok(const RequestOk& okMsg) { } WriteResult MessageReply::error(const SubscribeTracksError& errorMsg) { + if (errorMsg.errorCode == RequestErrorCode::REDIRECT) { + XLOG(ERR) << "REDIRECT not permitted in SUBSCRIBE_TRACKS error"; + return folly::makeUnexpected(quic::TransportErrorCode::INTERNAL_ERROR); + } auto res = moqFrameWriter_.writeRequestError( replyContext_->writeBuf(), errorMsg, FrameType::REQUEST_ERROR); // After ERROR the publisher is done with this stream — FIN it. diff --git a/moxygen/MoQSession.h b/moxygen/MoQSession.h index 3a4a4e4a..c09cedfc 100644 --- a/moxygen/MoQSession.h +++ b/moxygen/MoQSession.h @@ -969,6 +969,10 @@ class MoQSession : public Subscriber, return getFrameType(false); } + Type type() const { + return type_; + } + virtual folly::Expected setError( RequestError error, FrameType frameType); diff --git a/moxygen/MoQTypes.h b/moxygen/MoQTypes.h index b387f926..576843d5 100644 --- a/moxygen/MoQTypes.h +++ b/moxygen/MoQTypes.h @@ -1405,6 +1405,11 @@ using SubscribeTracksOk = RequestOk; using PublishNamespaceOk = RequestOk; using SubscribeUpdateOk = RequestOk; +struct Redirect { + std::string connectUri; + FullTrackName fullTrackName; +}; + // Consolidated request error structure struct RequestError { RequestID requestID; @@ -1415,6 +1420,8 @@ struct RequestError { // If the value is 0, the request SHOULD NOT be retried. // A value of 1 indicates the request can be retried immediately. std::optional retryInterval = std::nullopt; + // Draft 18+: present only when errorCode == REDIRECT. + std::optional redirect = std::nullopt; }; // Type aliases for backward compatibility diff --git a/moxygen/moqtest/MoQTestClient.cpp b/moxygen/moqtest/MoQTestClient.cpp index 9365c8c4..2ad8f921 100644 --- a/moxygen/moqtest/MoQTestClient.cpp +++ b/moxygen/moqtest/MoQTestClient.cpp @@ -189,7 +189,7 @@ folly::coro::Task MoQTestClient::fetch( receivingType_ = ReceivingType::FETCH; initializeExpecteds(params); - // Fetch to the reciever + // Fetch to the receiver auto res = co_await moqClient_->moqSession_->fetch(fetch, fetchReceiver_); moqClient_->moqSession_->drain(); if (!res.hasError()) { @@ -230,7 +230,7 @@ ObjectReceiverCallback::FlowControlState MoQTestClient::onObject( return ObjectReceiverCallback::FlowControlState::UNBLOCKED; } - // Adjust the expected data (If Still recieving data, leave unblocked) + // Adjust the expected data (If Still receiving data, leave unblocked) adjustExpected(params_, &objHeader); return ObjectReceiverCallback::FlowControlState::UNBLOCKED; } diff --git a/moxygen/test/MoQFramerTest.cpp b/moxygen/test/MoQFramerTest.cpp index 1e33e3db..536263c9 100644 --- a/moxygen/test/MoQFramerTest.cpp +++ b/moxygen/test/MoQFramerTest.cpp @@ -4778,6 +4778,159 @@ TEST_F(MoQFramerV18Test, SubscribeTracksForwardFalseSerializedAsParameter) { EXPECT_EQ(parsed->forward, false); } +// Draft 18+ REQUEST_ERROR carries a Redirect structure when errorCode == +// REDIRECT. Verify round-trip of a fully-populated Redirect. +TEST_F(MoQFramerV18Test, RequestErrorRedirectRoundtrip) { + RequestError requestError; + requestError.requestID = RequestID(7); + requestError.errorCode = RequestErrorCode::REDIRECT; + requestError.reasonPhrase = "moved"; + requestError.retryInterval = std::chrono::milliseconds{1}; + Redirect redirect; + redirect.connectUri = "https://relay.example.com/moq"; + redirect.fullTrackName.trackNamespace = + TrackNamespace(std::vector{"example.com", "live"}); + redirect.fullTrackName.trackName = "alt-track"; + requestError.redirect = redirect; + + folly::IOBufQueue writeBuf{folly::IOBufQueue::cacheChainLength()}; + ASSERT_TRUE( + writer_ + .writeRequestError(writeBuf, requestError, FrameType::REQUEST_ERROR) + .hasValue()); + + auto serialized = writeBuf.move(); + folly::io::Cursor cursor(serialized.get()); + auto frameType = decodeMoQVarint(cursor); + ASSERT_TRUE(frameType.has_value()); + EXPECT_EQ(frameType->first, folly::to_underlying(FrameType::REQUEST_ERROR)); + + auto bodyLen = frameLength(cursor); + auto parsed = + parser_.parseRequestError(cursor, bodyLen, FrameType::REQUEST_ERROR); + ASSERT_TRUE(parsed.hasValue()); + + EXPECT_EQ(parsed->requestID, requestError.requestID); + EXPECT_EQ(parsed->errorCode, RequestErrorCode::REDIRECT); + EXPECT_EQ(parsed->reasonPhrase, requestError.reasonPhrase); + EXPECT_EQ(parsed->retryInterval, requestError.retryInterval); + ASSERT_TRUE(parsed->redirect.has_value()); + EXPECT_EQ(parsed->redirect->connectUri, redirect.connectUri); + EXPECT_EQ( + parsed->redirect->fullTrackName.trackNamespace, + redirect.fullTrackName.trackNamespace); + EXPECT_EQ( + parsed->redirect->fullTrackName.trackName, + redirect.fullTrackName.trackName); +} + +// When errorCode == REDIRECT but no Redirect is supplied, the writer +// must refuse — silently substituting an empty Redirect would hide a +// caller bug and produce a misleading on-wire message. Callers that +// genuinely want "reuse current session URI / original Full Track Name" +// must set `redirect` to a default-constructed Redirect explicitly. +TEST_F(MoQFramerV18Test, RequestErrorRedirectWithoutRedirectIsRejected) { + RequestError requestError; + requestError.requestID = RequestID(3); + requestError.errorCode = RequestErrorCode::REDIRECT; + requestError.reasonPhrase = ""; + requestError.retryInterval = std::chrono::milliseconds{0}; + // Intentionally leave requestError.redirect unset. + + folly::IOBufQueue writeBuf{folly::IOBufQueue::cacheChainLength()}; + EXPECT_FALSE( + writer_ + .writeRequestError(writeBuf, requestError, FrameType::REQUEST_ERROR) + .hasValue()); +} + +// Per draft-ietf-moq-transport-18 §10.6.2, REDIRECT is only permitted in +// response to SUBSCRIBE, FETCH, TRACK_STATUS, PUBLISH_NAMESPACE and +// SUBSCRIBE_NAMESPACE. The framer does not know that original request +// context for v18+ REQUEST_ERROR; session-layer validation handles it. +TEST_F(MoQFramerV18Test, RequestErrorRedirectDoesNotValidateRequestType) { + RequestError requestError; + requestError.requestID = RequestID(1); + requestError.errorCode = RequestErrorCode::REDIRECT; + requestError.reasonPhrase = ""; + requestError.retryInterval = std::chrono::milliseconds{0}; + requestError.redirect = Redirect{}; + + folly::IOBufQueue writeBuf{folly::IOBufQueue::cacheChainLength()}; + + EXPECT_TRUE( + writer_ + .writeRequestError(writeBuf, requestError, FrameType::PUBLISH_ERROR) + .hasValue()); + EXPECT_TRUE(writer_ + .writeRequestError( + writeBuf, requestError, FrameType::SUBSCRIBE_UPDATE) + .hasValue()); +} + +// The parser only decodes REQUEST_ERROR bytes. It cannot decide whether a +// REDIRECT is valid for the original request; session-layer enforcement handles +// that using pending request state. +TEST_F(MoQFramerV18Test, ParseRequestErrorDoesNotValidateRequestType) { + // Hand-craft a REQUEST_ERROR body with errorCode == REDIRECT and a + // valid Redirect payload. + RequestError requestError; + requestError.requestID = RequestID(42); + requestError.errorCode = RequestErrorCode::REDIRECT; + requestError.reasonPhrase = "moved"; + requestError.retryInterval = std::chrono::milliseconds{0}; + requestError.redirect = Redirect{}; + + folly::IOBufQueue writeBuf{folly::IOBufQueue::cacheChainLength()}; + ASSERT_TRUE( + writer_.writeRequestError(writeBuf, requestError, FrameType::FETCH_ERROR) + .hasValue()); + + auto serialized = writeBuf.move(); + folly::io::Cursor cursor(serialized.get()); + ASSERT_TRUE(decodeMoQVarint(cursor).has_value()); // frame type + auto bodyLen = frameLength(cursor); + + // Parsing the same bytes as if they were a PUBLISH_ERROR still succeeds: + // request-type redirect validation requires session context. + auto parsed = + parser_.parseRequestError(cursor, bodyLen, FrameType::PUBLISH_ERROR); + ASSERT_TRUE(parsed.hasValue()); + EXPECT_EQ(parsed->errorCode, RequestErrorCode::REDIRECT); + EXPECT_TRUE(parsed->redirect.has_value()); +} + +// When errorCode is anything other than REDIRECT, no Redirect bytes are +// written, and the parsed RequestError has no redirect. +TEST_F(MoQFramerV18Test, RequestErrorNonRedirectOmitsRedirect) { + RequestError requestError; + requestError.requestID = RequestID(9); + requestError.errorCode = RequestErrorCode::INTERNAL_ERROR; + requestError.reasonPhrase = "boom"; + requestError.retryInterval = std::chrono::milliseconds{0}; + // Set redirect anyway to confirm the writer ignores it for non-REDIRECT. + Redirect redirect; + redirect.connectUri = "ignored"; + requestError.redirect = redirect; + + folly::IOBufQueue writeBuf{folly::IOBufQueue::cacheChainLength()}; + ASSERT_TRUE( + writer_ + .writeRequestError(writeBuf, requestError, FrameType::REQUEST_ERROR) + .hasValue()); + + auto serialized = writeBuf.move(); + folly::io::Cursor cursor(serialized.get()); + auto frameType = decodeMoQVarint(cursor); + ASSERT_TRUE(frameType.has_value()); + + auto bodyLen = frameLength(cursor); + auto parsed = + parser_.parseRequestError(cursor, bodyLen, FrameType::REQUEST_ERROR); + ASSERT_TRUE(parsed.hasValue()); + EXPECT_FALSE(parsed->redirect.has_value()); +} + // Draft 18 added a Track Properties block at the end of REQUEST_OK. Verify // that a populated TRACK_STATUS_OK round-trips its Track Properties through // the wire and back into the TrackStatusOk struct.