From 59debf7931b55af3a1c3869b044eb8abde87381b Mon Sep 17 00:00:00 2001 From: Solomon Jay Date: Sun, 28 Jun 2026 17:47:06 +0000 Subject: [PATCH] fix: resolve 4 security and performance issues (427, 430, 433, 440) - #427: Add setInterval cleanup guards in propertyCache.ts, rateLimit.ts, and structuredLogger.ts to prevent interval leaks on HMR and module re-imports - #430: Remove dangerouslySetInnerHTML from chart.tsx; use safe React style children instead - #433: Add normalizeToBigInt helper with parseEther in blockchainSecurity.ts to handle hex, scientific notation, and decimal ether input values - #440: Replace document.write in compare/page.tsx with safe DOM API calls (createElement, textContent) for PDF export Closes #427, Closes #430, Closes #433, Closes #440 --- src/app/compare/page.tsx | 98 +++++++++++++++--------- src/components/ui/chart.tsx | 20 +++-- src/lib/propertyCache.ts | 10 ++- src/lib/rateLimit.ts | 53 +++++++++---- src/utils/security/blockchainSecurity.ts | 84 +++++++++++++++++++- src/utils/structuredLogger.ts | 3 + 6 files changed, 204 insertions(+), 64 deletions(-) diff --git a/src/app/compare/page.tsx b/src/app/compare/page.tsx index 0ceaacf2..41d00d48 100644 --- a/src/app/compare/page.tsx +++ b/src/app/compare/page.tsx @@ -150,45 +150,69 @@ export default function ComparePage() { const printWindow = window.open('', '_blank'); if (!printWindow) return; - const html = ` - - - - Property Comparison - - - -

Property Comparison Report

-

Generated on ${new Date().toLocaleDateString()}

