Make PKCE implementation compliant with RFC7636

This checks for the PKCE code_verifier length as well as the characters
used. It also give better errors when the PKCE verifier is invalid.

Fixes #316

crates/data-model/src/lib.rs

@@ -36,7 +36,7 @@ pub use self::{
     },
     oauth2::{
         AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, Client,
-        InvalidRedirectUriError, JwksOrJwksUri, Pkce, Session,
+        InvalidRedirectUriError, JwksOrJwksUri, Pkce, PkceVerificationError, Session,
     },
     tokens::{AccessToken, RefreshToken, TokenFormatError, TokenType},
     traits::{StorageBackend, StorageBackendMarker},

crates/data-model/src/oauth2/authorization_grant.rs

@@ -24,6 +24,21 @@ use url::Url;
 use super::{client::Client, session::Session};
 use crate::{traits::StorageBackend, StorageBackendMarker};
 
+#[derive(Debug, Error)]
+pub enum PkceVerificationError {
+    #[error("code_verifier should be at least 43 characters long")]
+    TooShort,
+
+    #[error("code_verifier should be at most 128 characters long")]
+    TooLong,
+
+    #[error("code_verifier contains invalid characters")]
+    InvalidCharacters,
+
+    #[error("challenge verification failed")]
+    VerificationFailed,
+}
+
 #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
 pub struct Pkce {
     pub challenge_method: PkceCodeChallengeMethod,
@@ -39,9 +54,27 @@ impl Pkce {
         }
     }
 
-    #[must_use]
+    pub fn verify(&self, verifier: &str) -> Result<(), PkceVerificationError> {
-    pub fn verify(&self, verifier: &str) -> bool {
+        if verifier.len() < 43 {
-        self.challenge_method.verify(&self.challenge, verifier)
+            return Err(PkceVerificationError::TooShort);
+        }
+
+        if verifier.len() > 43 {
+            return Err(PkceVerificationError::TooLong);
+        }
+
+        if verifier
+            .chars()
+            .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '.' || c == '_' || c == '~')
+        {
+            return Err(PkceVerificationError::InvalidCharacters);
+        }
+
+        if !self.challenge_method.verify(&self.challenge, verifier) {
+            return Err(PkceVerificationError::VerificationFailed);
+        }
+
+        Ok(())
     }
 }
 

crates/data-model/src/oauth2/mod.rs

@@ -17,7 +17,9 @@ pub(self) mod client;
 pub(self) mod session;
 
 pub use self::{
-    authorization_grant::{AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, Pkce},
+    authorization_grant::{
+        AuthorizationCode, AuthorizationGrant, AuthorizationGrantStage, Pkce, PkceVerificationError,
+    },
     client::{Client, InvalidRedirectUriError, JwksOrJwksUri},
     session::Session,
 };

crates/handlers/src/oauth2/token.rs

@@ -22,7 +22,7 @@ use headers::{CacheControl, HeaderMap, HeaderMapExt, Pragma};
 use hyper::StatusCode;
 use mas_axum_utils::client_authorization::{ClientAuthorization, CredentialsVerificationError};
 use mas_config::Encrypter;
-use mas_data_model::{AuthorizationGrantStage, Client, TokenType};
+use mas_data_model::{AuthorizationGrantStage, Client, PkceVerificationError, TokenType};
 use mas_iana::jose::JsonWebSignatureAlg;
 use mas_jose::{
     claims::{self, ClaimError},
@@ -86,6 +86,9 @@ pub(crate) enum RouteError {
     #[error("bad request")]
     BadRequest,
 
+    #[error("pkce verification failed")]
+    PkceVerification(#[from] PkceVerificationError),
+
     #[error("client not found")]
     ClientNotFound,
 
@@ -129,6 +132,10 @@ impl IntoResponse for RouteError {
                 (StatusCode::INTERNAL_SERVER_ERROR, Json(SERVER_ERROR))
             }
             Self::BadRequest => (StatusCode::BAD_REQUEST, Json(INVALID_REQUEST)),
+            Self::PkceVerification(err) => (
+                StatusCode::BAD_REQUEST,
+                Json(INVALID_GRANT.with_description(format!("PKCE verification failed: {err}"))),
+            ),
             Self::ClientNotFound | Self::ClientCredentialsVerification(_) => {
                 (StatusCode::UNAUTHORIZED, Json(INVALID_CLIENT))
             }
@@ -274,9 +281,7 @@ async fn authorization_code_grant(
         (Some(_), None) | (None, Some(_)) => return Err(RouteError::BadRequest),
         // If we have both, we need to check the code validity
         (Some(pkce), Some(verifier)) => {
-            if !pkce.verify(verifier) {
+            pkce.verify(verifier)?;
-                return Err(RouteError::BadRequest);
-            }
         }
     };
 

crates/oauth2-types/src/errors.rs

@@ -12,10 +12,12 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#[derive(serde::Serialize)]
+use std::borrow::Cow;
+
+#[derive(serde::Serialize, Clone)]
 pub struct ClientError {
     pub error: &'static str,
-    pub error_description: &'static str,
+    pub error_description: Cow<'static, str>,
 }
 
 impl ClientError {
@@ -23,7 +25,15 @@ impl ClientError {
     pub const fn new(error: &'static str, error_description: &'static str) -> Self {
         Self {
             error,
-            error_description,
+            error_description: Cow::Borrowed(error_description),
+        }
+    }
+
+    #[must_use]
+    pub const fn with_description(&self, description: String) -> Self {
+        Self {
+            error: self.error,
+            error_description: Cow::Owned(description),
         }
     }
 }