Refactor/core hotspot files#325
Conversation
|
@gracekenn Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
There was a problem hiding this comment.
Code Review
This pull request introduces Fallow health checks and refactors configuration merging, express routing, and validation logic into separate helper files. Key feedback includes addressing a bug in the idempotency logic where failed transactions could falsely replay as successful, fixing a potential TypeError in the health check script's error handling, and mitigating a potential race condition in the auth challenge consumption check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const idempotencyRecord = await context.database.insertOrGetIdempotencyRecord({ | ||
| id: idempotencyId, | ||
| scope, | ||
| idempotencyKey, | ||
| requestHash, | ||
| statusCode: 201, | ||
| responseBody: '{}', | ||
| }); |
There was a problem hiding this comment.
When an idempotency key is provided, the idempotency record is initially inserted with a placeholder statusCode of 201 and an empty responseBody of '{}'.
If the subsequent call to context.database.insertInteractiveTransaction fails or throws an error, the execution will halt and return a 500 error to the client. However, the idempotency record remains in the database with statusCode: 201 and responseBody: '{}'.
When the client retries the request with the same idempotency key, the server will find the existing record and replay the response, returning a 201 Created status with an empty body {}. This falsely indicates to the client that the transaction was successfully created when it actually failed.
To prevent this, we should initialize the idempotency record with a failure status (e.g., 500) and an error response body. If the transaction is successfully created, it will be updated to 201 with the correct response body. If it fails, subsequent retries will correctly replay the failure instead of a silent empty success.
const idempotencyRecord = await context.database.insertOrGetIdempotencyRecord({
id: idempotencyId,
scope,
idempotencyKey,
requestHash,
statusCode: 500,
responseBody: JSON.stringify({
error: 'internal_server_error',
message: 'Transaction creation failed or was interrupted',
}),
});| if (result.status !== 0) { | ||
| process.stderr.write(result.stderr || result.stdout); | ||
| process.exit(result.status ?? 1); | ||
| } |
There was a problem hiding this comment.
If spawnSync fails to execute the command (for example, if bunx is not installed or not found in the PATH), result.status will be null and result.error will be populated. In this case, result.stderr and result.stdout will also be null or undefined.
Attempting to call process.stderr.write(result.stderr || result.stdout) will throw a TypeError because write expects a string or Buffer, crashing the script abruptly instead of printing a clear error message.
We should check for result.error first and handle it gracefully.
| if (result.status !== 0) { | |
| process.stderr.write(result.stderr || result.stdout); | |
| process.exit(result.status ?? 1); | |
| } | |
| if (result.error) { | |
| console.error('Failed to run fallow health check:', result.error); | |
| process.exit(1); | |
| } | |
| if (result.status !== 0) { | |
| process.stderr.write(result.stderr || result.stdout || 'Unknown error'); | |
| process.exit(result.status ?? 1); | |
| } |
| const stored = await context.database.getAuthChallengeByChallenge(nonce); | ||
| if (!stored || stored.account !== account) { | ||
| sendJson(res, 401, { error: 'invalid_challenge', message: 'Challenge not found' }); | ||
| return; | ||
| } | ||
|
|
||
| if (stored.consumedAt) { | ||
| sendJson(res, 401, { error: 'invalid_challenge', message: 'Challenge already used' }); | ||
| return; | ||
| } | ||
|
|
||
| if (new Date(stored.expiresAt).getTime() < Date.now()) { | ||
| sendJson(res, 401, { error: 'invalid_challenge', message: 'Challenge expired' }); | ||
| return; | ||
| } | ||
|
|
||
| await context.database.markAuthChallengeConsumed(stored.id); |
There was a problem hiding this comment.
There is a potential race condition (double-spend / replay vulnerability) in the challenge consumption check.
The code retrieves the challenge record using getAuthChallengeByChallenge(nonce), checks if stored.consumedAt is set, and then marks it as consumed using markAuthChallengeConsumed(stored.id).
If an attacker sends two concurrent requests with the same signed challenge, both requests might pass the stored.consumedAt check before either has updated the database, resulting in multiple active JWTs being issued for a single challenge.
To mitigate this, the database adapter should ideally perform an atomic update (e.g., updating the record only if consumed_at is null, and returning whether the update succeeded or the number of affected rows). Since the current DatabaseAdapter interface returns Promise<void> for markAuthChallengeConsumed, consider updating the adapter interface in the future to support atomic consumption checks.
fbd5174 to
067b086
Compare
What does this PR do?
Refactors the core maintainability hotspots called out by Fallow before MVP release.
express-router.tsinto a thin router wrapper and a request-handler helper modulesrc/core/config-defaults.tssrc/utils/validation-helpers.tsand keeps the public API stableHow to test?
bun run typecheckbun test tests/core/config.test.ts tests/utils/validation.test.ts tests/utils/server-config-schema.test.ts tests/mvp-express.integration.test.tsbun run scripts/check-fallow-health.mjsChecklist
bun run testandbun run lintlocally.Issue Reference
Closes #209