fix: two Origin headers state none a policy can answer for

Review of the previous commit. `HeaderMap::get` is first-wins, and the
deployment README recommends puts a reverse proxy in front of the socket — one
that APPENDS `Origin` rather than setting it leaves two, and answering from the
first lets the second be anything at all. No browser sends two, so two is not a
state any policy can answer for and the handshake is refused, exactly as an
`Origin` that is not text already was.

Guarded by two_origins_state_none_this_policy_can_answer_for, proved by
restoring the first-wins read, which fails that test alone across the
workspace. The test also asserts the allowed origin BY ITSELF is still allowed,
so the refusal is demonstrably the second header rather than the first.

Second review finding: a_handshake_from_a_foreign_origin_is_refused matched
"403" in a Display string, which cannot tell a refused upgrade from a dropped
connection. It now matches Error::Http and asserts the status. Proved by
changing forbidden() to UNAUTHORIZED, which fails it and
a_refusal_is_a_forbidden_response.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FC2jVn8Au7E9JPkw6bwXXH
This commit is contained in:
Edward Langley
2026-09-20 21:21:49 -07:00
parent ee19afbdd8
commit cead3210e5
4 changed files with 66 additions and 13 deletions
+27 -5
View File
@@ -198,14 +198,16 @@ async fn connection(
Ok(())
}
/// Whether `origins` permits this handshake. An `Origin` that is not text is not
/// one a policy can name, so it is refused rather than read as absent.
/// Whether `origins` permits this handshake. Two `Origin` headers, or one that
/// is not text, name no origin a policy could answer for and are refused.
fn admitted(origins: &OriginPolicy, req: &Request) -> bool {
match req.headers().get(header::ORIGIN) {
None => origins.permits(None),
Some(value) => value
let mut stated = req.headers().get_all(header::ORIGIN).iter();
match (stated.next(), stated.next()) {
(None, _) => origins.permits(None),
(Some(value), None) => value
.to_str()
.is_ok_and(|origin| origins.permits(Some(origin))),
(Some(_), Some(_)) => false,
}
}
@@ -276,6 +278,26 @@ mod tests {
req
}
/// A proxy that appends rather than sets `Origin` leaves two, and reading
/// the first would let the second be anything at all.
#[test]
fn two_origins_state_none_this_policy_can_answer_for() {
let mut req = handshake(Some(b"http://localhost:8080"));
req.headers_mut().append(
header::ORIGIN,
header::HeaderValue::from_static("https://evil.example"),
);
assert_eq!(req.headers().get_all(header::ORIGIN).iter().count(), 2);
assert!(!admitted(&OriginPolicy::default(), &req));
// And the allowed one alone is still allowed, so the refusal above is
// the second header and not the first.
assert!(admitted(
&OriginPolicy::default(),
&handshake(Some(b"http://localhost:8080"))
));
}
/// The seam between the policy and the handshake, including the one case
/// the policy cannot be asked about: an `Origin` that is not text.
#[test]
+7 -4
View File
@@ -4,6 +4,7 @@
use futures_util::{SinkExt, StreamExt};
use tokio::net::TcpListener;
use tokio_tungstenite::tungstenite::Message;
use tokio_tungstenite::tungstenite::http::StatusCode;
use improvise_command::ui::app::App;
use improvise_core::model::cell::CellValue;
@@ -281,10 +282,12 @@ fn a_handshake_from_a_foreign_origin_is_refused() {
else {
panic!("a foreign origin must not reach the document");
};
assert!(
format!("{refused}").contains("403"),
"the upgrade must be refused, not accepted and ignored: {refused}"
);
// The kind, not the wording: a refused upgrade and a dropped socket
// are different outcomes and only one of them is this fix.
let tokio_tungstenite::tungstenite::Error::Http(answer) = refused else {
panic!("the handshake failed for some other reason: {refused}");
};
assert_eq!(answer.status(), StatusCode::FORBIDDEN);
});
}