feat: add crypto package #190
Conversation
WalkthroughIntroduces a modular crypto package: adds Ed25519 key implementations, DER/RFC8410 PEM encoders, data-type and constant utilities, and JSON signing/verification helpers. Refactors index to a barrel of re-exports. Adds a dependency on @noble/ed25519. Updates tests accordingly and removes two core event tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Utils as utils/keys
participant Data as utils/data-types
participant Signer as Signer (Ed25519)
App->>Utils: signJson(jsonObj, signer)
Utils->>Data: encodeCanonicalJson(jsonObj)
Data-->>Utils: canonicalJson
Utils->>Data: toBinaryData(canonicalJson)
Data-->>Utils: bytes
Utils->>Signer: sign(bytes)
Signer-->>Utils: signature(bytes)
Utils->>Data: toUnpaddedBase64(signature)
Data-->>App: signature(base64)
sequenceDiagram
autonumber
participant App
participant Utils as utils/keys
participant Data as utils/data-types
participant Verifier as VerifierKey (Ed25519)
App->>Utils: verifyJsonSignature(jsonObj, signatureB64, verifier)
Utils->>Data: encodeCanonicalJson(jsonObj)
Data-->>Utils: canonicalJson
Utils->>Data: toBinaryData(canonicalJson)
Data-->>Utils: bytes
Utils->>Data: fromBase64ToBytes(signatureB64)
Data-->>Utils: signature(bytes)
Utils->>Verifier: verify(bytes, signature)
Verifier-->>App: success or throw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 10
🧹 Nitpick comments (10)
packages/crypto/src/utils/constants.ts (1)
1-17: Types and enum look good; tiny optional tidy-up.
isValidAlgorithmis fine. Optional: precompute aSetfor O(1) lookup if called frequently.packages/crypto/src/README.md (1)
9-18: Docs reference non-existent paths/APIs.
- Mentions
signing-key.ts; the code addscontracts/key.tsandkeys/ed25519.ts. Please align filenames and terminology (Signer/Verifier vs “SigningKey”).- Production note mentions “nacl/libsodium” but the implementation uses Node Crypto + noble; clarify runtime story.
packages/crypto/src/rfc/8410/ed25519-pem.ts (1)
34-37: Typos in comments.
- “CTET” → “NOTE” or remove.
- “subhjectPublicKey” → “subjectPublicKey”.
Also applies to: 81-83
packages/crypto/src/contracts/key.ts (1)
7-29: API surface is reasonable; export and immutability nits.
- Consider
export type KeyId = ...so consumers can type theid.- Consider
readonly publicKey/privateKey: Uint8Arrayto discourage mutation.packages/crypto/src/utils/keys.ts (1)
27-36: Signing flow is fine; keep base64 variant consistent.You emit unpadded Base64. Ensure all decoders accept unpadded form across runtimes.
If needed, add optional padding in
fromBase64ToBytesor normalize here before decoding.packages/crypto/src/utils/utils.spec.ts (1)
13-22: Helper is fine; tiny nit.Drop the non-null assertion; assert parts length instead.
- const seed = content.split(' ').pop()!; + const parts = content.trim().split(/\s+/); + if (parts.length < 3) throw new Error('Invalid key content'); + const seed = parts[parts.length - 1];packages/crypto/src/keys/ed25519.ts (2)
15-17: Nit: drop unnecessary “as const” on idIt over-narrows the type and is unnecessary for a string id.
- public get id() { - return `${this.algorithm}:${this.version}` as const; - } + public get id() { + return `${this.algorithm}:${this.version}`; + }
62-70: Optional: consider key material hygienePrivate key bytes remain in memory after PEM conversion. Consider zeroizing the Uint8Array after use or accepting KeyObject to avoid raw PEM handling in memory.
packages/crypto/src/der/index.ts (1)
108-111: Comment accuracy: Ed25519 AlgorithmIdentifier parameters MUST be absentRFC 8410 requires parameters to be absent (not NULL). Comment says “parameters for ed25519 is NULL”; keep it commented out but fix the note.
export const algorithmIdentifierTlv = sequenceOrderedTlv([ oidTlv, - // nullTlv, // parameters for ed25519 is NULL + // Parameters for Ed25519 MUST be absent (RFC 8410 §3); do not include NULL. ]);packages/crypto/src/utils/data-types.ts (1)
21-37: Verify TextEncoder/TextDecoder availability in Node buildGlobal TextEncoder/TextDecoder types aren’t always available unless lib DOM is enabled. If tsconfig doesn’t include DOM libs, import from 'node:util' or use Buffer-based conversions.
Example alternative:
- if (typeof value === 'string') { - return new TextEncoder().encode(value); - } + if (typeof value === 'string') { + return Buffer.from(value, 'utf8'); + } ... - return new TextDecoder().decode(value); + return Buffer.isBuffer(value) + ? value.toString('utf8') + : Buffer.from(value as ArrayBuffer).toString('utf8');Also applies to: 39-47
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/crypto/package.json(1 hunks)packages/crypto/src/README.md(1 hunks)packages/crypto/src/contracts/key.ts(1 hunks)packages/crypto/src/der/index.ts(1 hunks)packages/crypto/src/index.ts(1 hunks)packages/crypto/src/keys/ed25519.ts(1 hunks)packages/crypto/src/rfc/8410/ed25519-pem.ts(1 hunks)packages/crypto/src/utils/constants.ts(1 hunks)packages/crypto/src/utils/data-types.ts(1 hunks)packages/crypto/src/utils/keys.ts(1 hunks)packages/crypto/src/utils/utils.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/crypto/src/utils/keys.ts (3)
packages/crypto/src/contracts/key.ts (2)
Signer(25-29)VerifierKey(10-21)packages/crypto/src/keys/ed25519.ts (2)
Ed25519SigningKeyImpl(47-71)Ed25519VerifierKeyImpl(10-45)packages/crypto/src/utils/data-types.ts (4)
encodeCanonicalJson(63-82)toBinaryData(21-37)toUnpaddedBase64(49-60)fromBase64ToBytes(84-86)
packages/crypto/src/contracts/key.ts (1)
packages/crypto/src/utils/constants.ts (2)
DataType(5-5)SignatureType(7-7)
packages/crypto/src/utils/constants.ts (2)
packages/core/src/index.ts (1)
EncryptionValidAlgorithm(2-2)packages/crypto/src/index.ts (1)
isValidAlgorithm(11-11)
packages/crypto/src/utils/utils.spec.ts (2)
packages/crypto/src/utils/data-types.ts (2)
fromBase64ToBytes(84-86)encodeCanonicalJson(63-82)packages/crypto/src/utils/keys.ts (4)
loadEd25519SignerFromSeed(11-18)signJson(27-36)verifyJsonSignature(39-49)loadEd25519VerifierFromPublicKey(20-25)
packages/crypto/src/utils/data-types.ts (1)
packages/crypto/src/index.ts (7)
computeHashBuffer(3-3)encodeCanonicalJson(5-5)computeHashString(4-4)toUnpaddedBase64(2-2)toBinaryData(6-6)fromBinaryData(7-7)fromBase64ToBytes(8-8)
packages/crypto/src/keys/ed25519.ts (2)
packages/crypto/src/contracts/key.ts (2)
VerifierKey(10-21)Signer(25-29)packages/crypto/src/rfc/8410/ed25519-pem.ts (2)
ed25519PublicKeyRawToPem(76-88)ed25519PrivateKeyRawToPem(38-63)
packages/crypto/src/rfc/8410/ed25519-pem.ts (1)
packages/crypto/src/der/index.ts (5)
privateKeyVersionTlv(30-34)algorithmIdentifierTlv(108-111)octetStringTlv(89-106)sequenceOrderedTlv(58-86)bitStringTlv(114-126)
🔇 Additional comments (4)
packages/crypto/src/rfc/8410/ed25519-pem.ts (1)
38-63: PKCS#8 (RFC 5958/8410) structure: implementation looks correct.Validations and double OCTET for
privateKey(CurvePrivateKey insideprivateKey) match RFC 8410 examples. Good.Add a unit test that round-trips with Node/Bun crypto:
// pseudo-test const privPem = ed25519PrivateKeyRawToPem(seed32); const pubPem = ed25519PublicKeyRawToPem(pub32); const keyObj = crypto.createPrivateKey(privPem); const pubObj = crypto.createPublicKey(pubPem); expect(keyObj.asymmetricKeyType).toBe('ed25519'); expect(pubObj.asymmetricKeyType).toBe('ed25519');packages/crypto/src/utils/keys.ts (1)
38-49: Verifier path is correct modulo base64 padding concern.LGTM otherwise.
packages/crypto/package.json (1)
18-20: No change required — @noble/ed25519@^3.0.0 supports keygenAsync. ed.keygenAsync()/ed.keygen() return { secretKey, publicKey }; ed.utils.randomPrivateKey() and ed.getPublicKey/getPublicKeyAsync are available, so the dependency in packages/crypto/package.json is compatible with the current usage.packages/crypto/src/index.ts (1)
1-20: Barrel re-export looks goodPublic surface is cleanly organized and consistent with the new modular structure.
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
packages/crypto/src/index.spec.ts (3)
3-8: Remove unused import (encodeCanonicalJson).
encodeCanonicalJsonis imported but not used; this will fail linting/CI. Drop it.-import { encodeCanonicalJson, fromBase64ToBytes } from './utils/data-types'; +import { fromBase64ToBytes } from './utils/data-types';
47-51: Brittle assertion against a fixed signature.Hard-coding the exact base64 ties this test to implementation details of canonicalization. Keep if you intend to lock that exact canonical form; otherwise, prefer asserting that the produced signature verifies with the known public key in a separate assertion.
104-108: Assert verification outcome, not just absence of throw.Currently no assertion on the return value; the test would pass even if verification returns
falsebut doesn't throw.Option A (if it returns boolean):
- await verifyJsonSignature(json, signature, verifier); + const ok = await verifyJsonSignature(json, signature, verifier); + expect(ok).toBe(true);Option B (if it throws on failure and returns void):
- await verifyJsonSignature(json, signature, verifier); + await expect(verifyJsonSignature(json, signature, verifier)).resolves.toBeUndefined();Please confirm the contract and apply the appropriate option.
packages/federation-sdk/src/services/key.service.ts (3)
19-22: Handle signing-key load failures (unhandled rejection) or remove unused state.
this.keyis never used; at minimum, catch and log failures to avoid unhandled rejections.- this.configService.getSigningKey().then((key) => { - this.key = key; - }); + this.configService + .getSigningKey() + .then((key) => { + this.key = key; + }) + .catch((e) => { + this.logger.error('Failed to load signing key', e); + });If unused, consider removing
keyuntil needed to reduce confusion.
210-229: Store only activeverify_keys— confirm old keys policy.You ignore
old_verify_keys. If you need to validate historical events, you may want to persist those withexpiredTs.I can add an optional branch to persist
old_verify_keyswith theirexpired_tsif that’s required by your federation flows.
78-79: Nit: fix typos in comments.“notmal” → “normal”.
- // notmal http is enough + // normal http is enough
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/crypto/src/index.spec.ts(3 hunks)packages/federation-sdk/src/services/key.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/key.service.ts (2)
packages/crypto/src/contracts/key.ts (1)
Signer(25-29)packages/core/src/server.ts (1)
ServerKey(3-20)
packages/crypto/src/index.spec.ts (1)
packages/core/src/utils/signJson.ts (2)
signJson(17-50)verifyJsonSignature(115-134)
🔇 Additional comments (2)
packages/crypto/src/index.spec.ts (1)
45-47: Confirm seed decoding invariants (32 bytes, url-safe/unpadded) and add negative tests.Ensure fromBase64ToBytes(seed) returns exactly 32 bytes and accepts unpadded/base64url inputs. In packages/crypto/src/index.spec.ts (around lines 45–47) add: a positive test asserting a valid seed (including unpadded/base64url) decodes to 32 bytes, and negative tests asserting malformed base64 and decoded-length != 32 are rejected/throw. Verified sample seed decodes to 32 bytes (std & urlsafe).
packages/federation-sdk/src/services/key.service.ts (1)
3-7: Import verifySignaturesFromRemote from @hs/coreverifySignaturesFromRemote is exported by @hs/core (not @hs/crypto) — add in packages/federation-sdk/src/services/key.service.ts, e.g.:
import { verifySignaturesFromRemote } from '@hs/core';Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
packages/crypto/src/utils/utils.spec.ts (3)
25-29: Don't use asyncdescribe; move async setup tobeforeAll.
describeblocks should be synchronous. InitializesignerviabeforeAllfor proper test lifecycle management.Apply this diff:
-describe('Signing and verifying payloads', async () => { - const seedFileContent = - 'ed25519 a_XRhW YjbSyfqQeGto+OFswt+XwtJUUooHXH5w+czSgawN63U'; - - const signer = await getSignerFromKeyContent(seedFileContent); +describe('Signing and verifying payloads', () => { + const seedFileContent = + 'ed25519 a_XRhW YjbSyfqQeGto+OFswt+XwtJUUooHXH5w+czSgawN63U'; + let signer: Awaited<ReturnType<typeof getSignerFromKeyContent>>; + + beforeAll(async () => { + signer = await getSignerFromKeyContent(seedFileContent); + });
102-104: Fix Promise rejection assertion.Use
await expect(promise).rejectspattern for proper async rejection testing.Apply this diff:
- expect(verifyJsonSignature({}, signature, verifier)).rejects.toThrowError( - 'Invalid signature', - ); + await expect(verifyJsonSignature({}, signature, verifier)).rejects.toThrow( + 'Invalid signature' + );
85-86: Fix Promise rejection assertion.The current assertion syntax doesn't properly test Promise rejection. Use
await expect(promise).rejectspattern instead.Apply this diff:
- expect(verifyJsonSignature({}, signature, signer)).resolves; + await expect(verifyJsonSignature({}, signature, signer)).resolves.toBeDefined();packages/crypto/src/keys/ed25519.ts (2)
34-52: Fix: crypto.verify is synchronous; remove callback usage.Node's
crypto.verifyis synchronous and returns a boolean directly. The callback signature doesn't exist and will fail at runtime.Based on the retrieved learning, it appears there was confusion about the API. The Node.js documentation confirms that
crypto.verifyis synchronous when using the algorithm-less form.Apply this diff:
- public async verify(data: Uint8Array, signature: Uint8Array): Promise<void> { - return new Promise((resolve, reject) => { - crypto.verify( - null, - data, - this._publicKeyPem, - signature, - (err, verified) => { - if (err) { - reject(err); - } else if (verified) { - resolve(); - } else { - reject(new Error('Invalid signature')); - } - }, - ); - }); - } + public async verify(data: Uint8Array, signature: Uint8Array): Promise<void> { + const verified = crypto.verify(null, data, this._publicKeyPem, signature); + if (!verified) { + throw new Error('Invalid signature'); + } + }
59-68: Fix: crypto.sign is synchronous; remove callback usage.
crypto.signis synchronous and returns a Buffer directly. Convert to Uint8Array and return.Apply this diff:
- public async sign(data: Uint8Array) { - return new Promise<Uint8Array>((resolve, reject) => { - crypto.sign(null, data, this._privateKeyPem, (err, signature) => { - if (err) { - return reject(err); - } - return resolve(signature); - }); - }); - } + public async sign(data: Uint8Array): Promise<Uint8Array> { + const signature = crypto.sign(null, data, this._privateKeyPem); + return new Uint8Array(signature); + }
🧹 Nitpick comments (7)
packages/crypto/src/utils/utils.spec.ts (1)
10-12: Consider removing deprecated comments.The comment suggests these are temporary notes that "won't have much point later". Consider removing them before merging.
packages/crypto/src/index.spec.ts (1)
108-108: Fix Promise resolution assertion.The current assertion syntax doesn't properly test Promise resolution. Use
await expect(promise).resolvespattern.Apply this diff:
- expect(verifyJsonSignature(json, signature, verifier)).resolves; + await expect(verifyJsonSignature(json, signature, verifier)).resolves.toBeDefined();packages/crypto/src/keys/ed25519.ts (1)
70-70: Remove unnecessary non-null assertion.The
_privateKeyPemproperty is initialized in the constructor, so the!assertion is unnecessary.Apply this diff:
- private _privateKeyPem!: string; + private _privateKeyPem: string;packages/crypto/src/index.ts (4)
1-9: Solid barrel exports; minor naming polish for symmetryLGTM overall. Consider adding a non‑breaking alias for
fromBase64ToBytesto improve symmetry with the encoder names.Apply this diff to add an alias without breaking existing imports:
export { toUnpaddedBase64, computeHashBuffer, computeHashString, encodeCanonicalJson, toBinaryData, fromBinaryData, - fromBase64ToBytes, + fromBase64ToBytes, + // Alias for naming symmetry; keep original for BC + fromBase64ToBytes as base64ToBytes, } from './utils/data-types';
11-11: DisambiguateisValidAlgorithmor document scopeName is a bit generic; if it only relates to Ed25519 (or a fixed set), either narrow the name (e.g.,
isValidEd25519Algorithm) or add a brief doc comment inutils/constants.tsclarifying its domain.
1-20: Add package export map and mark side‑effect‑free for better DX and tree‑shakingExpose only intended entry points and discourage deep imports.
Add to packages/crypto/package.json:
{ "name": "@rocketchat/crypto", "sideEffects": false, "exports": { ".": { "types": "./dist/index.d.ts", "import": "./dist/index.js" } } }
13-13: Replaceexport *with type-only named re-exportspackages/crypto/src/contracts/key.ts only exports interfaces (VerifierKey, Signer) — replace the barrel line in packages/crypto/src/index.ts:
export * from './contracts/key';
with:
export type { VerifierKey, Signer } from './contracts/key';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/crypto/src/README.md(1 hunks)packages/crypto/src/contracts/key.ts(1 hunks)packages/crypto/src/index.spec.ts(3 hunks)packages/crypto/src/index.ts(1 hunks)packages/crypto/src/keys/ed25519.ts(1 hunks)packages/crypto/src/utils/utils.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-15T04:56:11.689Z
Learnt from: debdutdeb
PR: RocketChat/homeserver#190
File: packages/crypto/src/keys/ed25519.ts:34-52
Timestamp: 2025-09-15T04:56:11.689Z
Learning: Node.js crypto.verify function supports both synchronous and asynchronous (callback) modes. When a callback is provided as the fifth parameter, it uses libuv's threadpool for non-blocking operation.
Applied to files:
packages/crypto/src/keys/ed25519.ts
🧬 Code graph analysis (4)
packages/crypto/src/utils/utils.spec.ts (2)
packages/crypto/src/utils/data-types.ts (2)
fromBase64ToBytes(84-86)encodeCanonicalJson(63-82)packages/crypto/src/utils/keys.ts (4)
loadEd25519SignerFromSeed(11-18)signJson(27-36)verifyJsonSignature(39-49)loadEd25519VerifierFromPublicKey(20-25)
packages/crypto/src/keys/ed25519.ts (2)
packages/crypto/src/contracts/key.ts (2)
VerifierKey(10-23)Signer(27-32)packages/crypto/src/rfc/8410/ed25519-pem.ts (2)
ed25519PublicKeyRawToPem(76-88)ed25519PrivateKeyRawToPem(38-63)
packages/crypto/src/contracts/key.ts (1)
packages/crypto/src/utils/constants.ts (2)
DataType(5-5)SignatureType(7-7)
packages/crypto/src/index.spec.ts (1)
packages/core/src/utils/signJson.ts (2)
signJson(17-50)verifyJsonSignature(115-134)
🔇 Additional comments (3)
packages/crypto/src/contracts/key.ts (1)
1-33: LGTM! Well-structured key interfaces.The interfaces clearly define the cryptographic key abstractions with proper inheritance structure and documentation.
packages/crypto/src/README.md (1)
1-49: Well-written documentation with clear explanations.The README provides excellent documentation covering the package structure, interfaces, and utilities with clear explanations of the separation of concerns.
packages/crypto/src/index.ts (1)
15-20: Canonicalization confirmed: signJson and verifyJsonSignature both call encodeCanonicalJsonBoth functions call encodeCanonicalJson(jsonObject) in packages/crypto/src/utils/keys.ts (signJson at line 31; verifyJsonSignature at line 44).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/crypto/src/rfc/8410/ed25519-pem.ts (1)
16-18: PEM wrapping fix looks correct.
substring(i, i + 64)resolves the earlier line‑wrap bug. Good catch.
🧹 Nitpick comments (5)
packages/crypto/src/rfc/8410/ed25519-pem.ts (5)
34-36: Correct ASN.1 comment.Typo and structure: it should reference CurvePrivateKey and OCTET STRING.
- CTET - PrivateKey ::= OCTET { OCTET STRING } + CurvePrivateKey ::= OCTET STRING
71-74: RFC 8410: parameters must be ABSENT, not NULL (comment fix).Align the comment with the implementation (you already omit NULL in
algorithmIdentifierTlv).- parameters NULL + parameters ABSENT -- per RFC 8410, parameters MUST be absent
82-83: Comment typo.
subhjectPublicKey→subjectPublicKey.
77-79: Confirm input size expectations (32 vs 64 byte keys).Some libs hand around 64‑byte Ed25519 “secret keys” (seed+pub). Your check enforces 32 bytes (seed) per RFC 8410, which is correct. Verify callers won’t pass 64 bytes; if they might, slice the first 32 with a warning.
60-60: Make base64 encoding isomorphic (avoid Node-only Buffer)Buffer.from(...).toString('base64') is Node-only and will break browser builds without a Buffer polyfill — add a small isomorphic helper and replace both call sites in packages/crypto/src/rfc/8410/ed25519-pem.ts (ed25519PrivateKeyRawToPem and ed25519PublicKeyRawToPem).
Example helper:
function bytesToBase64(data: Uint8Array): string { if (typeof Buffer !== 'undefined') return Buffer.from(data).toString('base64'); if (typeof btoa !== 'undefined') { let s = ''; for (let i = 0; i < data.length; i++) s += String.fromCharCode(data[i]); return btoa(s); } throw new Error('No base64 encoder available'); }Example replacements:
-Buffer.from(oneAsymmetricKey).toString('base64') +bytesToBase64(oneAsymmetricKey)-Buffer.from(spki).toString('base64') +bytesToBase64(spki)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/crypto/src/rfc/8410/ed25519-pem.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/crypto/src/rfc/8410/ed25519-pem.ts (1)
packages/crypto/src/der/index.ts (5)
privateKeyVersionTlv(30-34)algorithmIdentifierTlv(108-111)octetStringTlv(89-106)sequenceOrderedTlv(58-86)bitStringTlv(114-126)
🔇 Additional comments (2)
packages/crypto/src/rfc/8410/ed25519-pem.ts (2)
38-63: PKCS#8 encoding looks correct for Ed25519.
- Version=0, AlgorithmIdentifier with OID and parameters absent, and privateKey as OCTET STRING of CurvePrivateKey (nested OCTET STRING) matches RFC 8410/5958.
Please confirm
algorithmIdentifierTlvencodes OID 1.3.101.112 with parameters absent (as suggested by your DER helpers).
80-86: SPKI structure is correct.AlgId + BIT STRING(public key), unused bits = 0, is per RFC 8410.
| enum KeyType { | ||
| private = 'PRIVATE KEY', | ||
| public = 'PUBLIC KEY', | ||
| } |
There was a problem hiding this comment.
Fix enum: reserved identifiers and private type leak in exported signature.
private/publicas enum members are reserved identifiers and will fail to compile.toPemexports a signature using a non‑exportedKeyType, which will break type declarations.
Apply this diff to use an inline string‑union and avoid the enum altogether:
-enum KeyType {
- private = 'PRIVATE KEY',
- public = 'PUBLIC KEY',
-}
-
-export function toPem(base64: string, type: KeyType): string {
+export function toPem(
+ base64: string,
+ type: 'PRIVATE KEY' | 'PUBLIC KEY',
+): string {- return toPem(
- Buffer.from(oneAsymmetricKey).toString('base64'),
- KeyType.private,
- );
+ return toPem(
+ Buffer.from(oneAsymmetricKey).toString('base64'),
+ 'PRIVATE KEY',
+ );- return toPem(Buffer.from(spki).toString('base64'), KeyType.public);
+ return toPem(Buffer.from(spki).toString('base64'), 'PUBLIC KEY');Also applies to: 14-14, 61-62, 87-87
🤖 Prompt for AI Agents
In packages/crypto/src/rfc/8410/ed25519-pem.ts around lines 9-12 (and also
touching lines 14, 61-62, 87), replace the private/public enum with an exported
inline string-union type (e.g. export type KeyType = 'PRIVATE KEY' | 'PUBLIC
KEY') and update all references to use that exported KeyType so compilation and
.d.ts generation succeed; remove the enum declaration and any uses of the
non-exported enum members, and ensure the toPem signature and any exported
functions/values reference the exported string-union type instead.
| // SubjectPublicKeyInfo -> SEQUENCE | ||
| const spki = sequenceOrderedTlv([algId, pubKeyBitString]); | ||
|
|
||
| return toPem(Buffer.from(spki).toString('base64'), KeyType.public); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same note as above for public key base64.
Swap to the isomorphic helper here as well if you adopt it.
🤖 Prompt for AI Agents
In packages/crypto/src/rfc/8410/ed25519-pem.ts around line 87, replace the
Node-specific Buffer.from(spki).toString('base64') call with the project’s
isomorphic base64 helper (e.g., base64Encode or the agreed helper) and pass its
result to toPem(..., KeyType.public); also add the corresponding import for that
helper at the top of the file so the function works in both browser and Node
environments.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
==========================================
+ Coverage 68.48% 69.41% +0.92%
==========================================
Files 58 62 +4
Lines 5210 5319 +109
==========================================
+ Hits 3568 3692 +124
+ Misses 1642 1627 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/crypto/src/utils/utils.spec.ts (3)
25-31: Don’t makedescribeasync; move async setup tobeforeAll.Async
describebreaks test discovery. InitializesignerinbeforeAllinstead.-describe('Signing and verifying payloads', async () => { - const seedFileContent = - 'ed25519 a_XRhW YjbSyfqQeGto+OFswt+XwtJUUooHXH5w+czSgawN63U'; - - const signer = await getSignerFromKeyContent(seedFileContent); +describe('Signing and verifying payloads', () => { + const seedFileContent = + 'ed25519 a_XRhW YjbSyfqQeGto+OFswt+XwtJUUooHXH5w+czSgawN63U'; + let signer: Awaited<ReturnType<typeof getSignerFromKeyContent>>; + beforeAll(async () => { + signer = await getSignerFromKeyContent(seedFileContent); + });
102-104: Await (and loosen) the rejection assertion.Await the promise and avoid brittle message matching.
- expect(verifyJsonSignature({}, signature, verifier)).rejects.toThrowError( - 'Invalid signature', - ); + await expect(verifyJsonSignature({}, signature, verifier)).rejects.toBeDefined();
85-86: Await the.resolvesassertion.Without
await(orreturn), the test can false‑pass.- expect(verifyJsonSignature({}, signature, signer)).resolves; + await expect(verifyJsonSignature({}, signature, signer)).resolves.toBeUndefined();
🧹 Nitpick comments (5)
packages/crypto/src/utils/utils.spec.ts (5)
1-1: ImportbeforeAll.-import { describe, expect, it } from 'bun:test'; +import { describe, expect, it, beforeAll } from 'bun:test';
13-22: Harden key-content parsing; avoid non-null assertion.Trim, split on any whitespace, validate presence, and drop the temporary comment.
-async function getSignerFromKeyContent(content: string) { - // biome-ignore lint/style/noNonNullAssertion: I can see pop won't fail, input isn't unknown -__- - const seed = content.split(' ').pop()!; - const seedBytes = fromBase64ToBytes(seed); - - //vvv - const signer = await loadEd25519SignerFromSeed(seedBytes); - - return signer; -} +async function getSignerFromKeyContent(content: string) { + const tokens = content.trim().split(/\s+/); + const seed = tokens.at(-1); + if (!seed) { + throw new Error('Invalid key content: missing seed base64'); + } + const seedBytes = fromBase64ToBytes(seed); + return loadEd25519SignerFromSeed(seedBytes); +}
76-86: Prefer verifying with a verifier key, not the signer instance.If accessible, derive a verifier from the public key to decouple tests from the signing interface.
If
signerexposes a public key, switch to:const verifier = await loadEd25519VerifierFromPublicKey(signer.publicKey, '0'); await expect(verifyJsonSignature({}, signature, verifier)).resolves.toBeUndefined();Confirm whether
Ed25519SigningKeyImplexposespublicKey(or an equivalent getter).
294-296: Remove duplicate test case.This entry duplicates the case at lines 269–272.
- { - input: { 'key\nnewLine': 1, 'key"quote': 2, 'key\\backslash': 3 }, - expected: '{"key\\nnewLine":1,"key\\"quote":2,"key\\\\backslash":3}', - },
10-12: Prune temporary comments.Drop the porting notes and stray
//vvv.Also applies to: 18-18
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
packages/core/src/events/m.room.message.edit.spec.ts(0 hunks)packages/core/src/events/m.room.message.spec.ts(0 hunks)packages/crypto/src/index.spec.ts(3 hunks)packages/crypto/src/utils/data-types.ts(1 hunks)packages/crypto/src/utils/utils.spec.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/core/src/events/m.room.message.spec.ts
- packages/core/src/events/m.room.message.edit.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/crypto/src/index.spec.ts
- packages/crypto/src/utils/data-types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/crypto/src/utils/utils.spec.ts (2)
packages/crypto/src/utils/data-types.ts (2)
fromBase64ToBytes(102-104)encodeCanonicalJson(63-100)packages/crypto/src/utils/keys.ts (4)
loadEd25519SignerFromSeed(11-18)signJson(27-36)verifyJsonSignature(39-49)loadEd25519VerifierFromPublicKey(20-25)
Summary by CodeRabbit