diff --git a/README.md b/README.md index a002463..823a692 100644 --- a/README.md +++ b/README.md @@ -184,11 +184,12 @@ Useful endpoints: - `POST /oauth/register` - `GET /oauth/authorize` (browser consent screen; loopback by default, optional trusted-header for non-loopback) - `POST /oauth/authorize/decision` (consent-screen form submission) -- `POST /oauth/token` for authorization-code + PKCE token exchange and refresh-token grants +- `POST /oauth/consent/revoke` (browser-session form; revokes a client's consent and its live tokens, after which `/oauth/authorize` re-renders the consent screen) +- `POST /oauth/token` for authorization-code + PKCE token exchange and refresh-token grants (RFC 6749 §5.2 JSON error bodies) - `GET /api/health` with `Authorization: Bearer ` - `/mcp` with `Authorization: Bearer ` -`/oauth/authorize` drives a browser consent flow (ADR-0020). After a client dynamic-registers via `POST /oauth/register`, it redirects the user to `/oauth/authorize` with the standard PKCE parameters. Aver renders a consent screen showing the client name, redirect URI, and all supported scopes as checkboxes, with the client's requested scopes pre-selected; on **Approve** it stores a per-client consent row, mints an authorization code bound to the checked scopes, and redirects back to the client's `redirect_uri` with `code` and `state`. The client then exchanges the code at `/oauth/token` for an `access_token` plus `refresh_token`; refresh grants issue a new access token while preserving the existing refresh token, and access tokens carry only the scopes recorded on the consent row. +`/oauth/authorize` drives a browser consent flow (ADR-0020). After a client dynamic-registers via `POST /oauth/register`, it redirects the user to `/oauth/authorize` with the standard PKCE parameters. Aver renders a consent screen showing the client name, redirect URI, and all supported scopes as checkboxes, with the client's requested scopes pre-selected; on **Approve** it stores a per-client consent row, mints an authorization code bound to the checked scopes, and redirects back to the client's `redirect_uri` with `code` and `state`. Approving with zero scopes records an empty grant that never skips the consent screen, so a later visit can grant real access. The client then exchanges the code at `/oauth/token` for an `access_token` plus `refresh_token` (and an `expires_in` lifetime); refresh grants rotate both halves of the token pair, presenting an already-rotated refresh token revokes the client's whole token family (RFC 6819 §5.2.2.3 reuse detection), and access tokens carry only the scopes recorded on the consent row. The flow supports loopback (`127.0.0.1` / `::1`) callers by default (Profile A in ADR-0020). Non-loopback callers can also authenticate via Profile C when `AVER_TRUSTED_AUTH_HEADER` is set to a trusted upstream identity header (for example `X-Forwarded-User`); otherwise they are rejected with an HTML 403. diff --git a/crates/aver-server/src/auth.rs b/crates/aver-server/src/auth.rs index 91e8bf1..d7ed198 100644 --- a/crates/aver-server/src/auth.rs +++ b/crates/aver-server/src/auth.rs @@ -103,7 +103,9 @@ fn now_unix() -> i64 { time::OffsetDateTime::now_utc().unix_timestamp() } -const ACCESS_TOKEN_TTL_SECS: i64 = 60 * 60; +/// Access-token lifetime, in seconds. Exposed so the token endpoint can +/// report it as RFC 6749 §5.1 `expires_in`. +pub const ACCESS_TOKEN_TTL_SECS: i64 = 60 * 60; const REFRESH_TOKEN_TTL_SECS: i64 = 30 * 24 * 60 * 60; fn encode_scopes(scopes: &[String]) -> String { @@ -139,6 +141,37 @@ fn random_session_id() -> String { base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes) } +/// Applies an `ALTER TABLE ... ADD COLUMN` migration. +/// +/// SQLite has no `ADD COLUMN IF NOT EXISTS`, so re-opening an already +/// migrated database surfaces a duplicate-column error — that one case means +/// "migration already ran" and is ignored. Every other error (syntax, I/O, +/// permissions, corruption) is a real failure and propagates instead of +/// being swallowed. rusqlite 0.32 has no dedicated error-code variant for +/// duplicate columns: SQLite reports it as `SQLITE_ERROR` (extended code 1, +/// mapped to [`rusqlite::ErrorCode::Unknown`]) with a +/// `duplicate column name: ` message, so we match code and message. +fn apply_column_migration(conn: &Connection, statement: &str) -> anyhow::Result<()> { + match conn.execute_batch(statement) { + Ok(()) => Ok(()), + Err(rusqlite::Error::SqliteFailure(err, Some(msg))) + if err.extended_code == 1 && msg.contains("duplicate column name") => + { + Ok(()) + } + Err(err) => Err(err.into()), + } +} + +/// Raw `refresh_tokens` row as loaded for the refresh grant. +struct RefreshTokenRow { + user_id: String, + client_id: Option, + granted_scopes: String, + revoked_at: Option, + expires_at: i64, +} + pub struct AuthDb { conn: Connection, } @@ -220,24 +253,30 @@ impl AuthDb { value BLOB NOT NULL );", )?; - // Migrate existing DBs that lack the new columns (SQLite returns an - // error if the column already exists; we intentionally ignore it). - let _ = conn.execute_batch( + // Migrate existing DBs that lack the new columns (SQLite has no + // `ADD COLUMN IF NOT EXISTS`; a duplicate-column error means the + // migration already ran). + apply_column_migration( + &conn, "ALTER TABLE authorization_codes ADD COLUMN redirect_uri TEXT NOT NULL DEFAULT '';", - ); - let _ = conn.execute_batch( + )?; + apply_column_migration( + &conn, "ALTER TABLE authorization_codes ADD COLUMN expires_at INTEGER NOT NULL DEFAULT 0;", - ); + )?; // ADR-0020 slice 3: per-token scope persistence. - let _ = conn.execute_batch( + apply_column_migration( + &conn, "ALTER TABLE authorization_codes ADD COLUMN granted_scopes TEXT NOT NULL DEFAULT '';", - ); - let _ = conn.execute_batch( + )?; + apply_column_migration( + &conn, "ALTER TABLE access_tokens ADD COLUMN granted_scopes TEXT NOT NULL DEFAULT '';", - ); - let _ = conn.execute_batch( + )?; + apply_column_migration( + &conn, "ALTER TABLE refresh_tokens ADD COLUMN granted_scopes TEXT NOT NULL DEFAULT '';", - ); + )?; for statement in [ "ALTER TABLE access_tokens ADD COLUMN client_id TEXT;", "ALTER TABLE access_tokens ADD COLUMN expires_at INTEGER NOT NULL DEFAULT 0;", @@ -246,17 +285,17 @@ impl AuthDb { "ALTER TABLE refresh_tokens ADD COLUMN expires_at INTEGER NOT NULL DEFAULT 0;", "ALTER TABLE refresh_tokens ADD COLUMN revoked_at INTEGER;", ] { - let _ = conn.execute_batch(statement); + apply_column_migration(&conn, statement)?; } let now = now_unix(); - let _ = conn.execute( + conn.execute( "UPDATE access_tokens SET expires_at = ?1 WHERE expires_at = 0", [now + ACCESS_TOKEN_TTL_SECS], - ); - let _ = conn.execute( + )?; + conn.execute( "UPDATE refresh_tokens SET expires_at = ?1 WHERE expires_at = 0", [now + REFRESH_TOKEN_TTL_SECS], - ); + )?; Ok(Self { conn }) } @@ -473,14 +512,13 @@ impl AuthDb { params![now, code], )?; let scopes = decode_scopes(&granted_scopes); - self.issue_token_pair(&user_id, Some(client_id), None, &scopes) + self.issue_token_pair(&user_id, Some(client_id), &scopes) } fn issue_token_pair( &self, user_id: &str, client_id: Option<&str>, - existing_refresh_token: Option, granted_scopes: &[String], ) -> anyhow::Result { let access_token = random_session_id(); @@ -490,7 +528,7 @@ impl AuthDb { client_id, granted_scopes, )?; - let refresh_token = existing_refresh_token.unwrap_or_else(random_session_id); + let refresh_token = random_session_id(); let now = time::OffsetDateTime::now_utc().unix_timestamp(); let scopes = encode_scopes(granted_scopes); self.conn.execute( @@ -514,22 +552,64 @@ impl AuthDb { pub fn refresh_access_token(&self, refresh_token: &str) -> anyhow::Result { let token_hash = hash_token(refresh_token); - let now = time::OffsetDateTime::now_utc().unix_timestamp(); - let (user_id, client_id, granted_scopes): (String, Option, String) = - self.conn.query_row( - "SELECT user_id, client_id, granted_scopes - FROM refresh_tokens - WHERE token_hash = ?1 AND revoked_at IS NULL AND expires_at > ?2", - params![token_hash, now], - |row| Ok((row.get(0)?, row.get(1)?, row.get(2)?)), + let now = now_unix(); + let row: Option = self + .conn + .query_row( + "SELECT user_id, client_id, granted_scopes, revoked_at, expires_at + FROM refresh_tokens + WHERE token_hash = ?1", + params![token_hash], + |row| { + Ok(RefreshTokenRow { + user_id: row.get(0)?, + client_id: row.get(1)?, + granted_scopes: row.get(2)?, + revoked_at: row.get(3)?, + expires_at: row.get(4)?, + }) + }, + ) + .optional()?; + let Some(row) = row else { + anyhow::bail!("unknown refresh token"); + }; + if row.revoked_at.is_some() { + // Reuse of an already-rotated (or revoked) refresh token: treat + // as potential token theft and kill the whole family for this + // (user, client), RFC 6819 §5.2.2.3 style. + self.revoke_token_family(&row.user_id, row.client_id.as_deref())?; + anyhow::bail!("refresh token reuse detected; token family revoked"); + } + anyhow::ensure!(now < row.expires_at, "refresh token expired"); + + // Rotate: retire the presented token, mint a fresh pair. The old + // refresh token is invalid from this point on. + self.conn.execute( + "UPDATE refresh_tokens SET revoked_at = ?2 WHERE token_hash = ?1", + params![token_hash, now], + )?; + let scopes = decode_scopes(&row.granted_scopes); + self.issue_token_pair(&row.user_id, row.client_id.as_deref(), &scopes) + } + + /// Revokes every live access and refresh token for a `(user, client)` + /// pair. Used when a rotated refresh token is presented again. + fn revoke_token_family(&self, user_id: &str, client_id: Option<&str>) -> anyhow::Result<()> { + let now = now_unix(); + for table in ["access_tokens", "refresh_tokens"] { + self.conn.execute( + &format!( + "UPDATE {table} + SET revoked_at = ?3 + WHERE user_id = ?1 + AND revoked_at IS NULL + AND (client_id = ?2 OR (client_id IS NULL AND ?2 IS NULL))" + ), + params![user_id, client_id, now], )?; - let scopes = decode_scopes(&granted_scopes); - self.issue_token_pair( - &user_id, - client_id.as_deref(), - Some(refresh_token.to_string()), - &scopes, - ) + } + Ok(()) } /// Returns the access-token row's `(user_id, granted_scopes_raw)` if the diff --git a/crates/aver-server/src/config.rs b/crates/aver-server/src/config.rs index 0b5c9b7..e72544c 100644 --- a/crates/aver-server/src/config.rs +++ b/crates/aver-server/src/config.rs @@ -16,10 +16,13 @@ pub struct ServerConfig { impl ServerConfig { pub fn from_env() -> anyhow::Result { + use anyhow::Context; + let host = std::env::var("AVER_HOST").unwrap_or_else(|_| "127.0.0.1".to_string()); let port = std::env::var("AVER_PORT") .unwrap_or_else(|_| "3317".to_string()) - .parse()?; + .parse() + .context("invalid AVER_PORT")?; let base_url = std::env::var("AVER_BASE_URL").unwrap_or_else(|_| format!("http://{host}:{port}")); let memory_dir = std::env::var("AVER_MEMORY_DIR").unwrap_or_else(|_| ".aver".to_string()); diff --git a/crates/aver-server/src/consent.rs b/crates/aver-server/src/consent.rs index 701d42c..67d70c2 100644 --- a/crates/aver-server/src/consent.rs +++ b/crates/aver-server/src/consent.rs @@ -27,6 +27,7 @@ use std::collections::BTreeMap; use std::net::SocketAddr; use std::sync::{Arc, Mutex}; +use anyhow::Context; use askama::Template; use axum::extract::{ConnectInfo, Form, Query, State}; use axum::http::{HeaderMap, HeaderName, HeaderValue, StatusCode, header}; @@ -38,6 +39,7 @@ use sha2::Sha256; use url::Url; use crate::auth::{AuthDb, Session, User, UserKind}; +use crate::oauth::constant_time_eq; use crate::origin::validate_browser_origin; use crate::scopes::{SUPPORTED, ScopeParseError, parse_scope_list}; @@ -59,7 +61,9 @@ type HmacSha256 = Hmac; /// /// - Loopback requests authenticate as the fixed local user. /// - Non-loopback requests may authenticate from the configured trusted header. -/// - Returns `None` when authentication is not possible. +/// - Returns `Ok(None)` when authentication is not possible. +/// - Returns `Err` when the auth database fails — surfacing the failure to +/// the caller (HTML 500) instead of silently degrading to a 403. /// /// `headers` is read for Profile C trusted-header auth (e.g. /// `X-Forwarded-User`). @@ -68,7 +72,7 @@ pub fn authenticate_loopback( headers: &HeaderMap, auth_db: &AuthDb, trusted_auth_header: Option<&str>, -) -> Option { +) -> anyhow::Result> { authenticate_request(remote_addr, headers, auth_db, trusted_auth_header) } @@ -77,7 +81,7 @@ pub fn authenticate_request( headers: &HeaderMap, auth_db: &AuthDb, trusted_auth_header: Option<&str>, -) -> Option { +) -> anyhow::Result> { if remote_addr.ip().is_loopback() { let now = time::OffsetDateTime::now_utc().unix_timestamp(); let user = User { @@ -86,25 +90,31 @@ pub fn authenticate_request( external_id: None, created_at: now, }; - if let Err(err) = auth_db.upsert_user(&user) { - tracing_unavailable_warn(&format!("upsert local user failed: {err}")); - return None; - } - return auth_db.get_user(LOCAL_USER_ID).ok().flatten(); + auth_db + .upsert_user(&user) + .context("upsert local user failed")?; + return auth_db + .get_user(LOCAL_USER_ID) + .context("load local user failed"); } - let header_name = trusted_auth_header?.trim(); - let header_name = match HeaderName::from_bytes(header_name.as_bytes()) { + let Some(header_name) = trusted_auth_header else { + return Ok(None); + }; + let header_name = match HeaderName::from_bytes(header_name.trim().as_bytes()) { Ok(v) => v, - Err(_) => return None, + Err(_) => return Ok(None), + }; + let raw = match headers.get(header_name).and_then(|v| v.to_str().ok()) { + Some(raw) => raw.trim(), + None => return Ok(None), }; - let raw = headers.get(header_name)?.to_str().ok()?.trim(); if raw.is_empty() { - return None; + return Ok(None); } let user_id = raw.split(',').next().unwrap_or("").trim(); if user_id.is_empty() { - return None; + return Ok(None); } let now = time::OffsetDateTime::now_utc().unix_timestamp(); @@ -114,18 +124,14 @@ pub fn authenticate_request( external_id: None, created_at: now, }; - if let Err(err) = auth_db.upsert_user(&user) { - tracing_unavailable_warn(&format!("upsert header-auth user failed: {err}")); - return None; - } - auth_db.get_user(&user.id).ok().flatten() + auth_db + .upsert_user(&user) + .context("upsert header-auth user failed")?; + auth_db + .get_user(&user.id) + .context("load header-auth user failed") } -/// Logging stub: aver-server does not yet pull in `tracing`. Keeps the call -/// site honest about the failure without panicking; in practice the upsert -/// path is exercised in tests so silent failure is acceptable. -fn tracing_unavailable_warn(_msg: &str) {} - /// Reads the session cookie from `headers` and returns the bound user if the /// session is still valid. pub fn current_session(headers: &HeaderMap, auth_db: &AuthDb) -> Option<(Session, User)> { @@ -194,7 +200,8 @@ pub fn compute_csrf_token( /// Constant-time-ish equality. `hmac::Mac::verify` cannot be used directly /// because we already encoded the token; comparing equal-length base64 is -/// fine for this surface and avoids re-deriving the raw bytes. +/// fine for this surface and avoids re-deriving the raw bytes. Uses the +/// shared helper from [`crate::oauth`]. pub fn verify_csrf_token( server_secret: &[u8], session_id: &str, @@ -206,17 +213,6 @@ pub fn verify_csrf_token( constant_time_eq(expected.as_bytes(), presented.as_bytes()) } -fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { - if a.len() != b.len() { - return false; - } - let mut diff = 0u8; - for (x, y) in a.iter().zip(b.iter()) { - diff |= x ^ y; - } - diff == 0 -} - /// True iff the consent record covers every requested scope (treating an /// empty `requested` set as the implicit "default access" scope). pub fn consent_covers(granted: &[String], requested: &[String]) -> bool { @@ -268,13 +264,17 @@ struct ErrorTemplate<'a> { detail: &'a str, } -fn html_error(status: StatusCode, title: &str, detail: &str) -> Response { +fn html_message(status: StatusCode, title: &str, detail: &str) -> Response { let body = ErrorTemplate { title, detail } .render() - .expect("error template should render"); + .expect("message template should render"); html_response(status, body, None) } +fn html_error(status: StatusCode, title: &str, detail: &str) -> Response { + html_message(status, title, detail) +} + #[derive(Debug, Deserialize)] pub struct AuthorizeQuery { pub response_type: String, @@ -479,14 +479,21 @@ pub async fn handle_loopback_get_authorize( auth_db, deps.trusted_auth_header.as_deref(), ) { - Some(u) => u, - None => { + Ok(Some(u)) => u, + Ok(None) => { return html_error( StatusCode::FORBIDDEN, "Authorization unavailable", "Authentication is unavailable for this request.", ); } + Err(err) => { + return html_error( + StatusCode::INTERNAL_SERVER_ERROR, + "Server error", + &format!("Failed to authenticate the user: {err}"), + ); + } }; let (client_name, redirect_uris, registered_at) = @@ -535,8 +542,13 @@ pub async fn handle_loopback_get_authorize( let scopes = parse_scope(query.scope.as_deref()); // Skip the screen if a live consent already covers the requested scopes. + // An empty grant never skips: `consent_covers` treats an empty request + // as satisfied, so a zero-scope consent row would otherwise skip forever, + // minting tokens that fail every per-tool scope check with no way back + // to this screen to grant real access. if let Ok(Some(consent)) = auth_db.get_consent(&user.id, &query.client_id) && consent.revoked_at.is_none() + && !consent.granted_scopes.is_empty() && consent_covers(&consent.granted_scopes, &scopes) { let _ = auth_db.touch_consent_last_used(&user.id, &query.client_id); @@ -710,14 +722,21 @@ pub async fn handle_authorize_decision( auth_db, deps.trusted_auth_header.as_deref(), ) { - Some(u) => u, - None => { + Ok(Some(u)) => u, + Ok(None) => { return html_error( StatusCode::FORBIDDEN, "Authorization unavailable", "Authentication is unavailable for this request.", ); } + Err(err) => { + return html_error( + StatusCode::INTERNAL_SERVER_ERROR, + "Server error", + &format!("Failed to authenticate the user: {err}"), + ); + } }; let allowed = match parse_allowed_origins(&deps.base_url) { @@ -852,6 +871,136 @@ pub async fn handle_authorize_decision( } } +#[derive(Debug, Deserialize)] +pub struct RevokeConsentForm { + pub client_id: String, +} + +/// `POST /oauth/consent/revoke`: revoke the session user's consent for one +/// client and invalidate that client's live access/refresh tokens (the DB +/// layer revokes both, see [`AuthDb::revoke_consent`]). This is the recovery +/// hatch for a consent the user no longer wants — including a zero-scope +/// grant recorded before empty grants stopped skipping the consent screen — +/// after which the next `/oauth/authorize` visit re-renders the screen. +pub async fn handle_revoke_consent( + State(deps): State>, + ConnectInfo(remote_addr): ConnectInfo, + headers: HeaderMap, + Form(form): Form, +) -> Response { + let auth_db_guard = match deps.auth_db.lock() { + Ok(g) => g, + Err(_) => { + return html_error( + StatusCode::INTERNAL_SERVER_ERROR, + "Server error", + "Auth database lock poisoned.", + ); + } + }; + let auth_db: &AuthDb = &auth_db_guard; + + let user = match authenticate_loopback( + remote_addr, + &headers, + auth_db, + deps.trusted_auth_header.as_deref(), + ) { + Ok(Some(u)) => u, + Ok(None) => { + return html_error( + StatusCode::FORBIDDEN, + "Authorization unavailable", + "Authentication is unavailable for this request.", + ); + } + Err(err) => { + return html_error( + StatusCode::INTERNAL_SERVER_ERROR, + "Server error", + &format!("Failed to authenticate the user: {err}"), + ); + } + }; + + let allowed = match parse_allowed_origins(&deps.base_url) { + Ok(v) => v, + Err(_) => { + return html_error( + StatusCode::INTERNAL_SERVER_ERROR, + "Server misconfiguration", + "AVER_BASE_URL is not a valid URL.", + ); + } + }; + if let Err(err) = validate_browser_origin(&headers, &allowed) { + return html_error( + StatusCode::FORBIDDEN, + "Cross-site POST rejected", + &err.to_string(), + ); + } + + // Revoking is a state-changing browser action: require the session + // cookie (SameSite=Lax) to bind the request to the consenting user. + let (_session, session_user) = match current_session(&headers, auth_db) { + Some(v) => v, + None => { + return html_error( + StatusCode::BAD_REQUEST, + "Missing session", + "No valid Aver session cookie was presented.", + ); + } + }; + if session_user.id != user.id { + return html_error( + StatusCode::FORBIDDEN, + "Wrong user", + "Session does not match authenticated user.", + ); + } + + if form.client_id.trim().is_empty() { + return html_error( + StatusCode::BAD_REQUEST, + "Missing client_id", + "Form value 'client_id' is required.", + ); + } + match auth_db.get_client(&form.client_id) { + Ok(Some(_)) => {} + Ok(None) => { + return html_error( + StatusCode::BAD_REQUEST, + "Unknown client", + "No OAuth client is registered with that client_id.", + ); + } + Err(_) => { + return html_error( + StatusCode::INTERNAL_SERVER_ERROR, + "Server error", + "Failed to look up the OAuth client.", + ); + } + } + + if let Err(err) = auth_db.revoke_consent(&user.id, &form.client_id) { + return html_error( + StatusCode::INTERNAL_SERVER_ERROR, + "Server error", + &format!("Failed to revoke consent: {err}"), + ); + } + + html_message( + StatusCode::OK, + "Consent revoked", + "The client's consent and its live tokens were revoked. Restart the client's OAuth flow (or revisit /oauth/authorize) to grant access again.", + ) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/aver-server/src/http.rs b/crates/aver-server/src/http.rs index 470ff3c..2738551 100644 --- a/crates/aver-server/src/http.rs +++ b/crates/aver-server/src/http.rs @@ -19,13 +19,17 @@ use tower_http::cors::{Any as CorsAny, CorsLayer}; use url::Url; use crate::{ - auth::{AuthDb, hash_token}, + auth::{ACCESS_TOKEN_TTL_SECS, AuthDb, hash_token}, config::ServerConfig, - consent::{ConsentDeps, handle_authorize_decision, handle_loopback_get_authorize}, + consent::{ + ConsentDeps, handle_authorize_decision, handle_loopback_get_authorize, + handle_revoke_consent, + }, mcp::AverMcpService, oauth::authorization_server_metadata, scope_resolution::resolve_scope, scopes::parse_scope_list_lossy, + tools::AverTools, }; /// Per-request bag of OAuth scopes granted by the bearer token. Inserted as @@ -67,13 +71,23 @@ pub fn build_router(config: ServerConfig) -> anyhow::Result { axum::middleware::from_fn_with_state(state.clone(), validate_bearer_token), ); - let memory_dir = state.config.memory_dir.clone(); + // Open ONE memory store for the whole process and share it across MCP + // sessions. `StreamableHttpService::new` runs the factory once per + // session; opening the store inside the factory would give every session + // its own SQLite connection on the same `memory_dir`, breaking the + // single-writer invariant (log rotation and claim-id pre-allocation + // race). rusqlite connections are Send-but-not-Sync, so the store sits + // behind the same `Arc>` pattern used for `AuthDb`; per-session + // state is limited to auth/scope context carried in request extensions. + let shared_tools = Arc::new(Mutex::new(AverTools::open(&state.config.memory_dir)?)); let base_url = state.config.base_url.clone(); let mcp_service: StreamableHttpService = StreamableHttpService::new( move || { - AverMcpService::open(memory_dir.clone(), base_url.clone()) - .map_err(std::io::Error::other) + Ok(AverMcpService::from_shared_tools( + shared_tools.clone(), + base_url.clone(), + )) }, LocalSessionManager::default().into(), StreamableHttpServerConfig::default(), @@ -113,6 +127,7 @@ pub fn build_router(config: ServerConfig) -> anyhow::Result { .route("/oauth/register", post(oauth_register)) .route("/oauth/authorize", get(oauth_authorize)) .route("/oauth/authorize/decision", post(oauth_authorize_decision)) + .route("/oauth/consent/revoke", post(oauth_consent_revoke)) .route("/oauth/token", post(oauth_token)) .merge(protected_api) .merge(protected_mcp) @@ -170,6 +185,16 @@ async fn resolve_request_scope(request: Request, next: Next) -> Response { } } +/// RFC 6750 §3: bearer-protected surfaces must answer 401s with a +/// `WWW-Authenticate: Bearer` challenge so clients know how to authenticate. +fn unauthorized_bearer() -> Response { + let mut response = StatusCode::UNAUTHORIZED.into_response(); + response + .headers_mut() + .insert(header::WWW_AUTHENTICATE, HeaderValue::from_static("Bearer")); + response +} + async fn validate_bearer_token( axum::extract::State(state): axum::extract::State, mut request: Request, @@ -181,7 +206,7 @@ async fn validate_bearer_token( .and_then(|value| value.to_str().ok()) .and_then(|value| value.strip_prefix("Bearer ")); let Some(token) = token else { - return StatusCode::UNAUTHORIZED.into_response(); + return unauthorized_bearer(); }; let granted_raw = { let Ok(db) = state.auth_db.lock() else { @@ -189,7 +214,7 @@ async fn validate_bearer_token( }; match db.validate_access_token(&hash_token(token)) { Ok(Some((_, scopes_raw))) => scopes_raw, - Ok(None) => return StatusCode::UNAUTHORIZED.into_response(), + Ok(None) => return unauthorized_bearer(), Err(_) => return StatusCode::INTERNAL_SERVER_ERROR.into_response(), } }; @@ -333,6 +358,39 @@ async fn oauth_authorize_decision( .await } +/// Dispatcher for `POST /oauth/consent/revoke`. Same shape as +/// [`oauth_authorize_decision`]: fail closed when ConnectInfo is absent. +async fn oauth_consent_revoke( + axum::extract::State(state): axum::extract::State, + request: Request, +) -> Response { + let connect_info = request + .extensions() + .get::>() + .copied(); + let Some(connect) = connect_info else { + return html_response_unavailable(); + }; + + let (mut parts, body) = request.into_parts(); + let headers = std::mem::take(&mut parts.headers); + let bytes = match axum::body::to_bytes(body, 64 * 1024).await { + Ok(b) => b, + Err(_) => return StatusCode::BAD_REQUEST.into_response(), + }; + let form: crate::consent::RevokeConsentForm = match serde_urlencoded::from_bytes(&bytes) { + Ok(f) => f, + Err(_) => return StatusCode::BAD_REQUEST.into_response(), + }; + handle_revoke_consent( + axum::extract::State(state.consent_deps.clone()), + connect, + headers, + Form(form), + ) + .await +} + #[derive(Debug, Deserialize)] struct TokenRequest { grant_type: String, @@ -348,31 +406,53 @@ struct TokenRequest { refresh_token: String, } +/// RFC 6749 §5.2 error body for the token endpoint. +fn token_error(status: StatusCode, error: &'static str) -> (StatusCode, Json) { + (status, Json(serde_json::json!({ "error": error }))) +} + async fn oauth_token( axum::extract::State(state): axum::extract::State, Form(request): Form, -) -> Result, axum::http::StatusCode> { +) -> Result, (StatusCode, Json)> { let db = state .auth_db .lock() - .map_err(|_| axum::http::StatusCode::INTERNAL_SERVER_ERROR)?; + .map_err(|_| token_error(StatusCode::INTERNAL_SERVER_ERROR, "server_error"))?; let tokens = match request.grant_type.as_str() { - "authorization_code" => db - .exchange_authorization_code_for_tokens( + "authorization_code" => { + if request.code.is_empty() + || request.client_id.is_empty() + || request.code_verifier.is_empty() + { + return Err(token_error(StatusCode::BAD_REQUEST, "invalid_request")); + } + db.exchange_authorization_code_for_tokens( &request.code, &request.client_id, &request.code_verifier, &request.redirect_uri, ) - .map_err(|_| axum::http::StatusCode::BAD_REQUEST)?, - "refresh_token" => db - .refresh_access_token(&request.refresh_token) - .map_err(|_| axum::http::StatusCode::BAD_REQUEST)?, - _ => return Err(axum::http::StatusCode::BAD_REQUEST), + .map_err(|_| token_error(StatusCode::BAD_REQUEST, "invalid_grant"))? + } + "refresh_token" => { + if request.refresh_token.is_empty() { + return Err(token_error(StatusCode::BAD_REQUEST, "invalid_request")); + } + db.refresh_access_token(&request.refresh_token) + .map_err(|_| token_error(StatusCode::BAD_REQUEST, "invalid_grant"))? + } + _ => { + return Err(token_error( + StatusCode::BAD_REQUEST, + "unsupported_grant_type", + )); + } }; Ok(Json(serde_json::json!({ "access_token": tokens.access_token, "refresh_token": tokens.refresh_token, "token_type": "Bearer", + "expires_in": ACCESS_TOKEN_TTL_SECS, }))) } diff --git a/crates/aver-server/src/mcp.rs b/crates/aver-server/src/mcp.rs index 830c212..ec04c9d 100644 --- a/crates/aver-server/src/mcp.rs +++ b/crates/aver-server/src/mcp.rs @@ -88,8 +88,6 @@ pub struct RememberClaimParams { pub struct RecallParams { pub query: String, #[serde(default)] - pub alpha: Option, - #[serde(default)] pub hops: Option, #[serde(default = "default_top_k")] pub top_k: usize, @@ -130,11 +128,22 @@ pub struct AverMcpService { #[tool_router] impl AverMcpService { pub fn open(memory_dir: impl AsRef, base_url: String) -> anyhow::Result { - Ok(Self { - tools: Arc::new(Mutex::new(AverTools::open(memory_dir)?)), + Ok(Self::from_shared_tools( + Arc::new(Mutex::new(AverTools::open(memory_dir)?)), + base_url, + )) + } + + /// Builds a service facade over an already-open, shared [`AverTools`]. + /// The HTTP server opens one store at startup and hands a clone of the + /// same `Arc` to every MCP session, preserving the single-writer + /// invariant on the underlying SQLite connection. + pub fn from_shared_tools(tools: Arc>, base_url: String) -> Self { + Self { + tools, base_url, tool_router: Self::tool_router(), - }) + } } fn lock_tools(&self) -> Result, McpError> { @@ -435,7 +444,6 @@ impl AverMcpService { }); CoreRecallParams { query: params.query, - alpha: params.alpha, hops: params.hops, top_k: Some(params.top_k), scope: params.scope.or(Some(resolved.scope)), @@ -458,9 +466,14 @@ fn json_tool_result( tool_name: &str, ) -> Result { match result { - Ok(value) => Ok(CallToolResult::success(vec![Content::text( - serde_json::to_string_pretty(&value).unwrap_or_default(), - )])), + Ok(value) => match serde_json::to_string_pretty(&value) { + Ok(text) => Ok(CallToolResult::success(vec![Content::text(text)])), + Err(err) => Err(McpError::new( + ErrorCode::INTERNAL_ERROR, + format!("{tool_name} result serialization failed: {err}"), + None, + )), + }, Err(err) => Err(McpError::new( ErrorCode::INTERNAL_ERROR, format!("{tool_name} failed: {}", tools.describe_error(&err)), @@ -542,6 +555,80 @@ impl ServerHandler for AverMcpService { mod tests { use super::*; + #[test] + fn services_sharing_tools_observe_each_others_writes() { + // Two service facades (one per MCP session) over one shared + // `Arc>` write into the same store — the + // single-writer invariant the HTTP layer relies on. + let dir = tempfile::tempdir().unwrap(); + let shared = Arc::new(Mutex::new(AverTools::open(dir.path()).unwrap())); + let service_a = + AverMcpService::from_shared_tools(shared.clone(), "http://localhost:3317".to_string()); + let service_b = + AverMcpService::from_shared_tools(shared.clone(), "http://localhost:3317".to_string()); + + std::thread::scope(|scope| { + let a = &service_a; + let b = &service_b; + let writer_a = scope.spawn(move || { + a.lock_tools() + .unwrap() + .remember_claim(crate::tools::RememberClaimParams { + subject: "writer-a".to_string(), + predicate: "relates_to".to_string(), + object: "shared".to_string(), + source: None, + agent_id: None, + agent_kind: None, + scope: None, + }) + .unwrap(); + }); + let writer_b = scope.spawn(move || { + b.lock_tools() + .unwrap() + .remember_claim(crate::tools::RememberClaimParams { + subject: "writer-b".to_string(), + predicate: "relates_to".to_string(), + object: "shared".to_string(), + source: None, + agent_id: None, + agent_kind: None, + scope: None, + }) + .unwrap(); + }); + writer_a.join().unwrap(); + writer_b.join().unwrap(); + }); + + let tools = shared.lock().unwrap(); + let recalled = tools + .recall(crate::tools::RecallParams { + query: "shared".to_string(), + hops: None, + top_k: Some(10), + scope: None, + scope_walk: None, + agent_id: None, + agent_kind: None, + predicate: None, + predicate_walk: None, + min_confidence: None, + status: None, + }) + .unwrap(); + let subjects: Vec<&str> = recalled + .triples + .iter() + .map(|claim| claim.subject.as_str()) + .collect(); + assert!( + subjects.contains(&"writer-a") && subjects.contains(&"writer-b"), + "both facades' writes land in the shared store: {subjects:?}", + ); + } + #[test] fn json_tool_result_enriches_unknown_predicate_for_any_tool_name() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/aver-server/src/oauth.rs b/crates/aver-server/src/oauth.rs index 44a8fe9..7de0ba5 100644 --- a/crates/aver-server/src/oauth.rs +++ b/crates/aver-server/src/oauth.rs @@ -8,8 +8,26 @@ pub fn pkce_s256_challenge(verifier: &str) -> String { base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(digest) } +/// Constant-time byte-slice equality. Shared by PKCE challenge verification +/// here and the consent flow's anti-CSRF HMAC comparison, so both surfaces +/// avoid early-exit `==` on secrets. Length inequality still leaks length +/// (both inputs are fixed-length digests in practice). +pub(crate) fn constant_time_eq(a: &[u8], b: &[u8]) -> bool { + if a.len() != b.len() { + return false; + } + let mut diff = 0u8; + for (x, y) in a.iter().zip(b.iter()) { + diff |= x ^ y; + } + diff == 0 +} + pub fn verify_pkce_s256(verifier: &str, challenge: &str) -> bool { - pkce_s256_challenge(verifier) == challenge + constant_time_eq( + pkce_s256_challenge(verifier).as_bytes(), + challenge.as_bytes(), + ) } pub fn authorization_server_metadata(base_url: &str) -> serde_json::Value { diff --git a/crates/aver-server/src/origin.rs b/crates/aver-server/src/origin.rs index 9855b9b..3f70747 100644 --- a/crates/aver-server/src/origin.rs +++ b/crates/aver-server/src/origin.rs @@ -1,9 +1,9 @@ //! Browser-origin validation helper for ADR-0020. //! -//! Slice 1 only exposes the helper; later slices mount it on -//! `/oauth/authorize` and the consent endpoints. The function is deliberately -//! permissive for non-browser clients (curl, MCP HTTP clients) which omit -//! both the `Origin` and `Sec-Fetch-Site` request headers. +//! Mounted by the consent flow ([`crate::consent`]) on `/oauth/authorize`, +//! `/oauth/authorize/decision`, and `/oauth/consent/revoke`. The function is +//! deliberately permissive for non-browser clients (curl, MCP HTTP clients) +//! which omit both the `Origin` and `Sec-Fetch-Site` request headers. use axum::http::{HeaderMap, header}; use url::Url; diff --git a/crates/aver-server/src/scope_resolution.rs b/crates/aver-server/src/scope_resolution.rs index b9dde7a..b702394 100644 --- a/crates/aver-server/src/scope_resolution.rs +++ b/crates/aver-server/src/scope_resolution.rs @@ -36,12 +36,20 @@ impl ResolvedScope { } } +/// Maximum accepted scope length in bytes. Unbounded scopes bloat the +/// `scope` column and the `X-Aver-Scope` header budget. +const MAX_SCOPE_LEN: usize = 256; + /// Validate a scope candidate without going through sqlite. Mirrors the -/// `[A-Za-z0-9_/-]` charset enforced by migration 0084. +/// `[A-Za-z0-9_/-]` charset enforced by migration 0084, additionally +/// rejecting empty path segments (`///`, `a//b`) and over-long values. fn validate(scope: &str, source: &'static str) -> anyhow::Result<()> { if scope.trim().is_empty() { anyhow::bail!("{source}: scope must not be blank"); } + if scope.len() > MAX_SCOPE_LEN { + anyhow::bail!("{source}: scope exceeds {MAX_SCOPE_LEN} bytes"); + } let ok = scope .bytes() .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-' || b == b'/'); @@ -50,6 +58,9 @@ fn validate(scope: &str, source: &'static str) -> anyhow::Result<()> { "{source}: scope {scope:?} contains invalid characters; allowed [A-Za-z0-9_/-]" ); } + if scope.split('/').any(|segment| segment.is_empty()) { + anyhow::bail!("{source}: scope {scope:?} contains an empty path segment"); + } Ok(()) } diff --git a/crates/aver-server/src/tools.rs b/crates/aver-server/src/tools.rs index 991dc8c..0211b2e 100644 --- a/crates/aver-server/src/tools.rs +++ b/crates/aver-server/src/tools.rs @@ -7,6 +7,12 @@ use aver_core::{ }; use serde::{Deserialize, Serialize}; +/// Embedding-model label recorded on vector chunks written via +/// `add_vector_chunk`. The server stores retrieval text only — embedding +/// happens outside the MCP surface — so this names the model the chunk is +/// tuned for rather than anything computed in-process. +const DEFAULT_EMBEDDING_MODEL: &str = "nomic-embed-text"; + #[derive(Debug, Clone, Deserialize, schemars::JsonSchema)] pub struct RememberClaimParams { /// Durable memory subject. @@ -33,9 +39,6 @@ pub struct RememberClaimParams { pub struct RecallParams { /// Natural-language or entity query to retrieve durable memory. Start broad enough to find related claims, then narrow with filters if needed. pub query: String, - /// Hybrid retrieval weight. - #[serde(default)] - pub alpha: Option, /// Graph hop limit. #[serde(default)] pub hops: Option, @@ -132,7 +135,9 @@ pub struct ContradictParams { #[derive(Debug, Clone, Deserialize, schemars::JsonSchema)] pub struct ConsolidateParams { - /// ADR-0021 scope filter. + /// Consolidation scope. Only `"all"` (or omission) is accepted today — + /// consolidation always recomputes derived state across every scope; + /// per-scope consolidation is not implemented. #[serde(default)] pub scope: Option, } @@ -598,13 +603,6 @@ impl AverTools { .store .recall_text_with_filters(¶ms.query, filters)?; claims.truncate(top_k); - let _alpha = if let Some(alpha) = params.alpha { - aver_core::retrieval::HybridWeights::try_new(alpha) - .map_err(|err| anyhow::anyhow!("invalid alpha: {err}"))? - .alpha - } else { - aver_core::retrieval::HybridWeights::for_query(¶ms.query).alpha - }; let hops = validate_hops(params.hops.unwrap_or(2))?; let mut subgraph = self .store @@ -859,7 +857,7 @@ impl AverTools { } let chunk_id = self.store - .add_vector_chunk(params.claim_id, ¶ms.text, "nomic-embed-text")?; + .add_vector_chunk(params.claim_id, ¶ms.text, DEFAULT_EMBEDDING_MODEL)?; Ok(VectorChunkView { id: chunk_id, claim_id: params.claim_id, diff --git a/crates/aver-server/tests/http_routes.rs b/crates/aver-server/tests/http_routes.rs index 9bd51b5..e4f51bc 100644 --- a/crates/aver-server/tests/http_routes.rs +++ b/crates/aver-server/tests/http_routes.rs @@ -93,6 +93,75 @@ async fn oauth_token_route_exchanges_authorization_code_with_pkce() { let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); assert_eq!(json["token_type"], "Bearer"); assert!(json["access_token"].as_str().unwrap().len() > 10); + assert_eq!( + json["expires_in"], + serde_json::json!(aver_server::auth::ACCESS_TOKEN_TTL_SECS), + "token response must carry RFC 6749 §5.1 expires_in", + ); +} + +#[tokio::test] +async fn oauth_token_route_returns_rfc6749_json_errors() { + let dir = tempfile::tempdir().unwrap(); + let config = ServerConfig { + host: "127.0.0.1".to_string(), + port: 3317, + base_url: "https://aver.example.com".to_string(), + memory_dir: dir.path().join("memory").to_string_lossy().to_string(), + auth_db_path: dir.path().join("auth.db").to_string_lossy().to_string(), + cors_origins: Vec::new(), + trusted_auth_header: None, + }; + let app = build_router(config).unwrap(); + + async fn token_request(app: &axum::Router, form: &str) -> (StatusCode, serde_json::Value) { + let response = app + .clone() + .oneshot( + Request::builder() + .method(Method::POST) + .uri("/oauth/token") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .body(Body::from(form.to_string())) + .unwrap(), + ) + .await + .unwrap(); + let status = response.status(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + let json: serde_json::Value = serde_json::from_slice(&body) + .unwrap_or_else(|err| panic!("token error body must be JSON: {err}")); + (status, json) + } + + // Unknown grant type → unsupported_grant_type. + let (status, json) = token_request(&app, "grant_type=client_credentials").await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(json["error"], "unsupported_grant_type"); + + // Bogus authorization code → invalid_grant. + let (status, json) = token_request( + &app, + "grant_type=authorization_code&code=nope&client_id=c&code_verifier=v&redirect_uri=http://localhost:8080/callback", + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(json["error"], "invalid_grant"); + + // Bogus refresh token → invalid_grant. + let (status, json) = token_request(&app, "grant_type=refresh_token&refresh_token=nope").await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(json["error"], "invalid_grant"); + + // Missing required fields → invalid_request. + let (status, json) = token_request(&app, "grant_type=refresh_token").await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(json["error"], "invalid_request"); + let (status, json) = token_request(&app, "grant_type=authorization_code&code=abc").await; + assert_eq!(status, StatusCode::BAD_REQUEST); + assert_eq!(json["error"], "invalid_request"); } #[tokio::test] @@ -130,6 +199,28 @@ async fn protected_health_requires_bearer_token() { .await .unwrap(); assert_eq!(unauthorized.status(), StatusCode::UNAUTHORIZED); + // RFC 6750 §3: 401s must carry a Bearer challenge. + assert_eq!( + unauthorized.headers().get(header::WWW_AUTHENTICATE), + Some(&header::HeaderValue::from_static("Bearer")), + ); + + let wrong_token = app + .clone() + .oneshot( + Request::builder() + .uri("/api/health") + .header(header::AUTHORIZATION, "Bearer wrong-token") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(wrong_token.status(), StatusCode::UNAUTHORIZED); + assert_eq!( + wrong_token.headers().get(header::WWW_AUTHENTICATE), + Some(&header::HeaderValue::from_static("Bearer")), + ); let authorized = app .oneshot( @@ -305,4 +396,9 @@ async fn mcp_route_requires_bearer_token() { .unwrap(); assert_eq!(response.status(), StatusCode::UNAUTHORIZED); + assert_eq!( + response.headers().get(header::WWW_AUTHENTICATE), + Some(&header::HeaderValue::from_static("Bearer")), + "MCP 401s must carry a Bearer challenge for OAuth-capable clients", + ); } diff --git a/crates/aver-server/tests/oauth_consent_flow.rs b/crates/aver-server/tests/oauth_consent_flow.rs index e153735..3b78a63 100644 --- a/crates/aver-server/tests/oauth_consent_flow.rs +++ b/crates/aver-server/tests/oauth_consent_flow.rs @@ -181,6 +181,8 @@ async fn loopback_get_authorize_redirect_uri_mismatch_yields_html_error() { } /// Drives the full consent flow: GET → POST approve → second GET (skip). +/// The approval grants `claims:read` — an empty grant deliberately does NOT +/// skip the screen anymore (see the stuck-loop regression test below). #[tokio::test] async fn approve_decision_records_consent_redirects_with_code_and_skips_screen() { let dir = tempfile::tempdir().unwrap(); @@ -194,7 +196,7 @@ async fn approve_decision_records_consent_redirects_with_code_and_skips_screen() // GET to receive cookie + csrf token. let uri = format!( - "/oauth/authorize?response_type=code&client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&state=stateA", + "/oauth/authorize?response_type=code&client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&state=stateA&scope=claims%3Aread", cid = client_id, ch = challenge, ); @@ -209,9 +211,9 @@ async fn approve_decision_records_consent_redirects_with_code_and_skips_screen() .unwrap(); let csrf = extract_csrf_token(std::str::from_utf8(&body).unwrap()); - // POST decision=approve. + // POST decision=approve with the claims:read checkbox checked. let form = format!( - "client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&state=stateA&csrf_token={csrf}&decision=approve", + "client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&state=stateA&csrf_token={csrf}&decision=approve&scope_selection_present=1&grant_claims_read=claims%3Aread", cid = client_id, ch = challenge, csrf = csrf, @@ -481,6 +483,263 @@ async fn non_loopback_get_authorize_with_trusted_header_is_allowed() { assert!(std::str::from_utf8(&body).unwrap().contains("csrf_token")); } +#[tokio::test] +async fn empty_scope_approval_does_not_trap_user_in_skip_loop() { + // Regression test for the consent empty-grant trap: approving with zero + // scopes records an empty grant, and the skip-screen path must NOT treat + // it as covering — otherwise the user never reaches the consent screen + // again and can never grant real access. + let dir = tempfile::tempdir().unwrap(); + let auth_db_path = dir.path().join("auth.db"); + let _ = AuthDb::open(&auth_db_path).unwrap(); + let redirect = "http://127.0.0.1:3917/callback"; + let client_id = register_client(&auth_db_path, redirect); + let config = base_config(&dir, &auth_db_path); + let app = build_router(config).unwrap(); + let challenge = pkce_s256_challenge("verifier-abc-1234567890"); + + // 1) First GET renders the consent screen. + let uri = format!( + "/oauth/authorize?response_type=code&client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256", + cid = client_id, + ch = challenge, + ); + let mut req = Request::builder().uri(&uri).body(Body::empty()).unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + let session_cookie = extract_session_cookie(response.headers()).unwrap(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + let csrf = extract_csrf_token(std::str::from_utf8(&body).unwrap()); + + // 2) Approve with every checkbox UNCHECKED (zero scopes granted). + let form = format!( + "client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&csrf_token={csrf}&decision=approve&scope_selection_present=1", + cid = client_id, + ch = challenge, + csrf = csrf, + ); + let mut req = Request::builder() + .method(Method::POST) + .uri("/oauth/authorize/decision") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, format!("aver_session={session_cookie}")) + .body(Body::from(form)) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + assert!( + response.status().is_redirection(), + "approval still redirects with a code: {}", + response.status(), + ); + { + let db = AuthDb::open(&auth_db_path).unwrap(); + let consent = db.get_consent("local", &client_id).unwrap().unwrap(); + assert!( + consent.granted_scopes.is_empty(), + "zero-scope approval records an empty grant" + ); + } + + // 3) Second GET: the empty grant must NOT skip the consent screen. + let mut req = Request::builder() + .uri(&uri) + .header(header::COOKIE, format!("aver_session={session_cookie}")) + .body(Body::empty()) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + assert_eq!( + response.status(), + StatusCode::OK, + "empty grant must not skip the consent screen (stuck loop regression)", + ); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + let csrf = extract_csrf_token(std::str::from_utf8(&body).unwrap()); + + // 4) Approve again, this time with claims:read checked. + let form = format!( + "client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&csrf_token={csrf}&decision=approve&scope_selection_present=1&grant_claims_read=claims%3Aread", + cid = client_id, + ch = challenge, + csrf = csrf, + ); + let mut req = Request::builder() + .method(Method::POST) + .uri("/oauth/authorize/decision") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, format!("aver_session={session_cookie}")) + .body(Body::from(form)) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + assert!(response.status().is_redirection()); + + // 5) Third GET requesting claims:read: consent now covers → skip. + let uri_scoped = format!("{uri}&scope=claims%3Aread"); + let mut req = Request::builder() + .uri(&uri_scoped) + .header(header::COOKIE, format!("aver_session={session_cookie}")) + .body(Body::empty()) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.oneshot(req).await.unwrap(); + assert!( + response.status().is_redirection(), + "non-empty covering consent skips the screen: {}", + response.status(), + ); + let location = response + .headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap() + .to_string(); + assert!(location.starts_with("http://127.0.0.1:3917/callback?code=")); +} + +#[tokio::test] +async fn revoke_route_revokes_consent_and_tokens_and_allows_reconsent() { + let dir = tempfile::tempdir().unwrap(); + let auth_db_path = dir.path().join("auth.db"); + let _ = AuthDb::open(&auth_db_path).unwrap(); + let redirect = "http://127.0.0.1:3917/callback"; + let client_id = register_client(&auth_db_path, redirect); + let config = base_config(&dir, &auth_db_path); + let app = build_router(config).unwrap(); + let challenge = pkce_s256_challenge("verifier-abc-1234567890"); + + // Consent to claims:read and mint tokens. + let uri = format!( + "/oauth/authorize?response_type=code&client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&scope=claims%3Aread", + cid = client_id, + ch = challenge, + ); + let mut req = Request::builder().uri(&uri).body(Body::empty()).unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + let session_cookie = extract_session_cookie(response.headers()).unwrap(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + let csrf = extract_csrf_token(std::str::from_utf8(&body).unwrap()); + let form = format!( + "client_id={cid}&redirect_uri=http%3A%2F%2F127.0.0.1%3A3917%2Fcallback&code_challenge={ch}&code_challenge_method=S256&csrf_token={csrf}&decision=approve&scope_selection_present=1&grant_claims_read=claims%3Aread", + cid = client_id, + ch = challenge, + csrf = csrf, + ); + let mut req = Request::builder() + .method(Method::POST) + .uri("/oauth/authorize/decision") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, format!("aver_session={session_cookie}")) + .body(Body::from(form)) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + let location = response + .headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap() + .to_string(); + let code = location + .split("code=") + .nth(1) + .unwrap() + .split('&') + .next() + .unwrap() + .to_string(); + let token_form = format!( + "grant_type=authorization_code&code={code}&client_id={cid}&code_verifier=verifier-abc-1234567890&redirect_uri={redirect}", + cid = client_id, + ); + let req = Request::builder() + .method(Method::POST) + .uri("/oauth/token") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .body(Body::from(token_form)) + .unwrap(); + let response = app.clone().oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); + let access_token = json["access_token"].as_str().unwrap().to_string(); + let refresh_token = json["refresh_token"].as_str().unwrap().to_string(); + + // Revoke without a session cookie is rejected. + let mut req = Request::builder() + .method(Method::POST) + .uri("/oauth/consent/revoke") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .body(Body::from(format!("client_id={client_id}"))) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + + // Revoke with the browser session succeeds. + let mut req = Request::builder() + .method(Method::POST) + .uri("/oauth/consent/revoke") + .header(header::CONTENT_TYPE, "application/x-www-form-urlencoded") + .header(header::COOKIE, format!("aver_session={session_cookie}")) + .body(Body::from(format!("client_id={client_id}"))) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.clone().oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + assert!( + std::str::from_utf8(&body) + .unwrap() + .contains("Consent revoked") + ); + + // Consent row is revoked and the client's tokens are dead. + let db = AuthDb::open(&auth_db_path).unwrap(); + let consent = db.get_consent("local", &client_id).unwrap().unwrap(); + assert!(consent.revoked_at.is_some(), "consent row must be revoked"); + assert!( + db.validate_access_token(&aver_server::auth::hash_token(&access_token)) + .unwrap() + .is_none(), + "revocation invalidates the access token", + ); + assert!( + db.refresh_access_token(&refresh_token).is_err(), + "revocation invalidates the refresh token", + ); + drop(db); + + // The consent screen is reachable again (no skip) for re-consent. + let mut req = Request::builder() + .uri(&uri) + .header(header::COOKIE, format!("aver_session={session_cookie}")) + .body(Body::empty()) + .unwrap(); + req.extensions_mut().insert(ConnectInfo(loopback_addr())); + let response = app.oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + assert!(std::str::from_utf8(&body).unwrap().contains("csrf_token")); +} + #[tokio::test] async fn non_loopback_authorize_decision_requires_matching_trusted_user_session() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/aver-server/tests/refresh_and_cors.rs b/crates/aver-server/tests/refresh_and_cors.rs index 214802b..a02ba45 100644 --- a/crates/aver-server/tests/refresh_and_cors.rs +++ b/crates/aver-server/tests/refresh_and_cors.rs @@ -36,7 +36,55 @@ fn auth_code_exchange_issues_refresh_token_and_allows_refresh_grant() { let refreshed = db.refresh_access_token(&tokens.refresh_token).unwrap(); assert_ne!(refreshed.access_token, tokens.access_token); - assert_eq!(refreshed.refresh_token, tokens.refresh_token); + // Rotation: the refresh grant mints a NEW refresh token and retires the + // presented one. + assert_ne!(refreshed.refresh_token, tokens.refresh_token); + assert!(db.refresh_access_token(&tokens.refresh_token).is_err()); +} + +#[test] +fn refresh_token_reuse_revokes_token_family() { + // RFC 6819 §5.2.2.3: presenting an already-rotated refresh token signals + // theft; every live token of the (user, client) family is revoked. + let dir = tempfile::tempdir().unwrap(); + let db = AuthDb::open(dir.path().join("auth.db")).unwrap(); + let verifier = "verifier"; + let redirect = "http://localhost:8080/callback"; + let code = db + .store_authorization_code( + "client-1", + "user-1", + &aver_server::oauth::pkce_s256_challenge(verifier), + redirect, + &["claims:read".to_string()], + ) + .unwrap(); + let tokens = db + .exchange_authorization_code_for_tokens(&code, "client-1", verifier, redirect) + .unwrap(); + + // Legitimate first refresh rotates the pair. + let rotated = db.refresh_access_token(&tokens.refresh_token).unwrap(); + assert!( + db.validate_access_token(&aver_server::auth::hash_token(&rotated.access_token)) + .unwrap() + .is_some() + ); + + // Attacker (or buggy client) replays the retired token. + let err = db.refresh_access_token(&tokens.refresh_token).unwrap_err(); + assert!( + err.to_string().contains("reuse"), + "expected reuse-detection error, got: {err}" + ); + + // The whole family — rotated access AND refresh tokens — is dead. + assert!( + db.validate_access_token(&aver_server::auth::hash_token(&rotated.access_token)) + .unwrap() + .is_none() + ); + assert!(db.refresh_access_token(&rotated.refresh_token).is_err()); } #[test] diff --git a/crates/aver-server/tests/scope_mcp.rs b/crates/aver-server/tests/scope_mcp.rs index ca4f204..35035ee 100644 --- a/crates/aver-server/tests/scope_mcp.rs +++ b/crates/aver-server/tests/scope_mcp.rs @@ -166,7 +166,6 @@ fn recall_with_scope_filters_by_walk() { let view = tools .recall(RecallParams { query: "uses".to_string(), - alpha: None, hops: None, top_k: Some(10), scope: Some("proj/aver".to_string()), diff --git a/crates/aver-server/tests/scope_resolution.rs b/crates/aver-server/tests/scope_resolution.rs index 80cf344..83a8453 100644 --- a/crates/aver-server/tests/scope_resolution.rs +++ b/crates/aver-server/tests/scope_resolution.rs @@ -80,6 +80,34 @@ fn empty_string_header_is_treated_as_unset() { assert_eq!(resolved.scope, "proj/env"); } +#[test] +fn rejects_empty_path_segments() { + for bad in ["///", "a//b", "/a", "a/", "//"] { + let err = resolve_scope(Some(bad), None, None, None) + .expect_err(&format!("scope {bad:?} with empty segment must reject")); + let chain = format!("{err:?}"); + assert!( + chain.contains("empty path segment") || chain.contains("blank"), + "{bad:?}: {chain}" + ); + } +} + +#[test] +fn rejects_overlong_scope() { + let too_long = "a".repeat(257); + let err = resolve_scope(Some(&too_long), None, None, None) + .expect_err("scope over 256 bytes must reject"); + let chain = format!("{err:?}"); + assert!(chain.contains("256"), "{chain}"); + + let at_max = "a".repeat(256); + assert!( + resolve_scope(Some(&at_max), None, None, None).is_ok(), + "256-byte scope is the accepted boundary" + ); +} + fn _assert_resolved_is_clone_send_sync() {} fn _exercise() { _assert_resolved_is_clone_send_sync::(); diff --git a/crates/aver-server/tests/shared_store.rs b/crates/aver-server/tests/shared_store.rs new file mode 100644 index 0000000..1d0a4c8 --- /dev/null +++ b/crates/aver-server/tests/shared_store.rs @@ -0,0 +1,192 @@ +//! The HTTP server opens ONE memory store at startup and shares it across +//! MCP sessions (per-session `Store::open` would break the single-writer +//! invariant on the SQLite database). These tests prove that two concurrent +//! sessions write into — and read from — the same store. + +use aver_server::{auth::AuthDb, config::ServerConfig, http::build_router}; +use axum::{ + body::Body, + http::{Method, Request, header}, +}; +use tower::ServiceExt; + +const BEARER: &str = "shared-store-test-token"; + +fn base_config(dir: &tempfile::TempDir, auth_db_path: &std::path::Path) -> ServerConfig { + ServerConfig { + host: "127.0.0.1".to_string(), + port: 3317, + base_url: "http://127.0.0.1:3317".to_string(), + memory_dir: dir.path().join("memory").to_string_lossy().to_string(), + auth_db_path: auth_db_path.to_string_lossy().to_string(), + cors_origins: Vec::new(), + trusted_auth_header: None, + } +} + +/// Parses the first JSON-RPC message out of an SSE stream body. Skips +/// priming events whose `data:` line is empty. +fn parse_first_sse_message(body: &[u8]) -> serde_json::Value { + let text = std::str::from_utf8(body).unwrap(); + for line in text.lines() { + let payload = match line.strip_prefix("data:") { + Some(rest) => rest.trim_start(), + None => continue, + }; + if payload.is_empty() { + continue; + } + return serde_json::from_str(payload) + .unwrap_or_else(|err| panic!("invalid SSE data payload {payload:?}: {err}")); + } + panic!("no JSON-bearing `data:` line in SSE body: {text:?}"); +} + +async fn mcp_post(app: &axum::Router, session_id: Option<&str>, body: String) -> (String, String) { + let mut builder = Request::builder() + .method(Method::POST) + .uri("/mcp") + .header(header::AUTHORIZATION, format!("Bearer {BEARER}")) + .header(header::CONTENT_TYPE, "application/json") + .header(header::ACCEPT, "application/json, text/event-stream") + .header(header::HOST, "127.0.0.1"); + if let Some(id) = session_id { + builder = builder + .header("Mcp-Session-Id", id) + .header("MCP-Protocol-Version", "2025-06-18"); + } + let response = app + .clone() + .oneshot(builder.body(Body::from(body)).unwrap()) + .await + .unwrap(); + // Notifications answer 202, requests 200 — both are fine here. + assert!( + response.status().is_success(), + "MCP POST failed: {}", + response.status(), + ); + let returned_session = response + .headers() + .get("Mcp-Session-Id") + .map(|v| v.to_str().unwrap().to_string()) + .unwrap_or_default(); + let body = axum::body::to_bytes(response.into_body(), usize::MAX) + .await + .unwrap(); + (returned_session, String::from_utf8(body.to_vec()).unwrap()) +} + +/// initialize + notifications/initialized; returns the session id. +async fn mcp_open_session(app: &axum::Router) -> String { + let init = serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "test", "version": "0"}, + }, + }) + .to_string(); + let (session_id, _) = mcp_post(app, None, init).await; + assert!(!session_id.is_empty(), "initialize returns a session id"); + + let notif = serde_json::json!({ + "jsonrpc": "2.0", + "method": "notifications/initialized", + }) + .to_string(); + mcp_post(app, Some(&session_id), notif).await; + session_id +} + +async fn mcp_call_tool( + app: &axum::Router, + session_id: &str, + tool: &str, + args: serde_json::Value, +) -> serde_json::Value { + let call = serde_json::json!({ + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": {"name": tool, "arguments": args}, + }) + .to_string(); + let (_, body) = mcp_post(app, Some(session_id), call).await; + parse_first_sse_message(body.as_bytes()) +} + +#[tokio::test] +async fn two_concurrent_sessions_write_into_one_shared_store() { + let dir = tempfile::tempdir().unwrap(); + let auth_db_path = dir.path().join("auth.db"); + let db = AuthDb::open(&auth_db_path).unwrap(); + db.store_access_token_hash( + &aver_server::auth::hash_token(BEARER), + "user-1", + &["claims:read".to_string(), "claims:write".to_string()], + ) + .unwrap(); + drop(db); + + let app = build_router(base_config(&dir, &auth_db_path)).unwrap(); + + // Two live MCP sessions against the same server. + let session_one = mcp_open_session(&app).await; + let session_two = mcp_open_session(&app).await; + assert_ne!(session_one, session_two); + + // Both sessions write a claim concurrently. + let (result_one, result_two) = tokio::join!( + mcp_call_tool( + &app, + &session_one, + "remember_claim", + serde_json::json!({ + "subject": "shared-marker-session-one", + "predicate": "relates_to", + "object": "shared-marker", + }), + ), + mcp_call_tool( + &app, + &session_two, + "remember_claim", + serde_json::json!({ + "subject": "shared-marker-session-two", + "predicate": "relates_to", + "object": "shared-marker", + }), + ), + ); + assert!( + result_one.get("error").is_none(), + "session one write failed: {result_one}", + ); + assert!( + result_two.get("error").is_none(), + "session two write failed: {result_two}", + ); + + // A recall from session two sees BOTH claims: the sessions share one + // store rather than each opening its own connection/database view. + let recall = mcp_call_tool( + &app, + &session_two, + "recall", + serde_json::json!({"query": "shared-marker"}), + ) + .await; + let recall_text = serde_json::to_string(&recall).unwrap(); + assert!( + recall_text.contains("shared-marker-session-one"), + "session two must see session one's claim: {recall_text}", + ); + assert!( + recall_text.contains("shared-marker-session-two"), + "session two must see its own claim: {recall_text}", + ); +} diff --git a/crates/aver-server/tests/tools.rs b/crates/aver-server/tests/tools.rs index d0a30b3..22b5cbc 100644 --- a/crates/aver-server/tests/tools.rs +++ b/crates/aver-server/tests/tools.rs @@ -26,7 +26,6 @@ fn remember_claim_tool_writes_claim_and_recall_returns_it() { let recalled = tools .recall(RecallParams { query: "MCP_tools".to_string(), - alpha: None, hops: None, top_k: Some(5), scope: None, @@ -137,7 +136,6 @@ fn adr0008_five_tool_surface_covers_claim_graph_lifecycle() { let recalled = tools .recall(RecallParams { query: "Stripe".to_string(), - alpha: None, hops: Some(2), top_k: Some(5), scope: None, @@ -307,31 +305,6 @@ fn observation_projection_tools_expose_recall_and_compaction_summary() { ); } -#[test] -fn recall_tool_rejects_alpha_outside_unit_interval() { - let dir = tempfile::tempdir().unwrap(); - let tools = AverTools::open(dir.path()).unwrap(); - - let err = tools - .recall(RecallParams { - query: "Stripe".to_string(), - alpha: Some(1.5), - hops: None, - top_k: Some(5), - scope: None, - scope_walk: None, - agent_id: None, - agent_kind: None, - predicate: None, - predicate_walk: None, - min_confidence: None, - status: None, - }) - .expect_err("invalid alpha should be rejected"); - - assert!(err.to_string().contains("alpha")); -} - #[test] fn add_triple_rejects_confidence_outside_unit_interval() { let dir = tempfile::tempdir().unwrap(); @@ -370,7 +343,6 @@ fn add_triple_persists_valid_confidence_override() { let recalled = tools .recall(RecallParams { query: "PaymentGateway".to_string(), - alpha: None, hops: None, top_k: Some(5), scope: None, @@ -418,7 +390,6 @@ fn recall_tool_rejects_zero_hops() { let err = tools .recall(RecallParams { query: "PaymentGateway".to_string(), - alpha: None, hops: Some(0), top_k: Some(5), scope: None, @@ -443,7 +414,6 @@ fn recall_tool_rejects_zero_top_k() { let err = tools .recall(RecallParams { query: "PaymentGateway".to_string(), - alpha: None, hops: None, top_k: Some(0), scope: None, @@ -479,7 +449,6 @@ fn recall_tool_returns_graph_context_for_entity_query() { let recalled = tools .recall(RecallParams { query: "PaymentGateway".to_string(), - alpha: None, hops: Some(1), top_k: Some(5), scope: None, @@ -523,7 +492,6 @@ fn recall_tool_expands_graph_from_recalled_claim_subject_when_query_is_phrase() let recalled = tools .recall(RecallParams { query: "what depends on PaymentGateway".to_string(), - alpha: None, hops: Some(1), top_k: Some(5), scope: None, @@ -567,7 +535,6 @@ fn recall_tool_reports_confidence_floor_for_returned_triples() { let recalled = tools .recall(RecallParams { query: "PaymentGateway".to_string(), - alpha: None, hops: Some(1), top_k: Some(5), scope: None, @@ -614,7 +581,6 @@ fn recall_tool_confidence_floor_includes_subgraph_edges() { let recalled = tools .recall(RecallParams { query: "status current".to_string(), - alpha: None, hops: Some(1), top_k: Some(1), scope: None,