postgres/docs/SECURITY_REVIEW.md

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:

{
  "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:

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:

// 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:

// 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

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

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

// 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:

// 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:

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:

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();
  });
});