spruceid / ssi

Core library for decentralized identity.
https://spruceid.dev
Apache License 2.0
180 stars 54 forks source link

Refactor error in PrimaryDIDURL::from_str; missing crucial validity check #525

Open vdods opened 12 months ago

vdods commented 12 months ago

I found an error in a refactor done in commit bd244dfd0474986aedf529cfb3124252f3a99fd5 which causes a crucial check done in PrimaryDIDURL::from_str to not occur except in cfg(test). The diff in question is

 /// Parse a primary DID URL.
 impl FromStr for PrimaryDIDURL {
     type Err = Error;
     fn from_str(didurl: &str) -> Result<Self, Self::Err> {
         // Allow non-DID URL for testing lds-ed25519-2020-issuer0
-        #[cfg(not(any(test, feature = "testing")))]
+        #[cfg(test)]
         if !didurl.starts_with("did:") {
             return Err(Error::DIDURL);
         }
         if didurl.contains('#') {
             return Err(Error::UnexpectedDIDFragment);
         }

From the comment above the changed line, clearly the intent is only to omit the .starts_with("did:") check when testing lds-ed25519-2020-issuer0, which presumably is what the "testing" feature was for. Though the previous condition also omitted the check in cfg(test).

I think the .starts_with("did:") check is crucial in testing and non-testing, since that's one of the most basic validity checks one can do on a DID. Would it be possible to change this to something like #[cfg(not(feature = "testing-lds-ed25519-2020-issuer0"))] so that the .starts_with("did:") check is always done except in that specific case?