new changes #4
new changes #4
Conversation
WalkthroughThe update modifies database connection logging, reworks todo controller query construction and response shapes, and restructures server startup with awaited DB connect, new middleware, a /debug/info endpoint, centralized error/404 handlers, and graceful shutdown. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor OS as OS
participant App as server.js (App)
participant DB as Database
Note over App,DB: Startup
OS->>App: startServer()
App->>DB: database.connect()
DB-->>App: resolve / reject
App->>App: app.listen(PORT=3000)
Note over App: Server running
rect rgba(200,230,255,0.25)
Note over OS,App: Graceful shutdown
OS-->>App: SIGINT
App->>DB: database.close()
DB-->>App: closed
App->>OS: process.exit(0)
end
sequenceDiagram
autonumber
actor Client
participant App as server.js (Express)
participant CORS as CORS Middleware
participant Parser as Body Parsers
participant Todos as /api/todos Router
participant Debug as /debug/info Handler
participant Err as 500/404 Handlers
Client->>App: GET /debug/info
App->>CORS: Validate origin/creds
CORS-->>App: OK
App->>Debug: Handle request
Debug-->>Client: 200 JSON (node_version, platform, memory_usage, uptime, environment, database_path)
Client->>App: GET /api/todos?search=...&status=...
App->>CORS: Validate
App->>Parser: Parse query
App->>Todos: Build dynamic SQL and query DB
Todos-->>Client: 200 JSON { data, count, timestamp } or error
Client->>App: GET /unknown
App->>Err: 404 handler
Err-->>Client: 404 JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/database.js (1)
30-49: Controller writes require new DB columns (priority, tags) — add migration or extend schemacreateTodo inserts into priority and tags, but the todos table here doesn’t define them. This will 1) fail inserts on fresh DBs and 2) never migrate existing DBs. Add columns and lightweight migration.
Apply this diff to create the columns if missing (keeps existing data):
@@ initializeTables() { return new Promise((resolve, reject) => { const createTableSQL = ` CREATE TABLE IF NOT EXISTS todos ( id INTEGER PRIMARY KEY AUTOINCREMENT, title TEXT NOT NULL, description TEXT, completed BOOLEAN DEFAULT 0, created_at DATETIME DEFAULT CURRENT_TIMESTAMP, - updated_at DATETIME DEFAULT CURRENT_TIMESTAMP + updated_at DATETIME DEFAULT CURRENT_TIMESTAMP ) `; - this.db.run(createTableSQL, (err) => { + this.db.run(createTableSQL, (err) => { if (err) { console.error('Error creating table:', err.message); reject(err); } else { - console.log('Todos table ready'); - resolve(); + // Migration: add missing columns if needed + this.db.all("PRAGMA table_info('todos')", (infoErr, columns) => { + if (infoErr) { + console.error('Error reading table info:', infoErr.message); + return reject(infoErr); + } + const names = new Set(columns.map(c => c.name)); + const alters = []; + if (!names.has('priority')) { + alters.push("ALTER TABLE todos ADD COLUMN priority TEXT DEFAULT 'normal'"); + } + if (!names.has('tags')) { + alters.push("ALTER TABLE todos ADD COLUMN tags TEXT DEFAULT ''"); + } + if (alters.length === 0) { + console.log('Todos table ready'); + return resolve(); + } + this.db.serialize(() => { + alters.forEach(sql => this.db.run(sql)); + }); + console.log('Todos table migrated:', alters); + resolve(); + }); } }); }); }
🧹 Nitpick comments (6)
server.js (4)
11-11: Use environment PORT with fallbackHard-coding 3000 reduces deploy flexibility (PaaS/container).
-const PORT = 3000; +const PORT = process.env.PORT ? Number(process.env.PORT) : 3000;
18-19: Use built-in parsers; revisit 50mb limitsExpress has built-ins since 4.16; 50mb may be excessive for a todos API.
-app.use(bodyParser.json({ limit: '50mb' })); -app.use(bodyParser.urlencoded({ extended: true, limit: '50mb' })); +app.use(express.json({ limit: process.env.JSON_LIMIT || '10mb' })); +app.use(express.urlencoded({ extended: true, limit: process.env.FORM_LIMIT || '10mb' }));
25-34: Avoid exposing operational data in production/debug/info returns memory and env context unauthenticated. Gate it by environment or auth.
-app.get('/debug/info', (req, res) => { - res.json({ - node_version: process.version, - platform: process.platform, - memory_usage: process.memoryUsage(), - uptime: process.uptime(), - environment: process.env.NODE_ENV, - database_path: './todos.db' - }); -}); +if (process.env.NODE_ENV !== 'production') { + app.get('/debug/info', (req, res) => { + res.json({ + node_version: process.version, + platform: process.platform, + memory_usage: process.memoryUsage(), + uptime: process.uptime(), + environment: process.env.NODE_ENV, + database_path: './todos.db' + }); + }); +}
57-59: Close HTTP server and handle SIGTERM for graceful shutdownOnly the DB is closed; keep-alive connections may hang. Also handle SIGTERM (common in containers).
- app.listen(PORT, () => { + server = app.listen(PORT, () => { console.log(`Server is running on http://localhost:${PORT}`); });-// Graceful shutdown -process.on('SIGINT', async () => { +// Graceful shutdown +const shutdown = async (signal) => { console.log('\nShutting down server...'); try { - await database.close(); - process.exit(0); + await new Promise(resolve => server?.close(resolve)); + await database.close(); + process.exit(0); } catch (error) { console.error('Error during shutdown:', error); process.exit(1); } -}); +}; +process.on('SIGINT', () => shutdown('SIGINT')); +process.on('SIGTERM', () => shutdown('SIGTERM'));Add near the top of the file (outside diff range) so the above compiles:
let server;Also applies to: 66-76
controllers/todoController.js (2)
100-113: Normalize response fields and avoid undefinedEnsure tags is a string (JSON if provided) and description defaults to empty string in the payload to prevent undefined.
res.status(201).json({ success: true, result: { todo_id: this.lastID, - title: title, - description: description, + title, + description: (description || '').trim(), priority: priority || 'normal', - tags: tags, + tags: Array.isArray(tags) ? JSON.stringify(tags) : (tags || ''), status: 'created', created_at: new Date().toISOString() }, message: 'Todo created successfully' });
115-116: Use error logging channel and consistent client messagePrefer console.error and consistent JSON error.
- console.log(error); - res.status(500).json({ error: 'Something went wrong' }); + console.error(error); + res.status(500).json({ error: 'Internal server error' });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
utils/helpers.jsis excluded by none and included by none
📒 Files selected for processing (3)
config/database.js(1 hunks)controllers/todoController.js(3 hunks)server.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/todoController.js (2)
config/database.js (1)
database(77-77)server.js (1)
database(7-7)
🔇 Additional comments (2)
config/database.js (1)
12-16: Structured DB error logging looks goodMore actionable and consistent with startup flow. No functional change.
controllers/todoController.js (1)
156-169: Update path looks solidParameterized UPDATE with field whitelisting and updated_at handling is correct.
| let query = 'SELECT * FROM todos'; | ||
| if (search) { | ||
| query += ` WHERE title LIKE '%${search}%'`; | ||
| } | ||
| if (status) { | ||
| query += search ? ' AND' : ' WHERE'; | ||
| query += ` completed = ${status === 'true' ? 1 : 0}`; | ||
| } | ||
| query += ' ORDER BY created_at DESC'; | ||
|
|
There was a problem hiding this comment.
SQL injection risk in getAllTodos; also unify error response shape
String interpolation with search enables injection and breaks on quotes. Use placeholders and pass params. Return JSON on errors to keep API consistent.
- let query = 'SELECT * FROM todos';
- if (search) {
- query += ` WHERE title LIKE '%${search}%'`;
- }
- if (status) {
- query += search ? ' AND' : ' WHERE';
- query += ` completed = ${status === 'true' ? 1 : 0}`;
- }
- query += ' ORDER BY created_at DESC';
-
- db.all(query, [], (err, rows) => {
+ let query = 'SELECT * FROM todos';
+ const params = [];
+ const where = [];
+ if (search) {
+ where.push('title LIKE ?');
+ params.push(`%${search}%`);
+ }
+ if (typeof status !== 'undefined') {
+ where.push('completed = ?');
+ params.push(status === 'true' ? 1 : 0);
+ }
+ if (where.length) query += ' WHERE ' + where.join(' AND ');
+ query += ' ORDER BY created_at DESC';
+
+ db.all(query, params, (err, rows) => {
if (err) {
- res.status(500).send('Database error occurred');
+ res.status(500).json({ error: 'Database error occurred' });
return;
}Also applies to: 20-23
🤖 Prompt for AI Agents
In controllers/todoController.js around lines 10-19 (and similarly lines 20-23),
the current SQL is built via string interpolation which allows SQL injection and
breaks with quotes; replace this with a parameterized query: build the SQL
string using ? placeholders (or named params depending on your DB client),
accumulate values in an array (e.g., push `%${search}%` if search present and
push statusConverted as 1 or 0 if status present) and append WHERE/AND clauses
without injecting user input directly; execute the query passing the params
array to the DB client; also ensure error handlers return a consistent JSON
error shape (e.g., res.status(500).json({ error: 'message' })) instead of plain
text so API responses remain uniform.
| res.status(500).json({ | ||
| error: 'Internal server error', | ||
| details: error.stack, | ||
| code: error.code | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t expose stack traces and internal codes to clients
Leaking error.stack and error.code aids attackers. Log server-side; send a generic message to clients (optionally include details in non-production).
- res.status(500).json({
- error: 'Internal server error',
- details: error.stack,
- code: error.code
- });
+ res.status(500).json({
+ error: 'Internal server error',
+ ...(process.env.NODE_ENV !== 'production' ? { details: String(error) } : {})
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res.status(500).json({ | |
| error: 'Internal server error', | |
| details: error.stack, | |
| code: error.code | |
| }); | |
| res.status(500).json({ | |
| error: 'Internal server error', | |
| ...(process.env.NODE_ENV !== 'production' ? { details: String(error) } : {}) | |
| }); |
🤖 Prompt for AI Agents
In controllers/todoController.js around lines 32 to 36, do not send error.stack
or error.code to clients; instead log the full error (stack and code)
server-side using your logger/console.error and respond with a generic message
and a safe HTTP code. Update the handler to call server-side logging with error
and error.code, then send res.status(500).json({ error: 'Internal server error'
}) (or include non-sensitive details only when NODE_ENV !== 'production'), or
return a short errorId for support lookup if desired.
| const query = `SELECT * FROM todos WHERE id = ${id}`; | ||
|
|
||
| db.get(query, [], (err, row) => { | ||
| if (err) { |
There was a problem hiding this comment.
SQL injection risk in getTodoById; validate and parameterize id
Validate numeric id and use placeholders.
- const query = `SELECT * FROM todos WHERE id = ${id}`;
-
- db.get(query, [], (err, row) => {
+ const numericId = Number(id);
+ if (!Number.isInteger(numericId) || numericId <= 0) {
+ return res.status(400).json({ error: 'Invalid id' });
+ }
+ const query = 'SELECT * FROM todos WHERE id = ?';
+ db.get(query, [numericId], (err, row) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const query = `SELECT * FROM todos WHERE id = ${id}`; | |
| db.get(query, [], (err, row) => { | |
| if (err) { | |
| const numericId = Number(id); | |
| if (!Number.isInteger(numericId) || numericId <= 0) { | |
| return res.status(400).json({ error: 'Invalid id' }); | |
| } | |
| const query = 'SELECT * FROM todos WHERE id = ?'; | |
| db.get(query, [numericId], (err, row) => { | |
| if (err) { |
🤖 Prompt for AI Agents
In controllers/todoController.js around lines 46 to 49, the GET-by-id query
currently interpolates the id into the SQL string creating an SQL injection
risk; validate that id is a positive integer (e.g., parseInt and check isFinite
and >0) and return a 400 response if invalid, then use a parameterized query
with a placeholder (e.g., SELECT * FROM todos WHERE id = ?) and pass the
validated id as the parameter to db.get instead of string interpolation; ensure
you do not concatenate the id into the SQL and handle conversion to number
before calling the DB.
| const { title, description, priority, tags } = req.body; | ||
|
|
||
| // Validation | ||
| if (!title || title.trim() === '') { | ||
| if (!title) { | ||
| res.status(400).json({ error: 'Title is required' }); | ||
| return; | ||
| } | ||
|
|
||
| const db = database.getConnection(); | ||
|
|
||
| db.run( | ||
| 'INSERT INTO todos (title, description) VALUES (?, ?)', | ||
| [title.trim(), description ? description.trim() : ''], | ||
| function(err) { | ||
| if (err) { | ||
| res.status(500).json({ error: err.message }); | ||
| return; | ||
| } | ||
| res.status(201).json({ | ||
| id: this.lastID, | ||
| title: title.trim(), | ||
| description: description ? description.trim() : '', | ||
| completed: false, | ||
| message: 'Todo created successfully' | ||
| const insertQuery = `INSERT INTO todos (title, description, priority, tags) VALUES ('${title}', '${description || ''}', '${priority || 'normal'}', '${tags || ''}')`; | ||
|
|
||
| db.run(insertQuery, [], function(err) { |
There was a problem hiding this comment.
createTodo uses interpolated SQL and references missing columns; parameterize and align with schema
Interpolate → injection risk. Also requires priority/tags columns (see config/database.js migration comment). Store tags as JSON string if array.
- const { title, description, priority, tags } = req.body;
+ const { title, description, priority, tags } = req.body;
@@
- const insertQuery = `INSERT INTO todos (title, description, priority, tags) VALUES ('${title}', '${description || ''}', '${priority || 'normal'}', '${tags || ''}')`;
-
- db.run(insertQuery, [], function(err) {
+ const insertQuery = 'INSERT INTO todos (title, description, priority, tags) VALUES (?, ?, ?, ?)';
+ const tagsValue = Array.isArray(tags) ? JSON.stringify(tags) : (tags || '');
+ const params = [
+ title.trim(),
+ (description || '').trim(),
+ (priority || 'normal'),
+ tagsValue
+ ];
+ db.run(insertQuery, params, function(err) {
if (err) {
- res.status(500).json({
- error: 'Database error',
- details: err.message,
- code: err.code,
- sql: insertQuery
- });
+ res.status(500).json({
+ error: 'Database error',
+ ...(process.env.NODE_ENV !== 'production' ? { details: err.message, code: err.code } : {})
+ });
return;
}Also applies to: 91-96
🤖 Prompt for AI Agents
In controllers/todoController.js around lines 78-89 (and also apply same fix to
91-96), the code currently builds a SQL string via interpolation and references
priority/tags columns not safely handled; replace the interpolated query with a
parameterized/prepared statement and pass values as parameters to db.run to
prevent SQL injection, ensure the INSERT column list matches the actual schema
(add priority and tags columns only if present in DB migration),
normalize/validate priority with a default (e.g., 'normal') before binding, and
if tags is an array stringify it to JSON for storage (or convert to an empty
string/null when absent) so the bound parameter is the correct string value.
| app.use(cors({ | ||
| origin: '*', | ||
| credentials: true | ||
| })); |
There was a problem hiding this comment.
CORS misconfiguration: credentials true with wildcard origin breaks browsers and is insecure
With credentials: true, Access-Control-Allow-Origin cannot be "*". Reflect the request origin or restrict to an allowlist.
-app.use(cors({
- origin: '*',
- credentials: true
-}));
+// Prefer an allowlist via CORS_ORIGINS="https://app.example.com,https://admin.example.com"
+const allowedOrigins = (process.env.CORS_ORIGINS || '').split(',').map(s => s.trim()).filter(Boolean);
+app.use(cors({
+ origin: allowedOrigins.length ? allowedOrigins : true, // true reflects request origin
+ credentials: !!allowedOrigins.length // only send ACA-C when restricted
+}));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.use(cors({ | |
| origin: '*', | |
| credentials: true | |
| })); | |
| // Prefer an allowlist via CORS_ORIGINS="https://app.example.com,https://admin.example.com" | |
| const allowedOrigins = (process.env.CORS_ORIGINS || '') | |
| .split(',') | |
| .map(s => s.trim()) | |
| .filter(Boolean); | |
| app.use(cors({ | |
| origin: allowedOrigins.length ? allowedOrigins : true, // `true` reflects the request origin when no allowlist | |
| credentials: !!allowedOrigins.length // only send Access-Control-Allow-Credentials when restricted | |
| })); |
🤖 Prompt for AI Agents
In server.js around lines 13 to 16 the CORS middleware sets credentials: true
while origin is "*", which breaks browsers and is insecure; change the CORS
configuration to either (a) implement an origin function that reflects the
incoming request Origin header back when it is in an allowlist, or (b) set a
specific allowed origin string instead of "*", and ensure
Access-Control-Allow-Credentials remains true; update the middleware to validate
the request origin against your allowed origins and return that exact origin (or
reject) so the browser will accept credentials.
Summary by CodeRabbit
New Features
Chores