Enforce the DID-matches-document check, relax decode to the canonical schema - #58
Merged
Merged
Conversation
… schema verified(expecting:did:) shadowed its own did: parameter on the first line of its body and never checked it, so the DID-matches-document-id check Resolver.swift's own doc comment already claims to enforce never actually ran. Fixed on both the sync overload and the async one, which previously had no way to express an expected DID at all — it now takes an optional expectedDid: Atproto.DID? = nil, checked only when supplied, so existing callers passing nothing keep today's behavior. Traced every call site this session could find; all but one are provable no-ops (the document's id was always derived from the same DID being compared) — the one real gap is germ-atproto-resolver's plcQuery, which decodes a plc.directory response with no id check at all. DIDDocument's decode was stricter than the canonical schema (bluesky-social/atproto's did-doc.ts): context, verificationMethod, service, and publicKeyMultibase are now optional, @context accepts a bare string as well as an array, and Service.serviceEndpoint is URL? (an object-shaped endpoint, or anything else that isn't string-or-object, 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. checkServiceForAtproto/pdsUrl throw on a first matching entry with an unusable endpoint rather than searching past it, matching the DID spec's own "first matching entry should be used, any others ignored." Reviewed twice before landing: the plan review found SwiftPM's `from:` doesn't gate 0.x releases the way SemVer's own convention might suggest (a 0.5.0 lands on any consumer's next resolve, patch or minor makes no difference — Package.resolved pins are the only real gate), and a third production call site into the unchecked async overload that hadn't been found yet (AtprotoClient's verifiedResolve(atIdentifier:) .did leg, reached by AtprotoOAuth's live authorize-by-DID flow). The diff review found the service-endpoint doc comment misdescribed why a malformed string becomes nil (URL(string:) is far more lenient than the comment implied; Service. validate is the real screen) and that the deliberately-lenient paths on garbage @context/serviceEndpoint input were undocumented and unpinned. Both fixed, with tests. Wiring the new expectedDid: parameter into AtprotoClient's and germ-atproto- resolver's call sites is deferred — it can't compile against either repo's currently-pinned AtprotoTypes version, so making that edit now would be dead, uncommittable code in a repo with its own CI. Follow-up once this releases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: bf05899 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merged
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
verified(expecting:did:)shadowed its owndid:parameter and never checked it — the DID-matches-document-id checkResolver.swiftalready claims to enforce never actually ran. Fixed on both the sync overload and the async one, which now takes an optionalexpectedDid: Atproto.DID? = nil(checked only when supplied, so existing callers keep today's behavior).DIDDocument's decode was stricter than the canonical schema (did-doc.ts):context/verificationMethod/service/publicKeyMultibaseare now optional,@contextaccepts a bare string,Service.serviceEndpointisURL?(an unusable shape decodes tonilrather than failing the whole document). PLC hides this gap; did:web documents are self-hosted and far more likely to hit it.Sequencing note
Wiring the new
expectedDid:parameter intoAtprotoClient's andgerm-atproto-resolver's call sites is deferred — neither compiles against their currently-pinned AtprotoTypes version, so that edit would be dead code in a repo with its own CI. Follow-up once this releases; the one real call site (germ-atproto-resolver'splcQuery, which today accepts a plc.directory response with no id check at all) is tracked.Test plan
swift build && swift test— 56 tests, 14 suites, all greenswift format lint -rclean🤖 Generated with Claude Code