security challenge expiration validation#326
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! 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration validation and HTTP routing logic by extracting default configuration merging, validation helpers, and Express router request handling into separate files (config-defaults.ts, validation-helpers.ts, and express-router-impl.ts). It also refactors several tests to use deterministic deferred promises. Feedback on these changes highlights a critical bug in express-router-impl.ts where setTimebounds is called with incorrect arguments, a high-severity idempotency issue where failed transactions can leave empty placeholder records in the database, and a loss of integration test coverage in malformed-bearer.test.ts due to testing a local helper instead of the actual middleware.
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.
| source: account, | ||
| }), | ||
| ) | ||
| .setTimebounds(now, expiresAtUnix) |
There was a problem hiding this comment.
In @stellar/stellar-sdk, TransactionBuilder.prototype.setTimebounds expects a single TimeBounds object (e.g., { minTime: number | string, maxTime: number | string }) rather than two separate arguments. Passing now, expiresAtUnix directly will result in invalid timebounds or a runtime error during transaction building.
| .setTimebounds(now, expiresAtUnix) | |
| .setTimebounds({ minTime: now, maxTime: expiresAtUnix }) |
| if (typeof idempotencyKey === 'string' && idempotencyKey.length > 0) { | ||
| const idempotencyId = randomUUID(); | ||
| const idempotencyRecord = await context.database.insertOrGetIdempotencyRecord({ | ||
| id: idempotencyId, | ||
| scope, | ||
| idempotencyKey, | ||
| requestHash, | ||
| statusCode: 201, | ||
| responseBody: '{}', | ||
| }); | ||
|
|
||
| if (idempotencyRecord.id === idempotencyId) { |
There was a problem hiding this comment.
There is a critical correctness issue with the idempotency implementation. If insertInteractiveTransaction (or any subsequent step) fails and throws an error, the placeholder idempotency record remains in the database with statusCode: 201 and responseBody: '{}'.
A subsequent request with the same idempotency key will match the existing record, bypass transaction creation, and replay the empty 201 response. This makes the client believe the transaction was successfully created when it was actually aborted.
Consider wrapping the transaction creation in a try-catch block to delete or mark the idempotency record as failed if the transaction cannot be created.
| function isAcceptedBearerHeader(authHeader: unknown): boolean { | ||
| if (!authHeader || typeof authHeader !== 'string' || !authHeader.startsWith('Bearer ')) { | ||
| return false; | ||
| } | ||
|
|
||
| if (req.url === '/protected') { | ||
| const auth = req.headers['authorization'] || ''; | ||
| if (!auth || typeof auth !== 'string' || !auth.startsWith('Bearer ')) { | ||
| res.statusCode = 401; | ||
| res.end('Unauthorized'); | ||
| return; | ||
| } | ||
|
|
||
| const token = auth.slice('Bearer '.length).trim(); | ||
| // Very small validation: JWTs have three dot-separated parts | ||
| if (token.split('.').length !== 3) { | ||
| res.statusCode = 401; | ||
| res.end('Unauthorized'); | ||
| return; | ||
| } | ||
|
|
||
| res.statusCode = 200; | ||
| res.end('OK'); | ||
| return; | ||
| } | ||
|
|
||
| res.statusCode = 404; | ||
| res.end(); | ||
| }); | ||
|
|
||
| return new Promise<http.Server>((resolve, reject) => { | ||
| server.listen(0, () => resolve(server)); | ||
| server.on('error', reject); | ||
| }); | ||
| } | ||
|
|
||
| function httpRequest( | ||
| port: number, | ||
| opts: { method?: string; path?: string; headers?: Record<string, string> }, | ||
| ) { | ||
| return new Promise<{ statusCode: number; body: string }>((resolve, reject) => { | ||
| const request = http.request( | ||
| { port, method: opts.method || 'GET', path: opts.path || '/', headers: opts.headers }, | ||
| (res) => { | ||
| const chunks: Buffer[] = []; | ||
| res.on('data', (c) => chunks.push(Buffer.from(c))); | ||
| res.on('end', () => | ||
| resolve({ statusCode: res.statusCode || 0, body: Buffer.concat(chunks).toString() }), | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| request.on('error', reject); | ||
| request.end(); | ||
| }); | ||
| const token = authHeader.slice('Bearer '.length).trim(); | ||
| return token.split('.').length === 3; | ||
| } |
There was a problem hiding this comment.
This test has been refactored to test a local helper function isAcceptedBearerHeader defined entirely within the test file itself. It no longer starts a test server or exercises the actual router middleware/authentication logic in express-router-impl.ts (which uses jwt.verify). This results in a complete loss of integration test coverage for bearer token validation.
0xNgoo
force-pushed
the
codex/security-challenge-expiration-validation
branch
from
5f80412 to
32c6113
Compare
2 participants
What does this PR do?
Adds validation for
challengeExpirationSecondsinSecurityConfigSchema.authTokenLifetimeSecondsvalidation unchangedHow to test?
bun test tests/utils/validation.test.tsbun run typecheckbun run lintbun run format:checkbun run buildbun run scripts/check-fallow-health.mjsChecklist
bun run testandbun run lintlocally.Issue Reference
Closes #252