Reject malformed registered claims instead of dropping them

CHANGELOG.md

@@ -35,6 +35,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   crate's own `verify` would reject. This state is only reachable by manually
   constructing a `Token` via `Token::new` without an algorithm.
 
+- `Token::from_bytes` rejects malformed registered claims (RFC 8392 §3.1) with
+  the new `Error::InvalidRegisteredClaim`, instead of silently dropping them.
+  Previously a negative `exp` was cast to a far-future time, so the token never
+  expired, and an `iss`, `sub` or `aud` of the wrong type was dropped, so it
+  passed `expected_issuer` / `expected_audience` unless the claim was required.
+  NumericDates (`exp`, `nbf`, `iat`) must be non-negative integers, and `cti`
+  must be a byte string; a text `cti`, previously tolerated, is now rejected.
+- An array-valued `aud` is now checked: `expected_audience` must match one of
+  its entries. Previously such tokens passed any expected audience. Use the new
+  `Token::audiences()` to read all entries.
+- `RegisteredClaims::from_map` no longer casts a negative NumericDate to a
+  far-future time; it skips it. The new `RegisteredClaims::try_from_map` and
+  `Claims::try_from_map` are the strict variants.
+
 ### Security
 
 - ES256 signatures are now produced in canonical "low-S" form, and `verify`

FOLLOWUPS.md

@@ -52,30 +52,18 @@ write-only.
 
 ## Data model (separate PR — breaking)
 
