From 777ec5385f6978b1aeaa3a81d3987db6c2cfe34f Mon Sep 17 00:00:00 2001 From: nikhilshastry2003 <98012865+nikhilshastry2003@users.noreply.github.com> Date: Sun, 13 Sep 2026 20:07:48 +0530 Subject: [PATCH] fix(airplay): carry the HTTP status code in the error instead of re-parsing the message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `post_stream` turned a failed status line into the text of an `AirPlayError::Negotiation`, and `run_session` then parsed the code back out of that text to decide whether to fall back into HAP pairing. #44 made that parse read the first line only, which fixed the header-digit false positives, but the design stayed: the message format was a contract between two functions that nothing enforced. Change the wording of the error and the retry silently stops. Now the code travels as a number. `AirPlayError::HttpStatus { request, code, status_line }` is what a non-success response becomes, and the retry decision (`should_retry_with_pairing`) matches on `code` with the same five statuses as before. A `Negotiation` error whose text happens to quote a status line is no longer a retry — there is no text to mis-read a number out of. The message a user sees is unchanged: `POST /stream failed: HTTP/1.1 404 Not Found`. The crate also had three status-line parsers — `status_is_success`, `status_code` in session.rs and the inline one in `hap_pairing::check_http_status`. There is one now, `http_session::status_code`, and the other two call it. The encrypted `POST /stream` check in `negotiate_with_auth` used `status_line.contains("200")`, the substring pattern #44 removed one layer up; it goes through `status_is_success` too. Tests: the auth-fallback tests keep every case from #44 (the real receiver statuses, the original two, the retryable-code-inside-a-header regressions) but now build the real error through `http_failure`, so they exercise the path production takes. New: a typed error carries the right code and status line, its message reads as before, an unreadable status line stays untyped, and a `Negotiation` error quoting `HTTP/1.1 404` does not retry. Verified: cargo fmt --all --check clean; cargo clippy -p openplay-airplay --all-targets --all-features -D warnings clean; cargo test -p openplay-airplay 71 + 27 passed. --- crates/openplay-airplay/src/hap_pairing.rs | 17 +-- crates/openplay-airplay/src/http_session.rs | 117 ++++++++++++--- crates/openplay-airplay/src/lib.rs | 19 +++ crates/openplay-airplay/src/session.rs | 150 ++++++++++---------- 4 files changed, 197 insertions(+), 106 deletions(-) diff --git a/crates/openplay-airplay/src/hap_pairing.rs b/crates/openplay-airplay/src/hap_pairing.rs index 55c0eb2..555145c 100644 --- a/crates/openplay-airplay/src/hap_pairing.rs +++ b/crates/openplay-airplay/src/hap_pairing.rs @@ -648,17 +648,12 @@ async fn recv_response(stream: &mut TcpStream) -> anyhow::Result> { /// when it is set to "Current User" and the caller is not signed into the same /// Apple ID. No pairing credential can satisfy that; the setting has to change. fn check_http_status(headers: &str) -> anyhow::Result<()> { - let status_line = headers - .lines() - .next() - .ok_or_else(|| anyhow::anyhow!("Empty HTTP response"))?; - - // "HTTP/1.1 403 Forbidden" → 403 - let code: u16 = match status_line - .split_whitespace() - .nth(1) - .and_then(|c| c.parse().ok()) - { + if headers.lines().next().is_none() { + anyhow::bail!("Empty HTTP response"); + } + + // "HTTP/1.1 403 Forbidden" → 403, read off the first line only. + let code = match crate::http_session::status_code(headers) { Some(c) => c, // Not a status line we recognise; let the body parser decide. None => return Ok(()), diff --git a/crates/openplay-airplay/src/http_session.rs b/crates/openplay-airplay/src/http_session.rs index c494566..e00c13e 100644 --- a/crates/openplay-airplay/src/http_session.rs +++ b/crates/openplay-airplay/src/http_session.rs @@ -192,14 +192,12 @@ async fn post_stream( // The status has to be read off the first line and nowhere else: the whole // header block of a 500 can contain "200" three times over, and treating // that as success would hand the caller a failed connection to write video - // into. The status line is also what the caller's retry logic parses, so it - // has to lead the message. + // into. A failure goes back as `AirPlayError::HttpStatus`, carrying the + // code as a number, so the caller's retry logic never has to read it out + // of a message. let (headers, _body) = read_http_response(stream).await?; if !status_is_success(&headers) { - let status_line = headers.lines().next().unwrap_or(""); - return Err(AirPlayError::Negotiation(format!( - "POST /stream failed: {status_line}" - ))); + return Err(http_failure("POST /stream", &headers)); } info!("POST /stream accepted — mirror stream active"); @@ -263,27 +261,58 @@ fn find_header_end(buf: &[u8]) -> Option { buf.windows(4).position(|w| w == b"\r\n\r\n") } -/// Whether a response's **status line** reports success. -/// -/// `headers` is the whole header block, so the status has to be read off the -/// first line and nowhere else. This was `headers.contains("200")`, which every -/// one of `Content-Length: 1200`, `Server: AirTunes/200.20` and a `Date` -/// containing "200" satisfies — so a 500 or a 403 was accepted as a working -/// mirror session, and the failure surfaced later as an unexplained stall. +/// Reads the status code off the **first line** of a response, and nowhere +/// else. /// -/// `hap_pairing::check_http_status` already parsed the status line correctly; -/// this is the same parse, kept separate only because the two report failures -/// differently. +/// `headers` is the whole header block. Headers are full of digits — +/// `Content-Length: 1401`, `Server: AirTunes/470.8.1`, a `Date` — so any parse +/// that looks past the first line will eventually read one of them as a +/// status. `HTTP/1.1 404 Not Found` gives `Some(404)`; a first line that is +/// not a status line gives `None`. /// -/// An unparseable first line is not success: if the status cannot be read there -/// is nothing to justify sending video down the connection. -fn status_is_success(headers: &str) -> bool { +/// This is the one status-line parse in the crate. `status_is_success`, +/// `http_failure` and `hap_pairing::check_http_status` all go through it. +pub(crate) fn status_code(headers: &str) -> Option { headers .lines() .next() .and_then(|line| line.split_whitespace().nth(1)) .and_then(|code| code.parse::().ok()) - .is_some_and(|code| (200..300).contains(&code)) +} + +/// Whether a response's **status line** reports success. +/// +/// This was `headers.contains("200")`, which every one of +/// `Content-Length: 1200`, `Server: AirTunes/200.20` and a `Date` containing +/// "200" satisfies — so a 500 or a 403 was accepted as a working mirror +/// session, and the failure surfaced later as an unexplained stall. +/// +/// An unparseable first line is not success: if the status cannot be read there +/// is nothing to justify sending video down the connection. +pub(crate) fn status_is_success(headers: &str) -> bool { + matches!(status_code(headers), Some(200..=299)) +} + +/// The error for a response whose status line is not a success. +/// +/// A readable code becomes [`AirPlayError::HttpStatus`], so the caller can act +/// on the number. A first line that is not a status line at all stays a plain +/// [`AirPlayError::Negotiation`]: there is no code to act on, so nothing +/// downstream should ever treat it as one. +pub(crate) fn http_failure(request: &'static str, headers: &str) -> AirPlayError { + let status_line = headers + .lines() + .next() + .unwrap_or("") + .to_string(); + match status_code(headers) { + Some(code) => AirPlayError::HttpStatus { + request, + code, + status_line, + }, + None => AirPlayError::Negotiation(format!("{request} failed: {status_line}")), + } } fn parse_content_length(headers: &str) -> Option { @@ -510,6 +539,54 @@ mod tests { )); } + #[test] + fn status_code_reads_only_the_first_line() { + assert_eq!( + status_code("HTTP/1.1 404 Not Found\r\nContent-Length: 401"), + Some(404) + ); + assert_eq!(status_code("HTTP/1.0 501 Not Implemented"), Some(501)); + assert_eq!(status_code("HTTP/1.1 470 "), Some(470)); + assert_eq!(status_code("Connection refused"), None); + assert_eq!(status_code(""), None); + } + + #[test] + fn a_failure_becomes_a_typed_error_carrying_the_code() { + match http_failure("POST /stream", FIVE_HUNDRED) { + AirPlayError::HttpStatus { + request, + code, + status_line, + } => { + assert_eq!(request, "POST /stream"); + assert_eq!(code, 500); + assert_eq!(status_line, "HTTP/1.1 500 Internal Server Error"); + } + other => panic!("expected HttpStatus, got {other:?}"), + } + } + + #[test] + fn the_message_reads_as_it_always_did() { + let err = http_failure( + "POST /stream", + "HTTP/1.1 404 Not Found\r\nContent-Length: 0", + ); + assert_eq!( + err.to_string(), + "POST /stream failed: HTTP/1.1 404 Not Found" + ); + } + + #[test] + fn an_unreadable_status_line_stays_untyped() { + assert!(matches!( + http_failure("POST /stream", "garbage without a code"), + AirPlayError::Negotiation(_) + )); + } + #[test] fn a_success_status_is_accepted() { assert!(status_is_success( diff --git a/crates/openplay-airplay/src/lib.rs b/crates/openplay-airplay/src/lib.rs index 7ddad71..caad48e 100644 --- a/crates/openplay-airplay/src/lib.rs +++ b/crates/openplay-airplay/src/lib.rs @@ -49,6 +49,25 @@ pub enum AirPlayError { #[error("Negotiation failed: {0}")] Negotiation(String), + /// The receiver answered a request with a status line that is not a + /// success. + /// + /// The code travels as a number so callers can decide on it directly. + /// The fallback into HAP pairing in `session.rs` matches on `code`; it + /// used to parse the code back out of the *text* of a `Negotiation` + /// error, which made the message format a contract between two functions + /// that nothing enforced. The message reads the same as it did: + /// `POST /stream failed: HTTP/1.1 404 Not Found`. + #[error("{request} failed: {status_line}")] + HttpStatus { + /// What was sent, e.g. `POST /stream`. + request: &'static str, + /// The status code read from the first line of the response. + code: u16, + /// That first line, verbatim, for the human reading the error. + status_line: String, + }, + #[error("I/O error: {0}")] Io(#[from] std::io::Error), diff --git a/crates/openplay-airplay/src/session.rs b/crates/openplay-airplay/src/session.rs index 3b58515..c857b71 100644 --- a/crates/openplay-airplay/src/session.rs +++ b/crates/openplay-airplay/src/session.rs @@ -150,9 +150,9 @@ async fn run_session( info!("AirPlay negotiation complete (no auth required)"); n } - Err(AirPlayError::Negotiation(ref msg)) if wants_authentication(msg) => { + Err(ref e) if should_retry_with_pairing(e) => { // Server requires authentication — try HAP pairing - info!("Receiver requires authentication, attempting HAP pairing"); + info!(%e, "Receiver requires authentication, attempting HAP pairing"); negotiate_with_auth(receiver_addr, ¶ms).await? } Err(e) => return Err(e), @@ -205,38 +205,34 @@ async fn run_session( Ok(()) } -/// Whether an unauthenticated `POST /stream` failure is worth retrying behind -/// pairing. +/// Whether a failed unauthenticated `POST /stream` should be retried through +/// HAP pairing. /// -/// This was `msg.contains("501") || msg.contains("403")`, which missed the -/// statuses real receivers actually answer with. A Mac with AirPlay Receiver -/// set to *Everyone* and no password returns **404** — the legacy AirPlay 1 -/// endpoint simply does not exist there — and one with a password returns -/// **470**. Neither matched, so casting gave up without ever attempting to -/// pair, while `pair_probe` reached M4 happily by calling pair-setup directly. +/// Only a typed [`AirPlayError::HttpStatus`] qualifies, and only these codes. +/// Observed from real receivers: /// -/// The code is read from the **status line only**. Substring-matching the whole -/// message cannot work: `AirPlayError::Negotiation` carries the entire header -/// block, so `Content-Length: 1401` in a permanent `500` would be read as a -/// `401` and provoke a full SRP-6a pair-setup against a receiver that was never -/// going to authenticate. `Server: AirTunes/470.x` does the same. Anything past -/// the first line is a header, and headers are full of digits. -fn wants_authentication(msg: &str) -> bool { - matches!(status_code(msg), Some(401 | 403 | 404 | 470 | 501)) -} - -/// Extracts the HTTP status code from a message whose first line is a status -/// line, e.g. `POST /stream failed: HTTP/1.1 404 Not Found`. +/// | Receiver state | Status | +/// |-----------------------------------------------|----------| +/// | Mac, AirPlay Receiver = Everyone, no password | 404 | +/// | Mac, Require Password on | 470 | +/// | Receiver behind HTTP Digest | 401 | +/// | The original two the fallback was written for | 403, 501 | /// -/// Returns `None` for anything that is not a status line at all — a connection -/// error, a timeout — so those never provoke a retry. -fn status_code(msg: &str) -> Option { - let status_line = msg.lines().next()?; - let after_version = status_line - .split("HTTP/1.1") - .nth(1) - .or_else(|| status_line.split("HTTP/1.0").nth(1))?; - after_version.split_whitespace().next()?.parse().ok() +/// A connection error, a timeout, or a response whose first line is not a +/// status line never provoke a retry: none of those is an `HttpStatus`, so +/// there is no text for a number to be mis-read out of. That is the whole +/// reason the code is carried as a field rather than parsed back out of a +/// message — `Content-Length: 1401` in a permanent 500 once read as a 401 and +/// provoked a full SRP-6a pair-setup against a receiver that was never going +/// to authenticate. +fn should_retry_with_pairing(err: &AirPlayError) -> bool { + matches!( + err, + AirPlayError::HttpStatus { + code: 401 | 403 | 404 | 470 | 501, + .. + } + ) } /// Negotiate AirPlay connection with authentication. @@ -336,11 +332,11 @@ async fn negotiate_with_auth( .map_err(|e| AirPlayError::Negotiation(format!("Encrypted POST /stream failed: {e}")))?; let status = String::from_utf8_lossy(&response); - let status_line = status.lines().next().unwrap_or(""); - if !status_line.contains("200") { - return Err(AirPlayError::Negotiation(format!( - "POST /stream over the encrypted channel returned: {status_line}" - ))); + if !http_session::status_is_success(&status) { + return Err(http_session::http_failure( + "POST /stream over the encrypted channel", + &status, + )); } info!("POST /stream accepted over the encrypted control channel"); @@ -363,19 +359,21 @@ async fn negotiate_with_auth( #[cfg(test)] mod auth_fallback_tests { - use super::{status_code, wants_authentication}; - - /// What `AirPlayError::Negotiation` actually carries: the whole header - /// block, not just the status line. Every test below uses this shape, - /// because the single-line strings the first version of these tests used - /// were the reason the header-matching bug survived them. - fn negotiation_failure(status_line: &str, headers: &[&str]) -> String { - let mut msg = format!("POST /stream failed: {status_line}"); + use super::should_retry_with_pairing; + use crate::http_session::http_failure; + use crate::AirPlayError; + + /// The error `post_stream` produces for a response, built from the full + /// header block — the shape production sees, not a one-line string. The + /// single-line strings the first version of these tests used were the + /// reason the header-matching bug survived them. + fn failure(status_line: &str, headers: &[&str]) -> AirPlayError { + let mut block = status_line.to_string(); for header in headers { - msg.push_str("\r\n"); - msg.push_str(header); + block.push_str("\r\n"); + block.push_str(header); } - msg + http_failure("POST /stream", &block) } /// The statuses observed from real receivers that the original @@ -383,21 +381,18 @@ mod auth_fallback_tests { #[test] fn retries_on_statuses_real_receivers_actually_send() { assert!( - wants_authentication(&negotiation_failure( + should_retry_with_pairing(&failure( "HTTP/1.1 404 Not Found", &["Content-Length: 0", "Server: AirTunes/950.7.1"] )), "a Mac set to Everyone with no password answers 404" ); assert!( - wants_authentication(&negotiation_failure( - "HTTP/1.1 470 ", - &["Content-Length: 32"] - )), + should_retry_with_pairing(&failure("HTTP/1.1 470 ", &["Content-Length: 32"])), "a Mac with Require Password answers 470" ); assert!( - wants_authentication(&negotiation_failure( + should_retry_with_pairing(&failure( "HTTP/1.1 401 Unauthorized", &["WWW-Authenticate: Digest realm=\"airplay\""] )), @@ -407,47 +402,45 @@ mod auth_fallback_tests { #[test] fn still_retries_on_the_original_two() { - assert!(wants_authentication(&negotiation_failure( + assert!(should_retry_with_pairing(&failure( "HTTP/1.1 501 Not Implemented", &["Content-Length: 0"] ))); - assert!(wants_authentication(&negotiation_failure( + assert!(should_retry_with_pairing(&failure( "HTTP/1.1 403 Forbidden", &["Content-Length: 0"] ))); } - /// The regression this rewrite exists for. - /// /// Every one of these is a permanent failure whose *headers* contain a - /// retryable code. Substring-matching the message provoked a full SRP-6a - /// pair-setup against a receiver that was never going to authenticate, and - /// replaced an accurate error with a misleading pairing one. + /// retryable code. Substring-matching the message once provoked a full + /// SRP-6a pair-setup against a receiver that was never going to + /// authenticate. #[test] fn a_retryable_code_inside_a_header_does_not_trigger_a_retry() { assert!( - !wants_authentication(&negotiation_failure( + !should_retry_with_pairing(&failure( "HTTP/1.1 500 Internal Server Error", &["Content-Length: 1401", "Connection: close"] )), "Content-Length: 1401 contains 401" ); assert!( - !wants_authentication(&negotiation_failure( + !should_retry_with_pairing(&failure( "HTTP/1.1 400 Bad Request", &["Content-Length: 404"] )), "Content-Length: 404 contains 404" ); assert!( - !wants_authentication(&negotiation_failure( + !should_retry_with_pairing(&failure( "HTTP/1.1 500 Internal Server Error", &["Server: AirTunes/470.8.1"] )), "a version string can contain 470" ); assert!( - !wants_authentication(&negotiation_failure( + !should_retry_with_pairing(&failure( "HTTP/1.1 200 OK", &["Date: Mon, 01 Jan 2024 05:01:03 GMT"] )), @@ -455,20 +448,27 @@ mod auth_fallback_tests { ); } + /// The point of carrying the code as a number: what an error *says* no + /// longer decides anything. A message that happens to quote a retryable + /// status line is still not a retry. #[test] - fn does_not_retry_on_unrelated_failures() { - assert!(!wants_authentication("Connection refused (os error 61)")); - assert!(!wants_authentication("connection closed while reading")); - assert!(!wants_authentication("")); + fn does_not_retry_on_untyped_or_unrelated_failures() { + assert!(!should_retry_with_pairing(&AirPlayError::Connection( + "Connection refused (os error 61)".to_string() + ))); + assert!(!should_retry_with_pairing(&AirPlayError::Negotiation( + "POST /stream failed: HTTP/1.1 404 Not Found".to_string() + ))); + assert!(!should_retry_with_pairing(&failure( + "garbage without a code", + &["Content-Length: 403"] + ))); } #[test] - fn status_code_reads_only_the_first_line() { - assert_eq!( - status_code("POST /stream failed: HTTP/1.1 404 Not Found\r\nContent-Length: 401"), - Some(404) - ); - assert_eq!(status_code("HTTP/1.0 501 Not Implemented"), Some(501)); - assert_eq!(status_code("Connection refused"), None); + fn the_code_travels_as_a_number_and_the_message_is_unchanged() { + let err = failure("HTTP/1.1 470 ", &["Content-Length: 32"]); + assert!(matches!(err, AirPlayError::HttpStatus { code: 470, .. })); + assert_eq!(err.to_string(), "POST /stream failed: HTTP/1.1 470 "); } }