fabi/PiCloud
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(stage-1): audit High-severity backend correctness + CI unblocker

Browse Source 
Closes 4 High-severity audit findings:

- describe_event broken for HTTP-async payloads: was reading source/op
  JSON fields that HttpDispatchPayload doesn't carry, producing empty
  source on DL rows. from_wire("") then fell back to Kv on replay, the
  dispatcher tried to resolve a non-existent trigger, and replay no-op'd
  silently. Now branches on OutboxSourceKind: HTTP rows emit
  "<method> <path>" + source="http"; Invoke rows emit "invoke_async" +
  source="invoke"; TriggerEvent payloads keep top-level field extraction.

- HTTP outbound Authorization leaked across cross-origin redirects.
  Manual redirect loop (Policy::none) reused header_map on every hop
  and only scrubbed Content-Type for POST->GET. Now compares
  url::Url::origin() across hops and strips Authorization,
  Proxy-Authorization, Cookie when origin changes — matching reqwest's
  default policy.

- F-S-010 inbound email HMAC had no replay protection. Signature was
  computed over body only with no timestamp or nonce, so captured POSTs
  were replayable indefinitely. Adds X-Picloud-Timestamp header bound
  into the HMAC input (signed string is ts || "." || body), 300s
  tolerance window, and a process-local LRU keyed on
  (ts, sha256(body)) with 600s TTL to catch within-window replays.
  Breaking change: webhook senders must include the timestamp header.

- expected_schema.txt re-blessed for migrations 0036-0039 (the previous
  snapshot stopped at 0035 so CI would have gone red on first run).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-06-10 21:03:10 +02:00
parent bce44769dd
commit 3c9816daf3
7 changed files with 450 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

154
crates/manager-core/src/http_service.rs
View File

