Files
gravl/.planning/codebase/CONCERNS.md
T
2026-02-15 21:49:31 +01:00

334 lines
18 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Codebase Concerns
**Analysis Date:** 2026-02-15
## Tech Debt
**Hardcoded Program ID in Backend:**
- Issue: Multiple API endpoints hardcode `program_id = 1` in queries, preventing multi-program support
- Files: `backend/src/index.js` (lines 27, 198, 386, 410)
- Impact: Cannot support multiple training programs; system is locked to single PPL program. Future features requiring program selection will require significant refactoring
- Fix approach: Add `program_id` parameter to endpoints; refactor to accept program ID from request or user preferences
**Hardcoded User ID Default:**
- Issue: Backend defaults to `user_id = 1` when not provided; frontend also uses fallback `user?.id || 1`
- Files: `backend/src/index.js` (line 290), `frontend/src/App.jsx` (line 21), `frontend/src/pages/ProfilePage.jsx` (lines 25-27, 48)
- Impact: Multi-user isolation broken; all users can see/modify each other's data if API auth fails. Critical security concern
- Fix approach: Remove all fallback user IDs; enforce auth token verification; validate user ownership on all endpoints
**Single Backend File Architecture:**
- Issue: All 425 lines of API logic in one file (`backend/src/index.js`); no separation into routes, controllers, middleware
- Files: `backend/src/index.js`
- Impact: Difficult to maintain, test, or extend. Mixed concerns (auth, database, business logic) in same file. No clear patterns for new endpoints
- Fix approach: Refactor into: `routes/`, `controllers/`, `middleware/`, `services/` directories
**No Request Validation:**
- Issue: No input validation on any API endpoints; accepts any data and passes to database
- Files: `backend/src/index.js` (all POST/PUT routes: lines 35-50, 103-118, 121-136, 153-168, 299-329)
- Impact: SQL injection risk (mitigated by parameterized queries but semantic validation missing), malformed data in database, inconsistent state
- Fix approach: Add validation library (e.g., joi, zod); validate types, ranges, required fields before database operations
**Weak Default JWT Secret:**
- Issue: JWT secret defaults to plain string `'gravl-secret-key-change-in-production'` if env var not set
- Files: `backend/src/index.js` (line 9)
- Impact: If deployment forgets to set JWT_SECRET env var, all tokens can be forged. Auth completely broken in that scenario
- Fix approach: Require JWT_SECRET as mandatory env var; fail at startup if not set; remove default
**Exposed Database Password in Docker Compose:**
- Issue: Database password hardcoded in plaintext in `docker-compose.yml`
- Files: `docker-compose.yml` (line 12: `DB_PASSWORD=homelab_postgres_2026`)
- Impact: Secret visible in git history and version control. Deployed as plain text to running containers
- Fix approach: Use `.env` file (gitignored) with env var substitution; never commit secrets
**Hardcoded Database Connection Defaults:**
- Issue: Database credentials have weak defaults if env vars missing
- Files: `backend/src/index.js` (lines 11-17)
- Impact: If env vars not set, connects with default user/password/database
- Fix approach: Require critical env vars (DB_HOST, DB_PORT, DB_USER, DB_PASSWORD, DB_NAME) as mandatory at startup
**No Error Handling for Database Failures:**
- Issue: Database errors logged to console but no graceful degradation or retry logic
- Files: `backend/src/index.js` (lines 46-50, 64-67, 97-100, 114-117, 132-135, 164-167, 189-191, 227-230, 245-248, 275-278, 292-295, 325-328, 379-382, 416-419)
- Impact: Client gets generic "Database error" message; no way to debug; connection pool exhaustion not handled
- Fix approach: Add structured error logging; implement connection retry logic; differentiate error messages (auth vs DB vs validation)
**Vague Error Messages:**
- Issue: Many endpoints return generic `{ error: 'Server error' }` without details
- Files: `backend/src/index.js` (lines 49, 66, 116, 134, 166, 191, 229, 248, 278, 295, 328, 381, 418)
- Impact: Frontend cannot distinguish between different failure modes; users see unhelpful messages; debugging impossible
- Fix approach: Use structured error codes (e.g., `ERR_EMAIL_EXISTS`, `ERR_INVALID_CREDENTIALS`, `ERR_DB_UNAVAILABLE`)
## Known Bugs
**User ID Fallback Breaks Multi-User:**
- Symptoms: Any endpoint called without proper auth gets user ID 1; all data accessible to wrong users
- Files: `backend/src/index.js` (line 290: `user_id || 1`)
- Trigger: Call `/api/logs/last/...?program_exercise_id=X` without user_id parameter, or auth fails silently
- Workaround: Frontend always provides user ID; but if auth token expires mid-session, falls back to user 1
**Progression Calculation Assumes User Context:**
- Symptoms: `/api/progression/:id` endpoint without auth header returns data for user_id=1, not current user
- Files: `backend/src/index.js` (line 334: `user_id || 1`)
- Trigger: Make unauthenticated request to progression endpoint
- Workaround: Frontend includes user_id in query param, but no validation that it matches auth token
**Missing Auth Validation on Read Endpoints:**
- Symptoms: `/api/logs`, `/api/logs/last/...`, `/api/progression/...` do not require auth; anyone can see anyone's data
- Files: `backend/src/index.js` (lines 252-279, 282-296, 332-383 — none have `authMiddleware`)
- Trigger: Unauthenticated request to any of these endpoints returns full data
- Workaround: None; endpoint is truly public
**Profile Fetch Endpoints Bypass Auth:**
- Symptoms: `/api/user/profile` GET and other profile endpoints sometimes called without token
- Files: `frontend/src/pages/ProfilePage.jsx` (lines 25-27 make calls with optional header)
- Trigger: Token expires or not in localStorage; frontend still tries to fetch profile
- Workaround: Redirect on 401, but data may be partially loaded
**Onboarding Does Not Validate Strength Input:**
- Symptoms: Can enter non-numeric strength values; API accepts and stores as invalid data
- Files: `frontend/src/pages/OnboardingWizard.jsx` (lines 150-153); `backend/src/index.js` (no validation)
- Trigger: Enter "abc" in 1RM field; submit saves to database
- Workaround: None; data is corrupted
## Security Considerations
**Authentication Not Required on Data Endpoints:**
- Risk: Public endpoints `/api/logs`, `/api/progression/...`, `/api/logs/last/...` expose all user workout data without auth
- Files: `backend/src/index.js` (routes at lines 252, 282, 332)
- Current mitigation: None; these routes are public
- Recommendations: Add `authMiddleware` to all endpoints that return user data; validate user_id from request matches decoded token
**User ID Not Validated on Update Operations:**
- Risk: Client sends PUT request with any user_id; no validation that it matches auth token
- Files: `backend/src/index.js` (lines 103, 121, 153, 299)
- Current mitigation: Database constraint on user_id, but not enforced at API level
- Recommendations: Extract user_id from JWT token (`req.user.id`); never accept from request body; validate ownership
**Password Hashing Strength Acceptable But Not Tested:**
- Risk: Uses bcryptjs with rounds=10 (line 39); no test for hash strength
- Files: `backend/src/index.js` (line 39)
- Current mitigation: bcryptjs with 10 rounds is secure
- Recommendations: Increase to 12+ rounds; add integration test for password hashing
**JWT Token Expiry Too Long:**
- Risk: 30-day token expiry is long; stolen token has extended window
- Files: `backend/src/index.js` (lines 44, 61)
- Current mitigation: Token stored in localStorage; vulnerable to XSS
- Recommendations: Reduce to 1-7 days; implement refresh token rotation; consider httpOnly cookies
**No CORS Validation:**
- Risk: CORS enabled for all origins (`app.use(cors())`)
- Files: `backend/src/index.js` (line 19)
- Current mitigation: None; frontend is localhost during dev, but prod deployment may not restrict
- Recommendations: Add whitelist: `cors({ origin: process.env.FRONTEND_URL })`
**Database Connection Not SSL in Docker:**
- Risk: PostgreSQL connection from Docker unencrypted if over network
- Files: `backend/src/index.js` (lines 11-17); `docker-compose.yml`
- Current mitigation: On internal `homelab` network, but not encrypted
- Recommendations: Add `ssl: true` to pool config if connecting over untrusted network
## Performance Bottlenecks
**N+1 Query Problem in Program Endpoints:**
- Problem: `/api/programs/:id` loads program, then for each day, joins exercises separately
- Files: `backend/src/index.js` (lines 196-231)
- Cause: Single query with complex LEFT JOINs and json_agg; works but could be optimized with batching
- Improvement path: Already optimized with single query; no issue here. Performance is acceptable
**No Database Indexes on Common Queries:**
- Problem: `workout_logs` queries filter by `(user_id, date, program_exercise_id)` but indexes only on two columns
- Files: `db/init.sql` (line 77); `backend/src/index.js` (lines 252-279)
- Cause: Missing composite index on `(user_id, date)` and separate on `program_exercise_id`
- Improvement path: Add `CREATE INDEX idx_workout_logs_user_date_exercise ON workout_logs(user_id, date, program_exercise_id)`
**Measurements Fetch Not Limited:**
- Problem: `/api/user/measurements` returns up to 100 rows without pagination
- Files: `backend/src/index.js` (line 142)
- Cause: LIMIT 100 hardcoded; if user has years of data, transfers unnecessary payload
- Improvement path: Add `limit` and `offset` query params; default to last 30 records
**Strength History Not Limited:**
- Problem: `/api/user/strength` also LIMIT 100
- Files: `backend/src/index.js` (line 174)
- Cause: Same as measurements
- Improvement path: Add pagination; default to last 12 records (1 year monthly checks)
**Frontend Fetches All Logs for All Exercises at Once:**
- Problem: `App.jsx` `fetchLogs()` makes one request per exercise (loop)
- Files: `frontend/src/App.jsx` (lines 35-51)
- Cause: Not batched; if 6 exercises, makes 6 API calls sequentially
- Improvement path: Batch into single endpoint; return all day's logs in one query
**Progression Calculation Fetches Last 10 Logs Per Request:**
- Problem: `/api/progression/:id` fetches last 10 logs for every exercise opened
- Files: `backend/src/index.js` (line 354)
- Cause: Called on component expand; if user expands 6 exercises, 6 queries
- Improvement path: Batch progression calculations; include in WorkoutPage's initial fetch
## Fragile Areas
**AuthContext Token Refresh Not Automatic:**
- Files: `frontend/src/context/AuthContext.jsx`
- Why fragile: 30-day token expiry means users logged in for <30d get sudden 401. No refresh token mechanism. Token stored in localStorage (XSS vulnerable)
- Safe modification: Add refresh token endpoint; implement automatic refresh-before-expiry; consider httpOnly cookies
- Test coverage: No tests; AuthContext has no test file
**Profile Fetch Without Error Boundaries:**
- Files: `frontend/src/pages/ProfilePage.jsx` (lines 22-42)
- Why fragile: Fetch errors caught in console but no UI feedback; Promise.all() fails if one fetch fails, but all three fetches (profile, measurements, strength) are separate queries
- Safe modification: Wrap each fetch in try-catch separately; show partial data if some fail; add error toast
- Test coverage: No tests
**Onboarding State Not Persisted During Request:**
- Files: `frontend/src/pages/OnboardingWizard.jsx` (lines 24-72)
- Why fragile: If user fills all 4 steps and network fails during save, form clears but data lost. No draft save
- Safe modification: Auto-save to localStorage after each step; restore on remount
- Test coverage: No tests; body fat calculation not tested
**Warmup Completion State Lost on Navigation:**
- Files: `frontend/src/pages/WorkoutPage.jsx` (lines 51-53, 79-87)
- Why fragile: `completedWarmups` is local state in component; navigating away loses progress. No persistence
- Safe modification: Save to localStorage keyed by date+day
- Test coverage: No tests
**Exercise Progression Display Race Condition:**
- Files: `frontend/src/pages/WorkoutPage.jsx` (lines 48-67)
- Why fragile: `loadProgressions()` called in useEffect with `[day]` dependency; if day changes rapidly, multiple requests in flight; setState after unmount possible
- Safe modification: Add cleanup function; abort controller for fetch; cache by day ID
- Test coverage: No tests
**Database Schema Missing User FK in Measurements:**
- Files: `db/init.sql` (lines 64-74)
- Why fragile: `user_measurements` table has no explicit FOREIGN KEY to users table; can create orphaned records; cascading delete not enforced
- Safe modification: Add `user_id INTEGER REFERENCES users(id) ON DELETE CASCADE NOT NULL`
- Test coverage: Schema not tested
**Hardcoded Warmup Exercises Not Database-Driven:**
- Files: `frontend/src/pages/WorkoutPage.jsx` (lines 5-35)
- Why fragile: Warmup data hardcoded in component; if new muscle group added to exercises, no warmups for it; mapping is manual and fragile
- Safe modification: Move to database table `warmup_exercises(muscle_group, name, duration, type)`; fetch on page load
- Test coverage: No tests; mapping logic untested
## Scaling Limits
**Single Program Per User:**
- Current capacity: System assumes all users follow program_id=1
- Limit: Cannot support multiple programs or user switching between programs
- Scaling path: Refactor endpoints to accept program_id; add `user_programs` join table; allow user to select active program
**No Pagination on History Endpoints:**
- Current capacity: `/api/user/measurements` returns LIMIT 100; `/api/user/strength` returns LIMIT 100
- Limit: If user has >100 measurements (100+ days of data), response grows indefinitely
- Scaling path: Implement cursor-based pagination; return 20-30 records per page; add date range filters
**Database Connection Pool Not Configured:**
- Current capacity: `pg` module defaults to pool size 10
- Limit: 10 concurrent connections; 11th request queues
- Scaling path: Add explicit pool configuration: `max: 20, min: 5` adjusted per load; monitor with `pg_stat_activity`
**Logs Stored Flat Without Aggregate Summary:**
- Current capacity: Every set logged individually; querying 100 workouts × 6 exercises × 3 sets = 1800 rows
- Limit: As user history grows, workout fetches slow down
- Scaling path: Add `workout_sessions` table with aggregate stats; denormalize common queries
**Frontend Loads Entire Program Structure:**
- Current capacity: `/api/programs/:id` returns all days + all exercises in one response
- Limit: With 12+ exercises per day, response grows; not a problem now but scales poorly
- Scaling path: Lazy-load exercises per day; separate endpoint for day details; cache aggressively
## Dependencies at Risk
**Express 4.x Minor Versions:**
- Risk: No automatic security updates; express vulnerabilities not patched unless manually updated
- Impact: Known CVEs in middleware could be exploited
- Migration plan: Upgrade to Express 5.x (breaking changes); or add `npm audit fix` to CI pipeline
**bcryptjs No Longer Maintained:**
- Risk: bcryptjs is unmaintained library; use native Node.js crypto instead
- Impact: Security bugs in bcryptjs won't be fixed; but practical risk is low (bcrypt algorithm is solid)
- Migration plan: Switch to `bcrypt` (native binding) or Node.js `crypto.scrypt()` + built-in functions
**jsonwebtoken Known Vulns:**
- Risk: `jsonwebtoken` has history of algorithm confusion vulnerabilities (prior versions)
- Impact: Current version 9.0.2 (in package.json) is recent; likely patched
- Migration plan: Keep up with minor/patch updates; add `npm audit` to CI
**pg Library Version:**
- Risk: `pg` 8.11.3 is from 2023; no known critical issues but check advisories
- Impact: Low; PostgreSQL driver is stable
- Migration plan: Keep updated; monitor npm advisories
## Missing Critical Features
**No Input Sanitization:**
- Problem: User inputs not sanitized before database storage; e.g., XSS in exercise names, injection in auth fields
- Blocks: Any user-generated content features (notes, comments); social features
- Fix approach: Add input sanitization library (e.g., DOMPurify for frontend, `xss` for backend); validate at both layers
**No Rate Limiting:**
- Problem: No rate limits on auth endpoints; brute force attack possible on `/api/auth/login`
- Blocks: Public deployment; production security
- Fix approach: Add `express-rate-limit` middleware; 5 attempts per 15 min per IP
**No Audit Logging:**
- Problem: No record of who did what when; can't detect unauthorized access or data changes
- Blocks: Compliance requirements; forensics
- Fix approach: Add `audit_logs` table; log all create/update/delete with user_id, timestamp, action
**No Soft Deletes:**
- Problem: No way to recover deleted data; hard deletes cascade immediately
- Blocks: Undo features; data recovery
- Fix approach: Add `deleted_at` column to tables; use soft deletes; implement undelete mechanism
**No API Versioning:**
- Problem: No v1, v2 paths; breaking changes would affect all clients
- Blocks: Safe API evolution
- Fix approach: Add `/api/v1/...` prefix; maintain backward compatibility when possible
## Test Coverage Gaps
**No Backend Tests:**
- What's not tested: All 425 lines of `backend/src/index.js` have zero test coverage
- Files: `backend/src/index.js`
- Risk: Auth logic not verified; SQL injection prevention untested; progression calculation not validated; error paths not covered
- Priority: High
**No Frontend Unit Tests:**
- What's not tested: No Jest/Vitest config; components not tested
- Files: `frontend/src/**/*.jsx` (all files)
- Risk: UI bugs not caught; hooks logic untested; state transitions not verified
- Priority: High
**No Integration Tests:**
- What's not tested: API-to-database flow untested; full workout logging flow untested
- Risk: Database schema changes break endpoints; race conditions in concurrent requests not caught
- Priority: Medium
**No E2E Tests:**
- What's not tested: User flows untested (register → onboard → log workout → progress)
- Risk: Broken onboarding, broken login, navigation issues in production not caught
- Priority: Medium
**Auth Logic Not Tested:**
- What's not tested: Token verification, expiry, malformed tokens, missing auth headers
- Files: `backend/src/index.js` (lines 22-29); `frontend/src/context/AuthContext.jsx`
- Risk: Auth bypass vulnerabilities not detected
- Priority: Critical
**Progression Calculation Not Tested:**
- What's not tested: Algorithm for suggesting weight increases
- Files: `backend/src/index.js` (lines 332-383)
- Risk: Incorrect progression logic goes unnoticed; users stay on same weight or jump too much
- Priority: High
**Database Schema Not Validated:**
- What's not tested: Foreign key constraints, cascading deletes, data types
- Files: `db/init.sql`
- Risk: Invalid data created; orphaned records; type mismatches
- Priority: Medium
---
*Concerns audit: 2026-02-15*