Fix/restrict interactive metadata urls#283
Conversation
…eDomain for interactive flows; add tests
|
@augustine00z 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 implements a usable MVP for Anchor-Kit, featuring Express-style router mounting, SQL persistence (SQLite and PostgreSQL), background job processing, and webhook support with signature verification. Feedback on the changes highlights several critical reliability and validation improvements: adding error handling to the transaction watcher's tick interval to prevent unhandled promise rejections from crashing the process, wrapping JSON.parse and decodeURIComponent in try-catch blocks to return clean 400 errors instead of 500s, validating configuration fields like retentionDays and challengeExpirationSeconds to prevent accidental data loss or unexpected behavior, and replacing an explicit any type in TransactionWatcher.ts to comply with the newly enforced ESLint rules.
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.
| try { | ||
| const cutoff = new Date(Date.now() - this.transactionTimeoutMs).toISOString(); | ||
| const pendingTransactions = await this.database.listPendingTransactionsBefore(cutoff); | ||
|
|
||
| for (const transaction of pendingTransactions) { | ||
| await this.queue.enqueue({ | ||
| type: 'expire_transaction', | ||
| payload: { transactionId: transaction.id }, | ||
| }); | ||
| } | ||
|
|
||
| const watcherTaskId = randomUUID(); | ||
| await this.database.insertWatcherTask({ | ||
| id: watcherTaskId, | ||
| watcherName: this.name, | ||
| payload: { | ||
| pendingTransactionsChecked: pendingTransactions.length, | ||
| checkedAt: new Date().toISOString(), | ||
| }, | ||
| }); | ||
|
|
||
| await this.queue.enqueue({ | ||
| type: 'process_watcher_task', | ||
| payload: { watcherTaskId }, | ||
| }); | ||
|
|
||
| await this.queue.enqueue({ | ||
| type: 'cleanup_records', | ||
| payload: { | ||
| retentionDays: this.retentionDays, | ||
| }, | ||
| }); | ||
| } finally { | ||
| this.isTickInProgress = false; | ||
| } |
There was a problem hiding this comment.
The tick() method performs asynchronous database queries and queue operations without a catch block. Since it is called as void this.tick() inside setInterval, any transient database error or network failure will result in an unhandled promise rejection, which can crash the Node.js/Bun process. Adding a catch block to log or handle the error safely is highly recommended to prevent production crashes.
try {
const cutoff = new Date(Date.now() - this.transactionTimeoutMs).toISOString();
const pendingTransactions = await this.database.listPendingTransactionsBefore(cutoff);
for (const transaction of pendingTransactions) {
await this.queue.enqueue({
type: 'expire_transaction',
payload: { transactionId: transaction.id },
});
}
const watcherTaskId = randomUUID();
await this.database.insertWatcherTask({
id: watcherTaskId,
watcherName: this.name,
payload: {
pendingTransactionsChecked: pendingTransactions.length,
checkedAt: new Date().toISOString(),
},
});
await this.queue.enqueue({
type: 'process_watcher_task',
payload: { watcherTaskId },
});
await this.queue.enqueue({
type: 'cleanup_records',
payload: {
retentionDays: this.retentionDays,
},
});
} catch (error) {
console.error(`[TransactionWatcher] Error during tick execution:`, error);
} finally {
this.isTickInProgress = false;
}| // Framework Numbers | ||
| if (framework.queue?.concurrency !== undefined && framework.queue.concurrency < 1) { | ||
| throw new Error('framework.queue.concurrency must be >= 1'); | ||
| } | ||
| if ( | ||
| framework.watchers?.pollIntervalMs !== undefined && | ||
| framework.watchers.pollIntervalMs < 10 | ||
| ) { | ||
| throw new Error('framework.watchers.pollIntervalMs must be >= 10'); | ||
| } | ||
| if (framework.http?.maxBodyBytes !== undefined && framework.http.maxBodyBytes < 1024) { | ||
| throw new Error('framework.http.maxBodyBytes must be >= 1024'); | ||
| } |
There was a problem hiding this comment.
framework.watchers.retentionDays and framework.watchers.transactionTimeoutMs are not validated in AnchorKitConfigSchema.validate(). If retentionDays is configured as 0 (perhaps intending to disable it), it will delete all records up to Date.now(), causing catastrophic data loss. Validating these fields ensures system stability and prevents accidental data loss.
// Framework Numbers
if (framework.queue?.concurrency !== undefined && framework.queue.concurrency < 1) {
throw new Error('framework.queue.concurrency must be >= 1');
}
if (
framework.watchers?.pollIntervalMs !== undefined &&
framework.watchers.pollIntervalMs < 10
) {
throw new Error('framework.watchers.pollIntervalMs must be >= 10');
}
if (
framework.watchers?.transactionTimeoutMs !== undefined &&
framework.watchers.transactionTimeoutMs < 1000
) {
throw new Error('framework.watchers.transactionTimeoutMs must be >= 1000');
}
if (
framework.watchers?.retentionDays !== undefined &&
framework.watchers.retentionDays < 1
) {
throw new Error('framework.watchers.retentionDays must be >= 1');
}
if (framework.http?.maxBodyBytes !== undefined && framework.http.maxBodyBytes < 1024) {
throw new Error('framework.http.maxBodyBytes must be >= 1024');
}| * subsequent intervals are skipped until the current tick finishes. | ||
| */ | ||
| export class TransactionWatcher { | ||
| private intervalId: any = null; |
There was a problem hiding this comment.
Explicit any type is used for intervalId. This violates the project's ESLint configuration where @typescript-eslint/no-explicit-any is set to 'error'. Use ReturnType<typeof setInterval> | null instead for better type safety and to adhere to the lint rules.
| private intervalId: any = null; | |
| private intervalId: ReturnType<typeof setInterval> | null = null; |
| function jsonParseObject(rawBody: string): Record<string, unknown> { | ||
| if (!rawBody) return {}; | ||
|
|
||
| const parsed: unknown = JSON.parse(rawBody); | ||
| if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) { | ||
| throw new ValidationError('Request JSON body must be an object'); | ||
| } | ||
|
|
||
| return parsed as Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
JSON.parse can throw a SyntaxError if the request body is malformed. This is not caught inside jsonParseObject, leading to a 500 Internal Server Error or propagating as an unhandled error to the Express error handler instead of returning a clean 400 Bad Request / ValidationError to the client. Wrapping JSON.parse in a try-catch block is recommended.
function jsonParseObject(rawBody: string): Record<string, unknown> {
if (!rawBody) return {};
try {
const parsed: unknown = JSON.parse(rawBody);
if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
throw new ValidationError('Request JSON body must be an object');
}
return parsed as Record<string, unknown>;
} catch (error) {
if (error instanceof ValidationError) {
throw error;
}
throw new ValidationError('Invalid JSON payload');
}
}| return; | ||
| } | ||
|
|
||
| const transactionId = decodeURIComponent(transactionMatch[1]); |
There was a problem hiding this comment.
decodeURIComponent can throw a URIError if the transaction ID in the URL is malformed (e.g., contains invalid % sequences). This should be caught to return a 400 Bad Request instead of a 500 or crashing.
let transactionId: string;
try {
transactionId = decodeURIComponent(transactionMatch[1]);
} catch {
sendJson(res, 400, {
error: 'invalid_request',
message: 'Invalid transaction ID encoding',
});
return;
}| validate(config: SecurityConfig): void { | ||
| if (!config) throw new Error('Missing required field: security'); | ||
| if (!config.sep10SigningKey) | ||
| throw new Error('Missing required secret: security.sep10SigningKey'); | ||
| if (!config.interactiveJwtSecret) | ||
| throw new Error('Missing required secret: security.interactiveJwtSecret'); | ||
| if (!config.distributionAccountSecret) | ||
| throw new Error('Missing required secret: security.distributionAccountSecret'); | ||
| if ( | ||
| config.authTokenLifetimeSeconds !== undefined && | ||
| (typeof config.authTokenLifetimeSeconds !== 'number' || | ||
| !Number.isFinite(config.authTokenLifetimeSeconds) || | ||
| config.authTokenLifetimeSeconds <= 0) | ||
| ) { | ||
| throw new Error('security.authTokenLifetimeSeconds must be > 0'); | ||
| } | ||
| }, |
There was a problem hiding this comment.
challengeExpirationSeconds is used in the authentication flow but is not validated in SecurityConfigSchema.validate(). If configured with a negative or non-numeric value, it could cause unexpected behavior. Adding validation is recommended.
validate(config: SecurityConfig): void {
if (!config) throw new Error('Missing required field: security');
if (!config.sep10SigningKey)
throw new Error('Missing required secret: security.sep10SigningKey');
if (!config.interactiveJwtSecret)
throw new Error('Missing required secret: security.interactiveJwtSecret');
if (!config.distributionAccountSecret)
throw new Error('Missing required secret: security.distributionAccountSecret');
if (
config.authTokenLifetimeSeconds !== undefined &&
(typeof config.authTokenLifetimeSeconds !== 'number' ||
!Number.isFinite(config.authTokenLifetimeSeconds) ||
config.authTokenLifetimeSeconds <= 0)
) {
throw new Error('security.authTokenLifetimeSeconds must be > 0');
}
if (
config.challengeExpirationSeconds !== undefined &&
(typeof config.challengeExpirationSeconds !== 'number' ||
!Number.isFinite(config.challengeExpirationSeconds) ||
config.challengeExpirationSeconds <= 0)
) {
throw new Error('security.challengeExpirationSeconds must be > 0');
}
},|
Hi! This PR has 12 merge conflicts and is built on a very old version of main. Please rebase onto the latest main and resolve the conflicts. The branch has diverged significantly - you may want to create a fresh branch with just your metadata URL validation changes. Thanks! |
What does this PR do?
Summary
Restrict externally-exposed URLs to HTTP(S) and remove the localhost fallback for interactive flows. Require server.interactiveDomain before generating SEP-24 interactive URLs and add tests for both validation and runtime behavior.
Checklist
bun run testandbun run lintlocally.Issue Reference
Closes #204