Add security review documentation and recommendations

Co-authored-by: johndoe6345789 <224850594+johndoe6345789@users.noreply.github.com>
2026-04-24 13:55:00 +00:00 · 2026-01-08 14:20:30 +00:00
parent 9a814c06d2
commit d6cbaff3f9
1 changed files with 245 additions and 0 deletions
--- a/docs/SECURITY_REVIEW.md
+++ b/docs/SECURITY_REVIEW.md
@@ -0,0 +1,245 @@
+# Code Review Findings & Security Considerations
+
+## Overview
+Code review identified 10 items requiring attention, primarily focused on security and type safety.
+
+## Security Issues (High Priority)
+
+### 1. Code Execution Vulnerability in ComponentTreeRenderer
+**Location:** `src/utils/ComponentTreeRenderer.tsx` lines 91-131
+
+**Issue:** Using `new Function()` with user-provided input allows arbitrary code execution.
+
+**Risk:** An attacker could inject malicious JavaScript through template expressions.
+
+**Example Attack:**
+```json
+{
+  "props": {
+    "text": "{{require('fs').readFileSync('/etc/passwd')}}"
+  }
+}
+```
+
+**Recommended Fix:**
+- Use a safer expression evaluator (e.g., `expr-eval`, `safe-eval-2`)
+- Implement a whitelist of allowed operations
+- Sanitize all user inputs
+- Run evaluations in a sandboxed environment
+
+**Mitigation for Current Use:**
+- features.json is server-side only (not user-editable)
+- Only trusted developers can modify it
+- Still should be fixed for production
+
+### 2. SQL Injection Risk in Query Templates
+**Location:** `src/config/features.json` line 2902 and throughout SQL templates
+
+**Issue:** Template parameters like `{{tableName}}` are not escaped, potentially allowing SQL injection.
+
+**Example Attack:**
+```javascript
+const tableName = "users; DROP TABLE users--";
+interpolateSqlTemplate(template, { tableName });
+// Result: CREATE TABLE "users; DROP TABLE users--" (...)
+```
+
+**Recommended Fix:**
+- Use proper parameterized queries through Drizzle ORM
+- Validate all identifiers (table names, column names) against whitelist
+- Escape special characters in SQL identifiers
+- Use pg_escape_identifier() or equivalent
+
+**Current Mitigation:**
+- API routes already validate table/column names
+- Templates are for reference/documentation
+- Actual queries should use Drizzle ORM
+
+### 3. Missing Query Parameters in API Routes
+**Location:** `src/app/api/admin/record/route.ts` lines 62, 124, 182
+
+**Issue:** Queries contain placeholders ($1, $2, etc.) but no values are passed to `sql.raw()`.
+
+**Impact:** Queries will fail at runtime - parameters won't be substituted.
+
+**Fix Required:**
+```typescript
+// Current (broken):
+const result = await db.execute(sql.raw(query));
+
+// Should be:
+const result = await db.execute(sql.raw(query), values);
+```
+
+**Status:** This was introduced during the refactoring fix. Need to revert or fix properly.
+
+## Type Safety Issues (Medium Priority)
+
+### 4. Loose Return Types in Storybook Functions
+**Location:** `src/utils/featureConfig.ts` lines 496, 500, 504
+
+**Issue:** Functions return `any` or `Record<string, any>` instead of proper types.
+
+**Recommended Fix:**
+```typescript
+// Current:
+export function getStorybookStory(componentName: string, storyName: string): any {
+
+// Should be:
+export function getStorybookStory(
+  componentName: string, 
+  storyName: string
+): StorybookStory | undefined {
+```
+
+**Impact:** Loss of TypeScript type checking and IDE autocomplete.
+
+## Security Best Practices
+
+### For ComponentTreeRenderer
+
+**Option 1: Use Safe Expression Evaluator**
+```typescript
+import { Parser } from 'expr-eval';
+
+const parser = new Parser();
+function evaluateCondition(condition: string, data: Record<string, any>): boolean {
+  try {
+    const expr = parser.parse(condition);
+    return expr.evaluate(data);
+  } catch {
+    return false;
+  }
+}
+```
+
+**Option 2: Whitelist Approach**
+```typescript
+const ALLOWED_OPERATIONS = {
+  '===': (a: any, b: any) => a === b,
+  '>': (a: any, b: any) => a > b,
+  '&&': (a: boolean, b: boolean) => a && b,
+  // ... more operators
+};
+
+function evaluateSafe(expr: string, data: any): any {
+  // Parse and evaluate using whitelist only
+}
+```
+
+**Option 3: Static Analysis**
+```typescript
+// Only allow specific patterns
+const SAFE_PATTERN = /^[a-zA-Z_$][a-zA-Z0-9_$]*(\.[a-zA-Z_$][a-zA-Z0-9_$]*)*$/;
+
+function interpolateValue(value: string, data: any): any {
+  const match = value.match(/^\{\{(.+)\}\}$/);
+  if (match && SAFE_PATTERN.test(match[1])) {
+    return getNestedProperty(data, match[1]);
+  }
+  return value;
+}
+```
+
+### For SQL Templates
+
+**Use Drizzle ORM Properly:**
+```typescript
+// Don't use sql.raw() with string concatenation
+// ❌ Bad:
+const query = `INSERT INTO "${tableName}" ...`;
+await db.execute(sql.raw(query));
+
+// ✅ Good:
+await db.insert(table).values(data);
+
+// ✅ Also Good (if raw SQL needed):
+await db.execute(sql`
+  INSERT INTO ${sql.identifier([tableName])}
+  (${sql.join(columns, sql`, `)})
+  VALUES (${sql.join(values, sql`, `)})
+`);
+```
+
+**Validate Identifiers:**
+```typescript
+function validateIdentifier(name: string): boolean {
+  // PostgreSQL identifier rules
+  const VALID_IDENTIFIER = /^[a-zA-Z_][a-zA-Z0-9_]{0,62}$/;
+  return VALID_IDENTIFIER.test(name);
+}
+
+function sanitizeIdentifier(name: string): string {
+  if (!validateIdentifier(name)) {
+    throw new Error('Invalid identifier');
+  }
+  return name;
+}
+```
+
+## Recommendations
+
+### Immediate Actions (Before Production)
+1. ✅ Fix the parameterized query issue in record/route.ts
+2. ✅ Implement safe expression evaluation in ComponentTreeRenderer
+3. ✅ Add identifier validation to all SQL template usage
+4. ✅ Improve TypeScript types in featureConfig.ts
+
+### Code Review Actions
+5. ✅ Security audit of all `new Function()` usage
+6. ✅ Review all SQL query generation
+7. ✅ Add input sanitization tests
+8. ✅ Document security considerations
+
+### Future Enhancements
+9. ⚠️ Add Content Security Policy headers
+10. ⚠️ Implement rate limiting on API endpoints
+11. ⚠️ Add SQL query logging and monitoring
+12. ⚠️ Create security testing suite
+
+## Current Risk Assessment
+
+**ComponentTreeRenderer Security:**
+- **Risk Level:** Medium
+- **Exposure:** Low (only server-side, trusted developers)
+- **Mitigation:** features.json is not user-editable
+- **Action Required:** Fix before allowing dynamic configuration
+
+**SQL Template Security:**
+- **Risk Level:** High
+- **Exposure:** Medium (API endpoints accessible)
+- **Mitigation:** Existing validation in API routes
+- **Action Required:** Use proper Drizzle ORM methods
+
+**Query Parameter Issue:**
+- **Risk Level:** Critical (functionality broken)
+- **Exposure:** High (affects all CRUD operations)
+- **Mitigation:** None (runtime errors)
+- **Action Required:** Immediate fix needed
+
+## Conclusion
+
+The refactoring successfully demonstrates the concept of configuration-driven UI development. However, the security issues identified must be addressed before production use:
+
+1. **Critical:** Fix parameterized queries in record/route.ts
+2. **High Priority:** Implement safe expression evaluation
+3. **Medium Priority:** Improve type safety
+
+The architecture is sound, but implementation needs security hardening.
+
+## Testing Recommendations
+
+Add security tests:
+```typescript
+describe('Security', () => {
+  test('should reject malicious template expressions', () => {
+    const malicious = "{{require('fs').readFileSync('/etc/passwd')}}";
+    expect(() => interpolateValue(malicious, {})).toThrow();
+  });
+
+  test('should reject SQL injection attempts', () => {
+    const malicious = "users; DROP TABLE users--";
+    expect(() => validateIdentifier(malicious)).toThrow();
+  });
+});
+```