chore: use crypto package in services #166
Conversation
debdutdeb
force-pushed
the
fdr108-use-crytpo-everywhere
branch
2 times, most recently
from
e173524 to
513d80c
Compare
debdutdeb
force-pushed
the
fdr108-use-crytpo-everywhere
branch
from
53ce61f to
6e5a30e
Compare
Pull Request AnalysisWalkthroughThis pull request introduces a comprehensive cryptographic refactoring across multiple packages, transitioning from file-based key management to an in-memory, signer-based approach. The changes modernize key handling, signing, and verification utilities with a focus on Ed25519 cryptographic operations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ConfigService
participant SignerUtils
participant CryptoService
Client->>ConfigService: Request Signing Key
ConfigService->>SignerUtils: Load/Generate Signer
SignerUtils->>CryptoService: Create Ed25519 Signer
CryptoService-->>SignerUtils: Return Signer
SignerUtils-->>ConfigService: Return Cached Signer
ConfigService-->>Client: Provide Signer
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes are highly intricate, involving multiple packages, introducing new cryptographic abstractions, and fundamentally changing key management and signing workflows. The complexity stems from the comprehensive nature of the refactoring and the precision required in cryptographic implementations. Possibly Related PRs
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: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/federation-sdk/src/services/federation.service.ts (3)
85-93: Parse homeserver domains correctly (handles ports).Using split(':').pop() drops ports (e.g., example.com:8448 -> 8448). Extract substring after the first colon.
Apply:
- const residentServer = joinEvent.roomId.split(':').pop(); + const firstColon = joinEvent.roomId.indexOf(':'); + if (firstColon === -1) { + this.logger.debug(joinEvent.event, 'invalid room_id'); + throw new Error(`invalid room_id ${joinEvent.roomId}, no server_name part`); + } + const residentServer = joinEvent.roomId.slice(firstColon + 1);- const residentServer = inviteEvent.stateKey.split(':').pop(); + const firstColon = inviteEvent.stateKey.indexOf(':'); + if (firstColon === -1) { + throw new Error(`invalid state_key ${inviteEvent.stateKey}, no domain found, failed to send invite`); + } + const residentServer = inviteEvent.stateKey.slice(firstColon + 1);- if (server === this.configService.serverName) { + if (server === this.configService.serverName) { this.logger.info(`Skipping transaction to local server: ${server}`); continue; }Also applies to: 226-233, 254-256
259-263: Sign once, send many.signEvent mutates the event by adding a signature; doing it per-server is redundant.
- for (const server of servers) { + await this.stateService.signEvent(event); + for (const server of servers) { if (server === event.origin) { this.logger.info( `Skipping transaction to event origin: ${event.origin}`, ); continue; } @@ - // TODO: signing should happen here over local persisting - // should be handled in transaction queue implementation - await this.stateService.signEvent(event);
51-56: Build repeatedverquery params (ver=3&ver=4...) and update request encoderMatrix requires repeated
verparameters; current code emits keys likever3/ver4. Send an array of versions and update the request encoder to append repeated keys.
- Change in packages/federation-sdk/src/services/federation.service.ts (around lines 51–56):
- for (let ver = 3; ver <= 11; ver++) { - queryParams[`ver${ver === 1 ? '' : ver}`] = ver.toString(); - } + const versions = Array.from({ length: 9 }, (_, i) => String(3 + i)); + // update requestService to accept arrays and encode repeated keys + // e.g., { ver: ['3','4',...,'11'] } + // @ts-expect-error: extend type to Record<string, string|string[]> + queryParams.ver = versions;
- Update encoder in packages/federation-sdk/src/services/federation-request.service.ts (around lines 129–133): handle array values when building URLSearchParams:
- If value is Array, iterate and call params.append(key, String(v)) for each item; otherwise append single value.
packages/federation-sdk/src/services/state.service.ts (1)
817-821: Parse member domains correctly (handles ports).member.split(':').pop() drops ports. Use substring after the first colon.
- return this.getMembersOfRoom(roomId).then((members) => - members.map((member) => member.split(':').pop()!), - ); + return this.getMembersOfRoom(roomId).then((members) => + members.map((member) => { + const i = member.indexOf(':'); + return i === -1 ? member : member.slice(i + 1); + }), + );packages/federation-sdk/src/services/federation-request.service.ts (1)
1-11: Remove unused imports and fix import order.Unused imports detected: SigningKey (type), authorizationHeaders, computeAndMergeHash, extractURIfromURL, EncryptionValidAlgorithm, and the tweetnacl (nacl) import — remove them and reorder the remaining imports to satisfy the project's import-order lint rule (file: packages/federation-sdk/src/services/federation-request.service.ts, imports at lines 1–11).
🧹 Nitpick comments (34)
.vscode/settings.json (1)
18-26: Avoid redundant per-language formatter blocks or go fully explicitYou already set a global default formatter to Biome at Line 2. The new [json], [jsonc], and [javascript] blocks are redundant. Either:
- Keep the global default and remove these blocks (simpler), or
- Remove the global default and go fully explicit (also add [javascriptreact] and [typescriptreact]) to cover JSX/TSX.
Suggested (keep global default; remove redundancy):
-"[json]": { - "editor.defaultFormatter": "biomejs.biome" -}, -"[jsonc]": { - "editor.defaultFormatter": "biomejs.biome" -}, -"[javascript]": { - "editor.defaultFormatter": "biomejs.biome" -}If you prefer explicit per-language settings, drop the global default and add [javascriptreact]/[typescriptreact] instead.
packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
3-3: Drop unusedtimport if reverting to DTOs.Keeps imports minimal and avoids confusion.
-import { Elysia, t } from 'elysia'; +import { Elysia } from 'elysia';packages/crypto/package.json (1)
18-20: Consider pinning or widening with intent.Caret on a crypto primitive can cause surprise updates. Either pin exact (and rely on lockfile updates) or document the allowed range. Also consider
"sideEffects": falsefor better tree-shaking.benchmark-scripts/README.md (1)
11-13: Clarify how to run benchmarks end-to-end.Add commands for starting the server (if any) and executing the load script so readers can reproduce results without guessing.
packages/crypto/src/README.md (2)
9-11: Docs reference filenames that differ from the actual code.Update paths to match the codebase:
- constants.ts → utils/constants.ts
- signing-key.ts → contracts/key.ts
18-19: Implementation detail in docs is outdated.Docs say “Production uses nacl/libsodium,” but the package depends on @noble/ed25519. Align wording to avoid confusion.
benchmark-scripts/.gitignore (2)
13-16: Typo in log pattern.
_.loglikely intended to be*.log.Apply:
-_.log +*.log
1-35: Ignore Bun lockfile and typical bench artifacts.Add
bun.lockband any k6 outputs you generate.# dependencies (bun install) node_modules +bun.lockb @@ # logs logs -_.log +*.log report.[0-9]_.[0-9]_.[0-9]_.[0-9]_.json +# k6 outputs (if used) +*.csv +*.jsonbenchmark-scripts/tsconfig.json (2)
5-6: Redundant outDir with noEmit.
outDiris unused whennoEmit: true. Remove to avoid confusion.- "outDir": "dist",
10-15: Consider setting types for Bun/Node.If you reference
Buffer, timers, or Bun globals, add explicit types to avoid ambient drift.- "verbatimModuleSyntax": true, + "verbatimModuleSyntax": true, + "types": ["bun-types", "node"]packages/crypto/src/utils/constants.ts (1)
9-17: Simplify isValidAlgorithm and keep it constant-time-ish.For a single value, a direct comparison is clearer and avoids array allocation.
-export function isValidAlgorithm(alg: string): alg is EncryptionValidAlgorithm { - for (const validAlg of Object.values(EncryptionValidAlgorithm)) { - if (alg === validAlg) { - return true; - } - } - return false; -} +export function isValidAlgorithm(alg: string): alg is EncryptionValidAlgorithm { + return alg === EncryptionValidAlgorithm.ed25519; +}packages/crypto/src/der/index.ts (3)
32-34: Minor typo in comment.“legth” → “length”.
- 0x01 /* legth */, + 0x01 /* length */,
39-45: Remove conversational/copilot comments.Keeps the DER module tight and professional.
-// copilot: break _ed25519oid into bytes with hex codes, save to bytes array -// ^ thanks copilot -// const bytes = Uint8Array.of( 0x2b /* 1*40 + 3 = 43 = 0x2b */, 0x65, 0x70 /* 101, 112 */ );
108-111: RFC 8410: parameters MUST be absent, not NULL.Your code correctly omits parameters; adjust the comment to avoid implying NULL.
export const algorithmIdentifierTlv = sequenceOrderedTlv([ oidTlv, - // nullTlv, // parameters for ed25519 is NULL + // parameters MUST be absent for Ed25519 (RFC 8410 §3) ]);packages/federation-sdk/src/services/federation.service.ts (1)
114-116: Transaction id uniqueness.Date.now().toString() can collide across processes. Add randomness.
- const txnId = Date.now().toString(); + const txnId = `${Date.now()}_${crypto.randomUUID()}`;packages/federation-sdk/src/services/state.service.ts (1)
760-762: Polish comments.Minor typos hinder readability.
- // TODO: i know thisd is overcomplicated - //but writing this comment while not remembering what exactkly it does while not wanting to get my brain to do it either + // TODO: This is overcomplicated. Document the intent and simplify in a follow-up.benchmark-scripts/signing-and-verifications/server.ts (1)
30-36: Clarify streaming behavior for native engine.Currently returns success without doing anything when stream=true. If intentional, add a comment; otherwise, implement a streamed path or reject like tweetnacl.
Also applies to: 68-76
packages/federation-sdk/src/services/federation-request.service.spec.ts (1)
1-15: Remove unused import.The
naclimport on line 12 is not used in this test file.import * as core from '@hs/core'; -import * as nacl from 'tweetnacl'; import { ConfigService } from './config.service';packages/federation-sdk/src/services/signature-verification.service.spec.ts (1)
189-195: Avoid module mocking for constants.Mocking the module to change MAX_SIGNATURE_LENGTH_FOR_ED25519 could be fragile and affect other tests if run in parallel. Consider refactoring the service to accept this as a configuration parameter or use dependency injection.
packages/crypto/src/utils/utils.spec.ts (3)
10-12: Remove unnecessary TODO comments.These comments about porting tests should be removed as they don't provide value in the committed code.
-// NOTE(deb): listing file names as I port the tests -// can and should be removed later, won't have must point later -
13-22: Extract helper function outside of describe block.The
getSignerFromKeyContenthelper is defined inside the describe block but could be reused. Consider moving it to module scope.-// NOTE(deb): listing file names as I port the tests -// can and should be removed later, won't have must point later - -async function getSignerFromKeyContent(content: string) { +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; } // authentication.spec.ts (packages/core) describe('Signing and verifying payloads', async () => { - const seedFileContent = - 'ed25519 a_XRhW YjbSyfqQeGto+OFswt+XwtJUUooHXH5w+czSgawN63U'; - - const signer = await getSignerFromKeyContent(seedFileContent);
256-265: Document NaN/Infinity → null behavior in encodeCanonicalJsonencodeCanonicalJson uses JSON.stringify for primitives (so NaN and ±Infinity become null); add an explicit comment in packages/crypto/src/utils/data-types.ts (encodeCanonicalJson) and update docs/tests to call out this behavior.
packages/crypto/src/rfc/8410/ed25519-pem.ts (2)
23-37: Fix typo in comment.There's a typo in the ASN.1 structure comment.
- CTET + -- Note: PrivateKey ::= OCTET { OCTET STRING }
60-61: Add a bytes→base64 helper and replace ad‑hoc Buffer→base64 calls.packages/crypto/src/utils/data-types.ts already exports fromBase64ToBytes — add the inverse (bytesToBase64 that accepts a Uint8Array) and replace the Buffer.from(...).toString('base64') uses in packages/crypto/src/rfc/8410/ed25519-pem.ts (around lines 60–61 and 87) with that helper; implement platform-safe encoding (btoa in browsers, Buffer in Node) to centralize logic and avoid scattered ad‑hoc conversions.
packages/crypto/src/keys/ed25519.ts (1)
62-70: Avoid re‑declaring inherited property; pass version without creating a new class memberRedefining public version in the subclass can cause confusion. Inherit from base; just accept version as a param.
- private _privateKeyPem!: string; - constructor( - public version: string, - public readonly privateKey: Uint8Array, - publicKey: Uint8Array, - ) { + private _privateKeyPem!: string; + constructor( + version: string, + public readonly privateKey: Uint8Array, + publicKey: Uint8Array, + ) { super(version, publicKey); this._privateKeyPem = ed25519PrivateKeyRawToPem(privateKey); }packages/crypto/src/utils/data-types.ts (1)
1-1: Optional: drop crypto import from this module if tree-shaking is a concernOnly computeHashBuffer uses it; that’s fine as-is.
packages/federation-sdk/src/services/server.service.ts (2)
41-43: Nit: prefer Date.now()Tiny readability nit.
- valid_until_ts: new Date().getTime() + 60 * 60 * 24 * 1000, // 1 day + valid_until_ts: Date.now() + 24 * 60 * 60 * 1000, // 1 day
1-1: Resolve organizeImports CI errorNormalize imports if linter insists on a specific order/grouping.
packages/room/src/manager/event-wrapper.ts (2)
2-6: Import statement organization needs fixing.The pipeline failure indicates that import statements need to be reorganized. This is a formatting issue that should be addressed.
Run your formatter/linter to properly organize the imports according to the project's import ordering rules.
377-383: Potential edge case: Empty events array.The method assumes
eventsis non-empty when accessingevents[events.length - 1]. This could cause a runtime error if an empty array is passed.Add a guard clause to handle empty arrays:
addPrevEvents(events: PersistentEventBase<T>[]) { + if (events.length === 0) { + return this; + } this.rawEvent.prev_events.push(...events.map((e) => e.eventId)); if (this.rawEvent.depth <= events[events.length - 1].depth) { this.rawEvent.depth = events[events.length - 1].depth + 1; } return this; }packages/federation-sdk/src/services/signature-verification.service.ts (4)
1-9: Import organization issue detected.The pipeline failure indicates import statements need reorganization.
Run your formatter/linter to organize imports according to project conventions.
81-81: Empty method stub needs implementation or documentation.The
verifyRequestSignaturemethod is empty. If this is intentional, add a TODO comment or throw a NotImplementedError.Would you like me to help implement this method or add appropriate documentation/error handling?
- async verifyRequestSignature() {} + async verifyRequestSignature() { + // TODO: Implement request signature verification + throw new Error('Request signature verification not yet implemented'); + }
127-139: Consider parallel key fetching for better performance.The current implementation fetches keys sequentially, stopping at the first success. Consider fetching in parallel with a timeout for better performance when multiple keys are available.
- let verifier: VerifierKey | undefined; - for (const [keyId] of validSignatureEntries) { - try { - verifier = await this.getSignatureVerifierForServer(origin, keyId); - break; // found one, should be enough - } catch (error) { - this.logger.warn( - `Failed to get verifier for ${origin} with keyId ${keyId.id}: ${error instanceof Error ? error.message : String(error)}`, - ); - } - } + const verifierPromises = validSignatureEntries.map(async ([keyId]) => { + try { + return await this.getSignatureVerifierForServer(origin, keyId); + } catch (error) { + this.logger.warn( + `Failed to get verifier for ${origin} with keyId ${keyId.id}: ${error instanceof Error ? error.message : String(error)}`, + ); + return null; + } + }); + + const verifiers = await Promise.all(verifierPromises); + const verifier = verifiers.find(v => v !== null);
231-234: Use unknown instead of any for better type safety.Using
anybypasses TypeScript's type checking.- } catch (error: any) { + } catch (error: unknown) { this.logger.error( - `Error fetching public key: ${error.message}`, - error.stack, + `Error fetching public key: ${error instanceof Error ? error.message : String(error)}`, + error instanceof Error ? error.stack : undefined, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
benchmark-scripts/bun.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.vscode/settings.json(1 hunks)benchmark-scripts/.gitignore(1 hunks)benchmark-scripts/README.md(1 hunks)benchmark-scripts/package.json(1 hunks)benchmark-scripts/signing-and-verifications/script.js(1 hunks)benchmark-scripts/signing-and-verifications/server.ts(1 hunks)benchmark-scripts/tsconfig.json(1 hunks)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.spec.ts(0 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/tsconfig.json(0 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)packages/federation-sdk/src/services/config.service.ts(3 hunks)packages/federation-sdk/src/services/federation-request.service.spec.ts(3 hunks)packages/federation-sdk/src/services/federation-request.service.ts(2 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/server.service.ts(2 hunks)packages/federation-sdk/src/services/signature-verification.service.spec.ts(1 hunks)packages/federation-sdk/src/services/signature-verification.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 hunks)packages/homeserver/src/homeserver.module.ts(1 hunks)packages/room/src/manager/event-wrapper.ts(3 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- packages/crypto/src/tsconfig.json
- packages/crypto/src/index.spec.ts
🧰 Additional context used
🧬 Code graph analysis (16)
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/federation-sdk/src/services/federation-request.service.spec.ts (2)
packages/federation-sdk/src/services/config.service.ts (2)
ConfigService(70-147)version(94-96)packages/crypto/src/index.ts (2)
loadEd25519SignerFromSeed(16-16)fromBase64ToBytes(8-8)
benchmark-scripts/signing-and-verifications/server.ts (3)
packages/crypto/src/utils/keys.ts (1)
loadEd25519SignerFromSeed(11-18)benchmark-scripts/signing-and-verifications/script.js (2)
stream(7-7)engine(5-5)packages/crypto/src/utils/data-types.ts (1)
encodeCanonicalJson(63-82)
packages/federation-sdk/src/services/signature-verification.service.spec.ts (2)
packages/federation-sdk/src/services/signature-verification.service.ts (1)
SignatureVerificationService(56-239)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-968)
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/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)
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/federation-sdk/src/services/config.service.ts (3)
packages/crypto/src/contracts/key.ts (1)
Signer(25-29)packages/crypto/src/utils/keys.ts (1)
loadEd25519SignerFromSeed(11-18)packages/crypto/src/index.ts (2)
loadEd25519SignerFromSeed(16-16)fromBase64ToBytes(8-8)
packages/federation-sdk/src/services/server.service.ts (1)
packages/crypto/src/utils/keys.ts (1)
signJson(27-36)
packages/federation-sdk/src/services/federation-request.service.ts (3)
packages/federation-sdk/src/server-discovery/discovery.ts (1)
getHomeserverFinalAddress(77-165)packages/crypto/src/utils/keys.ts (1)
signJson(27-36)packages/core/src/utils/fetch.ts (1)
fetch(7-62)
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/federation-sdk/src/services/state.service.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
event(111-121)packages/crypto/src/utils/keys.ts (1)
signJson(27-36)packages/core/src/utils/signJson.ts (1)
signJson(17-50)
packages/crypto/src/contracts/key.ts (1)
packages/crypto/src/utils/constants.ts (2)
DataType(5-5)SignatureType(7-7)
packages/federation-sdk/src/services/signature-verification.service.ts (3)
packages/room/src/manager/event-wrapper.ts (3)
event(111-121)origin(91-97)redactedEvent(271-273)packages/crypto/src/contracts/key.ts (1)
VerifierKey(10-21)packages/crypto/src/utils/keys.ts (1)
loadEd25519VerifierFromPublicKey(20-25)
packages/room/src/manager/event-wrapper.ts (1)
packages/crypto/src/index.ts (1)
computeHashBuffer(3-3)
🪛 GitHub Actions: my-workflow
packages/federation-sdk/src/services/federation-request.service.spec.ts
[error] organizeImports: Import statements differs from the output.
packages/federation-sdk/src/services/signature-verification.service.spec.ts
[error] organizeImports: Import statements differs from the output.
packages/federation-sdk/src/services/config.service.ts
[error] organizeImports: Import statements differs from the output.
packages/federation-sdk/src/services/server.service.ts
[error] organizeImports: Import statements differs from the output.
packages/federation-sdk/src/services/federation-request.service.ts
[error] 1-1: bun lint:ci: Import statements differs from the output.
packages/federation-sdk/src/services/signature-verification.service.ts
[error] organizeImports: Import statements differs from the output.
[error] organizeImports: Import statements differs from the output.
packages/room/src/manager/event-wrapper.ts
[error] organizeImports: Import statements differs from the output.
🪛 Gitleaks (8.27.2)
packages/federation-sdk/src/services/signature-verification.service.spec.ts
[high] 69-69: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (31)
.vscode/settings.json (1)
17-17: Confirm VS Code support for typescript.enablePromptUseWorkspaceTsdkThis setting can cause an extra prompt for devs to switch to the workspace TypeScript. Please confirm your team’s minimum VS Code version supports it and that the prompt is desirable. Otherwise, consider omitting it since typescript.tsdk is already set.
tsconfig.json (2)
9-14: Excluding benchmark-scripts is appropriate.Keeps the root project clean from bench-only files. LGTM.
16-30: Formatting-only refactor to references is fine.No behavior change; improves readability.
packages/federation-sdk/src/services/state.service.ts (1)
369-379: Good: canonical signing excludes signatures/unsigned.The toSign destructure aligns with the spec and the new signJson API. The signature is stored with the signer id. LGTM.
Please ensure event.addSignature handles overwrite idempotently for repeated calls with the same key id.
packages/crypto/src/contracts/key.ts (1)
9-29: Interfaces look solid.The split between VerifierKey and Signer is clean; id typing via template literal is nice.
packages/crypto/src/utils/keys.ts (1)
27-36: Replace browser btoa/atob with Buffer-based base64 helpers; confirm @noble/ed25519 seed API
- Replace toUnpaddedBase64/fromBase64ToBytes in packages/crypto/src/utils/data-types.ts (btoa/atob found at lines 55 and 85) with Buffer-based implementations. Apply patch:
-export function toUnpaddedBase64( - value: Uint8Array | Buffer, - options: { urlSafe?: boolean } = { urlSafe: false }, -): string { - const hash = btoa(String.fromCharCode(...value)).replace(/=+$/, ''); +export function toUnpaddedBase64( + value: Uint8Array | Buffer, + options: { urlSafe?: boolean } = { urlSafe: false }, +): string { + const hash = Buffer.from(value).toString('base64').replace(/=+$/, ''); if (!options.urlSafe) return hash; return hash.replace(/\+/g, '-').replace(/\//g, '_'); } -export function fromBase64ToBytes(base64: string): Uint8Array { - return Uint8Array.from(atob(base64), (c) => c.charCodeAt(0)); -} +export function fromBase64ToBytes(base64: string): Uint8Array { + return new Uint8Array(Buffer.from(base64, 'base64')); +}
- Confirm which @noble/ed25519 version is pinned and whether it supports seed-derived keypair (e.g., keygenAsync(seed) / keyPairFromSeed/getPublicKey). node_modules/@noble/ed25519 was not present in the sandbox run, so verify the dependency and update loadEd25519SignerFromSeed if the API differs.
benchmark-scripts/signing-and-verifications/server.ts (1)
4-6: No change — encodeCanonicalJson is re-exported from package entrypackages/crypto/src/index.ts re-exports encodeCanonicalJson from ./utils/data-types, so the current import from '../../packages/crypto/src' is valid.
Likely an incorrect or invalid review comment.
packages/federation-sdk/src/services/federation-request.service.spec.ts (3)
83-131: Good test coverage for signed PUT request.The test comprehensively validates the signing flow, including URL construction, method, and Authorization header format with all required X-Matrix components.
133-166: Good test coverage for signed GET request.The test properly validates GET request signing with query parameters, ensuring the query string is included in the signature calculation.
1-16: Fix import organization in packages/federation-sdk/src/services/federation-request.service.spec.tsReorder imports to follow the project's import-order conventions and re-run CI. No explicit import-order rule was found in biome.jsonc — run the project's Biome formatter/linter or confirm the canonical grouping and update the file accordingly.
packages/federation-sdk/src/services/federation-request.service.ts (2)
29-119: Well-implemented Matrix federation request authentication.The implementation correctly follows the Matrix specification for request authentication, properly constructing the auth payload, signing it, and formatting the X-Matrix Authorization header.
89-97: Don't pass a function to logger.debug; guard expensive debug object with a level check.packages/core/src/utils/logger.ts creates/exports a pino logger (createLogger returns logger.child). There is no support in this repo for passing a function to logger.debug, so the proposed function-arg diff is not applicable. Replace with an explicit level guard in packages/federation-sdk/src/services/federation-request.service.ts (lines 89–97), e.g.:
if (this.logger.level === 'debug') {
this.logger.debug(
{ method, body, headers, url: url.toString() },
'making http request',
);
}Prefer using pino's level-check helper (if available) instead of direct string comparisons.
Likely an incorrect or invalid review comment.
packages/federation-sdk/src/services/signature-verification.service.spec.ts (3)
53-102: Excellent test setup with proper mocking.The test setup properly mocks the fetch function for deterministic testing and correctly restores the original fetch after tests to prevent side effects.
172-216: Comprehensive invalid signature testing.Good coverage of multiple failure modes including invalid base64, incorrect signature length, and invalid signatures. The dynamic mocking of MAX_SIGNATURE_LENGTH_FOR_ED25519 is a clever testing technique.
1-16: Fix import ordering in packages/federation-sdk/src/services/signature-verification.service.spec.ts
- CI reports import-statement organization issues in that file (lines 1–16).
- Verification in the sandbox found no import/format configuration; run the project's import-sorting/formatter (biome/eslint/prettier or eslint) or reorder imports to match project rules and re-run CI.
packages/crypto/src/utils/utils.spec.ts (1)
106-328: Excellent comprehensive test coverage for canonical JSON.The test suite thoroughly covers edge cases including Unicode, escape sequences, special characters in keys, nested structures, empty values, and special number values (NaN, Infinity). This is exemplary test coverage.
packages/crypto/src/rfc/8410/ed25519-pem.ts (2)
38-63: Well-implemented Ed25519 private key PEM encoding.The implementation correctly follows RFC 8410 for encoding Ed25519 private keys with proper ASN.1 structure, including the double OCTET STRING wrapping as specified.
76-88: Well-implemented Ed25519 public key PEM encoding.The implementation correctly follows the SubjectPublicKeyInfo structure for Ed25519 public keys.
packages/federation-sdk/src/services/config.service.ts (3)
110-116: Cache check LGTMEarly return when signer is cached is correct.
138-146: Key id getter LGTMReusing getSigningKey() and returning signer.id is correct.
1-1: Resolve organizeImports CI errorVerification failed: pnpm errored ("--workspace-root may only be used inside a workspace") and bunx not found. Run the formatter/linter from the repository root and commit the normalized import order for packages/federation-sdk/src/services/config.service.ts (e.g. from repo root: pnpm -w lint:fix || npx eslint --fix packages/federation-sdk/src/services/config.service.ts; then pnpm -w lint).
packages/crypto/src/utils/data-types.ts (3)
62-82: Canonical JSON encoder LGTMDeterministic ordering and primitive handling look correct.
21-37: Binary conversion helpers LGTMCovers string, Uint8Array, ArrayBuffer, and views correctly.
13-19: Hash-to-string helper LGTMUnpadded base64 hash is useful; will benefit from the Buffer-based base64 fix above.
packages/federation-sdk/src/services/server.service.ts (2)
32-44: Key exposure structure LGTMverify_keys keyed by signer.id with unpadded base64 public key matches expected shape.
46-57: OK — signing call is correct hereThe signJson used by ServerService is packages/crypto/src/utils/keys.ts (it signs the canonicalized object as-is and does not strip fields). packages/federation-sdk/src/services/server.service.ts builds the response without any signatures or unsigned fields before calling signJson, so the signed payload already complies with the Matrix requirement to exclude signatures/unsigned.
packages/crypto/src/index.ts (1)
1-20: Re-exports LGTMAPI surface looks coherent and consistent with internal usage.
packages/room/src/manager/event-wrapper.ts (2)
24-30: Good improvement: Domain extraction validation added.The addition of proper error handling for missing colons in identifiers prevents potential runtime errors. The comment clarifies that port numbers are correctly handled.
284-284: LGTM: Using centralized hash computation.The refactoring to use
computeHashBufferfrom the crypto package is a good improvement that consolidates hash computation logic.packages/federation-sdk/src/services/signature-verification.service.ts (2)
37-37: Well-documented constant with clear mathematical basis.Great job documenting the calculation for the Ed25519 signature length constant. The mathematical breakdown helps future maintainers understand the value.
156-160: Good validation: Signature length check prevents processing invalid data early.The length validation against
MAX_SIGNATURE_LENGTH_FOR_ED25519is a good defensive programming practice that catches malformed signatures early.
| { | ||
| "name": "benchmark-scripts", | ||
| "module": "index.ts", | ||
| "type": "module", | ||
| "private": true, | ||
| "devDependencies": { | ||
| "@types/bun": "latest", | ||
| "@types/express": "^5.0.3", | ||
| "express": "^5.1.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "typescript": "^5" | ||
| }, | ||
| "scripts": { | ||
| "crypto": "bun run ./signing-and-verifications/server.ts" | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add missing dependency and tidy package fields.
- benchmark server imports tweetnacl but it’s not declared. Add it to dependencies.
- Optional: “module” isn’t generally used by Bun/Node here; prefer “main” or remove.
Suggested patch:
{
"name": "benchmark-scripts",
- "module": "index.ts",
+ "main": "index.ts",
"type": "module",
"private": true,
+ "dependencies": {
+ "tweetnacl": "^1.0.3"
+ },
"devDependencies": {
"@types/bun": "latest",
"@types/express": "^5.0.3",
"express": "^5.1.0"
},
"peerDependencies": {
"typescript": "^5"
},Quick check:
🏁 Script executed:
#!/bin/bash
jq -r '.dependencies, .devDependencies' benchmark-scripts/package.json | rg -n 'tweetnacl' || echo "tweetnacl missing"Length of output: 17
Add missing runtime dependency (tweetnacl) to benchmark-scripts/package.json
Confirmed: quick-check shows tweetnacl is missing — add it to "dependencies". Optional: replace "module" with "main" (or remove) if you want a conventional entry field.
File: benchmark-scripts/package.json (lines 1-17)
{
"name": "benchmark-scripts",
- "module": "index.ts",
+ "main": "index.ts",
"type": "module",
"private": true,
+ "dependencies": {
+ "tweetnacl": "^1.0.3"
+ },
"devDependencies": {
"@types/bun": "latest",
"@types/express": "^5.0.3",
"express": "^5.1.0"
},
"peerDependencies": {
"typescript": "^5"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "name": "benchmark-scripts", | |
| "module": "index.ts", | |
| "type": "module", | |
| "private": true, | |
| "devDependencies": { | |
| "@types/bun": "latest", | |
| "@types/express": "^5.0.3", | |
| "express": "^5.1.0" | |
| }, | |
| "peerDependencies": { | |
| "typescript": "^5" | |
| }, | |
| "scripts": { | |
| "crypto": "bun run ./signing-and-verifications/server.ts" | |
| } | |
| } | |
| { | |
| "name": "benchmark-scripts", | |
| "main": "index.ts", | |
| "type": "module", | |
| "private": true, | |
| "dependencies": { | |
| "tweetnacl": "^1.0.3" | |
| }, | |
| "devDependencies": { | |
| "@types/bun": "latest", | |
| "@types/express": "^5.0.3", | |
| "express": "^5.1.0" | |
| }, | |
| "peerDependencies": { | |
| "typescript": "^5" | |
| }, | |
| "scripts": { | |
| "crypto": "bun run ./signing-and-verifications/server.ts" | |
| } | |
| } |
🤖 Prompt for AI Agents
In benchmark-scripts/package.json around lines 1 to 17, the package is missing
the runtime dependency "tweetnacl" which is required by the benchmark scripts;
add a "dependencies" section (or extend it if present) with "tweetnacl":
"latest" (or a specific semver) so the runtime can install it, and optionally
replace the nonstandard "module" field with "main" (or remove it) to use a
conventional entry field.
| const messagelength = __ENV.MESSAGE_LENGTH || '1024'; | ||
| const engine = __ENV.ENGINE || 'tweetnacl'; | ||
|
|
||
| const stream = !!__ENV.STREAM; | ||
|
|
||
| return JSON.stringify({ | ||
| message: messagelength, | ||
| api: { | ||
| engine, | ||
| stream, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Fix env parsing and generate actual payload size.
- STREAM parsing treats "false" as true.
- message currently equals the string "1024", not a payload of that size.
- const messagelength = __ENV.MESSAGE_LENGTH || '1024';
- const engine = __ENV.ENGINE || 'tweetnacl';
- const stream = !!__ENV.STREAM;
+ const messagelength = parseInt(__ENV.MESSAGE_LENGTH || '1024', 10);
+ const engine = __ENV.ENGINE || 'tweetnacl';
+ const stream = String(__ENV.STREAM || '').toLowerCase() === 'true';
@@
- return JSON.stringify({
- message: messagelength,
+ const message = 'a'.repeat(Number.isFinite(messagelength) ? messagelength : 1024);
+ return JSON.stringify({
+ message,
api: {
engine,
stream,
},
});Also applies to: 22-27
🤖 Prompt for AI Agents
In benchmark-scripts/signing-and-verifications/script.js around lines 4-15 (and
similarly lines 22-27), ENV parsing is wrong: STREAM treats the string "false"
as truthy and MESSAGE_LENGTH is left as a string instead of generating a payload
of that size; update parsing so STREAM is converted to a proper boolean (e.g.,
treat 'false', '0', '' as false, otherwise true) and parse MESSAGE_LENGTH as an
integer (use parseInt with a numeric default like 1024), then generate an actual
message payload of that length (for example repeat a single character or
generate a buffer/string of the parsed size) and return that payload rather than
the length string; apply the same fixes to the code block at lines 22-27.
| function handleTweetnaclSignAndVerify(message: Uint8Array) { | ||
| const signature = nacl.sign.detached(message, tweetKeyPair.secretKey); | ||
|
|
||
| nacl.sign.detached.verify(message, signature, tweetKeyPair.publicKey); | ||
| } |
There was a problem hiding this comment.
Check the result of tweetnacl.verify.
tweetnacl’s verify returns a boolean; currently ignored.
function handleTweetnaclSignAndVerify(message: Uint8Array) {
const signature = nacl.sign.detached(message, tweetKeyPair.secretKey);
-
- nacl.sign.detached.verify(message, signature, tweetKeyPair.publicKey);
+ const ok = nacl.sign.detached.verify(message, signature, tweetKeyPair.publicKey);
+ if (!ok) throw new Error('Invalid signature');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function handleTweetnaclSignAndVerify(message: Uint8Array) { | |
| const signature = nacl.sign.detached(message, tweetKeyPair.secretKey); | |
| nacl.sign.detached.verify(message, signature, tweetKeyPair.publicKey); | |
| } | |
| function handleTweetnaclSignAndVerify(message: Uint8Array) { | |
| const signature = nacl.sign.detached(message, tweetKeyPair.secretKey); | |
| const ok = nacl.sign.detached.verify(message, signature, tweetKeyPair.publicKey); | |
| if (!ok) throw new Error('Invalid signature'); | |
| } |
🤖 Prompt for AI Agents
In benchmark-scripts/signing-and-verifications/server.ts around lines 22 to 26,
the boolean return value of nacl.sign.detached.verify is currently ignored;
capture the result into a variable and handle the failure path (e.g., if verify
returns false, throw an Error or return a failure indicator and/or log the
verification failure) so callers can react to a bad signature. Ensure the
function returns an explicit success/failure value or throws on verification
failure and include a clear error message when verification fails.
| "dependencies": { | ||
| "@noble/ed25519": "^3.0.0" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ESM-only dep + dual export mapping can break require() consumers.
@noble/ed25519 v3 is ESM-only. If our build outputs ESM, the "require" export will fail at runtime; if it outputs CJS, requiring an ESM-only dep will fail at build/run. Choose one:
- ESM-only: add
"type": "module", drop the"require"condition. - Dual-build: ship a CJS artifact (e.g., dist/index.cjs) and map
"require"to it.
Minimal ESM-only tweak:
{
"name": "@hs/crypto",
"version": "0.0.1",
+ "type": "module",
"scripts": {
@@
"exports": {
".": {
"types": "./dist/index.d.ts",
- "import": "./dist/index.js",
- "require": "./dist/index.js"
+ "import": "./dist/index.js"
}
},Run this script to confirm module kind and any CJS consumers:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "TS module setting (crypto):"
if [ -f packages/crypto/tsconfig.json ]; then jq -r '.compilerOptions.module // empty' packages/crypto/tsconfig.json || true; fi
echo "Imports of @noble/ed25519:"
rg -n --glob 'packages/crypto/**' -C1 "@noble/ed25519"
echo "Downstream require('@hs/crypto') call sites:"
rg -n "require\\(['\"]@hs/crypto" -g '!**/dist/**' -C2 || trueLength of output: 575
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== packages/crypto/package.json ==="
cat packages/crypto/package.json || true
echo
echo "=== Parsed fields (packages/crypto/package.json) ==="
jq -r '{name:.name,version:.version,type:.type,main:.main,module:.module,exports:.exports,scripts:.scripts}' packages/crypto/package.json || true
echo
echo "=== packages/crypto/tsconfig.json ==="
if [ -f packages/crypto/tsconfig.json ]; then jq -C . packages/crypto/tsconfig.json || cat packages/crypto/tsconfig.json; else echo "no tsconfig.json"; fi
echo
echo "=== Files importing @noble/ed25519 under packages/crypto ==="
rg -n --glob 'packages/crypto/**' -C2 "@noble/ed25519" || true
echo
echo "=== Search for require('@hs/crypto') across repo (excluding node_modules/dist) ==="
rg -n "require\\(['\"]@hs/crypto['\"]\\)" -S -g '!**/node_modules/**' -g '!**/dist/**' -C2 || true
echo
echo "=== Search for imports from '@hs/crypto' across repo ==="
rg -n "from ['\"]@hs/crypto['\"]" -S -g '!**/node_modules/**' -g '!**/dist/**' -C2 || trueLength of output: 3023
Action required — resolve ESM/CJS mismatch in packages/crypto
packages/crypto depends on @noble/ed25519@^3.0.0 (ESM-only). packages/crypto/package.json maps both "import" and "require" to ./dist/index.js and has no "type": "module", so the package as-published will break either require() consumers or runtime resolution of the ESM-only dependency.
- ESM-only: add "type": "module" to packages/crypto/package.json and remove the "require" condition from exports.
- Dual-build: produce a CJS artifact (e.g., dist/index.cjs) and map "require" to it while keeping "import" → ./dist/index.js.
Locations: packages/crypto/package.json; import usage: packages/crypto/src/utils/keys.ts.
🤖 Prompt for AI Agents
In packages/crypto/package.json around lines 18-20, the package currently maps
both "import" and "require" to ./dist/index.js while depending on @noble/ed25519
(ESM-only), causing an ESM/CJS mismatch; fix by either (A) making the package
ESM-only: add "type": "module" to package.json and remove the "require"
condition from "exports" so only "import" -> ./dist/index.js remains, or (B)
produce a dual build: compile a CJS artifact (e.g., dist/index.cjs), keep
"import" -> ./dist/index.js and map "require" -> ./dist/index.cjs in "exports",
and update your build scripts (and possibly rollup/tsconfig) to emit both files;
also check packages/crypto/src/utils/keys.ts import usage to ensure it uses ES
module syntax if you choose option A.
| 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')); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fix: crypto.verify is synchronous; current callback usage won’t compile/run
Node’s crypto.verify(null, data, key, sig) returns boolean (no callback). Replace the Promise wrapper with a simple sync call and throw on false.
Apply:
- 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 ok = crypto.verify(null, data, this._publicKeyPem, signature);
+ if (!ok) throw new Error('Invalid signature');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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 ok = crypto.verify(null, data, this._publicKeyPem, signature); | |
| if (!ok) throw new Error('Invalid signature'); | |
| } |
🤖 Prompt for AI Agents
In packages/crypto/src/keys/ed25519.ts around lines 26 to 44, the current
implementation wraps crypto.verify in a Promise and supplies a callback, but
Node's crypto.verify is synchronous and returns a boolean; replace the Promise
wrapper with a direct synchronous call: call crypto.verify(...) with the same
arguments, if it returns true simply return (the method is async so returning
void is fine), and if it returns false throw new Error('Invalid signature');
ensure any thrown errors propagate to the caller.
| private async getOrFetchPublicKey( | ||
| serverName: string, | ||
| keyId: string, | ||
| ): Promise<KeyData | null | undefined> { | ||
| const cacheKey = `${serverName}:${keyId}`; | ||
|
|
||
| if (this.cachedKeys.has(cacheKey)) { | ||
| return this.cachedKeys.get(cacheKey); | ||
| } | ||
|
|
||
| try { | ||
| const response = await fetch( | ||
| `https://${serverName}/_matrix/key/v2/server`, | ||
| ); | ||
|
|
||
| if (!response.ok) { | ||
| this.logger.error( | ||
| `Failed to fetch keys from ${serverName}: ${response.status}`, | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| const keyData = (await response.json()) as KeyData; | ||
|
|
||
| this.cachedKeys.set(cacheKey, keyData); | ||
|
|
||
| return keyData; | ||
| } catch (error: any) { | ||
| this.logger.error( | ||
| `Error fetching public key: ${error.message}`, | ||
| error.stack, | ||
| ); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Cache invalidation strategy missing.
The cache implementation doesn't have any TTL or invalidation mechanism. Keys could become stale if a server rotates its keys.
Consider implementing cache expiration:
- private cachedKeys = new Map<string, KeyData>();
+ private cachedKeys = new Map<string, { data: KeyData; timestamp: number }>();
+ private readonly CACHE_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours
private async getOrFetchPublicKey(
serverName: string,
keyId: string,
): Promise<KeyData | null | undefined> {
const cacheKey = `${serverName}:${keyId}`;
- if (this.cachedKeys.has(cacheKey)) {
- return this.cachedKeys.get(cacheKey);
+ const cached = this.cachedKeys.get(cacheKey);
+ if (cached && Date.now() - cached.timestamp < this.CACHE_TTL_MS) {
+ return cached.data;
}
// ... fetch logic ...
const keyData = (await response.json()) as KeyData;
- this.cachedKeys.set(cacheKey, keyData);
+ this.cachedKeys.set(cacheKey, { data: keyData, timestamp: Date.now() });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async getOrFetchPublicKey( | |
| serverName: string, | |
| keyId: string, | |
| ): Promise<KeyData | null | undefined> { | |
| const cacheKey = `${serverName}:${keyId}`; | |
| if (this.cachedKeys.has(cacheKey)) { | |
| return this.cachedKeys.get(cacheKey); | |
| } | |
| try { | |
| const response = await fetch( | |
| `https://${serverName}/_matrix/key/v2/server`, | |
| ); | |
| if (!response.ok) { | |
| this.logger.error( | |
| `Failed to fetch keys from ${serverName}: ${response.status}`, | |
| ); | |
| return null; | |
| } | |
| const keyData = (await response.json()) as KeyData; | |
| this.cachedKeys.set(cacheKey, keyData); | |
| return keyData; | |
| } catch (error: any) { | |
| this.logger.error( | |
| `Error fetching public key: ${error.message}`, | |
| error.stack, | |
| ); | |
| return null; | |
| } | |
| } | |
| private cachedKeys = new Map<string, { data: KeyData; timestamp: number }>(); | |
| private readonly CACHE_TTL_MS = 24 * 60 * 60 * 1000; // 24 hours | |
| private async getOrFetchPublicKey( | |
| serverName: string, | |
| keyId: string, | |
| ): Promise<KeyData | null | undefined> { | |
| const cacheKey = `${serverName}:${keyId}`; | |
| const cached = this.cachedKeys.get(cacheKey); | |
| if (cached && Date.now() - cached.timestamp < this.CACHE_TTL_MS) { | |
| return cached.data; | |
| } | |
| try { | |
| const response = await fetch( | |
| `https://${serverName}/_matrix/key/v2/server`, | |
| ); | |
| if (!response.ok) { | |
| this.logger.error( | |
| `Failed to fetch keys from ${serverName}: ${response.status}`, | |
| ); | |
| return null; | |
| } | |
| const keyData = (await response.json()) as KeyData; | |
| this.cachedKeys.set(cacheKey, { data: keyData, timestamp: Date.now() }); | |
| return keyData; | |
| } catch (error: any) { | |
| this.logger.error( | |
| `Error fetching public key: ${error.message}`, | |
| error.stack, | |
| ); | |
| return null; | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: my-workflow
[error] organizeImports: Import statements differs from the output.
[error] organizeImports: Import statements differs from the output.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/signature-verification.service.ts around
lines 204 to 238, the cachedKeys lookup never expires entries so rotated keys
may be served indefinitely; implement a TTL-based cache by storing alongside
each KeyData a timestamp (or wrap in an object {data, storedAt}), introduce a
configurable TTL constant or constructor option, and on getOrFetchPublicKey
check the storedAt + TTL and treat expired entries as a miss (delete/overwrite
and re-fetch); ensure you set storedAt when caching, and consider exposing a
method to force-refresh or to invalidate an entry on explicit errors.
| const response = await fetch( | ||
| `https://${serverName}/_matrix/key/v2/server`, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security consideration: HTTPS-only fetching without fallback.
The hardcoded HTTPS protocol is good for security, but consider adding a timeout to prevent hanging requests.
Add a timeout to the fetch request:
const response = await fetch(
`https://${serverName}/_matrix/key/v2/server`,
+ {
+ signal: AbortSignal.timeout(10000) // 10 second timeout
+ }
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch( | |
| `https://${serverName}/_matrix/key/v2/server`, | |
| ); | |
| const response = await fetch( | |
| `https://${serverName}/_matrix/key/v2/server`, | |
| { | |
| signal: AbortSignal.timeout(10000) // 10 second timeout | |
| } | |
| ); |
🧰 Tools
🪛 GitHub Actions: my-workflow
[error] organizeImports: Import statements differs from the output.
[error] organizeImports: Import statements differs from the output.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/signature-verification.service.ts around
lines 215 to 217, the fetch to `https://${serverName}/_matrix/key/v2/server`
lacks a request timeout which can cause the process to hang; update the code to
create an AbortController, start a timer (e.g., 5–10s) that calls
controller.abort() on expiry, pass controller.signal to fetch, clear the timer
on success, and handle the abort error path to surface a clear timeout error
while keeping the HTTPS-only URL unchanged.
| async ({ params, query }: any) => { | ||
| const { roomId, userId } = params; | ||
|
|
||
| const { ver } = query; | ||
|
|
||
| return profilesService.makeJoin(roomId, userId, ver ?? ['1']) as any; | ||
| console.log(ver); | ||
|
|
||
| return profilesService.makeJoin(roomId, userId, ['10']) as any; | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t hard‑code Matrix make_join version; honor query.ver and remove debug logging.
Hard‑coding ['10'] breaks federation negotiation per spec and may cause interop failures. Also avoid : any on the handler arg and remove console.log.
Apply:
- async ({ params, query }: any) => {
+ async ({ params, query }) => {
const { roomId, userId } = params;
- const { ver } = query;
-
- console.log(ver);
-
- return profilesService.makeJoin(roomId, userId, ['10']) as any;
+ const { ver } = query;
+ const versions = Array.isArray(ver) ? ver : ver ? [ver] : ['1'];
+ return profilesService.makeJoin(roomId, userId, versions) as any;
},Verification script to ensure no other make_join routes ignore ver and to locate DTOs:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 "_matrix/federation/v1/make_join"
rg -nP -C3 'MakeJoin(Query|Params|Response)Dto|ErrorResponseDto' --type=tsLength of output: 23482
Don’t hard‑code Matrix make_join version; honor query.ver and remove debug logging.
Hard‑coding ['10'] breaks federation negotiation — MakeJoinQueryDto and tests expect ver to be passed through (array or repeated params). Remove the debug log and avoid : any.
- async ({ params, query }: any) => {
+ async ({ params, query }) => {
const { roomId, userId } = params;
- const { ver } = query;
-
- console.log(ver);
-
- return profilesService.makeJoin(roomId, userId, ['10']) as any;
+ const { ver } = query;
+ const versions = Array.isArray(ver) ? ver : ver ? [ver] : ['1'];
+ return profilesService.makeJoin(roomId, userId, versions) as any;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async ({ params, query }: any) => { | |
| const { roomId, userId } = params; | |
| const { ver } = query; | |
| return profilesService.makeJoin(roomId, userId, ver ?? ['1']) as any; | |
| console.log(ver); | |
| return profilesService.makeJoin(roomId, userId, ['10']) as any; | |
| }, | |
| async ({ params, query }) => { | |
| const { roomId, userId } = params; | |
| const { ver } = query; | |
| const versions = Array.isArray(ver) ? ver : ver ? [ver] : ['1']; | |
| return profilesService.makeJoin(roomId, userId, versions) as any; | |
| }, |
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/federation/profiles.controller.ts around
lines 74 to 82, remove the console.log and the use of `: any`, and stop
hard‑coding the make_join version; instead pass the request query.ver through to
profilesService.makeJoin. Ensure you accept ver as either an array or repeated
query param (if it's a string convert to a single‑element array), type the
handler params and return value properly (use the MakeJoinQueryDto or the
correct typed field for ver), and call return profilesService.makeJoin(roomId,
userId, verArray) without casting to any.
| params: t.Any(), | ||
| query: t.Any(), | ||
| response: { | ||
| 200: MakeJoinResponseDto, | ||
| 400: ErrorResponseDto, | ||
| 200: t.Any(), | ||
| 400: t.Any(), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restore runtime validation: replace t.Any() with DTO schemas.
Using t.Any() drops input/output validation on a federated endpoint. Re‑use the existing DTOs for strong contracts.
- params: t.Any(),
- query: t.Any(),
+ params: MakeJoinParamsDto,
+ query: MakeJoinQueryDto,
response: {
- 200: t.Any(),
- 400: t.Any(),
+ 200: MakeJoinResponseDto,
+ 400: ErrorResponseDto,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| params: t.Any(), | |
| query: t.Any(), | |
| response: { | |
| 200: MakeJoinResponseDto, | |
| 400: ErrorResponseDto, | |
| 200: t.Any(), | |
| 400: t.Any(), | |
| }, | |
| params: MakeJoinParamsDto, | |
| query: MakeJoinQueryDto, | |
| response: { | |
| 200: MakeJoinResponseDto, | |
| 400: ErrorResponseDto, | |
| }, |
🤖 Prompt for AI Agents
In packages/homeserver/src/controllers/federation/profiles.controller.ts around
lines 84 to 89, replace the four t.Any() usages with the actual runtime DTO
schemas: use the request params DTO schema for params, the query DTO schema for
query, the success response DTO schema for 200, and the error response DTO
schema for 400. Import the existing runtime validator/schema objects (the DTOs
used elsewhere in this controller/module) and reference them here so the route
enforces the correct runtime input/output validation rather than t.Any().
| } | ||
|
|
||
| const config = new ConfigService({ | ||
| signingKey: process.env.SIGNING_KEY || undefined, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix always-true thumbnails flag; confirm SIGNING_KEY format.
- Line 75 currently coerces to true regardless of env. Respect the env override.
- Please confirm the expected env format for SIGNING_KEY (e.g., algorithm:version:seed) matches ConfigService.getSigningKey(), or this will fail at runtime.
Apply:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true,
+ enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS
+ ? process.env.MEDIA_ENABLE_THUMBNAILS === 'true'
+ : true,Verify usage:
Also applies to: 75-75
🏁 Script executed:
#!/bin/bash
# Inspect how ConfigService parses/uses the signing key
rg -nC3 -g 'packages/federation-sdk/**' 'getSigningKey|signingKey|Signer|SIGNING_KEY'Length of output: 11525
Fix always-true thumbnails flag; SIGNING_KEY is space-separated (not colon-separated)
- Apply this diff to respect MEDIA_ENABLE_THUMBNAILS (default true):
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true,
+ enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS
+ ? process.env.MEDIA_ENABLE_THUMBNAILS === 'true'
+ : true,- SIGNING_KEY expected format: "algorithm version seed" (space-separated), e.g. "ed25519 a_FAET FC6cwY3..." — ConfigService parses with .split(' ') and calls loadEd25519SignerFromSeed(fromBase64ToBytes(seed)). See packages/federation-sdk/src/services/config.service.ts (~lines 115–126).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signingKey: process.env.SIGNING_KEY || undefined, | |
| enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS | |
| ? process.env.MEDIA_ENABLE_THUMBNAILS === 'true' | |
| : true, | |
| signingKey: process.env.SIGNING_KEY || undefined, |
1 participant
This pull request refactors and updates the signature verification test suite for Matrix federation events. It introduces more realistic test data, modernizes the testing style, and aligns the tests with the latest implementation and spec requirements for event signature verification. The changes also update crypto imports and remove legacy code.
Test suite modernization and realistic data:
spyOn/itto Bun'stestandmockutilities, and updates the test style for better readability and maintainability.Test coverage improvements:
verifySignaturetests with newverifyEventSignaturetests that cover all major spec steps, including cases for missing signatures, unsupported algorithms, missing keys, and invalid signatures.test.todoplaceholders for additional edge cases not yet implemented, making it easier to expand coverage in the future.Crypto module updates:
@hs/cryptoutilities, removing direct dependencies ontweetnacland aligning with the latest cryptographic API.Legacy code cleanup:
These changes make the test suite more robust, maintainable, and representative of actual Matrix federation event signature verification.
Summary by CodeRabbit
New Features
Configuration
Documentation
Chores
Tests