diff --git a/.changeset/verified-did-check-and-decode-laxity.md b/.changeset/verified-did-check-and-decode-laxity.md new file mode 100644 index 0000000..d65a3f2 --- /dev/null +++ b/.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. diff --git a/Sources/AtprotoTypes/Atproto/DIDDocument+Verified.swift b/Sources/AtprotoTypes/Atproto/DIDDocument+Verified.swift index 1119cc8..44b0a5b 100644 --- a/Sources/AtprotoTypes/Atproto/DIDDocument+Verified.swift +++ b/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 diff --git a/Sources/AtprotoTypes/Atproto/DIDDocument.swift b/Sources/AtprotoTypes/Atproto/DIDDocument.swift index bdab061..318e87c 100644 --- a/Sources/AtprotoTypes/Atproto/DIDDocument.swift +++ b/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,11 +46,11 @@ 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 @@ -55,8 +58,15 @@ extension Atproto { 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 } } diff --git a/Tests/AtprotoTypesTests/DIDDocumentDecodingTests.swift b/Tests/AtprotoTypesTests/DIDDocumentDecodingTests.swift new file mode 100644 index 0000000..844f7c3 --- /dev/null +++ b/Tests/AtprotoTypesTests/DIDDocumentDecodingTests.swift @@ -0,0 +1,164 @@ +// +// DIDDocumentDecodingTests.swift +// AtprotoTypesTests +// + +import AtprotoTypes +import AtprotoTypesMocks +import Foundation +import Testing + +@Suite struct DIDDocumentDecodingTests { + private func decode(_ json: String) throws -> Atproto.DIDDocument { + try JSONDecoder().decode(Atproto.DIDDocument.self, from: Data(json.utf8)) + } + + // MARK: - The shape a real, self-hosted did:web document may take + + @Test func aMinimalDidWebDocumentDecodes() throws { + let document = try decode( + """ + {"@context": "https://www.w3.org/ns/did/v1", "id": "did:web:example.com"} + """ + ) + #expect(document.context == ["https://www.w3.org/ns/did/v1"]) + #expect(document.id == "did:web:example.com") + #expect(document.verificationMethod == nil) + #expect(document.service == nil) + } + + @Test func contextAsAnArrayIsPreserved() throws { + let document = try decode( + """ + {"@context": ["https://www.w3.org/ns/did/v1", "https://w3id.org/security/v1"], "id": "did:web:example.com"} + """ + ) + #expect( + document.context == [ + "https://www.w3.org/ns/did/v1", "https://w3id.org/security/v1", + ]) + } + + @Test func absentContextDecodesToNil() throws { + let document = try decode(#"{"id": "did:web:example.com"}"#) + #expect(document.context == nil) + } + + /// Deliberately lenient, not a decode error — see the doc comment on + /// `DIDDocument.init(from:)`. Pinned so a future refactor can't flip this + /// silently either direction. + @Test(arguments: ["42", "null", "true", "{\"x\": 1}", "[\"a\", 5]"]) + func aContextThatsNeitherStringNorStringArrayDecodesToNil( + _ contextJSON: String + ) throws { + let document = try decode( + #"{"@context": \#(contextJSON), "id": "did:web:example.com"}"# + ) + #expect(document.context == nil) + } + + @Test func aPublicKeyJwkOnlyVerificationMethodDecodesWithNilMultibase() throws { + let document = try decode( + """ + { + "id": "did:web:example.com", + "verificationMethod": [{ + "id": "did:web:example.com#atproto", + "type": "JsonWebKey2020", + "controller": "did:web:example.com" + }] + } + """ + ) + let method = try #require(document.verificationMethod?.first) + #expect(method.publicKeyMultibase == nil) + } + + @Test func absentServiceDecodesToNilAndPdsUrlThrowsMissingServiceUrl() throws { + let document = try decode(#"{"id": "did:web:example.com"}"#) + #expect(document.service == nil) + #expect(throws: Atproto.DIDDocument.Errors.missingServiceUrl) { + try document.pdsUrl + } + } + + /// The canonical schema permits an object-shaped `serviceEndpoint`; this + /// type never stores one (nothing upstream treats it as usable either — + /// see the property's doc comment). The entry survives decode with a + /// `nil` endpoint rather than failing the whole document, and + /// `checkServiceForAtproto` throws on it rather than searching past it, + /// matching "the first matching entry should be used, any others + /// ignored." + @Test func anObjectShapedServiceEndpointDecodesToANilURL() throws { + let document = try decode( + """ + { + "id": "did:web:example.com", + "service": [{ + "id": "#atproto_pds", + "type": "AtprotoPersonalDataServer", + "serviceEndpoint": {"uri": "https://pds.example.com"} + }] + } + """ + ) + #expect(document.service?.first?.serviceEndpoint == nil) + #expect(throws: Atproto.DIDDocument.Errors.missingServiceUrl) { + try document.checkServiceForAtproto() + } + } + + /// Same leniency as `@context`, and same reasoning: `serviceEndpoint` + /// fails sanely at use (`missingServiceUrl`) rather than failing the + /// whole document at decode. + @Test(arguments: ["42", "null", "true"]) + func aServiceEndpointThatsNeitherStringNorObjectDecodesToNil( + _ endpointJSON: String + ) throws { + let document = try decode( + """ + {"id": "did:web:example.com", "service": [{"id": "#atproto_pds", \ + "type": "AtprotoPersonalDataServer", "serviceEndpoint": \(endpointJSON)}]} + """ + ) + #expect(document.service?.first?.serviceEndpoint == nil) + } + + // MARK: - Regression: the uniform PLC shape must keep decoding exactly as before + + @Test func aFullPLCShapedDocumentStillDecodesEveryField() throws { + // A real plc.directory-shaped payload, decoded — not just the mock's + // memberwise construction, which would pass even if decoding broke. + let document = try decode( + """ + { + "@context": ["https://www.w3.org/ns/did/v1"], + "id": "did:plc:4yvwfwxfz5sney4twepuzdu7", + "alsoKnownAs": ["at://example.com"], + "verificationMethod": [{ + "id": "did:plc:4yvwfwxfz5sney4twepuzdu7#atproto", + "type": "Multikey", + "controller": "did:plc:4yvwfwxfz5sney4twepuzdu7", + "publicKeyMultibase": "zQ3shPrWRUXva2mWziWZt1vrjuXUx3E28WfgsAwStMcAmDt93" + }], + "service": [{ + "id": "#atproto_pds", + "type": "AtprotoPersonalDataServer", + "serviceEndpoint": "https://blusher.us-east.host.bsky.network" + }] + } + """ + ) + #expect(document.context?.isEmpty == false) + #expect(document.verificationMethod?.first?.publicKeyMultibase != nil) + #expect(document.service?.first?.serviceEndpoint != nil) + #expect(try document.pdsUrl.host() == "blusher.us-east.host.bsky.network") + } + + @Test func theMockDocumentRoundTripsThroughEncodeAndDecode() throws { + let original = try Atproto.DIDDocument.mock() + let data = try JSONEncoder().encode(original) + let decoded = try JSONDecoder().decode(Atproto.DIDDocument.self, from: data) + #expect(decoded == original) + } +} diff --git a/Tests/AtprotoTypesTests/DIDDocumentVerifiedTests.swift b/Tests/AtprotoTypesTests/DIDDocumentVerifiedTests.swift new file mode 100644 index 0000000..09d2c36 --- /dev/null +++ b/Tests/AtprotoTypesTests/DIDDocumentVerifiedTests.swift @@ -0,0 +1,110 @@ +// +// DIDDocumentVerifiedTests.swift +// AtprotoTypesTests +// + +import AtprotoTypes +import Foundation +import Testing + +@Suite struct DIDDocumentVerifiedTests { + private func document(id: String, alsoKnownAs: [String]? = nil) -> Atproto.DIDDocument { + .init(id: id, alsoKnownAs: alsoKnownAs) + } + + // MARK: - Synchronous overload + + @Test func mismatchedDidThrows() throws { + let document = document(id: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + let requested = try Atproto.DID(string: "did:plc:bbbbbbbbbbbbbbbbbbbbbbbbbb") + #expect( + throws: Atproto.DIDDocument.Errors.documentIdMismatch( + requested: requested.rawValue, + returned: document.id + ) + ) { + try document.verified(expecting: .invalid, did: requested) + } + } + + @Test func matchingDidAndHandleVerifies() throws { + let did = try Atproto.DID(string: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + let handle = try Atproto.Handle(string: "alice.example.com") + let document = document(id: did.rawValue, alsoKnownAs: ["at://alice.example.com"]) + + let verified = try document.verified(expecting: handle, did: did) + #expect(verified.did == did) + #expect(verified.verifiedHandle == handle) + } + + /// A handle mismatch degrades to `.invalid` rather than throwing — + /// deliberately asymmetric with the DID check above. + @Test func matchingDidButMismatchedHandleDegradesRatherThanThrows() throws { + let did = try Atproto.DID(string: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + let expected = try Atproto.Handle(string: "alice.example.com") + let document = document( + id: did.rawValue, alsoKnownAs: ["at://someone-else.example.com"]) + + let verified = try document.verified(expecting: expected, did: did) + #expect(verified.did == did) + #expect(verified.verifiedHandle == .invalid) + } + + @Test func matchingDidWithNoAlsoKnownAsDegrades() throws { + let did = try Atproto.DID(string: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + let document = document(id: did.rawValue, alsoKnownAs: nil) + + let verified = try document.verified( + expecting: try Atproto.Handle(string: "alice.example.com"), + did: did + ) + #expect(verified.verifiedHandle == .invalid) + } + + /// Pins that the rename off the shadowed `did` local didn't disturb the + /// DID-parse failure path. + @Test func aNonDidIdStillThrowsDIDParseErrors() throws { + let document = document(id: "not-a-did") + #expect(throws: Atproto.DID.Errors.invalidPrefix) { + try document.verified( + expecting: .invalid, + did: try Atproto.DID(string: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + ) + } + } + + // MARK: - Async overload + + @Test func asyncOverloadWithNoExpectedDidSkipsTheCheck() async throws { + let document = document(id: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa", alsoKnownAs: nil) + // No expectedDid supplied — matches the pre-fix behavior exactly, for + // callers with no DID to compare against yet. + let verified = try await document.verified { _ in + try Atproto.DID(string: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + } + #expect(verified.verifiedHandle == .invalid) + } + + @Test func asyncOverloadWithMismatchedExpectedDidThrows() async throws { + let document = document(id: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + let requested = try Atproto.DID(string: "did:plc:bbbbbbbbbbbbbbbbbbbbbbbbbb") + await #expect( + throws: Atproto.DIDDocument.Errors.documentIdMismatch( + requested: requested.rawValue, + returned: document.id + ) + ) { + try await document.verified(expectedDid: requested) { _ in requested } + } + } + + @Test func asyncOverloadWithMatchingExpectedDidVerifies() async throws { + let did = try Atproto.DID(string: "did:plc:aaaaaaaaaaaaaaaaaaaaaaaaaa") + let handle = try Atproto.Handle(string: "alice.example.com") + let document = document(id: did.rawValue, alsoKnownAs: ["at://alice.example.com"]) + + let verified = try await document.verified(expectedDid: did) { _ in did } + #expect(verified.did == did) + #expect(verified.verifiedHandle == handle) + } +} diff --git a/Tests/AtprotoTypesTests/PDSEndpointTests.swift b/Tests/AtprotoTypesTests/PDSEndpointTests.swift index 4a43702..69d5e1f 100644 --- a/Tests/AtprotoTypesTests/PDSEndpointTests.swift +++ b/Tests/AtprotoTypesTests/PDSEndpointTests.swift @@ -216,10 +216,11 @@ struct PDSEndpointTests { @Test func checkServiceForAtprotoHonorsPolicy() throws { let document = try document(endpoint: "http://localhost:2583") - #expect( + let endpoint = try #require( try document.checkServiceForAtproto(policy: .developmentLoopback) - .serviceEndpoint.host() == "localhost" + .serviceEndpoint ) + #expect(endpoint.host() == "localhost") #expect(throws: Atproto.DIDDocument.Errors.insecureServiceUrlScheme("http")) { try document.checkServiceForAtproto() } @@ -236,9 +237,8 @@ struct PDSEndpointTests { @Test func checkServiceForAtprotoAcceptsPublicEndpoint() throws { let document = try document(endpoint: "https://pds.example.com") - #expect( - try document.checkServiceForAtproto().serviceEndpoint.host() - == "pds.example.com") + let endpoint = try #require(try document.checkServiceForAtproto().serviceEndpoint) + #expect(endpoint.host() == "pds.example.com") } //the mock is the fixture every other suite builds on, so it has to stay valid