fix(security): server-authoritative approval gate + auth/session/file hardening
Remediate the HIGH and security-relevant findings from the 2026-07-11 audit. H1 — the per-env approval gate is now server-authoritative. The governing project is resolved from the target node's nearest-claimed ancestor (`governing_env_policy`/`_tree` + `governing_project_id` + `ProjectRepository::get_environments_by_id`), independent of the client-supplied `[project]`. Omitting or spoofing the project block can no longer skip a gate the owning project established; a to-create group resolves its declared parent's chain so a fresh subtree node inherits the gate. Fails closed on any read error. H2 — the API-key prefix slice (`&rest[..8]`) is now the boundary-safe `rest.get(..8)`, so an attacker-supplied multibyte bearer can't panic the request task (unauthenticated per-request DoS). Regression test added. C1 — admin sessions gain an absolute lifetime cap (migration 0070, `PICLOUD_SESSION_ABSOLUTE_TTL_HOURS`, default 30d): `lookup` filters it, `touch` clamps the sliding bump to it, so a continuously-used or stolen-but-warm token self-expires. Mirrors the data-plane app-user cap. C2 — `Cache-Control: no-store` on the login and API-key-mint responses (the two that return a raw credential), so a proxy/CDN/browser cache can't retain it. B8 — file downloads are header-safe: `sanitize_stored_filename` guarantees a valid `HeaderValue` (no panic on a control-char name) and BOTH the per-app and group download paths now set attachment + `X-Content-Type-Options: nosniff` + a restrictive CSP, closing a group-path stored-XSS gap. Also folds in the server-side plan-warning plumbing (`plan_warnings`, `PlanResult::warnings`) and the `app_only_reject` message helper that the CLI plan-preview change builds on, plus operator security notes (reads-open shared- topic SSE; the `--env` label is advisory, not a boundary). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -137,6 +137,11 @@ pub struct AuthState {
|
||||
pub sessions: Arc<dyn AdminSessionRepository>,
|
||||
pub keys: Arc<dyn ApiKeyRepository>,
|
||||
pub ttl: Duration,
|
||||
/// C1: absolute hard cap on an admin session's lifetime. The sliding
|
||||
/// `touch` bump is clamped at `session_start + absolute_ttl`, so even a
|
||||
/// continuously-used token self-expires. Set at login via the session's
|
||||
/// stored `absolute_expires_at`; this value seeds that at create time.
|
||||
pub absolute_ttl: Duration,
|
||||
/// F-P-009 — shared cache of resolved Principals. Constructed once
|
||||
/// at startup and cloned (same `Arc`) into every router state that
|
||||
/// resolves or revokes credentials, so a revocation-side eviction is
|
||||
@@ -279,8 +284,10 @@ async fn verify_session(
|
||||
};
|
||||
|
||||
// Sliding-window bump — inline so a DB blip surfaces as 500 rather
|
||||
// than silent stale sessions. Same shape as Phase 3a.
|
||||
let new_expires_at = Utc::now() + chrono::Duration::from_std(state.ttl).unwrap_or_default();
|
||||
// than silent stale sessions. C1: clamp the bump at the session's absolute
|
||||
// cap so a continuously-used token can't slide forever.
|
||||
let sliding = Utc::now() + chrono::Duration::from_std(state.ttl).unwrap_or_default();
|
||||
let new_expires_at = sliding.min(lookup.absolute_expires_at);
|
||||
if let Err(err) = state.sessions.touch(&token_hash, new_expires_at).await {
|
||||
tracing::error!(?err, "admin_sessions touch failed");
|
||||
return Err(InternalError);
|
||||
@@ -304,7 +311,15 @@ async fn verify_api_key(state: &AuthState, rest: &str) -> Result<Option<Principa
|
||||
if rest.len() <= API_KEY_PREFIX_LEN {
|
||||
return Ok(None);
|
||||
}
|
||||
let prefix = &rest[..API_KEY_PREFIX_LEN];
|
||||
// `rest` is the raw, attacker-controlled bearer value after `pic_` — it is
|
||||
// NOT yet validated as the base32 body a real key has, so a byte-index slice
|
||||
// (`&rest[..8]`) could split a multibyte UTF-8 codepoint straddling byte 8
|
||||
// and panic, aborting the request task (an unauthenticated DoS). `get(..)`
|
||||
// returns `None` at a non-char-boundary, so a malformed bearer falls through
|
||||
// to "no match" instead of panicking.
|
||||
let Some(prefix) = rest.get(..API_KEY_PREFIX_LEN) else {
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let mut candidates = match state.keys.find_active_by_prefix(prefix).await {
|
||||
Ok(v) => v,
|
||||
@@ -508,6 +523,20 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn api_key_prefix_slice_is_char_boundary_safe() {
|
||||
// H2 regression (audit 2026-07-11): `rest` is the raw bearer body after
|
||||
// `pic_`, sliced BEFORE any base32 validation. A byte-index slice
|
||||
// `&rest[..API_KEY_PREFIX_LEN]` panics when a multibyte codepoint
|
||||
// straddles byte 8. `€` is 3 bytes, so `aaaaaa€…` puts byte 8 mid-
|
||||
// codepoint — the boundary-safe `get(..)` must return None, not panic.
|
||||
let malformed = "aaaaaa\u{20ac}bbbb";
|
||||
assert!(malformed.len() > API_KEY_PREFIX_LEN);
|
||||
assert!(malformed.get(..API_KEY_PREFIX_LEN).is_none());
|
||||
// A clean ASCII body slices to exactly the indexed prefix.
|
||||
assert_eq!("abcdefghXYZ".get(..API_KEY_PREFIX_LEN), Some("abcdefgh"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn evict_user_drops_only_that_users_entries() {
|
||||
// Audit 2026-06-11 (PrincipalCache revocation-lag).
|
||||
|
||||
Reference in New Issue
Block a user