Add expense tracker app with CodeRabbit review config#7
Closed
Add expense tracker app with CodeRabbit review config#7
Conversation
Adds full backend (Express/SQLite) and frontend (React) source files with annotated bugs across all review categories: SQL injection, auth vulnerabilities, logic errors, performance/memory leaks, and accessibility violations. Also aligns OpenAI workflow prompt with Claude workflow for fair comparison. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Claude: instruct action to post inline review comments per finding - OpenAI: request structured JSON, use pulls.createReview for inline comments with fallback to table format if line mapping fails
Configure assertive review profile with path-specific instructions, security/performance quality gates, sequence diagrams, and auto-review for the expense tracker app.
Review Summary by QodoAdd CodeRabbit review configuration and full-stack expense tracker demo app
WalkthroughsDescription• Adds full-stack TypeScript expense tracker with intentional bugs for review testing • Introduces .coderabbit.yaml configuration with assertive review profile and quality gates • Removes Claude and OpenAI GitHub Actions workflows to isolate CodeRabbit as sole reviewer • Implements backend (Express/SQLite) and frontend (React) with annotated security, performance, and accessibility vulnerabilities Diagramflowchart LR
A["CodeRabbit Config"] -->|"assertive profile"| B["Review Settings"]
B -->|"path-specific instructions"| C["Backend Routes"]
B -->|"path-specific instructions"| D["Frontend Components"]
C -->|"SQL injection, auth bugs"| E["Intentional Vulnerabilities"]
D -->|"XSS, a11y issues"| E
F["Remove Workflows"] -->|"Claude & OpenAI"| G["Isolate CodeRabbit"]
File Changes1. .coderabbit.yaml
|
Code Review by Qodo
1. SQLi in login
|
|
Caution Review failedPull request was closed or merged during review Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. 📝 WalkthroughWalkthroughThis PR introduces a complete expense-tracking application with a backend Express.js API, SQLite database, JWT authentication middleware, and a React frontend with multiple pages. It also adds CodeRabbit automated review configuration and removes legacy Claude/OpenAI review workflows. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend as Frontend App
participant API as Backend API
participant DB as SQLite DB
User->>Frontend: 1. Enter credentials & submit
Frontend->>API: 2. POST /auth/login
API->>DB: 3. Query user by email
DB-->>API: 4. Return user record
API->>API: 5. Verify password (bcrypt)
API-->>Frontend: 6. Return JWT token
Frontend->>Frontend: 7. Store token in localStorage
Frontend->>Frontend: 8. Navigate to /dashboard
User->>Frontend: 9. View dashboard
Frontend->>API: 10. GET /reports/my-summary (with JWT)
API->>DB: 11. Query expenses by user_id
DB-->>API: 12. Return expense summary
API-->>Frontend: 13. Return aggregated summary
Frontend->>Frontend: 14. Render dashboard with totals
sequenceDiagram
actor User
participant Frontend as Frontend App
participant API as Backend API
participant DB as SQLite DB
User->>Frontend: 1. Fill expense form & submit
Frontend->>API: 2. POST /api/expenses (with JWT)
API->>API: 3. Validate amount & category
API->>DB: 4. Query user's total spending
DB-->>API: 5. Return current total
API->>API: 6. Check against BUDGET_LIMIT
API->>DB: 7. INSERT new expense record
DB-->>API: 8. Return created expense
API-->>Frontend: 9. Return expense with status
Frontend->>Frontend: 10. Navigate to /expenses list
User->>Frontend: 11. View expense list
Frontend->>API: 12. GET /api/expenses (with pagination & JWT)
API->>DB: 13. Query expenses with pagination
DB-->>API: 14. Return paginated results
API-->>Frontend: 15. Return expenses array
Frontend->>Frontend: 16. Render expense cards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+52
to
+58
| // BUG: SQL injection — user input interpolated directly into query | ||
| const user = db.prepare(`SELECT * FROM users WHERE email = '${email}'`).get() as any; | ||
|
|
||
| if (!user) { | ||
| // BUG: Leaks whether an email exists — allows user enumeration | ||
| return res.status(401).json({ error: "No account found with that email" }); | ||
| } |
There was a problem hiding this comment.
1. Sqli in login 🐞 Bug ⛨ Security
The login endpoint interpolates user-controlled email directly into SQL, allowing SQL injection to bypass authentication or exfiltrate user data.
Agent Prompt
## Issue description
`POST /api/auth/login` uses string interpolation for `email` in SQL, enabling SQL injection and user enumeration.
## Issue Context
Use `better-sqlite3` parameter binding (`?`) and avoid revealing whether an email exists.
## Fix Focus Areas
- backend/src/routes/auth.ts[45-63]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+4
to
+33
| const JWT_SECRET = "supersecretkey123"; // BUG: Hardcoded JWT secret — should use environment variable | ||
|
|
||
| export interface AuthRequest extends Request { | ||
| user?: { id: number; email: string; role: string }; | ||
| } | ||
|
|
||
| export function authenticate(req: AuthRequest, res: Response, next: NextFunction) { | ||
| const header = req.headers.authorization; | ||
| if (!header) { | ||
| return res.status(401).json({ error: "No token provided" }); | ||
| } | ||
|
|
||
| // BUG: Does not validate "Bearer " prefix — any string with a valid JWT payload passes | ||
| const token = header.replace("Bearer ", ""); | ||
|
|
||
| try { | ||
| const payload = jwt.verify(token, JWT_SECRET) as any; | ||
| req.user = payload; | ||
| next(); | ||
| } catch { | ||
| return res.status(401).json({ error: "Invalid token" }); | ||
| } | ||
| } | ||
|
|
||
| export function requireAdmin(req: AuthRequest, res: Response, next: NextFunction) { | ||
| // BUG: Checks role from the JWT payload which the user controls — should verify role from DB | ||
| if (req.user?.role !== "admin") { | ||
| return res.status(403).json({ error: "Admin access required" }); | ||
| } | ||
| next(); |
There was a problem hiding this comment.
2. Insecure jwt auth 🐞 Bug ⛨ Security
JWT authentication is insecure: a hardcoded signing secret, non-expiring tokens, lax Authorization header parsing, and admin authorization based only on token claims enable token forgery and privilege escalation.
Agent Prompt
## Issue description
JWT auth is vulnerable to token forgery and privilege escalation due to (1) hardcoded `JWT_SECRET`, (2) missing token expiry, (3) weak Authorization parsing, and (4) `requireAdmin` trusting JWT role claim.
## Issue Context
Hardcoded secret + role-in-token check means anyone with the secret can mint an `admin` token. Even without leakage, role changes won't be reflected, and long-lived tokens increase blast radius.
## Fix Focus Areas
- backend/src/middleware/auth.ts[4-34]
- backend/src/routes/auth.ts[25-35]
- backend/src/routes/auth.ts[65-71]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+50
to
+66
| router.get("/", (req: AuthRequest, res: Response) => { | ||
| const userId = req.user!.id; | ||
| const page = parseInt(req.query.page as string) || 1; | ||
| const limit = parseInt(req.query.limit as string) || 20; | ||
| // BUG: No upper bound on limit — a client can request limit=999999 and dump the entire table | ||
| const offset = (page - 1) * limit; | ||
|
|
||
| // BUG: SQL injection — category filter is interpolated directly into the query string | ||
| const category = req.query.category as string; | ||
| let query = `SELECT * FROM expenses WHERE user_id = ?`; | ||
| if (category) { | ||
| query += ` AND category = '${category}'`; | ||
| } | ||
| query += ` ORDER BY created_at DESC LIMIT ${limit} OFFSET ${offset}`; | ||
|
|
||
| const expenses = db.prepare(query).all(userId); | ||
| return res.json({ expenses, page, limit }); |
There was a problem hiding this comment.
3. Expenses list sqli 🐞 Bug ⛨ Security
The expenses listing endpoint interpolates the category filter into SQL and allows unbounded limit, enabling SQL injection and large data dumps/DoS.
Agent Prompt
## Issue description
`GET /api/expenses` builds SQL dynamically with user-controlled `category` and an unbounded `limit`, enabling SQL injection and data dumping/DoS.
## Issue Context
Prefer `WHERE user_id = ? AND category = ?` with parameters and enforce `1 <= limit <= MAX_LIMIT`.
## Fix Focus Areas
- backend/src/routes/expenses.ts[49-67]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+69
to
+88
| // PATCH /api/expenses/:id/status | ||
| router.patch("/:id/status", (req: AuthRequest, res: Response) => { | ||
| const { status } = req.body; | ||
| const expenseId = req.params.id; | ||
|
|
||
| if (!["approved", "rejected"].includes(status)) { | ||
| return res.status(400).json({ error: "Status must be 'approved' or 'rejected'" }); | ||
| } | ||
|
|
||
| // BUG: No admin check — any authenticated user can approve/reject any expense | ||
| const expense = db.prepare("SELECT * FROM expenses WHERE id = ?").get(expenseId) as any; | ||
|
|
||
| if (!expense) { | ||
| return res.status(404).json({ error: "Expense not found" }); | ||
| } | ||
|
|
||
| // BUG: Users can approve their own expenses — no self-approval guard | ||
| db.prepare("UPDATE expenses SET status = ? WHERE id = ?").run(status, expenseId); | ||
| return res.json({ ...expense, status }); | ||
| }); |
There was a problem hiding this comment.
4. Expense status authz 🐞 Bug ⛨ Security
Any authenticated user can approve/reject any expense because the status endpoint lacks admin authorization and ownership/self-approval checks.
Agent Prompt
## Issue description
`PATCH /api/expenses/:id/status` allows any authenticated user to change any expense status.
## Issue Context
Use `requireAdmin` (after fixing it to verify role from DB) and add business-rule checks (e.g., no self-approval).
## Fix Focus Areas
- backend/src/routes/expenses.ts[69-88]
- backend/src/middleware/auth.ts[28-34]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+90
to
+104
| // DELETE /api/expenses/:id | ||
| router.delete("/:id", (req: AuthRequest, res: Response) => { | ||
| const expenseId = req.params.id; | ||
| const userId = req.user!.id; | ||
|
|
||
| const expense = db.prepare("SELECT * FROM expenses WHERE id = ?").get(expenseId) as any; | ||
|
|
||
| if (!expense) { | ||
| return res.status(404).json({ error: "Expense not found" }); | ||
| } | ||
|
|
||
| // BUG: IDOR — only checks if expense exists, not if it belongs to the requesting user | ||
| // Any authenticated user can delete any other user's expense | ||
| db.prepare("DELETE FROM expenses WHERE id = ?").run(expenseId); | ||
| return res.json({ message: "Expense deleted" }); |
There was a problem hiding this comment.
5. Expense delete idor 🐞 Bug ⛨ Security
DELETE /api/expenses/:id deletes by id without verifying the expense belongs to the requesting user, enabling IDOR and unauthorized deletion.
Agent Prompt
## Issue description
`DELETE /api/expenses/:id` is vulnerable to IDOR because it deletes expenses without verifying ownership.
## Issue Context
Use a delete statement that includes `user_id` or first check `expense.user_id === req.user.id`.
## Fix Focus Areas
- backend/src/routes/expenses.ts[90-105]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+28
to
+33
| {/* BUG: Renders description as raw HTML via dangerouslySetInnerHTML — XSS vulnerability | ||
| If a user submits '<img src=x onerror=alert(1)>' as a description, it executes */} | ||
| <div | ||
| style={{ margin: "8px 0", color: "#555" }} | ||
| dangerouslySetInnerHTML={{ __html: expense.description }} | ||
| /> |
There was a problem hiding this comment.
6. Stored xss via description 🐞 Bug ⛨ Security
Expense descriptions are rendered as raw HTML in the UI, enabling stored XSS; because auth tokens are stored in localStorage, XSS can steal tokens and take over accounts.
Agent Prompt
## Issue description
Stored XSS: user-controlled `description` is stored and later rendered via `dangerouslySetInnerHTML`, enabling arbitrary script execution and token theft.
## Issue Context
React escapes content by default when rendered as `{text}`; using `dangerouslySetInnerHTML` bypasses this. Since tokens are read from `localStorage`, any XSS can exfiltrate them.
## Fix Focus Areas
- frontend/src/components/ExpenseCard.tsx[28-33]
- backend/src/routes/expenses.ts[20-46]
- frontend/src/api/client.ts[7-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+34
to
+55
| // GET /api/reports/export | ||
| router.get("/export", requireAdmin, (req: AuthRequest, res: Response) => { | ||
| // BUG: No pagination or streaming — loads ALL expenses into memory at once | ||
| // Will cause OOM on large datasets | ||
| const expenses = db.prepare(` | ||
| SELECT e.*, u.name as user_name, u.email as user_email | ||
| FROM expenses e | ||
| JOIN users u ON e.user_id = u.id | ||
| ORDER BY e.created_at DESC | ||
| `).all(); | ||
|
|
||
| // BUG: No Content-Disposition header — browser won't prompt download | ||
| res.setHeader("Content-Type", "text/csv"); | ||
|
|
||
| // BUG: CSV injection — user-controlled fields (description, name) are not escaped. | ||
| // A description like '=CMD("calc")' will execute in Excel when opened. | ||
| let csv = "ID,User,Email,Amount,Description,Category,Status,Date\n"; | ||
| for (const e of expenses as any[]) { | ||
| csv += `${e.id},${e.user_name},${e.user_email},${e.amount},${e.description},${e.category},${e.status},${e.created_at}\n`; | ||
| } | ||
|
|
||
| return res.send(csv); |
There was a problem hiding this comment.
7. Unsafe csv export 🐞 Bug ⛨ Security
The CSV export loads all expenses into memory and outputs unescaped user fields, enabling DoS (OOM) and CSV injection when opened in spreadsheet programs.
Agent Prompt
## Issue description
`GET /api/reports/export` can OOM the server and is vulnerable to CSV injection because it loads all rows and concatenates unescaped fields.
## Issue Context
Use a streaming response (or bounded pagination) and CSV escaping/quoting; neutralize fields starting with `=`, `+`, `-`, `@`.
## Fix Focus Areas
- backend/src/routes/reports.ts[34-56]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+53
to
+60
| {teamSummary.map((row: any) => ( | ||
| <tr key={row.email}> | ||
| <td style={{ padding: 8 }}>{row.name}</td> | ||
| <td style={{ padding: 8 }}>{row.count}</td> | ||
| {/* BUG: row.total can be null (user with no expenses) — toFixed() will throw */} | ||
| <td style={{ padding: 8 }}>${row.total.toFixed(2)}</td> | ||
| </tr> | ||
| ))} |
There was a problem hiding this comment.
8. Reports page null crash 🐞 Bug ✓ Correctness
Team summary rendering calls toFixed() on row.total, but backend can return NULL totals for users with no expenses, causing a runtime crash.
Agent Prompt
## Issue description
Reports page crashes when `row.total` is null for users with no expenses.
## Issue Context
SQLite `SUM()` returns NULL when all inputs are NULL / no matching rows; with `LEFT JOIN`, this occurs for users without expenses.
## Fix Focus Areas
- backend/src/routes/reports.ts[24-29]
- frontend/src/pages/Reports.tsx[53-60]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+10
to
+24
| // BUG: Wildcard CORS — allows any origin to make authenticated requests | ||
| app.use(cors()); | ||
| app.use(express.json()); | ||
|
|
||
| app.use("/api/auth", authRoutes); | ||
| app.use("/api/expenses", expenseRoutes); | ||
| app.use("/api/reports", reportRoutes); | ||
|
|
||
| // BUG: No rate limiting on any endpoints — susceptible to brute-force attacks | ||
|
|
||
| // BUG: Global error handler leaks stack traces to the client in production | ||
| app.use((err: any, _req: express.Request, res: express.Response, _next: express.NextFunction) => { | ||
| console.error(err); | ||
| res.status(500).json({ error: err.message, stack: err.stack }); | ||
| }); |
There was a problem hiding this comment.
9. Insecure server defaults 🐞 Bug ⛨ Security
Server enables wildcard CORS, has no rate limiting, and returns error stack traces to clients, increasing attack surface and leaking sensitive internals.
Agent Prompt
## Issue description
Backend server defaults increase security risk: wildcard CORS, no rate limiting, and stack trace leakage.
## Issue Context
These settings commonly enable brute force attacks and information disclosure and can make other vulnerabilities easier to exploit.
## Fix Focus Areas
- backend/src/index.ts[10-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Greptile SummaryThis PR introduces a full-stack TypeScript expense tracker (Express + React + SQLite) alongside a Critical issues:
High-severity issues:
Medium-severity issues:
Confidence Score: 0/5
Last reviewed commit: 33e03d5 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| // BUG: SQL injection — user input interpolated directly into query | ||
| const user = db.prepare(`SELECT * FROM users WHERE email = '${email}'`).get() as any; |
There was a problem hiding this comment.
Critical: SQL injection in login query
The email value from req.body is directly interpolated into the SQL string using a template literal. An attacker can log in as any user — or destroy data — by submitting an email like ' OR '1'='1' -- or '; DROP TABLE users; --.
Use parameterized queries instead:
Suggested change
| const user = db.prepare(`SELECT * FROM users WHERE email = '${email}'`).get() as any; | |
| const user = db.prepare(`SELECT * FROM users WHERE email = ?`).get(email) as any; |
Comment on lines
+28
to
+33
| export function requireAdmin(req: AuthRequest, res: Response, next: NextFunction) { | ||
| // BUG: Checks role from the JWT payload which the user controls — should verify role from DB | ||
| if (req.user?.role !== "admin") { | ||
| return res.status(403).json({ error: "Admin access required" }); | ||
| } | ||
| next(); |
There was a problem hiding this comment.
High: Admin role authorization trusts JWT payload instead of database
requireAdmin checks req.user?.role which is read directly from the JWT claims. Because the JWT secret is hardcoded, an attacker who discovers it can craft a token with { "role": "admin" } and gain full admin access. Even with a strong secret, roles should be verified against the database on each request so that a revoked privilege takes effect immediately.
Verify the current database role rather than relying solely on the token payload.
Comment on lines
+59
to
+63
| let query = `SELECT * FROM expenses WHERE user_id = ?`; | ||
| if (category) { | ||
| query += ` AND category = '${category}'`; | ||
| } | ||
| query += ` ORDER BY created_at DESC LIMIT ${limit} OFFSET ${offset}`; |
There was a problem hiding this comment.
Critical: SQL injection in category filter and unbounded LIMIT
The category query parameter is interpolated directly into the SQL string on line 61. An attacker can inject arbitrary SQL via the ?category= parameter (e.g., ' OR '1'='1) to access other users' expenses or exfiltrate the entire table.
Additionally, the limit value from the query string is interpolated into the LIMIT clause on line 63 with no upper-bound cap, allowing a client to pass limit=999999 and dump the entire expenses table in one request.
Use parameterized queries and cap the limit:
Suggested change
| let query = `SELECT * FROM expenses WHERE user_id = ?`; | |
| if (category) { | |
| query += ` AND category = '${category}'`; | |
| } | |
| query += ` ORDER BY created_at DESC LIMIT ${limit} OFFSET ${offset}`; | |
| let query = `SELECT * FROM expenses WHERE user_id = ?`; | |
| const params: any[] = [userId]; | |
| if (category) { | |
| query += ` AND category = ?`; | |
| params.push(category); | |
| } | |
| query += ` ORDER BY created_at DESC LIMIT ? OFFSET ?`; | |
| const safeLimit = Math.min(limit, 100); | |
| const safeOffset = (page - 1) * safeLimit; | |
| params.push(safeLimit, safeOffset); | |
| const expenses = db.prepare(query).all(...params); |
Comment on lines
+70
to
+88
| router.patch("/:id/status", (req: AuthRequest, res: Response) => { | ||
| const { status } = req.body; | ||
| const expenseId = req.params.id; | ||
|
|
||
| if (!["approved", "rejected"].includes(status)) { | ||
| return res.status(400).json({ error: "Status must be 'approved' or 'rejected'" }); | ||
| } | ||
|
|
||
| // BUG: No admin check — any authenticated user can approve/reject any expense | ||
| const expense = db.prepare("SELECT * FROM expenses WHERE id = ?").get(expenseId) as any; | ||
|
|
||
| if (!expense) { | ||
| return res.status(404).json({ error: "Expense not found" }); | ||
| } | ||
|
|
||
| // BUG: Users can approve their own expenses — no self-approval guard | ||
| db.prepare("UPDATE expenses SET status = ? WHERE id = ?").run(status, expenseId); | ||
| return res.json({ ...expense, status }); | ||
| }); |
There was a problem hiding this comment.
High: Missing admin authorization on expense status update (privilege escalation)
The PATCH /:id/status route applies authenticate (any logged-in user) but does not call requireAdmin. Any regular employee can approve or reject any expense, including their own — bypassing the intended approval workflow entirely.
The requireAdmin middleware already exists. Add it to this route:
Suggested change
| router.patch("/:id/status", (req: AuthRequest, res: Response) => { | |
| const { status } = req.body; | |
| const expenseId = req.params.id; | |
| if (!["approved", "rejected"].includes(status)) { | |
| return res.status(400).json({ error: "Status must be 'approved' or 'rejected'" }); | |
| } | |
| // BUG: No admin check — any authenticated user can approve/reject any expense | |
| const expense = db.prepare("SELECT * FROM expenses WHERE id = ?").get(expenseId) as any; | |
| if (!expense) { | |
| return res.status(404).json({ error: "Expense not found" }); | |
| } | |
| // BUG: Users can approve their own expenses — no self-approval guard | |
| db.prepare("UPDATE expenses SET status = ? WHERE id = ?").run(status, expenseId); | |
| return res.json({ ...expense, status }); | |
| }); | |
| router.patch("/:id/status", requireAdmin, (req: AuthRequest, res: Response) => { |
Comment on lines
+91
to
+105
| router.delete("/:id", (req: AuthRequest, res: Response) => { | ||
| const expenseId = req.params.id; | ||
| const userId = req.user!.id; | ||
|
|
||
| const expense = db.prepare("SELECT * FROM expenses WHERE id = ?").get(expenseId) as any; | ||
|
|
||
| if (!expense) { | ||
| return res.status(404).json({ error: "Expense not found" }); | ||
| } | ||
|
|
||
| // BUG: IDOR — only checks if expense exists, not if it belongs to the requesting user | ||
| // Any authenticated user can delete any other user's expense | ||
| db.prepare("DELETE FROM expenses WHERE id = ?").run(expenseId); | ||
| return res.json({ message: "Expense deleted" }); | ||
| }); |
There was a problem hiding this comment.
Critical: IDOR — any authenticated user can delete any expense
The delete handler retrieves the expense by ID but never checks that expense.user_id === userId. User A can permanently delete User B's expense by knowing (or guessing) its numeric ID.
Add ownership verification before deletion:
Suggested change
| router.delete("/:id", (req: AuthRequest, res: Response) => { | |
| const expenseId = req.params.id; | |
| const userId = req.user!.id; | |
| const expense = db.prepare("SELECT * FROM expenses WHERE id = ?").get(expenseId) as any; | |
| if (!expense) { | |
| return res.status(404).json({ error: "Expense not found" }); | |
| } | |
| // BUG: IDOR — only checks if expense exists, not if it belongs to the requesting user | |
| // Any authenticated user can delete any other user's expense | |
| db.prepare("DELETE FROM expenses WHERE id = ?").run(expenseId); | |
| return res.json({ message: "Expense deleted" }); | |
| }); | |
| if (!expense) { | |
| return res.status(404).json({ error: "Expense not found" }); | |
| } | |
| if (expense.user_id !== userId) { | |
| return res.status(403).json({ error: "Forbidden" }); | |
| } | |
| db.prepare("DELETE FROM expenses WHERE id = ?").run(expenseId); | |
| return res.json({ message: "Expense deleted" }); |
Comment on lines
+8
to
+14
| useEffect(() => { | ||
| // BUG: No cleanup/abort — if the component unmounts before the request completes, | ||
| // this will attempt to setState on an unmounted component (memory leak) | ||
| client.get("/reports/my-summary").then((res) => { | ||
| setSummary(res.data.summary); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
Medium: Missing useEffect cleanup — potential state update on unmounted component
If the user navigates away before the fetch resolves, setSummary will be called on an unmounted component, producing a React warning and a potential memory leak. The same pattern exists in ExpenseList.tsx (lines 19–24) and Reports.tsx (lines 8–13).
Use an AbortController to cancel in-flight requests:
Suggested change
| useEffect(() => { | |
| // BUG: No cleanup/abort — if the component unmounts before the request completes, | |
| // this will attempt to setState on an unmounted component (memory leak) | |
| client.get("/reports/my-summary").then((res) => { | |
| setSummary(res.data.summary); | |
| }); | |
| }, []); | |
| useEffect(() => { | |
| const controller = new AbortController(); | |
| client.get("/reports/my-summary", { signal: controller.signal }).then((res) => { | |
| setSummary(res.data.summary); | |
| }).catch((err) => { | |
| if (err.name !== "AbortError") console.error(err); | |
| }); | |
| return () => controller.abort(); | |
| }, []); |
Comment on lines
+48
to
+53
| // BUG: CSV injection — user-controlled fields (description, name) are not escaped. | ||
| // A description like '=CMD("calc")' will execute in Excel when opened. | ||
| let csv = "ID,User,Email,Amount,Description,Category,Status,Date\n"; | ||
| for (const e of expenses as any[]) { | ||
| csv += `${e.id},${e.user_name},${e.user_email},${e.amount},${e.description},${e.category},${e.status},${e.created_at}\n`; | ||
| } |
There was a problem hiding this comment.
High: CSV formula injection
User-controlled fields (description, user_name, user_email) are written directly into CSV cells without sanitization. A description starting with =, +, -, or @ is treated as a spreadsheet formula when the file is opened in Excel or Google Sheets, which can execute arbitrary commands on the recipient's machine.
Sanitize all user-controlled fields by:
- Wrapping each field in double quotes with inner double-quotes escaped (replace
"→"") - Prefixing with a single quote any cell value that begins with
=,+,-,@, tab, or carriage return
| import { Request, Response, NextFunction } from "express"; | ||
| import jwt from "jsonwebtoken"; | ||
|
|
||
| const JWT_SECRET = "supersecretkey123"; // BUG: Hardcoded JWT secret — should use environment variable |
There was a problem hiding this comment.
Critical: Hardcoded JWT signing key in source control
The JWT signing key is a short, static string committed directly to the repository. Anyone with read access can forge valid JWTs for any user or role. This is further worsened by the key being re-exported on line 36.
Replace it with an environment variable and fail at startup if missing. The key should never be stored in code.
Comment on lines
+29
to
+33
| const token = jwt.sign( | ||
| { id: result.lastInsertRowid, email, role: "member" }, | ||
| JWT_SECRET | ||
| // BUG: No token expiration — JWTs are valid forever | ||
| ); |
There was a problem hiding this comment.
Critical: JWTs issued without expiration
Neither the /register nor the /login jwt.sign calls include an expiresIn option. Tokens are valid indefinitely — a stolen or leaked token can never be invalidated without rotating the signing key.
Add a short-lived expiry (e.g., 8 hours) as the third argument to each jwt.sign() call. Include an options object with { expiresIn: "8h" }.
Comment on lines
+65
to
+69
| const token = jwt.sign( | ||
| { id: user.id, email: user.email, role: user.role }, | ||
| JWT_SECRET | ||
| // BUG: No token expiration here either | ||
| ); |
There was a problem hiding this comment.
Critical: JWTs issued without expiration (login route)
The /login route also fails to set expiresIn. Add token expiration here as well to prevent indefinite token validity. Include an options object with a short expiry time (e.g., 8 hours) as the third argument to jwt.sign().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.coderabbit.yamlwith assertive review profile, path-specific instructions, security/performance quality gates, and sequence diagramsWhat to look for
CodeRabbit should automatically review this PR and demonstrate:
Summary by CodeRabbit
New Features
Chores