@@ -29,7 +29,10 @@ use std::time::Duration;
use async_trait::async_trait;
use picloud_shared::{HttpError, HttpRequest, HttpResponse, HttpService, SdkCallCx};
use reqwest::header::{HeaderMap, HeaderName, HeaderValue, CONTENT_TYPE, LOCATION, USER_AGENT};
use reqwest::header::{
HeaderMap, HeaderName, HeaderValue, AUTHORIZATION, CONTENT_TYPE, COOKIE, LOCATION,
PROXY_AUTHORIZATION, USER_AGENT,
};
use reqwest::{Client, Method, StatusCode};
use crate::authz::{self, AuthzRepo, Capability};
@@ -244,10 +247,24 @@ impl HttpServiceImpl {
let loc_str = loc.to_str().map_err(|_| {
HttpError::Backend("redirect Location not valid UTF-8".into())
})?;
current = current
let next = current
.join(loc_str)
.map_err(|e| HttpError::InvalidUrl(format!("redirect target: {e}")))?;
// Cross-origin sensitive-header scrub. reqwest's
// default redirect policy drops Authorization,
// Proxy-Authorization, and Cookie when a redirect
// crosses origin (scheme/host/port). Manual follow
// has to do the same; without this, a script's
// bearer token leaks to whatever target a 302
// points at on another origin.
if next.origin() != current.origin() {
header_map.remove(AUTHORIZATION);
header_map.remove(PROXY_AUTHORIZATION);
header_map.remove(COOKIE);
}
current = next;
// 303 always → GET; 301/302 historically downgrade
// POST→GET (matches browsers). 307/308 preserve.
if matches!(status.as_u16(), 301..=303) {
@@ -792,4 +809,137 @@ mod tests {
.unwrap();
assert_eq!(resp.status, 200);
}
/// Test fixture: capture every received Authorization header value
/// across hops. Server B (the redirect target) is the one we care
/// about — Authorization on B means the bearer leaked across origin.
async fn spawn_pair_redirect(
b_addr_template: impl Fn(SocketAddr) -> String + Send + Sync + 'static,
) -> (SocketAddr, SocketAddr, Arc<std::sync::Mutex<Vec<String>>>) {
let captured: Arc<std::sync::Mutex<Vec<String>>> =
Arc::new(std::sync::Mutex::new(Vec::new()));
// Server B: records Authorization header values, replies 200.
let captured_b = captured.clone();
let listener_b = TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr_b = listener_b.local_addr().unwrap();
tokio::spawn(async move {
loop {
let Ok((mut sock, _)) = listener_b.accept().await else {
break;
};
let mut buf = vec![0u8; 65536];
let n = sock.read(&mut buf).await.unwrap_or(0);
let request = String::from_utf8_lossy(&buf[..n]).to_string();
let mut auth = String::new();
for line in request.lines() {
if line.to_ascii_lowercase().starts_with("authorization:") {
auth = line[14..].trim().to_string();
break;
}
}
captured_b.lock().unwrap().push(auth);
let _ = sock.write_all(&ok_response("on-b", "text/plain")).await;
}
});
// Server A: redirects to B (template chooses same-origin "/b" or
// a full cross-origin URL).
let target = b_addr_template(addr_b);
let listener_a = TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr_a = listener_a.local_addr().unwrap();
tokio::spawn(async move {
loop {
let Ok((mut sock, _)) = listener_a.accept().await else {
break;
};
let mut buf = vec![0u8; 4096];
let _ = sock.read(&mut buf).await;
let body = format!(
"HTTP/1.1 302 Found\r\nLocation: {target}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"
);
let _ = sock.write_all(body.as_bytes()).await;
}
});
(addr_a, addr_b, captured)
}
#[tokio::test]
async fn authorization_scrubbed_on_cross_origin_redirect() {
// A → B where A != B (different port = different origin). The
// request carries an Authorization header; after the redirect,
// Server B must NOT see Authorization.
let (addr_a, _addr_b, captured) =
spawn_pair_redirect(|b| format!("http://{b}/landing")).await;
let svc = dev_service(Arc::new(AllowAuthz));
let mut r = req("GET", format!("http://{addr_a}/start"));
r.headers
.insert("Authorization".into(), "Bearer secret-token".into());
let resp = svc.request(&anon_cx(), r).await.unwrap();
assert_eq!(resp.status, 200);
let captured = captured.lock().unwrap();
assert_eq!(
captured.len(),
1,
"server B should have been hit exactly once"
);
assert!(
captured[0].is_empty(),
"Authorization leaked to cross-origin redirect target: {:?}",
captured[0]
);
}
#[tokio::test]
async fn authorization_preserved_on_same_origin_redirect() {
// Server that 302s to itself preserves origin → Authorization
// must survive the hop.
let captured: Arc<std::sync::Mutex<Vec<String>>> =
Arc::new(std::sync::Mutex::new(Vec::new()));
let captured_clone = captured.clone();
let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
let addr = listener.local_addr().unwrap();
tokio::spawn(async move {
let mut hits = 0u32;
loop {
let Ok((mut sock, _)) = listener.accept().await else {
break;
};
let mut buf = vec![0u8; 4096];
let n = sock.read(&mut buf).await.unwrap_or(0);
let request = String::from_utf8_lossy(&buf[..n]).to_string();
let mut auth = String::new();
for line in request.lines() {
if line.to_ascii_lowercase().starts_with("authorization:") {
auth = line[14..].trim().to_string();
break;
}
}
captured_clone.lock().unwrap().push(auth);
hits += 1;
let body = if hits == 1 {
// Same-origin redirect (relative Location).
"HTTP/1.1 302 Found\r\nLocation: /next\r\nContent-Length: 0\r\nConnection: close\r\n\r\n".to_string()
} else {
String::from_utf8(ok_response("done", "text/plain")).unwrap()
};
let _ = sock.write_all(body.as_bytes()).await;
}
});
let svc = dev_service(Arc::new(AllowAuthz));
let mut r = req("GET", format!("http://{addr}/start"));
r.headers
.insert("Authorization".into(), "Bearer keep-me".into());
let resp = svc.request(&anon_cx(), r).await.unwrap();
assert_eq!(resp.status, 200);
let captured = captured.lock().unwrap();
assert_eq!(captured.len(), 2);
assert_eq!(captured[0], "Bearer keep-me", "first hop missing auth");
assert_eq!(
captured[1], "Bearer keep-me",
"same-origin hop should retain auth"
);
}
}