Enforce the DID-matches-document check, relax decode to the canonical schema

.changeset/verified-did-check-and-decode-laxity.md

@@ -0,0 +1,9 @@
+---
+"@germ-network/atprototypes": minor
+---
+
+Two fixes to `Atproto.DIDDocument`:
+
+`verified(expecting:did:)` shadowed its own `did:` parameter and never checked it, so the DID-matches-document-id check `Resolver.swift`'s own doc comment claims is enforced never actually ran. Both the synchronous and async (`verified(resolver:)`, now taking an optional `expectedDid:`) overloads check it and throw `documentIdMismatch` on a mismatch. Everywhere in the current call graph this was traced to be a provable no-op except one site (a plc.directory response accepted without comparing its `id` to the requested DID) — this closes that gap directly.
+
+`DIDDocument`'s decode was stricter than the canonical schema: `@context`, `verificationMethod`, `service`, and `VerificationMethod.publicKeyMultibase` are now optional, `@context` accepts a bare string as well as an array, and `Service.serviceEndpoint` is now `URL?` (an object-shaped endpoint, or a string that doesn't parse as a `URL`, decodes to `nil` rather than failing the whole document). PLC-issued documents hid this — plc.directory emits one uniform, tool-generated shape — but did:web documents are self-hosted and far more likely to be minimal or hand-authored.

Sources/AtprotoTypes/Atproto/DIDDocument+Verified.swift

