From 3f79a81dcff748b1e527a55ae4e0af2495dc1a5c Mon Sep 17 00:00:00 2001 From: opencode controller Date: Sun, 28 Jun 2026 19:53:50 -0400 Subject: [PATCH] =?UTF-8?q?fix(slice-1):=20F2=20=E2=80=94=20second=20offer?= =?UTF-8?q?=20returns=20409=20Conflict=20not=20panic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review F2 (Med-High, confirmed): accept_offer asserted audio_mid.is_none() on a path fed by unauthenticated external input. A second POST /v1/sessions/:id/offer panicked the handler task instead of returning a clean 4xx. Replace the assert! with a typed RtcSessionError::AlreadyNegotiated. routes.rs maps the variant to 409 Conflict, distinct from 400 Bad Request for malformed SDP. debug_assert! documents the invariant for tests; the production path returns Err, never panics. TDD: unit test second_accept_offer_returns_already_negotiated_not_panic red on the panic message; green after the typed error. Integration test double_post_offer_returns_409_conflict_not_panic red on 400; green after routes.rs is updated. --- crates/rutster-media/src/rtc_session.rs | 44 ++++++++++- crates/rutster/src/routes.rs | 8 ++ crates/rutster/tests/api_integration.rs | 97 +++++++++++++++++++++++++ 3 files changed, 148 insertions(+), 1 deletion(-) diff --git a/crates/rutster-media/src/rtc_session.rs b/crates/rutster-media/src/rtc_session.rs index b490c9d..931acd3 100644 --- a/crates/rutster-media/src/rtc_session.rs +++ b/crates/rutster-media/src/rtc_session.rs @@ -51,6 +51,13 @@ pub enum RtcSessionError { Codec(#[from] opus::Error), #[error("UDP socket bind failed: {0}")] Socket(#[from] std::io::Error), + /// Adversarial review F2: `accept_offer` called twice for the same + /// session. The old `assert!` panicked on a path fed by external input; + /// this typed variant lets `routes.rs` map it to `409 Conflict`. The + /// `Channel` reaches `Connecting` on the first offer; a second offer + /// is a real conflict (the SDP negotiation is locked in), not a 400. + #[error("session already negotiated; only one offer per session")] + AlreadyNegotiated, } use str0m::change::SdpOffer; @@ -226,7 +233,18 @@ impl RtcSession { /// host candidate (the UDP socket we just bound) *before* calling /// `accept_offer` so the answer carries it. pub fn accept_offer(&mut self, offer_sdp: &str) -> Result { - assert!(self.audio_mid.is_none(), "accept_offer called twice"); + // F2 (adversarial review): a second `accept_offer` was an `assert!` + // that panicked the handler — client-triggerable via double + // `POST /v1/sessions/:id/offer`. Typed error → `routes.rs` maps to + // `409 Conflict`. `debug_assert!` documents the invariant for tests + // (the production path returns the typed error, never panics). + if self.audio_mid.is_some() { + return Err(RtcSessionError::AlreadyNegotiated); + } + debug_assert!( + self.audio_mid.is_none(), + "accept_offer called twice (debug_assert only; production path returns Err)" + ); // Register our local UDP socket as a host candidate. str0m includes // this candidate's address + the ICE creds it generates in the SDP @@ -351,4 +369,28 @@ a=candidate:1 1 UDP 2113667327 192.168.1.42 50000 typ host\r let _ = session.accept_offer(BROWSER_SDP_OFFER).expect("answer"); assert_eq!(session.channel_state(), ChannelState::Connecting); } + + /// Adversarial review F2: a second `POST /v1/sessions/:id/offer` (or + /// any second call to `accept_offer`) must NOT panic the handler — + /// it returns a typed error so `routes.rs` can map it to `409 Conflict`. + /// The old `assert!(audio_mid.is_none())` panicked on a path fed by + /// unauthenticated external input; the typed error makes the contract + /// explicit. `debug_assert!` documents the invariant for tests. + #[test] + fn second_accept_offer_returns_already_negotiated_not_panic() { + let mut session = RtcSession::new().expect("session"); + let _ = session + .accept_offer(BROWSER_SDP_OFFER) + .expect("first offer ok"); + match session.accept_offer(BROWSER_SDP_OFFER) { + Err(RtcSessionError::AlreadyNegotiated) => { /* expected */ } + other => panic!("expected AlreadyNegotiated, got {other:?}"), + } + // Idempotency of the rejection: a third call still returns the + // same typed error, never escalates to a panic. + match session.accept_offer(BROWSER_SDP_OFFER) { + Err(RtcSessionError::AlreadyNegotiated) => { /* still expected */ } + other => panic!("third offer: expected AlreadyNegotiated, got {other:?}"), + } + } } diff --git a/crates/rutster/src/routes.rs b/crates/rutster/src/routes.rs index f58cb4d..58e0970 100644 --- a/crates/rutster/src/routes.rs +++ b/crates/rutster/src/routes.rs @@ -120,6 +120,14 @@ pub async fn post_offer( answer_sdp, ) .into_response(), + // F2: a second offer for the same session is a real conflict (the + // SDP negotiation is already locked in), not a malformed request. + // Distinct status code surfaces the operational shape to clients + // without forcing them to parse the error body. + Err(rutster_media::RtcSessionError::AlreadyNegotiated) => { + tracing::warn!(session_id = %id, "second offer rejected: already negotiated"); + StatusCode::CONFLICT.into_response() + } Err(e) => { tracing::error!(error = ?e, "SDP accept failed"); StatusCode::BAD_REQUEST.into_response() diff --git a/crates/rutster/tests/api_integration.rs b/crates/rutster/tests/api_integration.rs index 377824b..995ae38 100644 --- a/crates/rutster/tests/api_integration.rs +++ b/crates/rutster/tests/api_integration.rs @@ -10,10 +10,40 @@ use axum::http::{Request, StatusCode}; use rutster::session_map::AppState; use tower::ServiceExt; // enables `oneshot` on the Router for sync tests +/// Default tap URL for the test app — matches `RUTSTER_TAP_URL`'s documented +/// default (`ws://127.0.0.1:8081/echo`). No brain is actually running in +/// these tests; the URL just satisfies slice-2's `AppState::new(tap_url)` +/// signature. Slice-2's tap-integration test (in `tap_integration.rs`) is +/// where a real `EchoServer` gets stood up. fn default_tap_url() -> url::Url { url::Url::parse("ws://127.0.0.1:8081/echo").unwrap() } +/// A minimal SDP offer Chrome would send. Mirrors the unit-test fixture in +/// `rutster-media::rtc_session::tests::BROWSER_SDP_OFFER`; not shared across +/// crates because the integration test lives outside `src/`. Carries all +/// the str0m-required fields (BUNDLE group, fingerprint, ICE creds, Opus +/// rtpmap) so `accept_offer` returns `Ok(answer)` and sets `audio_mid`. +const SDP_OFFER: &str = "\ +v=0\r +o=- 4593482934 2 IN IP4 127.0.0.1\r +s=-\r +t=0 0\r +a=group:BUNDLE 0\r +m=audio 9 UDP/TLS/RTP/SAVPF 111\r +c=IN IP4 0.0.0.0\r +a=rtcp:9 IN IP4 0.0.0.0\r +a=ice-ufrag:abcd\r +a=ice-pwd:abcdefghijklmnopqrstuvwxyz0123456789\r +a=fingerprint:sha-256 AB:CD:EF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD:EE:FF:00:11:22:33:44:55:66:77:88:99:AA:BB:CC:DD\r +a=setup:actpass\r +a=mid:0\r +a=sendrecv\r +a=rtpmap:111 opus/48000/2\r +a=fmtp:111 minptime=10;useinbandfec=1\r +a=candidate:1 1 UDP 2113667327 192.168.1.42 50000 typ host\r +"; + #[tokio::test] async fn post_v1_sessions_returns_a_session_id() { let app = rutster::routes::router(AppState::new(default_tap_url())); @@ -51,3 +81,70 @@ async fn get_root_serves_html() { Some("text/html; charset=utf-8") ); } + +/// Adversarial review F2: a second `POST /v1/sessions/:id/offer` must +/// return `409 Conflict`, **not** panic the handler. The old `assert!` +/// panicked on a client-reachable path; the fix routes a typed +/// `RtcSessionError::AlreadyNegotiated` to `409 Conflict` (distinct from +/// `400 Bad Request` for malformed SDP). +#[tokio::test] +async fn double_post_offer_returns_409_conflict_not_panic() { + let app = rutster::routes::router(AppState::new(default_tap_url())); + + // Mint a session, then POST the offer. The first POST should succeed + // (200 OK with an SDP answer body) — proves the harness's fixture is + // valid and `audio_mid` is now `Some(_)`. + let session_id = create_session(&app).await; + let first = post_offer(&app, &session_id, SDP_OFFER).await; + assert_eq!( + first.status(), + StatusCode::OK, + "first offer must succeed for the test to be meaningful" + ); + + // Second POST to the same session: F2's bug path. Pre-fix, the handler + // task panicked at `assert!(self.audio_mid.is_none(), ...)`; the panic + // propagated up through `app.oneshot(...).await` and failed the test as + // an error (panicked task), not as a status code. Post-fix, the + // handler returns `409 Conflict` cleanly. + let second = post_offer(&app, &session_id, SDP_OFFER).await; + assert_eq!( + second.status(), + StatusCode::CONFLICT, + "second offer must return 409 Conflict, got {} (body: {:?})", + second.status(), + axum::body::to_bytes(second.into_body(), 1024).await + ); +} + +async fn create_session(app: &axum::Router) -> String { + let resp = app + .clone() + .oneshot( + Request::builder() + .method("POST") + .uri("/v1/sessions") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body = axum::body::to_bytes(resp.into_body(), 1024).await.unwrap(); + let v: serde_json::Value = serde_json::from_slice(&body).unwrap(); + v["session_id"].as_str().unwrap().to_string() +} + +async fn post_offer(app: &axum::Router, session_id: &str, sdp: &str) -> axum::response::Response { + app.clone() + .oneshot( + Request::builder() + .method("POST") + .uri(format!("/v1/sessions/{session_id}/offer")) + .header("content-type", "application/sdp") + .body(Body::from(sdp.to_owned())) + .unwrap(), + ) + .await + .unwrap() +}