Conversation
- Add bcryptjs, jsonwebtoken, pg dependencies to backend
- Create backend/src/db.js (PostgreSQL pool via DATABASE_URL)
- Create backend/src/routes/users.js with:
- POST /resources/api_user.php: register (bcrypt hash) or login (JWT)
- GET /resources/api_user.php/id/:id: fetch own profile (Bearer auth)
- PUT /resources/api_user.php/id/:id: update profile/password (Bearer auth)
- Email enumeration (getAllEmails) and unauthenticated lookup not implemented
- Mount user routes in backend/src/index.js
- Add DATABASE_URL and JWT_SECRET to backend/.env.example
- Expand test suite in backend/src/index.test.js (8 tests, all passing)
- Update js/components/app-register.js: use name field, remove age/gender
- Update js/components/app-login.js: handle new {id,name,email,token} response
- Update js/components/app-profile.js: send Authorization header, adapt to new schema
- Update js/components/app-reset-password.js: require auth, remove email-enumeration calls
Agent-Logs-Url: https://github.com/Josan88/BookRunner/sessions/75df8ea5-09c5-4995-b931-d149817a40e2
Co-authored-by: Josan88 <124897328+Josan88@users.noreply.github.com>
…on profile update - Apply express-rate-limit to auth (20 req/15 min) and profile endpoints (100 req/15 min) - Use String() coercion for JWT sub vs URL param comparison - Validate name/email are non-empty on PUT, hash-validate password length - Check for duplicate email when updating a user's email address Agent-Logs-Url: https://github.com/Josan88/BookRunner/sessions/75df8ea5-09c5-4995-b931-d149817a40e2 Co-authored-by: Josan88 <124897328+Josan88@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a910b12deb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const secret = process.env.JWT_SECRET; | ||
| if (!secret) throw new Error('JWT_SECRET is not configured'); | ||
| return jwt.sign({ sub: String(user.id), email: user.email }, secret, { expiresIn: '24h' }); |
There was a problem hiding this comment.
In the Docker Compose flow I inspected (compose.yml backend environment), JWT_SECRET is not passed to the backend, so after a user registers successfully, any successful login reaches signToken() with no secret and throws before returning a token. That makes login/profile access fail out of the box unless operators discover and add an undocumented root-level env value; either wire JWT_SECRET into the compose service or provide a safe startup/default behavior.
Useful? React with 👍 / 👎.
| <template v-if="!authState.isLoggedIn"> | ||
| <div class="alert alert-warning text-center"> | ||
| Please <router-link to="/login" class="alert-link">log in</router-link> to reset your password. |
There was a problem hiding this comment.
When users follow the unchanged Forgot Password? link on the login form, they are necessarily logged out, but this new branch only tells them to log in before resetting. That removes the actual forgot-password path for anyone who cannot authenticate with their current password; either keep an unauthenticated reset flow or relabel/move this page as an authenticated password-change screen.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR migrates the app’s registration, login, profile, and password reset flows away from the legacy PHP/MySQL auth endpoints to a Node.js/Express API backed by PostgreSQL, bcrypt password hashing, JWT auth, and rate limiting.
Changes:
- Added Node/Express auth + profile endpoints at
/resources/api_user.php(POST register/login, authenticated GET/PUT profile) with bcrypt + JWT and per-route rate limiting. - Updated frontend auth-related components to use
{id, name, email, token}and attachAuthorization: Bearer <token>for profile/password updates. - Expanded backend test coverage for basic input validation and auth-required behavior (401/400 cases).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| js/components/app-reset-password.js | Replaces legacy email-enumeration reset flow with authenticated password update via JWT. |
| js/components/app-register.js | Updates registration payload to send name (and removes old schema fields like age/gender). |
| js/components/app-profile.js | Fetches/updates profile via authenticated Node API and adapts UI to name/email-only schema. |
| js/components/app-login.js | Stores full user object (incl. token) in authState and improves error handling. |
| backend/src/routes/users.js | Implements auth/profile endpoints with bcrypt hashing, JWT verification, and rate limiting. |
| backend/src/index.test.js | Adds tests for new endpoint validation/auth-guard behavior without requiring a DB. |
| backend/src/index.js | Mounts the new user routes in the Express app. |
| backend/src/db.js | Adds a pg Pool initialized from DATABASE_URL. |
| backend/package.json | Adds dependencies for pg, bcryptjs, jsonwebtoken, and express-rate-limit. |
| backend/package-lock.json | Locks newly added backend dependencies. |
| backend/.env.example | Documents new required env vars: DATABASE_URL and JWT_SECRET. |
Files not reviewed (1)
- backend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const userId = this.authState.user?.id; | ||
| const token = this.authState.user?.token; | ||
|
|
||
| fetch(`resources/api_user.php/id/${userId}`, { | ||
| method: "PUT", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer ${token}`, | ||
| }, |
| const { email, password } = req.body ?? {}; | ||
| const name = req.body?.name ?? req.body?.username; | ||
|
|
||
| if (!email || !password) { | ||
| return res.status(400).json({ error: 'Email and password are required' }); | ||
| } | ||
|
|
||
| // --- REGISTRATION --- | ||
| if (name !== undefined && name !== null && name !== '') { |
| // Duplicate email check | ||
| const existing = await db.query('SELECT id FROM users WHERE email = $1', [email]); | ||
| if (existing.rows.length > 0) { | ||
| return res.status(409).json({ error: 'Email already registered' }); |
| await db.query( | ||
| `UPDATE users SET ${setClauses.join(', ')} WHERE id = $${values.length}`, | ||
| values, | ||
| ); | ||
|
|
||
| return res.status(200).json({ success: true, affected_rows: 1 }); |
| if (!data) { | ||
| this.msg = "Username or password incorrect."; | ||
| if (!data || data.error) { | ||
| this.msg = data?.error || "Username or password incorrect."; |
| // --------------------------------------------------------------------------- | ||
| // POST /resources/api_user.php – input validation (no database required) | ||
| // --------------------------------------------------------------------------- | ||
|
|
Josan88
left a comment
Josan88
left a comment
There was a problem hiding this comment.
Reviewed the auth migration. I would not merge this as-is.
Blocking issues:
-
JWT_SECRETis not wired into the Docker Compose backend environment. The documenteddocker compose up --buildpath starts the backend without the secret, so successful login reachessignToken()and fails before returning a JWT.docker compose configconfirms onlyDATABASE_URL,HOST,NODE_ENV, andPORTare passed. -
The unchanged login-page
Forgot Password?link now points to a page that requires the user to already be logged in. That removes password recovery for users who cannot authenticate. Either keep a real unauthenticated recovery flow, or rename/move this to an authenticated password-change screen. -
PUT /resources/api_user.php/id/:idignores theUPDATEresult and always returns{ success: true, affected_rows: 1 }, even if no row was updated. CapturerowCountand return 404 or the real affected count. -
The test suite passes, but it does not cover the main new behavior: successful register/login, JWT issuance, authenticated profile GET/PUT, duplicate email handling, or password update. These paths should be covered before merging an auth rewrite.
Verification run locally on the PR branch:
cd backend
npm ci
npm testResult: 8/8 tests passed.
|
Agent-Logs-Url: https://github.com/Josan88/BookRunner/sessions/c6df6a62-9aea-4d42-97c7-37ef44948af7 Co-authored-by: Josan88 <124897328+Josan88@users.noreply.github.com>
|
@copilot try again |
All four blocking issues are addressed in commit
|
Replaces the legacy PHP/MySQL auth layer (plaintext passwords, email enumeration, unauthenticated profile access) with a Node.js/Express + PostgreSQL implementation backed by bcrypt and JWT.
Backend — new endpoints at
/resources/api_user.phpPOST— register (name+email+password) or login (email+password); registration branches on field presence so an emptynamereturns 400 instead of falling through to login; passwords hashed with bcrypt (12 rounds); login returns{id, name, email, token}(24 h JWT)GET /id/:id— fetch own profile; requires `Authorization: ******; 403 if token user ≠ path user; 404 if user not foundPUT /id/:id— updatename,email(duplicate-checked), and/orpassword(re-hashed); same auth guard; returns realrowCountand 404 when no row is updated?getAllEmails=trueand?email=…— email enumeration and unauthenticated lookup are intentionally droppedNew files:
backend/src/db.js(pg pool viaDATABASE_URL),backend/src/routes/users.js. Addedbcryptjs,jsonwebtoken,pg,express-rate-limitdependencies.Infrastructure
compose.yml:JWT_SECRETis now wired into the backend service environment with a required-variable guard (${JWT_SECRET:?...});docker compose upwill fail fast with a clear message if it is not set.env.example: documentsJWT_SECRETwith a generation commandFrontend
app-register.js: sendsnameinstead ofusername;ageandgenderfields removed (not in new schema)app-login.js: stores full{id, name, email, token}response inauthState.user(persisted tosessionStoragevia existing watcher inapp.js); fallback error message corrected to "Email or password incorrect"app-login.js: "Forgot Password?" link replaced with an informational note directing users to change their password from their profile after logging inapp-profile.js: attachesAuthorization: Bearerheader on fetch/update; UI adapted toname/email-only schemaapp-reset-password.js: requires the user to be authenticated; guardssubmitNewPassword()against undefineduserId/token(surfaces a clear message if auth state is missing); uses the JWT token toPUTthe new password; removes the two-step email-enumeration → unauthenticated reset flow entirelyTests
Backend test suite expanded from 8 → 24 tests (all using
t.mock.methodondb.query, no real database required), covering:GET /id/:id— 200 with own profile, 403 mismatched sub, 404 user not foundPUT /id/:id— name/password update 200, no-fields 400, short password 400, empty name 400, 404 when user deleted, email-conflict 409Example login flow