Conversation
Summary 1. Updated ROADMAP.md - Changed ffi:readline references to ffi:libedit - Updated Use Cases section - Updated Tab Completion section noting callback dependency (shelved) 2. Extended sqlite.toml with Prepared Statements Added 8 new FFI functions: - db-prepare - Compile SQL to statement - db-step - Execute/fetch rows - db-finalize - Clean up statement - db-reset - Reset for re-execution - db-column-int - Get integer column - db-column-text - Get text column - db-column-count - Get column count - db-bind-int, db-bind-text - Parameter binding 3. Created Shopping Cart Example (examples/shopping-cart/) A complete REST server demonstrating: - HTTP API: GET/POST endpoints with query parameter parsing - SQLite Persistence: Products, cart items, orders tables - Prepared Statements: Queries with result iteration - Transactions: Checkout uses BEGIN/COMMIT/ROLLBACK - Concurrent Handling: Strands for parallel requests Files created: - shopping-cart.seq - Main server (~670 lines) - sqlite.toml - FFI manifest (copied for convenience) - README.md - Documentation Test Results Products: 1. Apple - $100 (stock: 50) Cart: #1: Apple x 2 @ $100 = $200 Checkout: Order total: $550 (3 items) Stock: Apple 50→48, Milk 20→19
Pull Request Review - Shopping Cart ExampleSummaryThis PR adds a comprehensive shopping cart REST server example demonstrating SQLite prepared statements, HTTP routing, database transactions, and concurrent request handling. The implementation is well-documented and showcases advanced FFI usage. StrengthsCode Quality
FFI Implementation
Architecture
Critical Security ConcernsSQL Injection VulnerabilityLocation: shopping-cart.seq:343-358, 360-369 The add-to-cart and remove-from-cart functions use string concatenation to build SQL queries with user input directly from query parameters, creating SQL injection vulnerabilities. Attack vector: Impact: Arbitrary SQL execution, data deletion, modification, or exfiltration Recommendation: Use prepared statements with db-bind-int for parameters, or add strict input validation to ensure parameters are valid integers before concatenation. Missing Input ValidationLocation: shopping-cart.seq:498-522
Race Condition in Stock UpdatesLocation: shopping-cart.seq:400-406 The stock update query could set stock to negative values if there is insufficient inventory. Transaction should verify stock availability. Code Issues
Documentation Issues
Test CoverageMissing automated tests, SQL injection test cases, concurrency tests, and error path testing. Suggestions for ImprovementHigh Priority
Medium Priority
ConclusionThis is an excellent addition demonstrating real-world FFI usage, database transactions, HTTP routing, and concurrency. However, the SQL injection vulnerabilities are critical and should be addressed before merge to avoid teaching unsafe practices. Overall Assessment: 7/10
|
1. SQL injection vulnerabilities - Rewrote add-to-cart and remove-from-cart using prepared statements with db-bind-int instead of string concatenation 2. Input validation - Added string->int parsing with success checking in handlers, plus validation that quantity must be positive 3. Stock race condition - Added check-stock-available function that verifies sufficient stock before checkout, and updated update-stock query to include WHERE stock >= ? to prevent negative values 4. Build command path - Fixed header comment from examples/ffi/sqlite/sqlite.toml to examples/shopping-cart/sqlite.toml The shopping cart example now demonstrates secure SQLite usage with parameterized queries.
PR Review: Shopping Cart REST Server ExampleGreat work on this comprehensive example! The shopping cart demo effectively showcases multiple Seq features working together. I've reviewed the code for quality, security, and best practices. StrengthsCode Quality
Security (Well Addressed in Commit 2)
FFI Design
Architecture
Areas for Consideration1. SQL Injection in create-order (Line 462-473)Severity: MEDIUM The create-order function uses string concatenation instead of prepared statements. While total and count come from db-column-int (trusted source), this breaks the pattern established elsewhere. For consistency and defense-in-depth, should use prepared statements. 2. Statement Finalization on Error PathsIn several functions, error paths may leak prepared statement handles. Example at list-products line 271 - the comment acknowledges needing to clean up stmt but doesn't finalize. Also affects list-cart:338 and potentially others where db-prepare fails but returns a handle. 3. Race Conditions in Cart OperationsIssue: Multiple concurrent requests can create race conditions. Scenario: Two requests checkout simultaneously - both read cart total, both deduct stock, stock gets double-deducted. Current mitigation: Each connection has its own DB handle, but SQLite allows concurrent reads. The check-stock-available helps but doesn't fully prevent TOCTOU issues. Recommendation: Consider using SQLite's locking or adding application-level serialization for checkout operations. 4. Error Messages Leak Implementation DetailsLines 686-687 expose Database error to clients. For production use, avoid exposing implementation details. Generic Internal Server Error is safer. 5. Missing Content-Type for Error Responses (Minor)The http-error, http-bad-request, and http-not-found helpers set Content-Type: text/plain, but line 686 hardcodes the response without it. Should be consistent. Test CoverageThe PR includes manual testing (curl examples), which is good for a demo. Consider adding concurrent test scenarios, edge cases (checkout with item that just went out of stock), and malformed input testing. Performance Considerations
Security Summary
Documentation Quality
Educational ValueThis is an excellent learning example because it demonstrates:
RecommendationApprove with suggestions - The second commit shows excellent responsiveness to feedback by fixing the critical SQL injection issues. The remaining items are mostly about consistency (create-order) and edge cases. For a demo/example, this is high quality. Priority fixes before merge:
Nice-to-have: Overall: This is a strong contribution that significantly enhances the examples. The code quality is good, security issues from commit 1 were properly addressed, and it serves as an excellent reference for Seq developers. Well done! Lines referenced:
|
1. create-order now uses prepared statements: - Changed from string concatenation to db-prepare with db-bind-int - Added proper statement finalization on both success and error paths 2. Statement leak fixes: - Fixed all error paths to call db-finalize before returning - Functions fixed: seed-products, list-products, list-cart, add-to-cart, remove-from-cart, calculate-cart-total, check-stock-available, collect-products, collect-cart-items 3. Critical db-step/db-finalize bug fix: - db-step consumes the statement handle, so we need dup db-step drop to preserve it for db-finalize - Fixed in: add-to-cart, remove-from-cart, create-order
PR Review: Shopping Cart Example and SQLite Prepared StatementsSummaryThis PR adds a comprehensive shopping cart REST server example that demonstrates SQLite FFI with prepared statements, transactions, and concurrent request handling. The author has already addressed several rounds of feedback regarding SQL injection, input validation, and statement leak fixes. ✅ StrengthsSecurity
Code Quality
Example Value
🔍 Issues Found1. CRITICAL: Race Condition in Concurrent Cart Access (shopping-cart.seq:693-712)Each HTTP request gets its own database connection, but the cart and checkout operations are not isolated per user/session. Multiple concurrent requests could add items to the same global cart simultaneously or race during checkout. Impact: Cart items appearing/disappearing, stock going negative, orders with incorrect totals Recommendation: Document this as a single-user cart limitation, OR add session management, OR use BEGIN IMMEDIATE/EXCLUSIVE 2. Statement Handle Leak Risk in Error Paths (shopping-cart.seq:358-398)In add-to-cart, error path clarity could be improved. SQLite's sqlite3_finalize handles NULL gracefully, but adding a comment would help. Severity: Low 3. Missing Error InformationDatabase failures return generic messages without using db-errmsg for actual SQLite error details, making debugging difficult. Recommendation: Use db-errmsg to include actual error messages 4. update-stock SQL Efficiency (shopping-cart.seq:472-476)The query repeats the same subquery 3 times. Consider using a JOIN-based UPDATE for better performance. 5. No Automated TestsManual curl examples provided but no automated integration tests for transactions, concurrency, and edge cases. Recommendation: Add test file for cart operations, edge cases, and transaction rollback 6. Documentation: Stock Race Condition (shopping-cart.seq:469-477)Comment should explain the two-phase check pattern (check-stock-available + WHERE clause atomic check). 🎯 Minor Suggestions
📊 ROADMAP.md Changes
🏆 Overall AssessmentRating: Approve with Minor Recommendations High-quality example demonstrating advanced FFI patterns. Author has been responsive and fixed all critical security issues. Completed:
Recommended for follow-up:
The code is production-ready as an EXAMPLE and provides excellent educational value for learning Seq's FFI system. Excellent work on this comprehensive example! The iterative improvements show great attention to security and correctness. |
Summary
Added 8 new FFI functions:
A complete REST server demonstrating:
Files created:
Test Results
Products: 1. Apple - $100 (stock: 50)
Cart: #1: Apple x 2 @ $100 = $200
Checkout: Order total: $550 (3 items)
Stock: Apple 50→48, Milk 20→19