From 2a7971eda14db46545b07c2f8b645e841950088a Mon Sep 17 00:00:00 2001 From: Developer Date: Fri, 20 Feb 2026 21:51:45 +0000 Subject: [PATCH] Security fixes: Remove PAT, add idempotency, fix admin auth - Remove exposed GitHub PAT from git remote URL - Remove admin password plaintext fallback (bcrypt only) - Add webhook idempotency protection to prevent duplicate payments - Fix webhook error handling to return 500 on errors (enables retry) - Upgrade archiver to v7 to fix npm vulnerabilities - Add production environment validation for critical secrets - Add comprehensive security review documentation --- chat/package.json | 2 +- chat/server.js | 124 ++++- .../SECURITY_AND_FUNCTIONALITY_REVIEW.md | 514 ++++++++++++++++++ 3 files changed, 630 insertions(+), 10 deletions(-) create mode 100644 security-review/SECURITY_AND_FUNCTIONALITY_REVIEW.md diff --git a/chat/package.json b/chat/package.json index 7b398c9..d0a28e9 100644 --- a/chat/package.json +++ b/chat/package.json @@ -15,7 +15,7 @@ "type": "commonjs", "dependencies": { "adm-zip": "^0.5.16", - "archiver": "^6.0.1", + "archiver": "^7.0.1", "bcrypt": "^6.0.0", "jsonwebtoken": "^9.0.2", "pdfkit": "^0.17.2", diff --git a/chat/server.js b/chat/server.js index 76d11d8..cbd521b 100644 --- a/chat/server.js +++ b/chat/server.js @@ -185,6 +185,8 @@ const FEATURE_REQUESTS_FILE = path.join(STATE_DIR, 'feature-requests.json'); const CONTACT_MESSAGES_FILE = path.join(STATE_DIR, 'contact-messages.json'); const INVOICES_FILE = path.join(STATE_DIR, 'invoices.json'); const INVOICES_DIR = path.join(STATE_DIR, 'invoices'); +const PROCESSED_WEBHOOKS_FILE = path.join(STATE_DIR, 'processed-webhooks.json'); +const PROCESSED_WEBHOOKS_MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; // 7 days // One-off top-up discounts (Business 2.5%, Enterprise 5%; boost add-ons keep higher 10%/25% rates) const BUSINESS_TOPUP_DISCOUNT = 0.025; const ENTERPRISE_TOPUP_DISCOUNT = 0.05; @@ -693,6 +695,10 @@ let lastMemoryCleanup = 0; let memoryCleanupTimer = null; const recentlyCleanedSessions = new Set(); // Track recently cleaned sessions to avoid redundant work +// Processed webhooks for idempotency protection (prevents duplicate payment processing) +const processedWebhookEvents = new Map(); // eventId -> { processedAt, type } +let processedWebhooksPersistTimer = null; + // Track spawned child processes for proper cleanup const childProcesses = new Map(); // processId -> { pid, startTime, sessionId, messageId } @@ -1087,6 +1093,72 @@ function stopMemoryCleanup() { } } +// ============================================================================ +// Webhook Idempotency Protection +// ============================================================================ + +async function loadProcessedWebhooks() { + try { + await ensureStateFile(); + const raw = await fs.readFile(PROCESSED_WEBHOOKS_FILE, 'utf8').catch(() => null); + if (raw) { + const parsed = JSON.parse(raw); + if (parsed && typeof parsed === 'object') { + const now = Date.now(); + // Clean up old entries on load + for (const [eventId, data] of Object.entries(parsed)) { + if (data && data.processedAt && (now - data.processedAt) < PROCESSED_WEBHOOKS_MAX_AGE_MS) { + processedWebhookEvents.set(eventId, data); + } + } + log('Loaded processed webhooks', { count: processedWebhookEvents.size }); + } + } + } catch (error) { + log('Failed to load processed webhooks', { error: String(error) }); + } +} + +async function persistProcessedWebhooks() { + try { + await ensureStateFile(); + const now = Date.now(); + const obj = {}; + // Only persist recent entries + for (const [eventId, data] of processedWebhookEvents.entries()) { + if (data && data.processedAt && (now - data.processedAt) < PROCESSED_WEBHOOKS_MAX_AGE_MS) { + obj[eventId] = data; + } + } + await safeWriteFile(PROCESSED_WEBHOOKS_FILE, JSON.stringify(obj, null, 2)); + } catch (error) { + log('Failed to persist processed webhooks', { error: String(error) }); + } +} + +function isWebhookProcessed(eventId) { + if (!eventId) return false; + return processedWebhookEvents.has(eventId); +} + +async function markWebhookProcessed(eventId, eventType) { + if (!eventId) return; + processedWebhookEvents.set(eventId, { + processedAt: Date.now(), + type: eventType || 'unknown' + }); + + // Debounce persistence + if (processedWebhooksPersistTimer) { + clearTimeout(processedWebhooksPersistTimer); + } + processedWebhooksPersistTimer = setTimeout(() => { + persistProcessedWebhooks().catch(err => { + log('Failed to persist processed webhooks (debounced)', { error: String(err) }); + }); + }, 1000); +} + /** * Registers a child process for tracking * @param {string} processId - Unique process identifier @@ -14385,6 +14457,13 @@ async function handleDodoWebhook(req, res) { const event = JSON.parse(rawBody); log('Dodo webhook received', { type: event.type, id: event.id }); + // Idempotency check - prevent duplicate processing + const eventId = event.id || event.data?.id; + if (eventId && isWebhookProcessed(eventId)) { + log('Dodo webhook already processed, skipping', { eventId, type: event.type }); + return sendJson(res, 200, { received: true, duplicate: true }); + } + switch (event.type) { case 'payment_succeeded': case 'subscription_payment_succeeded': @@ -14454,10 +14533,18 @@ async function handleDodoWebhook(req, res) { log('Unhandled Dodo webhook event', { type: event.type }); } + // Mark as processed for idempotency + const eventId = event.id || event.data?.id; + if (eventId) { + await markWebhookProcessed(eventId, event.type); + } + sendJson(res, 200, { received: true }); } catch (error) { log('Dodo webhook error', { error: String(error), stack: error.stack }); - sendJson(res, 200, { received: true }); + // Return 500 to trigger webhook retry from payment provider + // This ensures failed payments aren't silently lost + sendJson(res, 500, { error: 'Webhook processing failed', received: false }); } } @@ -15792,15 +15879,13 @@ async function handleAdminLogin(req, res) { return sendJson(res, 401, { error: 'Incorrect credentials' }); } - // Validate password using bcrypt - let passwordValid = false; - if (adminPasswordHash) { - passwordValid = await bcrypt.compare(pass, adminPasswordHash); - } else { - // Fallback to plaintext comparison if hashing failed at startup - passwordValid = pass === ADMIN_PASSWORD.trim(); + // Validate password using bcrypt - no plaintext fallback for security + if (!adminPasswordHash) { + log('admin login failed: bcrypt hash not initialized', { ip: clientIp }); + return sendJson(res, 500, { error: 'Authentication system not ready. Please try again.' }); } - + + const passwordValid = await bcrypt.compare(pass, adminPasswordHash); if (!passwordValid) { log('failed admin login', { user: userNormalized, ip: clientIp, reason: 'invalid_password' }); return sendJson(res, 401, { error: 'Incorrect credentials' }); @@ -19272,6 +19357,26 @@ async function routeInternal(req, res, url, pathname) { } async function bootstrap() { + // Production environment validation + const isProduction = process.env.NODE_ENV === 'production'; + const criticalEnvVars = []; + + if (isProduction) { + if (!process.env.USER_SESSION_SECRET && !process.env.SESSION_SECRET) { + criticalEnvVars.push('USER_SESSION_SECRET or SESSION_SECRET'); + } + if (!process.env.DODO_PAYMENTS_API_KEY && !process.env.DODO_API_KEY) { + criticalEnvVars.push('DODO_PAYMENTS_API_KEY'); + } + + if (criticalEnvVars.length > 0) { + console.error('❌ CRITICAL: Missing required environment variables for production:'); + criticalEnvVars.forEach(v => console.error(` - ${v}`)); + console.error('Please set these environment variables before running in production.'); + process.exit(1); + } + } + process.on('uncaughtException', async (error) => { log('Uncaught Exception, saving state before exit', { error: String(error), stack: error.stack }); try { @@ -19312,6 +19417,7 @@ async function bootstrap() { await loadAffiliatesDb(); await loadWithdrawalsDb(); await loadTrackingData(); + await loadProcessedWebhooks(); await loadFeatureRequestsDb(); contactMessagesDb = await loadContactMessagesDb(); await ensureAssetsDir(); diff --git a/security-review/SECURITY_AND_FUNCTIONALITY_REVIEW.md b/security-review/SECURITY_AND_FUNCTIONALITY_REVIEW.md new file mode 100644 index 0000000..31cf508 --- /dev/null +++ b/security-review/SECURITY_AND_FUNCTIONALITY_REVIEW.md @@ -0,0 +1,514 @@ +# Security and Functionality Review +## Plugin Compass AI App Builder - Pre-Launch Assessment + +**Review Date:** February 20, 2026 +**Review Scope:** Main chat application, builder, server, and payment systems +**Status:** CRITICAL ISSUES FOUND - DO NOT LAUNCH WITHOUT REMEDIATION + +--- + +## Critical Security Issues (MUST FIX BEFORE LAUNCH) + +### 1. EXPOSED GITHUB PERSONAL ACCESS TOKEN (CRITICAL) + +**Location:** `.git/config` line 7 +**Severity:** CRITICAL +**CVSS:** 9.1 (Critical) + +**Issue:** +``` +url = https://southseact-3d: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@github.com/... +``` + +A GitHub Personal Access Token (PAT) is embedded directly in the git remote URL configuration. This token has been committed to the repository and provides full access to the GitHub account. + +**Impact:** +- Complete compromise of the GitHub account +- Access to all repositories +- Potential for supply chain attacks +- Token may have already been scraped by automated tools + +**Remediation:** +1. **IMMEDIATELY** revoke the exposed token at https://github.com/settings/tokens +2. Remove the token from git config: + ```bash + git remote set-url origin https://github.com/southseact-3d/shopify-ai-backup.git + ``` +3. Force push to remove from history if necessary +4. Use SSH keys or a credential manager instead +5. Never commit credentials to version control + +--- + +### 2. Admin Password Plaintext Fallback (HIGH) + +**Location:** `chat/server.js` lines 15796-15802 +**Severity:** HIGH + +**Issue:** +```javascript +// Validate password using bcrypt +let passwordValid = false; +if (adminPasswordHash) { + passwordValid = await bcrypt.compare(pass, adminPasswordHash); +} else { + // Fallback to plaintext comparison if hashing failed at startup + passwordValid = pass === ADMIN_PASSWORD.trim(); +} +``` + +The admin login has a fallback to plaintext password comparison when bcrypt hashing fails. This creates a timing attack vulnerability and undermines the security model. + +**Impact:** +- Timing attacks possible on plaintext comparison +- Weaker authentication if bcrypt initialization fails silently +- Admin account compromise potential + +**Remediation:** +1. Remove the plaintext fallback entirely +2. Fail hard if bcrypt initialization fails +3. Log and alert on any password comparison failures +4. Add logging for bcrypt initialization status + +--- + +### 3. Session Secret Auto-Generation Warning (MEDIUM) + +**Location:** `chat/server.js` lines 377-385 +**Severity:** MEDIUM + +**Issue:** +```javascript +const USER_SESSION_SECRET = process.env.USER_SESSION_SECRET || process.env.SESSION_SECRET || (() => { + const generatedSecret = randomBytes(32).toString('hex'); + console.warn('⚠️ WARNING: No USER_SESSION_SECRET or SESSION_SECRET found. Generated a random secret...'); + return generatedSecret; +})(); +``` + +The session secret is auto-generated if not provided. While there's a warning, if this happens in production: +- All user sessions will be invalidated on server restart +- An attacker who reads logs could potentially forge session tokens + +**Remediation:** +1. Make `USER_SESSION_SECRET` a required environment variable in production +2. Exit the application if not set in production mode +3. Never log generated secrets + +--- + +### 4. Webhook Error Handling Returns 200 (MEDIUM) + +**Location:** `chat/server.js` lines 14458-14461 +**Severity:** MEDIUM + +**Issue:** +```javascript +} catch (error) { + log('Dodo webhook error', { error: String(error), stack: error.stack }); + sendJson(res, 200, { received: true }); +} +``` + +The webhook handler returns HTTP 200 even on errors, which prevents the payment provider from retrying. This could lead to: +- Lost payment confirmations +- Inconsistent subscription states +- Customers charged but not credited + +**Remediation:** +1. Return appropriate HTTP error codes (400/500) for actual errors +2. Only return 200 when processing is successful +3. Implement proper retry handling for transient errors + +--- + +## High Priority Issues + +### 5. No Idempotency Protection for Payments (HIGH) + +**Location:** Payment handling functions throughout `server.js` +**Severity:** HIGH + +**Issue:** +Payment processing lacks idempotency keys. If the same webhook is delivered multiple times (which payment providers do by design), the same payment could be processed multiple times. + +**Impact:** +- Customers could receive duplicate token credits +- Revenue loss from over-crediting +- Inconsistent account states + +**Remediation:** +1. Store processed webhook event IDs +2. Check for duplicates before processing +3. Implement idempotency keys for all payment operations + +--- + +### 6. Payment Method Save Endpoint Logs Without Verification (MEDIUM) + +**Location:** `chat/server.js` lines 14580-14582 +**Severity:** MEDIUM + +**Issue:** +```javascript +if (inferredType === 'payment_method_save') { + log('payment_method_save: payment method added via webhook', { userId, eventId: event.id, checkoutId }); + return; +} +``` + +Payment method save events are logged but not properly processed/verified. + +**Remediation:** +1. Verify the payment method save with the payment provider +2. Update user record with payment method details +3. Implement proper state tracking + +--- + +### 7. Email Marketing Webhook Hardcoded (LOW) + +**Location:** `chat/server.js` line 11441 +**Severity:** LOW + +**Issue:** +```javascript +await fetch('https://emailmarketing.modelrailway3d.co.uk/api/webhooks/incoming/wh_0Z49zi_DGj4-lKJMOPO8-g', ... +``` + +A webhook URL with an apparent API key is hardcoded. This should be configurable. + +**Remediation:** +1. Move to environment variable +2. Make configurable per environment + +--- + +## Functionality Issues + +### 8. Builder State Not Properly Cleared on Logout + +**Location:** `chat/public/builder.js` lines 52-66 +**Severity:** MEDIUM + +**Issue:** +The builder state is persisted to localStorage but the `clearBuilderState` function preserves `pluginPrompt` and `subsequentPrompt`. When a user logs out, sensitive state could persist. + +**Remediation:** +1. Clear all builder state on logout +2. Clear state when switching accounts +3. Consider encrypting sensitive builder state + +--- + +### 9. External Testing MCP Server Path + +**Location:** `chat/server.js` lines 9640-9651 +**Severity:** MEDIUM + +**Issue:** +```javascript +const wpMcpServerPath = path.resolve(__dirname, '../opencode/mcp-servers/wp-cli-testing/index.js'); +``` + +This path is relative to the chat folder and may not exist in all deployments. No validation that the file exists before use. + +**Remediation:** +1. Validate file exists before enabling feature +2. Add graceful degradation if file not found +3. Log warning when feature is disabled due to missing file + +--- + +### 10. Memory Management Could Cause Performance Issues + +**Location:** `chat/server.js` lines 681-999 +**Severity:** LOW + +**Issue:** +Memory cleanup runs every 5 minutes with debouncing to 60 seconds. For high-traffic deployments, this may not be sufficient. + +**Observations:** +- Session max age: 7 days +- Message history limit: 50 messages +- Stale stream timeout: 10 minutes + +**Recommendation:** +1. Consider making cleanup intervals configurable +2. Add memory pressure-based cleanup triggers +3. Implement connection pooling limits + +--- + +### 11. Token Validation May Reject Valid Usage + +**Location:** `chat/server.js` lines 7459-7499 +**Severity:** LOW + +**Issue:** +Token validation has strict character-per-token ratio checking that might reject legitimate edge cases (very dense code, minified JS, etc.). + +**Recommendation:** +1. Widen acceptable character-per-token ratio bounds +2. Add context-aware validation for code vs natural language + +--- + +## Payment System Issues + +### 12. Dodo Product IDs Using Placeholder Values + +**Location:** `chat/server.js` lines 254-276 +**Severity:** HIGH + +**Issue:** +```javascript +starter_monthly_usd: process.env.DODO_STARTER_MONTHLY_USD || 'prod_starter_monthly_usd', +``` + +Default product IDs are placeholder strings. If environment variables aren't set, payment checkouts will fail silently or create invalid products. + +**Remediation:** +1. Validate all required product IDs on startup +2. Exit application if payment product IDs not configured in production +3. Add startup health check for payment configuration + +--- + +### 13. Subscription Renewal Date Calculation + +**Location:** Multiple locations +**Severity:** MEDIUM + +The renewal date calculation should be reviewed to ensure proper handling of: +- Month-end dates (Feb 28/29) +- Timezone consistency +- Leap years + +--- + +## Authentication Issues + +### 14. CSRF Token Implementation + +**Location:** `chat/server.js` lines 1574, 1935-1946 +**Severity:** MEDIUM + +**Issue:** +CSRF tokens are implemented but it's unclear if they're consistently validated on all state-changing requests. + +**Recommendation:** +1. Audit all POST/PUT/DELETE endpoints for CSRF validation +2. Add CSRF token requirement to all forms +3. Consider using SameSite=Strict cookies for session cookies + +--- + +### 15. Password Requirements + +**Location:** `chat/server.js` - password validation function +**Severity:** LOW + +The password validation appears reasonable but should be reviewed to ensure it meets current security standards: +- Minimum 8 characters recommended +- No common password blacklist +- No breach database check + +**Recommendation:** +1. Implement password strength meter on frontend +2. Add common password blacklist check +3. Consider HaveIBeenPwned API integration + +--- + +## Builder Functionality Issues + +### 16. Model Selection Lock for Free Plans + +**Location:** `chat/public/builder.js` lines 1036-1090 +**Severity:** LOW + +**Issue:** +Free plan users have model selection locked to 'auto'. The logic adds an 'auto' option and prevents selection. This UX could be confusing. + +**Recommendation:** +1. Show clear messaging about auto model selection +2. Consider showing which model was actually used after request +3. Add upgrade prompt when user attempts to select specific model + +--- + +### 17. Message Input Caching + +**Location:** `chat/public/builder.js` lines 447-508 +**Severity:** LOW + +**Issue:** +Message input is cached in localStorage for 24 hours. If a user types sensitive information and doesn't send, it persists. + +**Recommendation:** +1. Clear cache on logout +2. Add user notification about caching +3. Consider shorter cache duration + +--- + +## Configuration Issues + +### 18. Missing Environment Variable Documentation + +**Severity:** MEDIUM + +**Issue:** +Many environment variables are required for proper operation but not documented in a single location. + +**Required Variables:** +- `DODO_PAYMENTS_API_KEY` - Payment processing +- `DODO_PAYMENTS_WEBHOOK_KEY` - Webhook verification +- `DODO_TOPUP_*_USD/GBP/EUR` - Product IDs (12 variables) +- `USER_SESSION_SECRET` - Session security +- `ADMIN_USER` / `ADMIN_PASSWORD` - Admin access +- `OPENROUTER_API_KEY` - AI model access +- Various model-specific API keys + +**Remediation:** +1. Create `.env.example` with all required variables +2. Add startup validation for required variables +3. Document in deployment guide + +--- + +### 19. Docker Configuration Security + +**Location:** `docker-compose.yml` (root) +**Severity:** MEDIUM + +Review Docker configuration for: +- Container user (should not run as root) +- Resource limits +- Secret management +- Network isolation + +--- + +## Recommendations Summary + +### Before Launch (MUST DO): +1. Revoke exposed GitHub PAT immediately +2. Remove plaintext password fallback +3. Add idempotency to payment processing +4. Validate all payment product IDs on startup +5. Fix webhook error response codes +6. Make session secret required in production + +### Soon After Launch: +1. Audit all CSRF validation +2. Add payment method save handling +3. External MCP server path validation +4. Environment variable documentation + +### Future Improvements: +1. Password breach database check +2. Memory management optimization +3. Token validation edge cases +4. Better free plan UX for model selection + +--- + +## Files Reviewed + +| File | Lines | Focus | +|------|-------|-------| +| `chat/server.js` | ~19,500 | Main server, auth, payments, API | +| `chat/public/builder.js` | ~2,500 | Builder frontend | +| `chat/public/app.js` | ~300 | Main app frontend | +| `chat/security/index.js` | 20 | Security module | +| `chat/security/prompt-sanitizer.js` | 497 | Prompt injection protection | +| `chat/package.json` | 27 | Dependencies | +| `.git/config` | - | Git configuration | + +--- + +## Testing Recommendations + +Before launch, perform: + +1. **Penetration Testing** + - Authentication bypass attempts + - Payment manipulation + - Session hijacking + - CSRF attacks + +2. **Load Testing** + - Concurrent user limits + - Payment processing under load + - Memory leak detection + +3. **Payment Flow Testing** + - Complete payment flows in test mode + - Webhook delivery failures + - Duplicate payment handling + - Refund processing + +4. **Security Scanning** + - Dependency vulnerability scan (`npm audit`) + - Static code analysis + - Secret detection in codebase + +--- + +## Dependency Vulnerabilities + +### 20. npm Audit Findings (HIGH) + +**Severity:** HIGH +**Status:** Multiple high-severity vulnerabilities found + +``` +archiver - HIGH severity (via archiver-utils, readdir-glob, zip-stream) +archiver-utils - HIGH severity (via glob) +glob - HIGH severity (via minimatch) +minimatch - HIGH severity (ReDoS vulnerability GHSA-3ppc-4f35-3m26) +``` + +**Remediation:** +```bash +npm update archiver +# Or pin to secure version: +npm install archiver@4.0.2 +``` + +--- + +### 21. Test Framework Dependency (LOW) + +**Severity:** LOW +**Location:** `chat/package.json` line 7 + +**Issue:** +```json +"test": "bun test" +``` + +Tests are written for Bun test framework but Bun is not installed in the environment. + +**Impact:** +- Cannot run tests in production environment +- CI/CD may fail + +**Remediation:** +1. Either install Bun in deployment environment +2. Or migrate tests to use Node.js built-in test runner +3. Add test runner to documentation + +--- + +## Conclusion + +**DO NOT LAUNCH** until critical issues #1-4 are resolved. The exposed GitHub token (#1) is a severe security breach that could lead to complete account compromise. The payment-related issues (#4, #5) could result in revenue loss or customer disputes. + +After remediation of critical and high-priority issues, the application should be suitable for launch with the noted improvements implemented over time. + +--- + +*This review was generated by automated analysis. Manual verification of all findings is recommended.*