- - - - - ${properties.map(p => ``).join('')} - - - - ${comparisonMetrics.map(metric => ` - - - ${properties.map(p => ``).join('')} - - `).join('')} - -
Metric${p.name}
${metric.label}${metric.format(getNestedValue(p, metric.key), p)}
- - + // Build document via safe DOM APIs instead of document.write to avoid + // CSP bypass and script re-execution risks. + const doc = printWindow.document; + + // Create title + const title = doc.createElement('title'); + title.textContent = 'Property Comparison'; + doc.head.appendChild(title); + + // Create styles + const style = doc.createElement('style'); + style.textContent = ` + body { font-family: Arial, sans-serif; padding: 20px; } + table { width: 100%; border-collapse: collapse; margin-top: 20px; } + th, td { border: 1px solid #ddd; padding: 12px; text-align: left; } + th { background-color: #f5f5f5; font-weight: bold; } + h1 { color: #333; } + @media print { body { padding: 0; } } `; + doc.head.appendChild(style); + + // Build body + const h1 = doc.createElement('h1'); + h1.textContent = 'Property Comparison Report'; + doc.body.appendChild(h1); + + const dateP = doc.createElement('p'); + dateP.textContent = `Generated on ${new Date().toLocaleDateString()}`; + doc.body.appendChild(dateP); + + // Build table + const table = doc.createElement('table'); + const thead = doc.createElement('thead'); + const headerRow = doc.createElement('tr'); + const metricTh = doc.createElement('th'); + metricTh.textContent = 'Metric'; + headerRow.appendChild(metricTh); + properties.forEach(p => { + const th = doc.createElement('th'); + th.textContent = p.name; + headerRow.appendChild(th); + }); + thead.appendChild(headerRow); + table.appendChild(thead); + + const tbody = doc.createElement('tbody'); + comparisonMetrics.forEach(metric => { + const row = doc.createElement('tr'); + const labelTd = doc.createElement('td'); + labelTd.textContent = metric.label; + row.appendChild(labelTd); + properties.forEach(p => { + const td = doc.createElement('td'); + td.textContent = metric.format(getNestedValue(p, metric.key), p); + row.appendChild(td); + }); + tbody.appendChild(row); + }); + table.appendChild(tbody); + doc.body.appendChild(table); - printWindow.document.write(html); - printWindow.document.close(); + // Close the document stream so browsers finish rendering before printing + doc.close(); printWindow.print(); }; diff --git a/src/components/ui/chart.tsx b/src/components/ui/chart.tsx index c1366bd1..f442190e 100644 --- a/src/components/ui/chart.tsx +++ b/src/components/ui/chart.tsx @@ -78,12 +78,11 @@ const ChartStyle = ({ id, config }: { id: string; config: ChartConfig }) => { return null } - return ( - } const ChartTooltip = RechartsPrimitive.Tooltip diff --git a/src/lib/propertyCache.ts b/src/lib/propertyCache.ts index 158ba998..51818e61 100644 --- a/src/lib/propertyCache.ts +++ b/src/lib/propertyCache.ts @@ -43,6 +43,9 @@ import { // Event listeners const eventListeners: Set = new Set(); +// Interval handle for cleanup timer (stored at module level to allow cleanup on re-init) +let cleanupIntervalHandle: ReturnType | null = null; + // Cache statistics let cacheStats: CacheStats = { totalEntries: 0, @@ -675,9 +678,14 @@ export const initPropertyCache = async (): Promise => { // Clean up expired entries on init await cleanupExpiredEntries(); + // Clear any existing cleanup interval (prevents accumulation on HMR / re-imports) + if (cleanupIntervalHandle !== null) { + clearInterval(cleanupIntervalHandle); + } + // Set up periodic cleanup const config = getCacheConfig(); - setInterval(cleanupExpiredEntries, config.cleanupInterval); + cleanupIntervalHandle = setInterval(cleanupExpiredEntries, config.cleanupInterval); logger.info('Property cache initialized'); } catch (error) { diff --git a/src/lib/rateLimit.ts b/src/lib/rateLimit.ts index 5629d40d..5895d0d1 100644 --- a/src/lib/rateLimit.ts +++ b/src/lib/rateLimit.ts @@ -20,20 +20,45 @@ interface RateLimitResult { const ipStore: RateLimitStore = {}; const walletStore: RateLimitStore = {}; -// Clean up expired entries periodically -setInterval(() => { - const now = Date.now(); - Object.keys(ipStore).forEach(key => { - if (ipStore[key].resetTime <= now) { - delete ipStore[key]; - } - }); - Object.keys(walletStore).forEach(key => { - if (walletStore[key].resetTime <= now) { - delete walletStore[key]; - } - }); -}, 60000); // Clean up every minute +// Interval handle for cleanup timer +let cleanupIntervalHandle: ReturnType | null = null; + +/** + * Start the periodic cleanup of expired rate-limit entries. + * Safe to call multiple times — clears any existing interval first. + */ +function startCleanupTimer(): void { + if (cleanupIntervalHandle !== null) { + clearInterval(cleanupIntervalHandle); + } + + cleanupIntervalHandle = setInterval(() => { + const now = Date.now(); + Object.keys(ipStore).forEach(key => { + if (ipStore[key].resetTime <= now) { + delete ipStore[key]; + } + }); + Object.keys(walletStore).forEach(key => { + if (walletStore[key].resetTime <= now) { + delete walletStore[key]; + } + }); + }, 60000); // Clean up every minute +} + +// Start the cleanup timer on module load +startCleanupTimer(); + +/** + * Stops the cleanup timer. Useful for tests and HMR teardown. + */ +export function stopRateLimitCleanup(): void { + if (cleanupIntervalHandle !== null) { + clearInterval(cleanupIntervalHandle); + cleanupIntervalHandle = null; + } +} function getRateLimitData(store: RateLimitStore, key: string, windowMs: number): { count: number; diff --git a/src/utils/security/blockchainSecurity.ts b/src/utils/security/blockchainSecurity.ts index f77e0dc2..d35524a4 100644 --- a/src/utils/security/blockchainSecurity.ts +++ b/src/utils/security/blockchainSecurity.ts @@ -1,3 +1,5 @@ +import { parseEther } from 'viem'; + export interface SecurityServiceConfig { apiKey?: string; baseUrl: string; @@ -32,6 +34,86 @@ export interface SecurityAlert { timestamp: number; } +/** + * Normalises a transaction value string into a BigInt. + * Handles hex strings (0x-prefixed), scientific notation, and decimal strings. + * Throws a typed error for unparseable values. + */ +function normalizeToBigInt(value: string): bigint { + if (typeof value !== 'string' || value.length === 0) { + throw new BlockchainSecurityError( + 'Invalid value: must be a non-empty string', + 'INVALID_VALUE' + ); + } + + let normalised = value.trim(); + + // Handle hex (0x-prefixed) — viem-style wei values + if (normalised.startsWith('0x') || normalised.startsWith('0X')) { + try { + return BigInt(normalised); + } catch { + throw new BlockchainSecurityError( + `Unable to parse hex value: "${normalised}"`, + 'INVALID_HEX_VALUE' + ); + } + } + + // Handle scientific notation (e.g. "1e18", "1.5e-3") + if (/[eE]/.test(normalised)) { + const asNumber = Number(normalised); + if (!Number.isFinite(asNumber)) { + throw new BlockchainSecurityError( + `Scientific notation overflow: "${normalised}"`, + 'VALUE_OVERFLOW' + ); + } + // Convert to a whole-number string suitable for BigInt + try { + return BigInt(Math.round(asNumber)); + } catch { + throw new BlockchainSecurityError( + `Unable to parse scientific notation: "${normalised}"`, + 'INVALID_SCI_VALUE' + ); + } + } + + // Handle fractional decimal (e.g. "1.5" — treat as ether value) + if (normalised.includes('.')) { + try { + return parseEther(normalised as `${number}`); + } catch { + throw new BlockchainSecurityError( + `Unable to parse decimal ether value: "${normalised}"`, + 'INVALID_ETHER_VALUE' + ); + } + } + + // Plain decimal string (wei) + try { + return BigInt(normalised); + } catch { + throw new BlockchainSecurityError( + `Unable to parse value: "${normalised}"`, + 'INVALID_VALUE' + ); + } +} + +export class BlockchainSecurityError extends Error { + public readonly code: string; + + constructor(message: string, code: string) { + super(message); + this.name = 'BlockchainSecurityError'; + this.code = code; + } +} + export class BlockchainSecurityService { private static instance: BlockchainSecurityService; private config: SecurityServiceConfig; @@ -218,7 +300,7 @@ export class BlockchainSecurityService { } // Check for high-value transaction to risky address - const valueBN = BigInt(value); + const valueBN = normalizeToBigInt(value); if (valueBN > BigInt('1000000000000000000') && recipientRisk.riskScore > 50) { // > 1 ETH warnings.push('High-value transaction to risky address'); } diff --git a/src/utils/structuredLogger.ts b/src/utils/structuredLogger.ts index 876fc046..e5f56fe5 100644 --- a/src/utils/structuredLogger.ts +++ b/src/utils/structuredLogger.ts @@ -390,6 +390,9 @@ class StructuredLogger { export const structuredLogger = new StructuredLogger(); +// Note: structuredLogger.destroy() should be called manually for cleanup +// in test teardowns or before recreating the singleton. + // ============================================================================ // Performance Monitoring Helper // ============================================================================