@@ -10,8 +10,8 @@ import Foundation
 extension Atproto.DIDDocument {
 	public struct Verified: Sendable {
 		public let document: Atproto.DIDDocument
-		//which may be reserved value "handle.invalid"
 		public let did: Atproto.DID
+		//which may be reserved value "handle.invalid"
 		public let verifiedHandle: Atproto.Handle
 
 		package init(
@@ -25,10 +25,24 @@ extension Atproto.DIDDocument {
 		}
 	}
 
+	/// `expectedDid`, when supplied, is compared against the document's own
+	/// `id` and throws on mismatch — the same check the synchronous overload
+	/// below always performs. A handle mismatch instead degrades to
+	/// `verifiedHandle: .invalid`; the two are not symmetric on purpose, since
+	/// a DID mismatch means the document does not belong to the identity this
+	/// call resolved, while a handle mismatch just means that handle isn't
+	/// (yet) verified.
 	public func verified(
+		expectedDid: Atproto.DID? = nil,
 		resolver: (Atproto.Handle) async throws -> Atproto.DID
 	) async throws -> Verified {
 		let did = try Atproto.DID(string: id)
+		if let expectedDid {
+			guard did == expectedDid else {
+				throw Errors.documentIdMismatch(
+					requested: expectedDid.rawValue, returned: id)
+			}
+		}
 
 		guard let unverifiedHandle else {
 			return .init(document: self, did: did, verifiedHandle: .invalid)
@@ -43,22 +57,27 @@ extension Atproto.DIDDocument {
 		return .init(document: self, did: did, verifiedHandle: unverifiedHandle)
 	}
 
-	///Synchronous version of the above if we just resolved handle to did
+	///Synchronous version of the above if we just resolved handle to did.
+	///Always throws on a DID mismatch — see the async overload's doc comment
+	///on why that's asymmetric with the handle check just below it.
 	public func verified(
 		expecting: Atproto.Handle,
 		did: Atproto.DID
 	) throws -> Verified {
-		let did = try Atproto.DID(string: id)
+		let documentDid = try Atproto.DID(string: id)
+		guard documentDid == did else {
+			throw Errors.documentIdMismatch(requested: did.rawValue, returned: id)
+		}
 
 		guard let unverifiedHandle else {
-			return .init(document: self, did: did, verifiedHandle: .invalid)
+			return .init(document: self, did: documentDid, verifiedHandle: .invalid)
 		}
 
 		guard expecting == unverifiedHandle else {
-			return .init(document: self, did: did, verifiedHandle: .invalid)
+			return .init(document: self, did: documentDid, verifiedHandle: .invalid)
 		}
 
-		return .init(document: self, did: did, verifiedHandle: unverifiedHandle)
+		return .init(document: self, did: documentDid, verifiedHandle: unverifiedHandle)
 	}
 
 	//the value we parse still needs to be resolved back to the same

Sources/AtprotoTypes/Atproto/DIDDocument.swift

@@ -18,8 +18,11 @@ extension Atproto {
 	public struct DIDDocument: Sendable, Codable, Equatable {
 
 		/// An array of context URLs for the DID document, providing additional semantics for
-		/// the properties.
-		public let context: [String]
+		/// the properties. Optional per the canonical schema, and a bare string is accepted
+		/// in addition to an array — the canonical schema restricts the bare-string form to
+		/// exactly `https://www.w3.org/ns/did/v1`; this type is more permissive since nothing
+		/// here interprets `@context` semantically.
+		public let context: [String]?
 
 		/// The unique identifier of the DID document.
 		public let id: String
@@ -29,12 +32,12 @@ extension Atproto {
 		public let alsoKnownAs: [String]?
 
 		/// An array of methods for verifying digital signatures, including the public signing key
-		/// for the account.
-		public let verificationMethod: [VerificationMethod]
+		/// for the account. Optional per the canonical schema.
+		public let verificationMethod: [VerificationMethod]?
 
 		/// An array of service endpoints related to the decentralized identifier (DID), including the
-		/// Personal Data Server's (PDS) location.
-		public let service: [Service]
+		/// Personal Data Server's (PDS) location. Optional per the canonical schema.
+		public let service: [Service]?
 
 		/// Checks if the ``service`` property array contains items, and if so, sees if `#atproto_pds`
 		/// is in the ``ATService/id`` property.
@@ -43,20 +46,27 @@ extension Atproto {
 		///
 		/// - Throws: ``DIDDocumentError`` if ``service`` is empty, if none of the items
 		/// contain `#atproto_pds`, or if that item's endpoint fails
-		/// ``Service/validate(endpoint:policy:)``.
+		/// ``Service/validate(endpoint:policy:)`` or didn't decode to a usable `URL`.
 		public func checkServiceForAtproto(
 			policy: EndpointPolicy = .default
 		) throws -> Service {
-			let services = self.service
+			let services = self.service ?? []
 
 			guard services.count > 0 else {
 				throw Errors.emptyArray
 			}
 
 			for service in services {
 				if service.id == "#atproto_pds" {
-					try Service.validate(
-						endpoint: service.serviceEndpoint, policy: policy)
+					guard let endpoint = service.serviceEndpoint else {
+						// The first matching entry is authoritative
+						// (https://atproto.com/specs/did#did-documents: "the
+						// first matching entry... should be used, and any
+						// others ignored") — an unusable endpoint here is not
+						// a reason to keep searching for a second match.
+						throw Errors.missingServiceUrl
+					}
+					try Service.validate(endpoint: endpoint, policy: policy)
 					return service
 				}
 			}
@@ -72,6 +82,40 @@ extension Atproto {
 			case service
 		}
 
+		/// `@context` accepts a bare string in addition to the usual array —
+		/// see the property's own doc comment. Everything else decodes as
+		/// `decodeIfPresent`, since the canonical schema makes all of it but
+		/// `id` optional.
+		///
+		/// A `@context` that's neither an array nor a string (a number,
+		/// `null`, an array of non-strings) also decodes to `nil` rather than
+		/// throwing — deliberately lenient, since nothing here reads
+		/// `@context` and a hand-authored did:web document is exactly where a
+		/// malformed-but-harmless context value is most likely to show up.
+		/// `id` has no such leniency: it stays a plain `try`, so a missing or
+		/// wrongly-typed `id` still fails the whole decode.
+		public init(from decoder: Decoder) throws {
+			let container = try decoder.container(keyedBy: CodingKeys.self)
+			if let contextArray = try? container.decodeIfPresent(
+				[String].self, forKey: .context)
+			{
+				context = contextArray
+			} else if let contextString = try? container.decode(
+				String.self, forKey: .context)
+			{
+				context = [contextString]
+			} else {
+				context = nil
+			}
+			id = try container.decode(String.self, forKey: .id)
+			alsoKnownAs = try container.decodeIfPresent(
+				[String].self, forKey: .alsoKnownAs)
+			verificationMethod = try container.decodeIfPresent(
+				[VerificationMethod].self, forKey: .verificationMethod)
+			service = try container.decodeIfPresent(
+				[Service].self, forKey: .service)
+		}
+
 		/// Errors relating to the DID Document.
 		public enum Errors: Error, Equatable {
 
@@ -91,14 +135,19 @@ extension Atproto {
 			/// The service endpoint's host is one we refuse to send traffic to,
 			/// such as a loopback, link-local, or private-range address.
 			case disallowedServiceUrlHost(String)
+
+			/// `verified(expecting:did:)` / `verified(expectedDid:resolver:)` —
+			/// the document's own `id` does not match the DID it was resolved
+			/// for.
+			case documentIdMismatch(requested: String, returned: String)
 		}
 
 		public init(
-			context: [String],
+			context: [String]? = nil,
 			id: String,
 			alsoKnownAs: [String]?,
-			verificationMethod: [VerificationMethod],
-			service: [Service]
+			verificationMethod: [VerificationMethod]? = nil,
+			service: [Service]? = nil
 		) {
 			self.context = context
 			self.id = id
@@ -125,13 +174,15 @@ extension Atproto.DIDDocument {
 		public let controller: String
 
 		/// The public key, in multibase encoding; used for verifying digital signatures.
-		public let publicKeyMultibase: String
+		/// Optional per the canonical schema — a `publicKeyJwk`-only method is legal,
+		/// though this type does not model that field, since nothing here reads it.
+		public let publicKeyMultibase: String?
 
 		package init(
 			id: String,
 			type: String,
 			controller: String,
-			publicKeyMultibase: String
+			publicKeyMultibase: String?
 		) {
 			self.id = id
 			self.type = type
@@ -152,9 +203,35 @@ extension Atproto.DIDDocument {
 		public let type: String
 
 		/// The endpoint URL for the service, specifying the location of the service.
-		public let serviceEndpoint: URL
+		/// The canonical schema permits an object shape here too — this type never
+		/// stores one, since nothing here (or upstream, in `@atproto/identity`)
+		/// treats an object-shaped endpoint as usable. `nil` covers that case and
+		/// anything else `serviceEndpoint` legally isn't (absent, a number,
+		/// `null`) — `URL(string:)` itself is too lenient to reject most garbage
+		/// strings, so the real screen for an unusable-but-URL-shaped value is
+		/// `Service.validate`, not this initializer. Either way a malformed
+		/// entry decodes with a `nil` endpoint rather than failing the whole
+		/// document.
+		public let serviceEndpoint: URL?
 
-		public init(id: String, type: String, serviceEndpoint: URL) {
+		enum CodingKeys: String, CodingKey {
+			case id, type, serviceEndpoint
+		}
+
+		public init(from decoder: Decoder) throws {
+			let container = try decoder.container(keyedBy: CodingKeys.self)
+			id = try container.decode(String.self, forKey: .id)
+			type = try container.decode(String.self, forKey: .type)
+			if let endpointString = try? container.decode(
+				String.self, forKey: .serviceEndpoint)
+			{
+				serviceEndpoint = URL(string: endpointString)
+			} else {
+				serviceEndpoint = nil
+			}
+		}
+
+		public init(id: String, type: String, serviceEndpoint: URL?) {
 			self.id = id
 			self.type = type
 			self.serviceEndpoint = serviceEndpoint
@@ -174,13 +251,16 @@ extension Atproto.DIDDocument {
 	///goes through this spelling
 	public func pdsUrl(policy: EndpointPolicy) throws -> URL {
 		guard
-			let service = service.first(where: {
+			let service = (service ?? []).first(where: {
 				$0.type == "AtprotoPersonalDataServer"
 			})
 		else {
 			throw Errors.missingServiceUrl
 		}
-		try Service.validate(endpoint: service.serviceEndpoint, policy: policy)
-		return service.serviceEndpoint
+		guard let endpoint = service.serviceEndpoint else {
+			throw Errors.missingServiceUrl
+		}
+		try Service.validate(endpoint: endpoint, policy: policy)
+		return endpoint
 	}
 }