fix(did): reject did:key:z in isDidKeyUri - #164
Conversation
The guard checked `startsWith("did:key:z")` and then tested the rest of
the string against the base58btc class, slicing from index 8. That is the
length of `"did:key:"`, not of `"did:key:z"`, so the `z` stayed in the
string being tested and satisfied the `+` in `z[a-km-zA-HJ-NP-Z1-9]+` on
its own. `isDidKeyUri("did:key:z")` returned true for a DID carrying no
key material, which `getDidResolver().resolve()` reports as `invalidDid`.
Replace the index arithmetic with a single pattern spanning the whole URI,
so the quantifier applies to the base58btc value as the documented grammar
intends. `z` is itself a base58btc character, so that was the only string
the old slice let through; no other input changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review. WalkthroughThe Changesdid:key validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR tightens Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
isDidKeyUrireturnstruefordid:key:z— the multibase prefix with no key material behind it.So the guard vouches for a DID the resolver in this same package rejects. Anything that narrows to
DidKeyUrion the strength of it is holding a value with no key bytes in it.Why
The check slices the multibase value off by index:
"did:key:"is 8 characters and"did:key:z"is 9, soslice(8)keeps thez— the comment describes the intent, not what the line does.zis itself a base58btc character, so it satisfies the+on its own and the empty value passes. The grammar quoted directly above the function requires at least one character after the prefix:Change
The guard is now the grammar as one pattern over the whole URI, so the quantifier lands on the base58btc value and there is no index to get wrong:
This is the same shape
createDidKeyUri's existing tests already assert its output against, so the two now agree on what adid:keyURI is.did:key:zis the only string whose result changes. The old slice differed from a correct one only by the leadingz, and sincezis in the character class, every other input was already classified the same way.Tests
Added to the existing
isDidKeyUriblock inpackages/did/src/methods/did-key.test.ts:did:key:zanddid:key:are rejected0,O,I,l) are rejectedgetDidResolver().resolve()agree ondid:key:zThe first and last fail on
main.packages/didis green (74 tests),oxlintreports nothing on the changed files, andoxfmt --checkpasses on them.One note on my local run:
packages/did/src/did-resolvers/pkh-did-resolver.test.tscannot run on Windows because thedid-pkhfixture filenames contain:, which is what #145 describes. That is unrelated to this change and I left it alone.AI assistance disclosure
Per the repository AI policy: this contribution was AI-assisted using Claude Code (Claude Opus). AI assistance was used to locate the defect, write the fix and the tests, and run verification locally. I reviewed the final diff, can explain the change and why it is confined to a single input, and take responsibility for what is submitted here.
Summary by CodeRabbit
did:keyURI validation to require valid key material.