shopify-ai-backup/security-review/SECURITY_AND_FUNCTIONALITY_REVIEW.md

# 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.*