From 9a929d98cbc8ee8c26fc9accbfb86f55ae318a66 Mon Sep 17 00:00:00 2001 From: Sunny Date: Tue, 3 Feb 2026 04:57:41 +0000 Subject: [PATCH] fix: address CodeRabbit review comments for PR #77 - Add SSRF validation to TTS route with baseUrl allowlist - Fix concurrency.ts: validate maxConcurrent >= 1, fill aborted slots - Add .env existence check to shell script with error messaging - Add language identifier to README code block (markdownlint MD040) - Add input validation and 30s timeout to groq-tts route - Preserve previous summary on error in SummarizeModal - Pass maxLength parameter through to summarize API - Fix unused docType parameter in dexie.ts getSummary - Reject whitespace-only text in summarize API Co-Authored-By: Claude Opus 4.5 --- scripts/README.md | 2 +- scripts/openreader-webui.sh | 12 +++-- src/app/api/groq-tts/route.ts | 32 ++++++++++++- src/app/api/summarize/route.ts | 2 +- src/app/api/tts/route.ts | 76 ++++++++++++++++++++++++++++++- src/components/SummarizeModal.tsx | 7 +++ src/lib/concurrency.ts | 14 +++++- src/lib/dexie.ts | 2 +- src/lib/summarize.ts | 7 +-- 9 files changed, 141 insertions(+), 13 deletions(-) diff --git a/scripts/README.md b/scripts/README.md index 065787c..217ef82 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -19,7 +19,7 @@ http://localhost:3003 ## Architecture -``` +```text OpenReader WebUI (:3003) ↓ Built-in /api/groq-tts route diff --git a/scripts/openreader-webui.sh b/scripts/openreader-webui.sh index 98a9ee4..0fb38c5 100755 --- a/scripts/openreader-webui.sh +++ b/scripts/openreader-webui.sh @@ -10,14 +10,20 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" PROJECT_DIR="$(dirname "$SCRIPT_DIR")" # Load GROQ_API_KEY -source "$SCRIPT_DIR/.env" -export GROQ_API_KEY +if [ -f "$SCRIPT_DIR/.env" ]; then + source "$SCRIPT_DIR/.env" + export GROQ_API_KEY +else + echo "ERROR: $SCRIPT_DIR/.env file not found" + echo "Please create the .env file with your GROQ_API_KEY" + exit 1 +fi # Stop existing service fuser -k 3003/tcp 2>/dev/null || true # Wait for port to be released -for i in {1..10}; do +for _ in {1..10}; do if ! fuser 3003/tcp 2>/dev/null; then break fi diff --git a/src/app/api/groq-tts/route.ts b/src/app/api/groq-tts/route.ts index 9694a78..80df495 100644 --- a/src/app/api/groq-tts/route.ts +++ b/src/app/api/groq-tts/route.ts @@ -17,6 +17,14 @@ export async function POST(req: NextRequest) { const body = await req.json(); + // Validate input text + if (!body.input || typeof body.input !== 'string' || !body.input.trim()) { + return NextResponse.json( + { error: 'Missing or empty input text' }, + { status: 400 } + ); + } + // Set default model if not provided or not a canopylabs model let model = body.model || ''; if (!model.startsWith('canopylabs/')) { @@ -36,6 +44,10 @@ export async function POST(req: NextRequest) { response_format: responseFormat, }; + // Create abort controller with 30-second timeout + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); + const response = await fetch(`${GROQ_BASE}/audio/speech`, { method: 'POST', headers: { @@ -43,12 +55,21 @@ export async function POST(req: NextRequest) { 'Authorization': `Bearer ${apiKey}`, }, body: JSON.stringify(groqBody), + signal: controller.signal, }); + clearTimeout(timeoutId); + if (!response.ok) { const errorText = await response.text(); + console.error('Groq API error:', errorText); + // Return sanitized error message to avoid exposing internal details + const sanitizedError = response.status === 401 ? 'Invalid API key' : + response.status === 429 ? 'Rate limit exceeded' : + response.status >= 500 ? 'Groq service error' : + 'Failed to generate speech'; return NextResponse.json( - { error: errorText }, + { error: sanitizedError }, { status: response.status } ); } @@ -64,8 +85,15 @@ export async function POST(req: NextRequest) { }); } catch (error) { console.error('Groq TTS error:', error); + // Handle timeout specifically + if (error instanceof Error && error.name === 'AbortError') { + return NextResponse.json( + { error: 'Request timed out' }, + { status: 504 } + ); + } return NextResponse.json( - { error: error instanceof Error ? error.message : 'Failed to generate speech' }, + { error: 'Failed to generate speech' }, { status: 500 } ); } diff --git a/src/app/api/summarize/route.ts b/src/app/api/summarize/route.ts index a1ed9f0..7471db8 100644 --- a/src/app/api/summarize/route.ts +++ b/src/app/api/summarize/route.ts @@ -145,7 +145,7 @@ export async function POST(req: NextRequest) { console.log('Received summarize request:', { provider, modelId, mode, textLength: text?.length, isChunk, isFinalPass }); - if (!text) { + if (!text || !text.trim()) { const errorBody: SummarizeError = { code: 'MISSING_TEXT', message: 'No text provided for summarization', diff --git a/src/app/api/tts/route.ts b/src/app/api/tts/route.ts index ef14baf..cdf1dd5 100644 --- a/src/app/api/tts/route.ts +++ b/src/app/api/tts/route.ts @@ -17,6 +17,72 @@ type AudioBufferValue = TTSAudioBuffer; const TTS_CACHE_MAX_SIZE_BYTES = Number(process.env.TTS_CACHE_MAX_SIZE_BYTES || 256 * 1024 * 1024); // 256MB const TTS_CACHE_TTL_MS = Number(process.env.TTS_CACHE_TTL_MS || 1000 * 60 * 30); // 30 minutes +// Default provider endpoints - these are the only allowed baseUrls in production +// unless TTS_ALLOWED_BASE_URLS is configured +const PROVIDER_DEFAULT_ENDPOINTS: Record = { + openai: 'https://api.openai.com/v1', + groq: 'https://api.groq.com/openai/v1', + deepinfra: 'https://api.deepinfra.com/v1/openai', +}; + +// Parse allowed base URLs from environment variable (comma-separated) +function getAllowedBaseUrls(): string[] { + const envUrls = process.env.TTS_ALLOWED_BASE_URLS; + if (!envUrls) return []; + return envUrls.split(',').map(url => url.trim()).filter(Boolean); +} + +// Validate baseUrl against allowlist in production +function validateBaseUrl(baseUrl: string, provider: string): string | null { + // Empty baseUrl is always allowed - will use provider defaults + if (!baseUrl) { + return null; + } + + // In development, allow any baseUrl + if (process.env.NODE_ENV !== 'production') { + return baseUrl; + } + + // Parse and normalize the URL + let parsedUrl: URL; + try { + parsedUrl = new URL(baseUrl); + } catch { + return null; // Invalid URL + } + + // Only allow https in production + if (parsedUrl.protocol !== 'https:') { + return null; + } + + const normalizedUrl = parsedUrl.origin + parsedUrl.pathname.replace(/\/$/, ''); + + // Check against provider defaults + const providerDefault = PROVIDER_DEFAULT_ENDPOINTS[provider]; + if (providerDefault && normalizedUrl === providerDefault) { + return baseUrl; + } + + // Check against configured allowlist + const allowedUrls = getAllowedBaseUrls(); + for (const allowed of allowedUrls) { + try { + const allowedParsed = new URL(allowed); + const normalizedAllowed = allowedParsed.origin + allowedParsed.pathname.replace(/\/$/, ''); + if (normalizedUrl === normalizedAllowed || normalizedUrl.startsWith(normalizedAllowed + '/')) { + return baseUrl; + } + } catch { + continue; + } + } + + // URL not in allowlist - return null to use provider default + return null; +} + const ttsAudioCache = new LRUCache({ maxSize: TTS_CACHE_MAX_SIZE_BYTES, sizeCalculation: (value) => value.byteLength, @@ -240,7 +306,13 @@ export async function POST(req: NextRequest) { // Get API key and base URL based on provider let apiKey = req.headers.get('x-openai-key') || ''; - let baseUrl = req.headers.get('x-openai-base-url') || ''; + const requestedBaseUrl = req.headers.get('x-openai-base-url') || ''; + + // Validate and sanitize baseUrl - in production, only allow configured endpoints + const validatedBaseUrl = validateBaseUrl(requestedBaseUrl, provider); + if (requestedBaseUrl && !validatedBaseUrl && process.env.NODE_ENV === 'production') { + console.warn(`[TTS] Rejected untrusted baseUrl: ${requestedBaseUrl} for provider: ${provider}`); + } if (!apiKey) { switch (provider) { @@ -256,6 +328,8 @@ export async function POST(req: NextRequest) { } } + // Use validated baseUrl or fall back to provider defaults + let baseUrl = validatedBaseUrl || ''; if (!baseUrl) { switch (provider) { case 'groq': diff --git a/src/components/SummarizeModal.tsx b/src/components/SummarizeModal.tsx index 2aff723..72b5de7 100644 --- a/src/components/SummarizeModal.tsx +++ b/src/components/SummarizeModal.tsx @@ -93,6 +93,10 @@ export function SummarizeModal({ }, [isOpen]); const handleGenerate = async () => { + // Preserve previous state in case of error + const previousSummary = summary; + const previousSavedSummary = savedSummary; + setIsGenerating(true); setError(null); setSummary(''); @@ -133,6 +137,9 @@ export function SummarizeModal({ } catch (err) { console.error('Error generating summary:', err); setError(err instanceof Error ? err.message : 'Failed to generate summary'); + // Restore previous summary on error + setSummary(previousSummary); + setSavedSummary(previousSavedSummary); } finally { setIsGenerating(false); setChunkProgress(null); diff --git a/src/lib/concurrency.ts b/src/lib/concurrency.ts index 3267dc1..62214f5 100644 --- a/src/lib/concurrency.ts +++ b/src/lib/concurrency.ts @@ -14,7 +14,7 @@ export type ConcurrencyResult = * * @param items - Array of items to process * @param processor - Async function to process each item - * @param maxConcurrent - Maximum number of concurrent processors + * @param maxConcurrent - Maximum number of concurrent processors (must be >= 1) * @param signal - Optional AbortSignal for cancellation * @returns Array of results in the same order as input items */ @@ -24,6 +24,11 @@ export async function processWithConcurrencyLimit( maxConcurrent: number, signal?: AbortSignal ): Promise[]> { + // Validate maxConcurrent to prevent crashes + if (maxConcurrent < 1 || !Number.isFinite(maxConcurrent)) { + throw new Error(`maxConcurrent must be >= 1, got ${maxConcurrent}`); + } + const results: ConcurrencyResult[] = new Array(items.length); let currentIndex = 0; @@ -46,5 +51,12 @@ export async function processWithConcurrencyLimit( const workerCount = Math.min(maxConcurrent, items.length); await Promise.all(Array(workerCount).fill(null).map(() => worker())); + // Fill any undefined slots (from aborted operations) with rejected results + for (let i = 0; i < items.length; i++) { + if (results[i] === undefined) { + results[i] = { status: 'rejected', reason: new Error('Operation aborted') }; + } + } + return results; } diff --git a/src/lib/dexie.ts b/src/lib/dexie.ts index dae97ed..960b42c 100644 --- a/src/lib/dexie.ts +++ b/src/lib/dexie.ts @@ -1053,7 +1053,7 @@ export async function saveSummary(summary: Omit): Promise { return withDB(async () => { diff --git a/src/lib/summarize.ts b/src/lib/summarize.ts index 78d000a..6afe3c8 100644 --- a/src/lib/summarize.ts +++ b/src/lib/summarize.ts @@ -79,7 +79,8 @@ async function callSummarizeAPI( text: string, mode: SummarizeMode, options: SummarizeOptions, - flags?: { isChunk?: boolean; isFinalPass?: boolean } + flags?: { isChunk?: boolean; isFinalPass?: boolean }, + maxLength?: number ): Promise { const headers: Record = { 'Content-Type': 'application/json', @@ -92,7 +93,7 @@ async function callSummarizeAPI( const response = await fetch('/api/summarize', { method: 'POST', headers, - body: JSON.stringify({ text, mode, ...flags }), + body: JSON.stringify({ text, mode, maxLength, ...flags }), }); const data = await response.json(); @@ -112,7 +113,7 @@ export async function generateSummary( // Direct summarization for small texts or non-whole-book modes if (mode !== 'whole_book' || !needsChunking(text, contextLimit)) { - return callSummarizeAPI(text, mode, options); + return callSummarizeAPI(text, mode, options, undefined, maxLength); } // Hierarchical summarization for large documents