From e95e6533797bb3644f676a6e5ccaa35f9831f2ee Mon Sep 17 00:00:00 2001 From: Ginger Date: Mon, 8 Jun 2026 10:37:44 -0400 Subject: [PATCH] fix: Use RFC-compliant error responses for OAuth endpoints --- src/api/client/oauth/register_client.rs | 2 +- src/api/client/oauth/token.rs | 4 +- src/service/oauth/grant.rs | 51 ++++++++++++++++++++++++- src/service/oauth/mod.rs | 44 +++++++++++---------- 4 files changed, 76 insertions(+), 25 deletions(-) diff --git a/src/api/client/oauth/register_client.rs b/src/api/client/oauth/register_client.rs index 3d43edcae..26eec3c89 100644 --- a/src/api/client/oauth/register_client.rs +++ b/src/api/client/oauth/register_client.rs @@ -22,7 +22,7 @@ pub(crate) async fn register_client_route( .oauth .register_client(&metadata) .await - .map_err(|err| (StatusCode::BAD_REQUEST, err.to_owned()).into_response())?; + .map_err(|err| (StatusCode::BAD_REQUEST, Json(err)).into_response())?; Ok(Json(RegisteredClient { client_id, metadata }).into_response()) } diff --git a/src/api/client/oauth/token.rs b/src/api/client/oauth/token.rs index fc6186c47..35660810a 100644 --- a/src/api/client/oauth/token.rs +++ b/src/api/client/oauth/token.rs @@ -8,7 +8,7 @@ pub(crate) async fn token_route( ) -> impl IntoResponse { match services.oauth.issue_token(request).await { | Ok(response) => Ok(Json(response)), - | Err(err) => Err((StatusCode::BAD_REQUEST, err.message())), + | Err(err) => Err((StatusCode::BAD_REQUEST, Json(err))), } } @@ -18,6 +18,6 @@ pub(crate) async fn revoke_token_route( ) -> impl IntoResponse { match services.oauth.revoke_token(request.token).await { | Ok(()) => Ok(StatusCode::OK), - | Err(err) => Err((StatusCode::BAD_REQUEST, err.message())), + | Err(err) => Err((StatusCode::BAD_REQUEST, Json(err))), } } diff --git a/src/service/oauth/grant.rs b/src/service/oauth/grant.rs index 156b0f851..67a25b299 100644 --- a/src/service/oauth/grant.rs +++ b/src/service/oauth/grant.rs @@ -1,4 +1,11 @@ -use std::{collections::BTreeSet, fmt::Debug, hash::Hash, mem::discriminant}; +use std::{ + borrow::Cow, + collections::BTreeSet, + error::Error, + fmt::{Debug, Display}, + hash::Hash, + mem::discriminant, +}; use regex::Regex; use ruma::OwnedDeviceId; @@ -63,7 +70,7 @@ impl Hash for Scope { fn hash(&self, state: &mut H) { discriminant(self).hash(state); } } -impl std::fmt::Display for Scope { +impl Display for Scope { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let urn = match self { | Self::ClientApi => "urn:matrix:client:api:*".to_owned(), @@ -111,6 +118,46 @@ pub fn to_scopes(&self) -> Result, String> { } } +#[derive(Serialize, Debug, Clone)] +pub struct OAuthError { + pub error: ErrorCode, + pub error_description: Cow<'static, str>, +} + +impl OAuthError { + pub const fn invalid_request(error_description: &'static str) -> Self { + Self { + error: ErrorCode::InvalidRequest, + error_description: Cow::Borrowed(error_description), + } + } + + pub const fn invalid_grant(error_description: &'static str) -> Self { + Self { + error: ErrorCode::InvalidGrant, + error_description: Cow::Borrowed(error_description), + } + } +} + +impl Display for OAuthError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "OAuth error {:?}: {}", self.error, self.error_description) + } +} + +impl Error for OAuthError {} + +#[derive(Serialize, Debug, Clone, Copy, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ErrorCode { + InvalidRequest, + AccessDenied, + InvalidScope, + InvalidGrant, + InvalidClientMetadata, +} + #[derive(Serialize)] pub struct AuthorizationCodeResponse { pub state: String, diff --git a/src/service/oauth/mod.rs b/src/service/oauth/mod.rs index d495e03ad..c0c25f8a4 100644 --- a/src/service/oauth/mod.rs +++ b/src/service/oauth/mod.rs @@ -6,7 +6,7 @@ use base64::Engine; use conduwuit::{ - Err, Result, err, info, + Result, info, utils::{self, hash::sha256}, }; use database::{Deserialized, Json, Map}; @@ -20,8 +20,8 @@ oauth::{ client_metadata::{ApplicationType, ClientMetadata, ResponseType}, grant::{ - AuthorizationCodeQuery, AuthorizationCodeResponse, CodeChallengeMethod, ResponseMode, - Scope, TokenRequest, TokenResponse, TokenType, + AuthorizationCodeQuery, AuthorizationCodeResponse, CodeChallengeMethod, ErrorCode, + OAuthError, ResponseMode, Scope, TokenRequest, TokenResponse, TokenType, }, }, users, @@ -131,11 +131,11 @@ impl Service { fn generate_token() -> String { utils::random_string(Self::RANDOM_TOKEN_LENGTH) } - pub async fn register_client( - &self, - metadata: &ClientMetadata, - ) -> Result { - metadata.validate()?; + pub async fn register_client(&self, metadata: &ClientMetadata) -> Result { + metadata.validate().map_err(|error| OAuthError { + error: ErrorCode::InvalidClientMetadata, + error_description: error.into(), + })?; let client_id = base64::prelude::BASE64_STANDARD .encode(sha256::hash(serde_json::to_string(metadata).unwrap().as_bytes())); @@ -263,7 +263,7 @@ pub async fn request_authorization_code( Ok(redirect_uri) } - pub async fn issue_token(&self, request: TokenRequest) -> Result { + pub async fn issue_token(&self, request: TokenRequest) -> Result { match request { | TokenRequest::AuthorizationCode { code, @@ -277,17 +277,17 @@ pub async fn issue_token(&self, request: TokenRequest) -> Result .remove(&code) .filter(|grant| grant.is_valid_for(&client_id)) else { - return Err!("Invalid code"); + return Err(OAuthError::invalid_grant("Invalid authorization code")); }; if redirect_uri != pending_grant.expected_redirect_uri { - return Err!("Unexpected redirect uri"); + return Err(OAuthError::invalid_grant("Invalid redirect URI")); } let expected_code_challenge = base64::prelude::BASE64_URL_SAFE_NO_PAD.encode(sha256::hash(&code_verifier)); if expected_code_challenge != pending_grant.code_challenge { - return Err!("Invalid code challenge"); + return Err(OAuthError::invalid_grant("Invalid code challenge")); } self.create_session( @@ -303,7 +303,7 @@ pub async fn issue_token(&self, request: TokenRequest) -> Result } } - pub async fn revoke_token(&self, token: String) -> Result<()> { + pub async fn revoke_token(&self, token: String) -> Result<(), OAuthError> { let (user_id, device_id) = if let Ok(refresh_token_info) = self .db .refreshtoken_refreshtokeninfo @@ -317,7 +317,7 @@ pub async fn revoke_token(&self, token: String) -> Result<()> { { (user_id, device_id) } else { - return Err!("Invalid token"); + return Err(OAuthError::invalid_grant("Invalid access or refersh token")); }; // This will also call [`Self::remove_session`] @@ -335,7 +335,7 @@ async fn create_session( requested_scopes: BTreeSet, client_name: Option, client_id: String, - ) -> Result { + ) -> Result { let access_token = Self::generate_token(); let refresh_token = Self::generate_token(); @@ -348,7 +348,7 @@ async fn create_session( None } }) - .ok_or_else(|| err!("No device ID scope supplied"))?; + .ok_or_else(|| OAuthError::invalid_grant("No device ID scope supplied"))?; self.services .users @@ -360,7 +360,10 @@ async fn create_session( client_name, None, ) - .await?; + .await + // This can only panic if the authorizing user suffered a spontaneous existence + // failure during authentication, which should(?) be impossible(?) + .expect("failed to create device"); self.db.userdeviceid_oauthsessioninfo.put( (&authorizing_user, device_id), @@ -401,7 +404,7 @@ async fn refresh_session( &self, client_id: String, refresh_token: String, - ) -> Result { + ) -> Result { let Some(refresh_token_info) = self .db .refreshtoken_refreshtokeninfo @@ -410,7 +413,7 @@ async fn refresh_session( .deserialized::() .ok() else { - return Err!("Invalid refresh token"); + return Err(OAuthError::invalid_grant("Invalid refresh token")); }; assert_eq!(&client_id, &refresh_token_info.client_id, "refresh token client id mismatch"); @@ -440,7 +443,8 @@ async fn refresh_session( &new_access_token, Some(Self::ACCESS_TOKEN_MAX_AGE), ) - .await?; + .await + .expect("should be able to set token"); self.db.userdeviceid_oauthsessioninfo.put( (&refresh_token_info.user_id, &refresh_token_info.device_id),