-### Array-valued `aud` is not readable through the typed API
-
-`RegisteredClaims.aud` is `Option<String>` and cannot represent an `aud`
-(CWT key 3) encoded as a CBOR **array** of audiences, which RFC 8392 / RFC 7519
-permit. On decode, `Claims::from_map` silently drops the array-valued `aud` and
-`Token::audience()` returns `None`.
-
-The **correctness** half is already handled: `get_payload_bytes` reuses the
-producer's original signed payload bytes (via `baseline_payload_projection`), so
-such tokens still **verify** and **round-trip byte-faithfully** even though the
-claim is invisible to the typed API (see `test_aud_as_array_verifies_and_roundtrips`
-and `test_cti_as_text_verifies_and_roundtrips` in `src/tests.rs`). The urgent
-interop/availability regression — valid tokens failing verification — is
-resolved.
-
-- **Deferred (not done):** making the array audience *readable* through the
-  typed API. That requires changing the type of `RegisteredClaims.aud` (and
+### Array-valued `aud` is not representable in `RegisteredClaims`
+
+`RegisteredClaims.aud` is `Option<String>`, so an `aud` (CWT key 3) encoded as
+a CBOR array of audiences, which RFC 8392 / RFC 7519 permit, leaves it `None`.
+Such tokens still verify and round-trip byte-faithfully. The entries are
+readable through `Token::audiences()`, and `expected_audience` matches if any
+entry does (see `test_aud_as_array_verifies_and_roundtrips`).
+
+- **Deferred (not done):** representing array audiences in `RegisteredClaims`
+  itself. That requires changing the type of `RegisteredClaims.aud` (and
   `with_audience` / `audience()`), a breaking public API change.
-- **Why deferred:** exposing the value is a nicety with a breaking cost; revisit
-  only when bumping a breaking version, and only if a consumer actually needs to
-  inspect array audiences. The same lossy-but-now-safe shape affects
-  non-conformant producers (e.g. `cti` as text); those are malformed and not
-  worth a data-model change.
-- Location: `src/claims.rs` — `RegisteredClaims` / `Claims::from_map`.
+- Location: `src/claims.rs` — `RegisteredClaims`.
 
 ## Performance (hot path)
 

src/claims.rs

@@ -10,6 +10,7 @@
 //! Claims are used to convey information about the token subject and context, and can be used for
 //! authorization decisions by the token verifier.
 
+use crate::error::Error;
 use crate::header::CborValue;
 use std::collections::BTreeMap;
 
@@ -389,9 +390,55 @@ impl RegisteredClaims {
         map
     }
 
-    /// Extracts registered claims from a claims map.
+    /// Strictly decode registered claims from a claims map.
+    ///
+    /// Unlike [`RegisteredClaims::from_map`], which skips values of the wrong
+    /// type, this rejects any registered claim that is present but malformed,
+    /// so a claim that would restrict the token can never be silently dropped:
+    ///
+    /// - `iss` and `sub` must be text strings.
+    /// - `aud` must be a text string, or an array of text strings (exposed by
+    ///   [`Token::audiences`](crate::Token::audiences); `aud` here is then `None`).
+    /// - `exp`, `nbf`, and `iat` must be non-negative integer NumericDates.
+    /// - `cti` must be a byte string.
+    ///
+    /// `Token::from_bytes` uses this, and returns
+    /// [`Error::InvalidRegisteredClaim`] on failure.
+    pub fn try_from_map(map: &ClaimsMap) -> Result<Self, Error> {
+        Ok(Self {
+            iss: text_claim(map, keys::ISS, "iss")?,
+            sub: text_claim(map, keys::SUB, "sub")?,
+            aud: match map.get(&keys::AUD) {
+                None => None,
+                Some(CborValue::Text(aud)) => Some(aud.clone()),
+                Some(CborValue::Array(_)) => {
+                    audience_list(map)?;
+                    None
+                }
+                Some(_) => {
+                    return Err(invalid(
+                        "aud",
+                        "must be a text string or array of text strings",
+                    ))
+                }
+            },
+            exp: numeric_date(map, keys::EXP, "exp")?,
+            nbf: numeric_date(map, keys::NBF, "nbf")?,
+            iat: numeric_date(map, keys::IAT, "iat")?,
+            cti: match map.get(&keys::CTI) {
+                None => None,
+                Some(CborValue::Bytes(cti)) => Some(cti.clone()),
+                Some(_) => return Err(invalid("cti", "must be a byte string")),
+            },
+        })
+    }
+
+    /// Extracts registered claims from a claims map, skipping any that are
+    /// malformed.
     ///
-    /// This method is primarily used internally for token decoding.
+    /// This is lenient: a claim of the wrong type or with an invalid value is
+    /// left as `None`. Use [`RegisteredClaims::try_from_map`] (as
+    /// `Token::from_bytes` does) to reject malformed claims instead.
     ///
     /// # Example
     ///
@@ -422,16 +469,18 @@ impl RegisteredClaims {
             claims.aud = Some(aud.clone());
         }
 
+        // Negative NumericDates are skipped like any other invalid value
+        // (casting would turn exp = -1 into "never expires").
         if let Some(CborValue::Integer(exp)) = map.get(&keys::EXP) {
-            claims.exp = Some(*exp as u64);
+            claims.exp = u64::try_from(*exp).ok();
         }
 
         if let Some(CborValue::Integer(nbf)) = map.get(&keys::NBF) {
-            claims.nbf = Some(*nbf as u64);
+            claims.nbf = u64::try_from(*nbf).ok();
         }
 
         if let Some(CborValue::Integer(iat)) = map.get(&keys::IAT) {
-            claims.iat = Some(*iat as u64);
+            claims.iat = u64::try_from(*iat).ok();
         }
 
         if let Some(CborValue::Bytes(cti)) = map.get(&keys::CTI) {
@@ -707,6 +756,14 @@ impl Claims {
         map
     }
 
+    /// Strictly decode claims from a map, rejecting malformed registered
+    /// claims (see [`RegisteredClaims::try_from_map`])
+    pub fn try_from_map(map: &ClaimsMap) -> Result<Self, Error> {
+        let mut claims = Self::from_map(map);
+        claims.registered = RegisteredClaims::try_from_map(map)?;
+        Ok(claims)
+    }
+
     /// Creates a Claims struct from a claims map.
     ///
     /// This method is primarily used internally for token decoding.
@@ -742,3 +799,152 @@ impl Claims {
         Self { registered, custom }
     }
 }
+
+fn invalid(claim: &str, reason: &str) -> Error {
+    Error::InvalidRegisteredClaim(format!("{claim} {reason}"))
+}
+
+fn text_claim(map: &ClaimsMap, key: i32, name: &str) -> Result<Option<String>, Error> {
+    match map.get(&key) {
+        None => Ok(None),
+        Some(CborValue::Text(s)) => Ok(Some(s.clone())),
+        Some(_) => Err(invalid(name, "must be a text string")),
+    }
+}
+
+/// A NumericDate (RFC 8392 §2): seconds since the Unix epoch, which must not
+/// be negative.
+fn numeric_date(map: &ClaimsMap, key: i32, name: &str) -> Result<Option<u64>, Error> {
+    match map.get(&key) {
+        None => Ok(None),
+        Some(CborValue::Integer(i)) => u64::try_from(*i)
+            .map(Some)
+            .map_err(|_| invalid(name, "must not be negative")),
+        Some(_) => Err(invalid(name, "must be an integer NumericDate")),
+    }
+}
+
+/// The audiences of an array-valued `aud` claim, or `None` if `aud` is absent
+/// or a single text string.
+pub(crate) fn audience_list(map: &ClaimsMap) -> Result<Option<Vec<String>>, Error> {
+    match map.get(&keys::AUD) {
+        Some(CborValue::Array(items)) => items
+            .iter()
+            .map(|item| match item {
+                CborValue::Text(aud) => Ok(aud.clone()),
+                _ => Err(invalid("aud", "array must contain only text strings")),
+            })
+            .collect::<Result<Vec<_>, _>>()
+            .map(Some),
+        _ => Ok(None),
+    }
+}
+
+#[cfg(test)]
+mod strict_decoding_tests {
+    use super::keys;
+    use crate::{Algorithm, CborValue, Error, Token, TokenBuilder, VerificationOptions};
+
+    const KEY: &[u8] = b"strict-claims-key";
+
+    /// Sign a token whose registered claim `key` carries `value` verbatim.
+    fn token_bytes_with(key: i32, value: CborValue) -> Vec<u8> {
+        TokenBuilder::new()
+            .algorithm(Algorithm::HmacSha256)
+            .custom_cbor(key, value)
+            .sign(KEY)
+            .unwrap()
+            .to_bytes()
+            .unwrap()
+    }
+
+    fn rejects(key: i32, value: CborValue) -> bool {
+        matches!(
+            Token::from_bytes(&token_bytes_with(key, value)),
+            Err(Error::InvalidRegisteredClaim(_))
+        )
+    }
+
+    #[test]
+    fn test_negative_numeric_dates_are_rejected() {
+        for key in [keys::EXP, keys::NBF, keys::IAT] {
+            assert!(rejects(key, CborValue::Integer(-1)), "key {key}");
+        }
+    }
+
+    #[test]
+    fn test_non_integer_numeric_dates_are_rejected() {
+        for key in [keys::EXP, keys::NBF, keys::IAT] {
+            assert!(
+                rejects(key, CborValue::Text("2000000000".into())),
+                "key {key}"
+            );
+            assert!(rejects(key, CborValue::Bytes(vec![1])), "key {key}");
+            assert!(rejects(key, CborValue::Null), "key {key}");
+        }
+    }
+
+    #[test]
+    fn test_text_claims_of_wrong_type_are_rejected() {
+        for key in [keys::ISS, keys::SUB] {
+            assert!(
+                rejects(key, CborValue::Bytes(b"issuer".to_vec())),
+                "key {key}"
+            );
+            assert!(rejects(key, CborValue::Integer(1)), "key {key}");
+        }
+        assert!(rejects(keys::AUD, CborValue::Bytes(b"aud".to_vec())));
+        assert!(rejects(
+            keys::AUD,
+            CborValue::Array(vec![CborValue::Text("ok".into()), CborValue::Integer(1)])
+        ));
+        assert!(rejects(keys::CTI, CborValue::Integer(7)));
+    }
+
+    #[test]
+    fn test_well_formed_claims_still_decode() {
+        let token = Token::from_bytes(&token_bytes_with(
+            keys::EXP,
+            CborValue::Integer(4_000_000_000),
+        ))
+        .unwrap();
+        assert_eq!(token.expiration(), Some(4_000_000_000));
+        let token = Token::from_bytes(&token_bytes_with(
+            keys::ISS,
+            CborValue::Text("issuer".into()),
+        ))
+        .unwrap();
+        assert_eq!(token.issuer(), Some("issuer"));
+    }
+
+    #[test]
+    fn test_array_audience_is_matched_by_any_entry() {
+        let token = Token::from_bytes(&token_bytes_with(
+            keys::AUD,
+            CborValue::Array(vec![
+                CborValue::Text("relay-a".into()),
+                CborValue::Text("relay-b".into()),
+            ]),
+        ))
+        .unwrap();
+        assert_eq!(token.audience(), None);
+        assert_eq!(token.audiences(), vec!["relay-a", "relay-b"]);
+
+        let check =
+            |aud: &str| token.verify_claims(&VerificationOptions::new().expected_audience(aud));
+        assert!(check("relay-b").is_ok());
+        // Previously an array aud was dropped, so any expected audience passed
+        assert!(matches!(check("relay-c"), Err(Error::InvalidAudience)));
+    }
+
+    #[test]
+    fn test_lenient_from_map_is_unchanged() {
+        let mut map = super::ClaimsMap::new();
+        map.insert(keys::EXP, CborValue::Integer(-1));
+        map.insert(keys::ISS, CborValue::Bytes(b"x".to_vec()));
+        let claims = super::RegisteredClaims::from_map(&map);
+        assert_eq!(claims.exp, None);
+        assert_eq!(claims.iss, None);
+        assert!(super::RegisteredClaims::try_from_map(&map).is_err());
+    }
+}

src/error.rs

@@ -63,6 +63,11 @@ pub enum Error {
     #[error("Invalid claim value: {0}")]
     InvalidClaimValue(String),
 
+    /// A registered claim (RFC 8392 §3.1) is present but has the wrong type or
+    /// an invalid value
+    #[error("Invalid registered claim: {0}")]
+    InvalidRegisteredClaim(String),
+
     /// Invalid URI claim
     #[error("Invalid URI claim: {0}")]
     InvalidUriClaim(String),

src/tests.rs

@@ -778,6 +778,9 @@ fn test_aud_as_array_verifies_and_roundtrips() {
         "array-valued aud cannot be represented by Option<String> and is dropped"
     );
 
+    // The entries are readable through `Token::audiences`.
+    assert_eq!(token.audiences(), vec!["aud-one", "aud-two"]);
+
     // Regression: an unmutated token must still verify against the original key.
     assert!(
         token.verify_with_key(VerifyingKey::HmacSha256(key)).is_ok(),
@@ -831,10 +834,9 @@ fn test_aud_as_array_mutation_is_reflected() {
 }
 
 #[test]
-fn test_cti_as_text_verifies_and_roundtrips() {
-    // A second lossy trigger: `cti` (key 7) carried as text rather than bytes.
-    // `RegisteredClaims::from_map` only accepts `cti` as bytes, so it is dropped,
-    // yet the token must still verify and round-trip byte-faithfully.
+fn test_cti_as_text_is_rejected() {
+    // RFC 8392 §3.1.7: `cti` (key 7) is a byte string. A text `cti` is
+    // malformed and is rejected on decode rather than silently dropped.
     let key = b"testSecret";
 
     let protected: &[u8] = &[0xa1, 0x01, 0x05];
@@ -843,20 +845,10 @@ fn test_cti_as_text_verifies_and_roundtrips() {
     push_text(&mut payload, "cti-as-text");
     let token_bytes = build_mac0_token_bytes(protected, &payload, key);
 
-    let token = Token::from_bytes(&token_bytes).expect("decode cti-as-text token");
-    assert_eq!(
-        token.claims.registered.cti, None,
-        "text-valued cti cannot be represented as bytes and is dropped"
-    );
-    assert!(
-        token.verify_with_key(VerifyingKey::HmacSha256(key)).is_ok(),
-        "unmutated cti-as-text token must verify"
-    );
-    assert_eq!(
-        token.to_bytes().expect("re-encode cti-as-text token"),
-        token_bytes,
-        "unmutated lossy token must re-encode byte-faithfully"
-    );
+    assert!(matches!(
+        Token::from_bytes(&token_bytes),
+        Err(crate::Error::InvalidRegisteredClaim(_))
+    ));
 }
 
 #[test]