chore: fix btc-base-58 slice #622
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughReworks signature-validator: adds decoding/parsing helpers, broadens public-key and signature decoding (0x-hex, 'z'+hex, base58btc, base64/url, DER/SPKI/raw), makes base58btc dynamic import resilient, and refactors certificate resolution and verifySignature with aggregated error handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Validator as SignatureValidator
participant Whois as eVault/Whois
participant Registry as Registry(JWKS)
participant Base58 as base58btc (dynamic)
Client->>Validator: verifySignature(options, signature, payload)
Validator->>Validator: validate inputs, normalize signature (try DER→raw)
Validator->>Whois: GET /whois (resolve eVault URL)
Whois-->>Validator: keyBindingCertificates (array)
Validator->>Registry: fetch JWKS for each certificate issuer / registry
Registry-->>Validator: JWKS (keys)
Validator->>Base58: dynamic import attempt (import() -> fallback)
Base58-->>Validator: base58 decode function
Validator->>Validator: attempt JWT verification / import public key per certificate
alt verification success
Validator-->>Client: verification result (success)
else all failed
Validator-->>Client: aggregated error (all attempts failed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
infrastructure/signature-validator/src/index.ts (2)
80-83: DER length field check may be overly simplistic.The check
bytes[1] >= 0x20 && bytes[1] <= 0x82conflates direct length values with the long-form length indicator. Whenbytes[1]is0x81or0x82, it signals that the actual length is in the following 1 or 2 bytes respectively, not a direct length value. This heuristic may still work for typical SPKI keys but could produce false positives/negatives for edge cases.
194-199: Consider adding a comment for the silent catch.The empty catch block silently swallows base58 decode failures as part of the fallback logic. While intentional, a brief comment would clarify this is expected behavior.
if (signature.startsWith("z") && base58Alphabet.test(signature.slice(1))) { try { const base58btc = await getBase58btc(); base58Bytes = base58btc.decode(signature); - } catch (error) { + } catch { + // Silently ignore - will try other decodings } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/signature-validator/src/index.ts(7 hunks)
🧰 Additional context used
🪛 GitHub Actions: Build All Packages
infrastructure/signature-validator/src/index.ts
[error] 240-240: TS2304: Cannot find name 'debugLog'.
🪛 GitHub Actions: Check Code
infrastructure/signature-validator/src/index.ts
[error] 240-240: Cannot find name 'debugLog'. (TS2304) in infrastructure/signature-validator postinstall during npm run build.
🪛 GitHub Actions: Check Format
infrastructure/signature-validator/src/index.ts
[error] 240-240: infrastructure/signature-validator postinstall: npm run build. TS2304: Cannot find name 'debugLog' (src/index.ts:240,5).
🔇 Additional comments (5)
infrastructure/signature-validator/src/index.ts (5)
8-36: LGTM!The dual-path dynamic import approach with caching is a sensible pattern for handling ESM/CJS module compatibility. The error propagation properly includes context from the original failure.
135-171: LGTM!The multi-format public key decoding logic correctly handles the various encoding formats with appropriate validation and fallback behavior.
254-282: LGTM!The certificate retrieval logic correctly handles the two-step resolution process with appropriate timeout configuration and graceful handling when no certificates exist.
357-364: LGTM with note on curve assumption.The signature normalization and buffer preparation are correct. The hardcoded
size: 32inderToRawEcdsais appropriate for P-256 (which uses 32-byte integers), matching thenamedCurve: "P-256"used throughout the verification logic.
391-443: LGTM!The adaptive key import logic correctly distinguishes between raw uncompressed EC points and DER SPKI formats. The verification loop appropriately continues on failures and returns immediately on success.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
infrastructure/signature-validator/src/index.ts (2)
177-241: Clean up empty conditional and clarify base58 vs base64 precedence
- The
if (base64Bytes) { }block at Lines 185‑186 is a no‑op and can be safely removed.- if (base64urlPattern.test(signature)) { - base64Bytes = tryDecodeBase64Like(signature); - if (base64Bytes) { - } - } + if (base64urlPattern.test(signature)) { + base64Bytes = tryDecodeBase64Like(signature); + }
- For signatures that:
- start with
'z'and- are valid base58 characters (
base58Alphabet),
you decode them both as base64 and as base58. If the decoded bytes are not DER‑shaped, the fallbackif (base64Bytes) return base64Bytes; if (base58Bytes) return base58Bytes;means you’ll always prefer the base64 interpretation, even though a leading'z'strongly suggests multibase base58btc.If you ever expect non‑DER, raw ECDSA signatures encoded as
z‑prefixed base58btc, they’d be misinterpreted as base64 by this logic. You might want to:
- prefer
base58Byteswhensignature.startsWith("z") && base58Bytesin the non‑DER fallback, or- gate the base64 path more strictly for
z‑prefixed inputs.Example adjustment:
- // Otherwise return the first successful decode - if (base64Bytes) return base64Bytes; - if (base58Bytes) return base58Bytes; + // Otherwise return the most plausible decode + if (signature.startsWith("z") && base58Bytes) return base58Bytes; + if (base64Bytes) return base64Bytes; + if (base58Bytes) return base58Bytes;Please double‑check this against the actual formats you expect in production.
301-459: Treat empty certificate sets as failure, not successWhen no key binding certificates are found,
verifySignaturecurrently returnsvalid: true:if (keyBindingCertificates.length === 0) { return { valid: true, }; }This means that if certificate lookup fails or a user has no certificates, any signature will be treated as valid, which is a significant security risk.
Suggest instead treating this as an error:
- if (keyBindingCertificates.length === 0) { - return { - valid: true, - }; - } + if (keyBindingCertificates.length === 0) { + return { + valid: false, + error: "No key binding certificates found for eName", + }; + }The rest of the verification loop (JWT + public‑key import + WebCrypto verify with aggregated errors) looks robust and is a nice improvement over a single‑shot attempt.
🧹 Nitpick comments (3)
infrastructure/signature-validator/src/index.ts (3)
8-35: Dynamic base58btc import flow looks solid; minor error‑message nitThe two‑stage import with caching is good and should fix the ESM/CJS interop issue. One small nit: inside the
catch, the thrown error message interpolateserrfrom the first attempt, even if the fallbackdynamicImportis what actually failed. Consider either:
- capturing the fallback error and using that in the message, or
- emitting a more generic “both import strategies failed” message.
This is purely cosmetic; behavior is otherwise fine.
63-125: DER/hex helpers are correct for P‑256;looksLikeDerSpkicurrently unusedThe hex and ECDSA DER helpers look correct for P‑256 (single‑byte length, 32‑byte r/s, uncompressed 65‑byte EC point) and match how you use them later in
verifySignature.
looksLikeDerSpkiis currently not referenced anywhere; if you don’t plan to use it soon, consider removing it to avoid dead code, or wiring it into the key‑import decision logic.
227-241: Base64‑like helper is fine, but duplicates fallback logic
tryDecodeBase64Likedoes the right padding and URL‑safe normalization for base64/base64url. Note there’s some duplication with the final base64 fallback block indecodeSignature(Lines 211‑218). If you end up evolving this further, consider centralizing base64 decoding in a single helper (this one) to keep behavior consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/signature-validator/src/index.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/signature-validator/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
infrastructure/signature-validator/src/index.ts (2)
134-169: Public‑key multibase decoding logic LGTMHandling
0xhex,z+hex (SoftwareKeyManager), and standardzbase58btc with a clear error on invalid input is straightforward and correct. Returning raw bytes and deferring format interpretation to the caller (SPKI vs raw EC point) keeps responsibilities nicely separated.
249-277: Certificate fetch flow looks good; behavior on malformedwhoisis clearThe two‑step
/resolve→/whoisflow, with explicit timeouts and a clearErrorwhenuriis missing, is solid. Returning[]whenkeyBindingCertificatesis absent or not an array is a reasonable normalization, though note this interacts withverifySignature’skeyBindingCertificates.length === 0branch (see separate comment there).
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.