Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
rivetkit-serverless Error Error Jan 2, 2026 8:02pm
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 2, 2026 8:02pm
rivet-inspector Ignored Ignored Preview Jan 2, 2026 8:02pm
rivet-site Ignored Ignored Preview Jan 2, 2026 8:02pm

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Dec 21, 2025

Pull Request Review: SQLite Support

This PR adds SQLite database support to RivetKit with a custom KV-backed VFS implementation.

✅ Strengths

  1. Solid Architecture - Clean separation between VFS layer and database providers with well-designed DatabaseProvider abstraction
  2. Good Test Coverage - Comprehensive VFS tests and driver integration tests
  3. Documentation - Examples follow template with clear READMEs

⚠️ Critical Issues

1. SQL Injection Vulnerability (rivetkit-typescript/packages/db/src/raw/mod.ts:36-38, 64-65)

The raw database wrapper accepts parameterized queries but doesn't bind them. The exec method only takes a callback, so parameters are ignored. This is misleading and potentially unsafe.

Fix: Either implement proper parameter binding or remove the args parameter.

2. Missing Error Handling in AsyncMutex (rivetkit-typescript/packages/sqlite-vfs/src/mod.ts:105-123)

If an error occurs between acquire and release, the mutex stays locked forever.

Fix: Add timeout support or document that callers MUST use try/finally.

High Priority Issues

3. Query Type Detection (rivetkit-typescript/packages/db/src/raw/mod.ts:45-63)

String-based query type detection is fragile and doesn't handle comments or CTEs.

4. Parameter Validation - getChunkKey and getMetaKey should validate inputs

5. Performance - Array operations could be optimized for large reads

Medium Priority

6. Incomplete Cleanup - Database cleanup might not handle all edge cases (failed close, journal files, KV cleanup)

7. Magic Numbers - CHUNK_SIZE and prefix bytes should be named constants

8. Code Duplication - Similar execute wrappers in multiple files

Security Concerns

  1. Parameter binding doesn't work (SQL injection risk)
  2. File paths should be validated (directory traversal)
  3. Error messages might expose schema info

Test Coverage

Good: CRUD, persistence, transactions, large data, multiple databases, actor isolation

Missing: Error conditions, concurrent access, migration failures, cleanup, parameter binding, SQL injection tests

Action Items

Must Fix:

  1. Fix or document parameter binding issue
  2. Add error handling docs for AsyncMutex
  3. Validate file paths

Should Fix:
4. Improve query type detection
5. Add error case tests
6. Document cleanup behavior

Summary

Well-architected implementation with good test coverage for happy paths. Main concerns are parameter binding (appears broken) and missing error handling.

Overall: ⚠️ Needs Revisions

The code is high quality but has a critical parameter binding issue that must be addressed.

Great work on the architecture! 🎉

@claude
Copy link

claude bot commented Dec 21, 2025

Pull Request Review: SQLite Support

This PR adds SQLite database support to RivetKit with a KV-backed VFS implementation. Overall, this is a significant and well-structured feature, but there are critical security vulnerabilities that must be addressed before merging.


🚨 Critical Security Issues

1. SQL Injection Vulnerabilities

Location: rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/actor-db-raw.ts

Multiple instances of SQL injection through string interpolation (lines 19, 48, 65, 74). Any user input passed to these functions could execute arbitrary SQL commands.

Fix: Use parameterized queries instead of string interpolation:

// WRONG - vulnerable to SQL injection
await c.db.execute(
  `INSERT INTO test_data (value, created_at) VALUES ('${value}', ${Date.now()})`
);

// CORRECT - use parameterized queries
await c.db.execute(
  "INSERT INTO test_data (value, created_at) VALUES (?, ?)",
  value,
  Date.now()
);

Note: The examples/sqlite-raw/src/registry.ts correctly uses parameterized queries.


⚠️ Code Quality & Best Practices

2. Missing Error Handling in Raw Database Client

Location: rivetkit-typescript/packages/rivetkit/src/db/mod.ts:68-82

The execute method has no bounds checking when building result objects. Add validation:

for (let i = 0; i < Math.min(row.length, columnNames.length); i++) {
  const columnName = columnNames[i];
  if (columnName) {
    rowObj[columnName] = row[i];
  }
}

3. Inconsistent Parameter Handling

Location: rivetkit-typescript/packages/rivetkit/src/db/drizzle/mod.ts:95-98

The Drizzle wrapper accepts args but ignores them. Either use them, remove them, or document why they're ignored.

4. Resource Leak Potential

Location: rivetkit-typescript/packages/rivetkit/src/db/vfs/mod.ts:234-264

If an error occurs between registerFile and database open, the file remains registered. Add try-catch cleanup.

5. Type Safety

Location: rivetkit-typescript/packages/rivetkit/src/db/drizzle/mod.ts:28

Avoid migrations?: any. Use a more specific type.

6. Missing Documentation

Add JSDoc to public APIs explaining when onMigrate is called, what happens on failure, etc.


✅ Strengths

  1. Well-structured VFS implementation - Clean separation and good documentation
  2. Good test coverage - Basic operations and persistence tested
  3. Performance optimization - Batch operations reduce KV round trips
  4. Clear examples - Both raw and Drizzle examples are helpful
  5. Proper chunking - 4KB chunks are well-balanced
  6. Concurrency handling - AsyncMutex prevents race conditions

📝 Recommendations

Testing

  1. Add SQL injection prevention tests
  2. Test concurrent database access
  3. Test large file truncation edge cases
  4. Test error handling (OOM, corrupted data)

Documentation

  1. Add JSDoc to all public APIs
  2. Document 4KB chunk size rationale
  3. Add migration guide from old @rivetkit/db

🎯 Summary

Must fix before merge:

  • ❌ SQL injection in actor-db-raw.ts:19, 48, 65, 74
  • ⚠️ Error handling in raw database client
  • ⚠️ Resource leak in VFS open

Should fix:

  • Parameter handling in Drizzle wrapper
  • Type safety (any types)
  • Missing documentation

The core implementation is solid, but SQL injection is a critical security blocker. Estimated fix time: 1-2 hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants