334 lines
18 KiB
Markdown
334 lines
18 KiB
Markdown
# 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*
|