From 5ae2970d94cc4c7b75ca6d0087fbf0bcfd7e340a Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 25 Mar 2025 15:00:48 +0100 Subject: [PATCH] Ensure client metadata hashing is stable This is done by using the indexmap crate to preserve insertion order for localized fields. --- Cargo.lock | 2 + Cargo.toml | 5 ++ crates/handlers/Cargo.toml | 2 +- crates/oauth2-types/Cargo.toml | 2 + .../src/registration/client_metadata_serde.rs | 49 ++++++++++++------- crates/oauth2-types/src/registration/mod.rs | 9 ++-- 6 files changed, 47 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac70e4431..e91f70268 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4009,6 +4009,8 @@ dependencies = [ "assert_matches", "base64ct", "chrono", + "indexmap 2.8.0", + "insta", "language-tags", "mas-iana", "mas-jose", diff --git a/Cargo.toml b/Cargo.toml index ddeac4da8..3c0bb8b83 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -188,6 +188,11 @@ version = "0.27.5" features = ["http1", "http2"] default-features = false +# HashMap which preserves insertion order +[workspace.dependencies.indexmap] +version = "2.8.0" +features = ["serde"] + # Snapshot testing [workspace.dependencies.insta] version = "1.42.2" diff --git a/crates/handlers/Cargo.toml b/crates/handlers/Cargo.toml index e2ab6a15a..bd78f9dc2 100644 --- a/crates/handlers/Cargo.toml +++ b/crates/handlers/Cargo.toml @@ -74,7 +74,7 @@ chrono.workspace = true elliptic-curve.workspace = true hex.workspace = true governor.workspace = true -indexmap = "2.8.0" +indexmap.workspace = true pkcs8.workspace = true psl = "2.1.96" sha2.workspace = true diff --git a/crates/oauth2-types/Cargo.toml b/crates/oauth2-types/Cargo.toml index 6efaed344..b0f4ffa69 100644 --- a/crates/oauth2-types/Cargo.toml +++ b/crates/oauth2-types/Cargo.toml @@ -21,9 +21,11 @@ serde_with = { version = "3.12.0", features = ["chrono"] } chrono.workspace = true sha2.workspace = true thiserror.workspace = true +indexmap.workspace = true mas-iana.workspace = true mas-jose.workspace = true [dev-dependencies] assert_matches = "1.5.0" +insta.workspace = true diff --git a/crates/oauth2-types/src/registration/client_metadata_serde.rs b/crates/oauth2-types/src/registration/client_metadata_serde.rs index 2e5903ac2..3483fecdd 100644 --- a/crates/oauth2-types/src/registration/client_metadata_serde.rs +++ b/crates/oauth2-types/src/registration/client_metadata_serde.rs @@ -4,9 +4,10 @@ // SPDX-License-Identifier: AGPL-3.0-only // Please see LICENSE in the repository root for full details. -use std::{borrow::Cow, collections::HashMap}; +use std::borrow::Cow; use chrono::Duration; +use indexmap::IndexMap; use language_tags::LanguageTag; use mas_iana::{ jose::{JsonWebEncryptionAlg, JsonWebEncryptionEnc, JsonWebSignatureAlg}, @@ -45,18 +46,18 @@ impl Localized { } fn deserialize( - map: &mut HashMap, Value>>, + map: &mut IndexMap, Value>>, field_name: &'static str, ) -> Result, serde_json::Error> where T: DeserializeOwned, { - let Some(map) = map.remove(field_name) else { + let Some(map) = map.shift_remove(field_name) else { return Ok(None); }; let mut non_localized = None; - let mut localized = HashMap::with_capacity(map.len() - 1); + let mut localized = IndexMap::with_capacity(map.len() - 1); for (k, v) in map { let value = serde_json::from_value(v)?; @@ -350,8 +351,8 @@ impl<'de> Deserialize<'de> for ClientMetadataLocalizedFields { where D: serde::Deserializer<'de>, { - let map = HashMap::, Value>::deserialize(deserializer)?; - let mut new_map: HashMap, Value>> = HashMap::new(); + let map = IndexMap::, Value>::deserialize(deserializer)?; + let mut new_map: IndexMap, Value>> = IndexMap::new(); for (k, v) in map { let (prefix, lang) = if let Some((prefix, lang)) = k.split_once('#') { @@ -392,6 +393,8 @@ impl<'de> Deserialize<'de> for ClientMetadataLocalizedFields { #[cfg(test)] mod tests { + use insta::assert_yaml_snapshot; + use super::*; #[test] @@ -464,16 +467,28 @@ mod tests { .validate() .unwrap(); - assert_eq!( - serde_json::to_value(metadata).unwrap(), - serde_json::json!({ - "redirect_uris": ["http://localhost/oidc"], - "client_name": "Postbox", - "client_name#fr": "Boîte à lettres", - "client_uri": "https://localhost/", - "client_uri#fr": "https://localhost/fr", - "client_uri#de": "https://localhost/de", - }) - ); + assert_yaml_snapshot!(metadata, @r###" + redirect_uris: + - "http://localhost/oidc" + client_name: Postbox + "client_name#fr": Boîte à lettres + client_uri: "https://localhost/" + "client_uri#fr": "https://localhost/fr" + "client_uri#de": "https://localhost/de" + "###); + + // Do a roundtrip, we should get the same metadata back with the same order + let metadata: ClientMetadata = + serde_json::from_value(serde_json::to_value(metadata).unwrap()).unwrap(); + let metadata = metadata.validate().unwrap(); + assert_yaml_snapshot!(metadata, @r###" + redirect_uris: + - "http://localhost/oidc" + client_name: Postbox + "client_name#fr": Boîte à lettres + client_uri: "https://localhost/" + "client_uri#fr": "https://localhost/fr" + "client_uri#de": "https://localhost/de" + "###); } } diff --git a/crates/oauth2-types/src/registration/mod.rs b/crates/oauth2-types/src/registration/mod.rs index 0290c08df..1cd7d43b2 100644 --- a/crates/oauth2-types/src/registration/mod.rs +++ b/crates/oauth2-types/src/registration/mod.rs @@ -8,9 +8,10 @@ //! //! [Dynamic Client Registration]: https://openid.net/specs/openid-connect-registration-1_0.html -use std::{collections::HashMap, ops::Deref}; +use std::ops::Deref; use chrono::{DateTime, Duration, Utc}; +use indexmap::IndexMap; use language_tags::LanguageTag; use mas_iana::{ jose::{JsonWebEncryptionAlg, JsonWebEncryptionEnc, JsonWebSignatureAlg}, @@ -58,7 +59,7 @@ pub const DEFAULT_ENCRYPTION_ENC_ALGORITHM: &JsonWebEncryptionEnc = #[derive(Debug, Clone, PartialEq, Eq)] pub struct Localized { non_localized: T, - localized: HashMap, + localized: IndexMap, } impl Localized { @@ -104,8 +105,8 @@ impl Localized { } } -impl From<(T, HashMap)> for Localized { - fn from(t: (T, HashMap)) -> Self { +impl From<(T, IndexMap)> for Localized { + fn from(t: (T, IndexMap)) -> Self { Localized { non_localized: t.0, localized: t.1,