refactor: streamline error handling and logging in makeSignedRequest method#288
Merged
refactor: streamline error handling and logging in makeSignedRequest method#288
Conversation
Contributor
WalkthroughRefactors the federation request signing service by eliminating the try/catch wrapper, reorganizing variable initialization to compute serverName and signingKey upfront, moving address resolution and discovery headers earlier in the flow, updating signing and authorization header generation with revised parameter sets, and implementing detailed error logging with comprehensive context information. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant FedReqSvc as Federation<br/>Request Service
participant Crypto as Crypto Ops
participant HomeServer as Home Server
participant Transport as Network
Caller->>FedReqSvc: makeSignedRequest()
activate FedReqSvc
Note over FedReqSvc: Early Initialization Phase
FedReqSvc->>FedReqSvc: Initialize serverName,<br/>signingKey, keyPair
Note over FedReqSvc: Resolution Phase
FedReqSvc->>HomeServer: Resolve home server address
HomeServer-->>FedReqSvc: Server address
FedReqSvc->>HomeServer: Fetch discovery headers
HomeServer-->>FedReqSvc: Discovery headers
Note over FedReqSvc: URL & Query Preparation
FedReqSvc->>FedReqSvc: Construct URL with<br/>query parameters
rect rgb(200, 220, 255)
Note over FedReqSvc: Signing Phase
FedReqSvc->>Crypto: signJson(signingKey, body)
Crypto-->>FedReqSvc: signed body
FedReqSvc->>Crypto: authorizationHeaders<br/>(serverName, key, domain,<br/>method, uri, signedBody)
Crypto-->>FedReqSvc: auth headers
end
FedReqSvc->>FedReqSvc: Assemble final headers
rect rgb(240, 200, 200)
FedReqSvc->>Transport: fetch(URL, headers)
Transport-->>FedReqSvc: Response
alt OK Response
FedReqSvc-->>Caller: Return response
else Error Response
FedReqSvc->>FedReqSvc: Log detailed error<br/>(URL, status, headers)
FedReqSvc-->>Caller: Throw error with context
end
end
deactivate FedReqSvc
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: Significant restructuring of control flow within a critical service module handling cryptographic signing and federation requests. The changes involve reordering of multiple operations (initialization → resolution → construction → signing → transport), updated method signatures with expanded parameter sets, and modified error handling logic. While changes are contained to a single file, the density of logic modifications and behavioral shifts across the request pipeline require careful verification of execution order correctness, parameter passing accuracy, and error handling completeness. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #288 +/- ##
=======================================
Coverage 60.49% 60.50%
=======================================
Files 67 67
Lines 6675 6676 +1
=======================================
+ Hits 4038 4039 +1
Misses 2637 2637 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/federation-request.service.ts (1)
63-66: Avoid clobbering pre-existing query params in URISetting url.search unconditionally can drop query already present in uri. Merge instead.
- if (queryString) { - url.search = queryString; - } + if (queryString) { + url.search = url.search + ? `${url.search.substring(1)}&${queryString}` + : queryString; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/federation-sdk/src/services/federation-request.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/federation-request.service.ts (5)
packages/core/src/types.ts (1)
SigningKey(20-26)packages/core/src/utils/signJson.ts (1)
signJson(17-50)packages/core/src/utils/authentication.ts (2)
computeAndMergeHash(138-160)authorizationHeaders(46-67)packages/core/src/url.ts (1)
extractURIfromURL(1-3)packages/core/src/utils/fetch.ts (1)
fetch(141-272)
🔇 Additional comments (2)
packages/federation-sdk/src/services/federation-request.service.ts (2)
70-77: Signing flow and auth header look correctHashing when absent, preserving existing hashes/signatures, and passing origin/serverName into both signJson and authorizationHeaders aligns with core utils.
Also applies to: 79-87
88-92: Review comment is incorrect - discovery module already guarantees capitalized Host headerThe
getHomeserverFinalAddress()function returns typePromise<[..., HostHeaders]>, whereHostHeaders = { Host: ... }(capital H). Every code path ingetHomeserverFinalAddressInternal()constructs this object with capitalizedHost:
- Line 117:
hostHeader = { Host: ... }- Line 127:
hostHeaders = { Host: ... }- Lines 290, 296, 329: all use
{ Host: ... }The type system contractually ensures
discoveryHeadersalways contains a capitalizedHostheader. The proposed fallback logic is redundant and unnecessary.Likely an incorrect or invalid review comment.
Comment on lines
+43
to
+57
| const serverName = this.configService.serverName; | ||
| const signingKeyBase64 = await this.configService.getSigningKeyBase64(); | ||
| const signingKeyId = await this.configService.getSigningKeyId(); | ||
| const privateKeyBytes = Buffer.from(signingKeyBase64, 'base64'); | ||
| const keyPair = nacl.sign.keyPair.fromSecretKey(privateKeyBytes); | ||
|
|
||
| const signingKey: SigningKey = { | ||
| algorithm: EncryptionValidAlgorithm.ed25519, | ||
| version: signingKeyId.split(':')[1] || '1', | ||
| privateKey: keyPair.secretKey, | ||
| publicKey: keyPair.publicKey, | ||
| sign: async (data: Uint8Array) => | ||
| nacl.sign.detached(data, keyPair.secretKey), | ||
| }; | ||
|
|
Contributor
There was a problem hiding this comment.
Harden signing key handling and key ID parsing
- Validate base64 length (seed 32B vs secretKey 64B) and derive keyPair accordingly.
- Fail fast if signingKeyId doesn’t match expected "ed25519:" format; defaulting to '1' can silently break federation auth.
Apply this diff:
- const signingKeyBase64 = await this.configService.getSigningKeyBase64();
- const signingKeyId = await this.configService.getSigningKeyId();
- const privateKeyBytes = Buffer.from(signingKeyBase64, 'base64');
- const keyPair = nacl.sign.keyPair.fromSecretKey(privateKeyBytes);
+ const signingKeyBase64 = await this.configService.getSigningKeyBase64();
+ const signingKeyId = await this.configService.getSigningKeyId();
+ const privateKeyBytes = Buffer.from(signingKeyBase64, 'base64');
+ if (
+ privateKeyBytes.length !== nacl.sign.secretKeyLength &&
+ privateKeyBytes.length !== nacl.sign.seedLength
+ ) {
+ throw new Error(
+ `Invalid ed25519 key length: expected ${nacl.sign.secretKeyLength} (secret) or ${nacl.sign.seedLength} (seed), got ${privateKeyBytes.length}`,
+ );
+ }
+ const keyPair =
+ privateKeyBytes.length === nacl.sign.seedLength
+ ? nacl.sign.keyPair.fromSeed(privateKeyBytes)
+ : nacl.sign.keyPair.fromSecretKey(privateKeyBytes);
@@
- const signingKey: SigningKey = {
+ if (!signingKeyId?.startsWith(`${EncryptionValidAlgorithm.ed25519}:`)) {
+ throw new Error(
+ `Invalid signing key id. Expected '${EncryptionValidAlgorithm.ed25519}:<version>', got '${signingKeyId}'`,
+ );
+ }
+ const [, version] = signingKeyId.split(':', 2);
+ const signingKey: SigningKey = {
algorithm: EncryptionValidAlgorithm.ed25519,
- version: signingKeyId.split(':')[1] || '1',
+ version,
privateKey: keyPair.secretKey,
publicKey: keyPair.publicKey,
sign: async (data: Uint8Array) => nacl.sign.detached(data, keyPair.secretKey),
};📝 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.
Suggested change
| const serverName = this.configService.serverName; | |
| const signingKeyBase64 = await this.configService.getSigningKeyBase64(); | |
| const signingKeyId = await this.configService.getSigningKeyId(); | |
| const privateKeyBytes = Buffer.from(signingKeyBase64, 'base64'); | |
| const keyPair = nacl.sign.keyPair.fromSecretKey(privateKeyBytes); | |
| const signingKey: SigningKey = { | |
| algorithm: EncryptionValidAlgorithm.ed25519, | |
| version: signingKeyId.split(':')[1] || '1', | |
| privateKey: keyPair.secretKey, | |
| publicKey: keyPair.publicKey, | |
| sign: async (data: Uint8Array) => | |
| nacl.sign.detached(data, keyPair.secretKey), | |
| }; | |
| const serverName = this.configService.serverName; | |
| const signingKeyBase64 = await this.configService.getSigningKeyBase64(); | |
| const signingKeyId = await this.configService.getSigningKeyId(); | |
| const privateKeyBytes = Buffer.from(signingKeyBase64, 'base64'); | |
| if ( | |
| privateKeyBytes.length !== nacl.sign.secretKeyLength && | |
| privateKeyBytes.length !== nacl.sign.seedLength | |
| ) { | |
| throw new Error( | |
| `Invalid ed25519 key length: expected ${nacl.sign.secretKeyLength} (secret) or ${nacl.sign.seedLength} (seed), got ${privateKeyBytes.length}`, | |
| ); | |
| } | |
| const keyPair = | |
| privateKeyBytes.length === nacl.sign.seedLength | |
| ? nacl.sign.keyPair.fromSeed(privateKeyBytes) | |
| : nacl.sign.keyPair.fromSecretKey(privateKeyBytes); | |
| if (!signingKeyId?.startsWith(`${EncryptionValidAlgorithm.ed25519}:`)) { | |
| throw new Error( | |
| `Invalid signing key id. Expected '${EncryptionValidAlgorithm.ed25519}:<version>', got '${signingKeyId}'`, | |
| ); | |
| } | |
| const [, version] = signingKeyId.split(':', 2); | |
| const signingKey: SigningKey = { | |
| algorithm: EncryptionValidAlgorithm.ed25519, | |
| version, | |
| privateKey: keyPair.secretKey, | |
| publicKey: keyPair.publicKey, | |
| sign: async (data: Uint8Array) => | |
| nacl.sign.detached(data, keyPair.secretKey), | |
| }; |
Comment on lines
+100
to
+120
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
|
|
||
| return response; | ||
| } catch (err) { | ||
| this.logger.error({ | ||
| msg: 'Error making signed federation request', | ||
| err, | ||
| msg: 'Federation request failed', | ||
| url, | ||
| status: response.status, | ||
| errorText, | ||
| sentHeaders: headers, | ||
| responseHeaders: response.headers, | ||
| }); | ||
| throw err; | ||
|
|
||
| let errorDetail = errorText; | ||
| try { | ||
| errorDetail = JSON.stringify(JSON.parse(errorText || '')); | ||
| } catch { | ||
| /* use raw text if parsing fails */ | ||
| } | ||
| throw new Error( | ||
| `Federation request failed: ${response.status} ${errorDetail}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Network-error path: response.text() rejects; add fallback, redact sig, truncate logs, and log url string
On network errors, core fetch returns stubs whose text() rejects, so this path throws before logging. Also, log may leak the signature and can be huge.
- if (!response.ok) {
- const errorText = await response.text();
-
- this.logger.error({
- msg: 'Federation request failed',
- url,
- status: response.status,
- errorText,
- sentHeaders: headers,
- responseHeaders: response.headers,
- });
-
- let errorDetail = errorText;
- try {
- errorDetail = JSON.stringify(JSON.parse(errorText || ''));
- } catch {
- /* use raw text if parsing fails */
- }
- throw new Error(
- `Federation request failed: ${response.status} ${errorDetail}`,
- );
- }
+ if (!response.ok) {
+ let errorText: string;
+ try {
+ errorText = await response.text();
+ } catch (e) {
+ errorText = e instanceof Error ? e.message : String(e);
+ }
+
+ const redactedAuth =
+ typeof headers.Authorization === 'string'
+ ? headers.Authorization.replace(/sig="[^"]+"/, 'sig="***"')
+ : headers.Authorization;
+ const MAX_LOG = 2048;
+ const safeErrorText =
+ errorText && errorText.length > MAX_LOG
+ ? `${errorText.slice(0, MAX_LOG)}…`
+ : errorText;
+
+ this.logger.error({
+ msg: 'Federation request failed',
+ url: url.toString(),
+ status: response.status,
+ errorText: safeErrorText,
+ sentHeaders: { ...headers, Authorization: redactedAuth },
+ responseHeaders: response.headers,
+ });
+
+ const raw = errorText ?? '';
+ let errorDetail = raw;
+ try {
+ errorDetail = JSON.stringify(JSON.parse(raw));
+ } catch {
+ // keep raw
+ }
+ throw new Error(
+ `Federation request failed: ${response.status ?? 'network'} ${errorDetail}`,
+ );
+ }Based on learnings (core fetch error shape).
📝 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.
Suggested change
| if (!response.ok) { | |
| const errorText = await response.text(); | |
| return response; | |
| } catch (err) { | |
| this.logger.error({ | |
| msg: 'Error making signed federation request', | |
| err, | |
| msg: 'Federation request failed', | |
| url, | |
| status: response.status, | |
| errorText, | |
| sentHeaders: headers, | |
| responseHeaders: response.headers, | |
| }); | |
| throw err; | |
| let errorDetail = errorText; | |
| try { | |
| errorDetail = JSON.stringify(JSON.parse(errorText || '')); | |
| } catch { | |
| /* use raw text if parsing fails */ | |
| } | |
| throw new Error( | |
| `Federation request failed: ${response.status} ${errorDetail}`, | |
| ); | |
| if (!response.ok) { | |
| let errorText: string; | |
| try { | |
| errorText = await response.text(); | |
| } catch (e) { | |
| errorText = e instanceof Error ? e.message : String(e); | |
| } | |
| const redactedAuth = | |
| typeof headers.Authorization === 'string' | |
| ? headers.Authorization.replace(/sig="[^"]+"/, 'sig="***"') | |
| : headers.Authorization; | |
| const MAX_LOG = 2048; | |
| const safeErrorText = | |
| errorText && errorText.length > MAX_LOG | |
| ? `${errorText.slice(0, MAX_LOG)}…` | |
| : errorText; | |
| this.logger.error({ | |
| msg: 'Federation request failed', | |
| url: url.toString(), | |
| status: response.status, | |
| errorText: safeErrorText, | |
| sentHeaders: { ...headers, Authorization: redactedAuth }, | |
| responseHeaders: response.headers, | |
| }); | |
| const raw = errorText ?? ''; | |
| let errorDetail = raw; | |
| try { | |
| errorDetail = JSON.stringify(JSON.parse(raw)); | |
| } catch { | |
| // keep raw | |
| } | |
| throw new Error( | |
| `Federation request failed: ${response.status ?? 'network'} ${errorDetail}`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/federation-request.service.ts around
lines 100 to 120, the path assumes response.text() always resolves and logs
potentially huge or sensitive headers (eg signature) and may log a URL object;
update the code to await response.text() inside a try/catch and fall back to
response.statusText or an empty string if text() rejects, convert url to a
string for logging, redact any signature-like header values (e.g. Authorization,
Signature, X-Signature) from sentHeaders/response.headers before logging, and
truncate long errorText and header values to a reasonable length (e.g. 1024
chars) so logs are safe and bounded; keep the existing errorDetail parsing logic
but operate on the safe/truncated errorText when constructing the thrown Error.
ggazzo
approved these changes
Oct 30, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Bug Fixes