From 07fd6113a79c9583a2ecdb0ab639373666d6805d Mon Sep 17 00:00:00 2001 From: AskaEth Date: Sun, 1 Nov 2026 08:27:28 +0800 Subject: [PATCH] =?UTF-8?q?fix(core):=20=E5=9B=9E=E8=B0=83=E5=85=88?= =?UTF-8?q?=E6=A0=A1=E9=AA=8C=20state=20=E5=86=8D=E5=A4=84=E7=90=86=20erro?= =?UTF-8?q?r=EF=BC=8C=E5=A0=B5=E6=8E=89=E4=BC=AA=E9=80=A0=E5=8F=96?= =?UTF-8?q?=E6=B6=88=E5=A4=B1=E6=95=88=E7=99=BB=E5=BD=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit reviewer P2（crates/core/src/auth/microsoft/callback.rs）：handle() 先处理 error 参数、 之后才校验 state，于是任意一次 `GET http://localhost:{port}/auth?error=access_denied` （state 随便给甚至不给）都会让 wait_for_code 直接返回 Canceled，登录被静默中止、账户不入库 —— 不需要知道随机 state、不需要 code、不需要任何凭据。违反 RFC 6749 §4.1.2.1 / RFC 9207 （错误响应同样要校验 state），也与本模块「与本次登录无关的请求一律继续等待」的声明矛盾。 - state 校验提到 error 之前：state 不匹配的请求（无论带不带 error）一律 400 后继续等； 只有带**本机 state** 的 access_denied 才判 Canceled。模块文档第 4 条同步改写。 - 顺带采纳 reviewer 的两条非计数观察： * 读头预算改为 accept 之后按 deadline 重算（原来复用 accept 前的 remaining， 总时长可超发 ≤5s）；预算归零直接超时。 * state 由 64 位升到 128 位（两个独立随机种子的 SipHash 输出拼接），对齐 RFC 6749 §10.10 的熵建议；新增单测断言 32 位 hex 且每次不同。 - 新增回归用例（crates/core/tests/auth/microsoft_authcode.rs）： `foreign_state_cannot_cancel_the_login` —— 先发 `?error=access_denied`（无 state） 再发 `?error=access_denied&state=not-our-state`，两次都必须 400 且服务器继续等待， 随后真回调拿到 code；`access_denied_with_our_state_is_still_canceled` 钉住另一半语义。 既有的「陈旧 state 回 400 后继续等」用例保持通过。 门禁：cargo fmt --all --check OK；cargo clippy --workspace --all-targets -- -D warnings exit 0 无告警；cargo test -p unlucky-core -p unlucky-app 18 个 suite 全绿（auth 59 例）。 AskaEth --- crates/core/src/auth/microsoft/authcode.rs | 25 +++++++-- crates/core/src/auth/microsoft/callback.rs | 22 +++++--- crates/core/tests/auth/microsoft_authcode.rs | 56 ++++++++++++++++++++ 3 files changed, 93 insertions(+), 10 deletions(-) diff --git a/crates/core/src/auth/microsoft/authcode.rs b/crates/core/src/auth/microsoft/authcode.rs index c53a8dc..6757829 100644 --- a/crates/core/src/auth/microsoft/authcode.rs +++ b/crates/core/src/auth/microsoft/authcode.rs @@ -195,18 +195,26 @@ where } /// 每次登录一个 `state`（不用于加密，只用来把迟到的旧回调挡掉）。 +/// +/// 128 位（两个独立随机种子的 SipHash 输出拼接）：RFC 6749 §10.10 建议 `state` 至少 +/// 128 位熵，挡住「猜到 state 就能伪造回调」这类攻击面。 fn nonce() -> String { use std::collections::hash_map::RandomState; use std::hash::{BuildHasher, Hasher}; - let mut hasher = RandomState::new().build_hasher(); let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) .map(|since| since.as_nanos()) .unwrap_or(0); - hasher.write_u128(nanos); - hasher.write_u32(std::process::id()); - format!("{:016x}", hasher.finish()) + let mut halves = [String::new(), String::new()]; + for half in &mut halves { + // 每次 `RandomState::new()` 都是独立随机种子 → 两段各 64 位、彼此不可推。 + let mut hasher = RandomState::new().build_hasher(); + hasher.write_u128(nanos); + hasher.write_u32(std::process::id()); + *half = format!("{:016x}", hasher.finish()); + } + format!("{}{}", halves[0], halves[1]) } /// 百分号编码（RFC 3986 unreserved 之外一律转义；空格用 `%20` 而不是 `+`）。 @@ -264,4 +272,13 @@ mod tests { fn grant_type_is_the_standard_one() { assert_eq!(AUTHORIZATION_CODE_GRANT, "authorization_code"); } + + #[test] + fn generated_state_has_128_bits_of_entropy() { + let first = super::nonce(); + let second = super::nonce(); + assert_eq!(first.len(), 32, "128 位 = 32 个 hex 字符：{first}"); + assert!(first.chars().all(|c| c.is_ascii_hexdigit()), "{first}"); + assert_ne!(first, second, "每次登录的 state 必须不同"); + } } diff --git a/crates/core/src/auth/microsoft/callback.rs b/crates/core/src/auth/microsoft/callback.rs index 4d9262f..2e8a27c 100644 --- a/crates/core/src/auth/microsoft/callback.rs +++ b/crates/core/src/auth/microsoft/callback.rs @@ -12,7 +12,9 @@ //! `redirect_uri` 必须用**最终绑到的端口**回填，换 token 时再原样带上（URI 严格一致）。 //! 3. **非 `/auth` 的请求（`/favicon.ico`、浏览器预连）回 404 后继续等**：让一个 favicon //! 把整次登录判成失败是最容易犯的错。 -//! 4. **`state` 不匹配的迟到回调回 400 后继续等**：旧标签页的回调不该打断新的登录； +//! 4. **`state` 不匹配的请求一律回 400 后继续等**——包括带 `error=` 的那些：旧标签页的 +//! 迟到回调不该打断新的登录，更要紧的是 RFC 6749 §4.1.2.1 / RFC 9207 要求**错误响应同样 +//! 要校验 `state`**；否则任何人 `GET /auth?error=access_denied` 就能静默中止别人的登录。 //! 真正的超时由调用方的时间预算兜底。 use std::time::Duration; @@ -131,7 +133,12 @@ impl CallbackServer { Err(_) => return Err(wait_timeout(timeout)), }; // 一条连接最多读这么久请求头：慢客户端/半开连接不能把整个登录拖过时间预算。 - let budget = REQUEST_READ_TIMEOUT.min(remaining); + // 预算按 **accept 之后** 的剩余时间重算（用 accept 前那份会让总时长超发最多 5s）。 + let budget = REQUEST_READ_TIMEOUT + .min(deadline.saturating_duration_since(tokio::time::Instant::now())); + if budget.is_zero() { + return Err(wait_timeout(timeout)); + } match tokio::time::timeout(budget, self.handle(&mut stream)).await { Ok(result) => { if let Some(code) = result? { @@ -157,14 +164,17 @@ impl CallbackServer { return Ok(None); } let params = Query::parse(query); - if let Some(error) = params.get("error") { - respond(stream, 200, DENIED_PAGE).await; - return Err(oauth_error(error, params.get("error_description"))); - } + // state 必须先校验，且**先于** error：错误响应同样要认 state（RFC 6749 §4.1.2.1 / + // RFC 9207）。否则任意一次 `GET /auth?error=access_denied`（state 随便给或不给）就能 + // 把这台机器上正在进行的登录静默中止 —— 不需要知道随机 state、不需要 code。 if params.get("state") != Some(self.state.as_str()) { respond(stream, 400, BAD_REQUEST_PAGE).await; return Ok(None); } + if let Some(error) = params.get("error") { + respond(stream, 200, DENIED_PAGE).await; + return Err(oauth_error(error, params.get("error_description"))); + } let Some(code) = params.get("code").filter(|code| !code.is_empty()) else { respond(stream, 400, BAD_REQUEST_PAGE).await; return Ok(None); diff --git a/crates/core/tests/auth/microsoft_authcode.rs b/crates/core/tests/auth/microsoft_authcode.rs index 70ff618..303101c 100644 --- a/crates/core/tests/auth/microsoft_authcode.rs +++ b/crates/core/tests/auth/microsoft_authcode.rs @@ -140,6 +140,62 @@ async fn callback_server_ignores_unrelated_requests_and_bad_state() { browser.await.expect("模拟浏览器任务"); } +#[tokio::test] +async fn foreign_state_cannot_cancel_the_login() { + // P2 回归：回调先校验 state，再处理 error（RFC 6749 §4.1.2.1 / RFC 9207）。 + // 修之前 `GET /auth?error=access_denied`（state 随便给或不给）就能把登录判成 Canceled。 + let server = CallbackServer::bind(None, 1, "state-ok") + .await + .expect("绑定"); + let port = server.port(); + let browser = tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(20)).await; + let base = format!("http://localhost:{port}/auth"); + // 1. 连 state 都不给：与本次登录无关 → 400，继续等。 + let bare = reqwest::get(format!("{base}?error=access_denied")) + .await + .expect("伪造取消"); + assert_eq!(bare.status(), 400); + // 2. state 明显不匹配：同样忽略，绝不当成用户取消。 + let foreign = reqwest::get(format!("{base}?error=access_denied&state=not-our-state")) + .await + .expect("伪造取消带错 state"); + assert_eq!(foreign.status(), 400); + // 3. 真正带对 state 的回调仍然能完成登录。 + let good = reqwest::get(format!("{base}?code=good-code&state=state-ok")) + .await + .expect("真回调"); + assert_eq!(good.status(), 200); + }); + + let code = server + .wait_for_code(Duration::from_secs(5)) + .await + .expect("伪造的 access_denied 不能中止登录"); + assert_eq!(code, "good-code"); + browser.await.expect("模拟浏览器任务"); +} + +#[tokio::test] +async fn access_denied_with_our_state_is_still_canceled() { + // 另一半语义：用户真的在授权页点了取消（带本机 state）→ 仍然是 Canceled。 + let server = CallbackServer::bind(None, 1, "state-ok") + .await + .expect("绑定"); + let port = server.port(); + let browser = tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(20)).await; + let url = format!("http://localhost:{port}/auth?error=access_denied&state=state-ok"); + let _ = reqwest::get(url).await; + }); + let error = server + .wait_for_code(Duration::from_secs(5)) + .await + .expect_err("本机 state 的取消要生效"); + assert_eq!(error, AuthError::Canceled); + browser.await.expect("模拟浏览器任务"); +} + #[tokio::test] async fn callback_timeout_is_reported() { let auth = MicrosoftAuth::new(CLIENT_ID)