codex/password-security-plan-comments #2
@@ -0,0 +1,77 @@
|
|||||||
|
# Password Security Hardening Plan
|
||||||
|
|
||||||
|
## Scope
|
||||||
|
- Applies to auth and password flows in:
|
||||||
|
- `index.js`
|
||||||
|
- `auth/authEmail.js`
|
||||||
|
- `mongoDB.js`
|
||||||
|
|
||||||
|
## 1. Replace insecure reset flow with token-based reset
|
||||||
|
- Problem:
|
||||||
|
- Current flow resets by username and emails a plaintext temporary password.
|
||||||
|
- Implementation:
|
||||||
|
- Add `POST /password/request-reset`:
|
||||||
|
- Accept identifier (email).
|
||||||
|
- Always return generic success response.
|
||||||
|
- If account exists, create one-time reset token with short TTL (15-30 min), store hashed token, email reset link.
|
||||||
|
- Add `POST /password/confirm-reset`:
|
||||||
|
- Accept token + new password.
|
||||||
|
- Validate token (exists, not expired, unused), then rotate password hash and invalidate all active sessions.
|
||||||
|
- Data model:
|
||||||
|
- New collection `password_reset_tokens` with fields:
|
||||||
|
- `userId`, `tokenHash`, `expiresAt`, `usedAt`, `createdAt`, `requestMeta`.
|
||||||
|
- Add TTL index on `expiresAt`.
|
||||||
|
|
||||||
|
## 2. Remove credential handling from GET/query
|
||||||
|
- Problem:
|
||||||
|
- `/signup` and `/login` accept GET and query params for credentials.
|
||||||
|
- Implementation:
|
||||||
|
- Change auth routes to `POST` only.
|
||||||
|
- Read credentials from JSON body only.
|
||||||
|
- Reject query-based credential inputs with `400`.
|
||||||
|
- Update Swagger docs and clients accordingly.
|
||||||
|
|
||||||
|
## 3. Add account-aware brute-force protection
|
||||||
|
- Problem:
|
||||||
|
- Only global IP limiter exists; auth endpoints are not sufficiently protected.
|
||||||
|
- Implementation:
|
||||||
|
- Add dedicated limiter middleware for auth endpoints (`/login`, `/password/request-reset`, `/password/confirm-reset`):
|
||||||
|
- Combined key: normalized username/email + source IP.
|
||||||
|
- Lower thresholds and progressive backoff/lockout window.
|
||||||
|
- Add telemetry for blocked attempts.
|
||||||
|
- Optionally store counters in Redis if horizontally scaled.
|
||||||
|
|
||||||
|
## 4. Prevent account enumeration
|
||||||
|
- Problem:
|
||||||
|
- API exposes different responses for unknown user vs wrong password.
|
||||||
|
- Implementation:
|
||||||
|
- Login: same response for invalid credentials regardless of user existence.
|
||||||
|
- Reset request: always same response regardless of account existence.
|
||||||
|
- Keep detailed reason only in internal logs/analytics.
|
||||||
|
|
||||||
|
## 5. Keep strong password hashing policy (clarify bcrypt behavior)
|
||||||
|
- Problem:
|
||||||
|
- Existing TODO comments incorrectly state bcrypt needs manual salt handling.
|
||||||
|
- Implementation:
|
||||||
|
- Keep bcrypt (or migrate to Argon2id in a separate change set).
|
||||||
|
- Centralize hash policy:
|
||||||
|
- cost factor (benchmark-backed; start at 12 if acceptable latency).
|
||||||
|
- minimum password length and strength checks.
|
||||||
|
- On login, detect outdated hash params and rehash after successful auth.
|
||||||
|
|
||||||
|
## Suggested rollout order
|
||||||
|
1. Tokenized reset flow (new endpoints + DB token store).
|
||||||
|
2. POST-only auth route enforcement.
|
||||||
|
3. Generic auth/reset responses.
|
||||||
|
4. Dedicated auth rate limiting.
|
||||||
|
5. Hash policy tuning + opportunistic rehash.
|
||||||
|
|
||||||
|
## Validation checklist
|
||||||
|
- Unit/integration tests:
|
||||||
|
- reset token creation, expiry, one-time use, invalid token paths.
|
||||||
|
- login generic error response behavior.
|
||||||
|
- auth rate limiter trigger and cooldown.
|
||||||
|
- query credential rejection.
|
||||||
|
- Manual:
|
||||||
|
- verify no plaintext password emails are sent.
|
||||||
|
- verify existing sessions are revoked after password reset.
|
||||||
+20
-3
@@ -14,6 +14,9 @@ const Profile = require("../def/profile.js");
|
|||||||
// When new users are subscribed, they have a single profile, which is the personal one.
|
// When new users are subscribed, they have a single profile, which is the personal one.
|
||||||
// Other profiles can be link to that user, like groups or courses.
|
// Other profiles can be link to that user, like groups or courses.
|
||||||
const signup = async function (req, res) {
|
const signup = async function (req, res) {
|
||||||
|
// SECURITY PLAN (point #2):
|
||||||
|
// Stop accepting credentials from query params and enforce POST body only.
|
||||||
|
// Query-string credentials can leak through logs/history/referers.
|
||||||
const username = req.query.username || req.body.username;
|
const username = req.query.username || req.body.username;
|
||||||
const password = req.query.password || req.body.password;
|
const password = req.query.password || req.body.password;
|
||||||
const email = req.query.email || req.body.email;
|
const email = req.query.email || req.body.email;
|
||||||
@@ -34,8 +37,9 @@ const signup = async function (req, res) {
|
|||||||
}
|
}
|
||||||
let isUserAlreadyRegistered = await DB.getUser(email);
|
let isUserAlreadyRegistered = await DB.getUser(email);
|
||||||
if (isUserAlreadyRegistered && isUserAlreadyRegistered._id) return res.json({ status: "This user is already registered" });
|
if (isUserAlreadyRegistered && isUserAlreadyRegistered._id) return res.json({ status: "This user is already registered" });
|
||||||
// Hash password to be stored on the DB.
|
// SECURITY PLAN (point #5):
|
||||||
// TODO: I think this is missing a Salt factor to improve security
|
// bcrypt.hash already includes a per-password salt.
|
||||||
|
// Future hardening: centralize cost factor policy (and consider rehash-on-login).
|
||||||
const hashedPassword = await bcrypt.hash(password, 10);
|
const hashedPassword = await bcrypt.hash(password, 10);
|
||||||
const newUserObject = await DB.newUser({
|
const newUserObject = await DB.newUser({
|
||||||
username: username.toLowerCase(),
|
username: username.toLowerCase(),
|
||||||
@@ -79,9 +83,13 @@ const login = async function (req, res) {
|
|||||||
const userInfo = await DB.checkSessionOnDB(session_id, user_sid);
|
const userInfo = await DB.checkSessionOnDB(session_id, user_sid);
|
||||||
if (userInfo) return res.redirect('/');
|
if (userInfo) return res.redirect('/');
|
||||||
}
|
}
|
||||||
|
// SECURITY PLAN (point #2):
|
||||||
|
// Migrate to req.body-only credentials after route-level POST-only enforcement.
|
||||||
const username = req.body.username || req.query.username;
|
const username = req.body.username || req.query.username;
|
||||||
const password = req.body.password || req.query.password || "";
|
const password = req.body.password || req.query.password || "";
|
||||||
const user = await DB.getUser(username);
|
const user = await DB.getUser(username);
|
||||||
|
// SECURITY PLAN (point #4):
|
||||||
|
// Replace user-existence specific response with generic invalid-credentials response.
|
||||||
if (!user) {
|
if (!user) {
|
||||||
client_logger.capture({
|
client_logger.capture({
|
||||||
distinctId: 'app_level',
|
distinctId: 'app_level',
|
||||||
@@ -92,8 +100,11 @@ const login = async function (req, res) {
|
|||||||
});
|
});
|
||||||
return res.json({ status: "user not founded" });
|
return res.json({ status: "user not founded" });
|
||||||
}
|
}
|
||||||
// TODO: Also add salt parameter here.
|
// SECURITY PLAN (point #5):
|
||||||
|
// bcrypt.compare validates salted hashes directly; no manual salt parameter is needed.
|
||||||
const isSamePassword = await bcrypt.compare(password, user.password);
|
const isSamePassword = await bcrypt.compare(password, user.password);
|
||||||
|
// SECURITY PLAN (point #4):
|
||||||
|
// Use the same generic auth error response for wrong password and unknown user.
|
||||||
if (!isSamePassword) return res.json({ status: "incorrect password" });
|
if (!isSamePassword) return res.json({ status: "incorrect password" });
|
||||||
try {
|
try {
|
||||||
// Store a new session loging on DB, and use ID as session ID
|
// Store a new session loging on DB, and use ID as session ID
|
||||||
@@ -178,6 +189,12 @@ const resetPassword = async function (req, res) {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// SECURITY PLAN (point #1):
|
||||||
|
// Replace this entire non-authenticated branch with reset-token flow:
|
||||||
|
// request-reset(email) -> email link -> confirm-reset(token, newPassword).
|
||||||
|
// Do not generate/send plaintext passwords by email.
|
||||||
|
// SECURITY PLAN (point #4):
|
||||||
|
// Return generic response regardless of account existence to avoid enumeration.
|
||||||
// Logic for non-logged in users.
|
// Logic for non-logged in users.
|
||||||
const username = req.body.username;
|
const username = req.body.username;
|
||||||
const user = await DB.getUser(username);
|
const user = await DB.getUser(username);
|
||||||
|
|||||||
@@ -37,6 +37,10 @@ const limiter = rateLimit({
|
|||||||
app.set('trust proxy', true);
|
app.set('trust proxy', true);
|
||||||
app.use(limiter);
|
app.use(limiter);
|
||||||
|
|
||||||
|
// SECURITY PLAN (point #3):
|
||||||
|
// Add dedicated auth limiter(s) for /login and password reset endpoints.
|
||||||
|
// Use tighter thresholds than the global limiter and key by account+IP.
|
||||||
|
|
||||||
// Authentication
|
// Authentication
|
||||||
const { signup, login, logout, resetPassword } = require('./auth/authEmail.js');
|
const { signup, login, logout, resetPassword } = require('./auth/authEmail.js');
|
||||||
/**
|
/**
|
||||||
@@ -71,6 +75,8 @@ const { signup, login, logout, resetPassword } = require('./auth/authEmail.js');
|
|||||||
* 400:
|
* 400:
|
||||||
* description: Bad request.
|
* description: Bad request.
|
||||||
*/
|
*/
|
||||||
|
// SECURITY PLAN (point #2):
|
||||||
|
// Make signup/login POST-only once clients are aligned.
|
||||||
app.route('/signup').get(signup).post(signup);
|
app.route('/signup').get(signup).post(signup);
|
||||||
/**
|
/**
|
||||||
* @swagger
|
* @swagger
|
||||||
@@ -153,6 +159,10 @@ app.get('/logout', logout);
|
|||||||
* description: Bad request.
|
* description: Bad request.
|
||||||
*/
|
*/
|
||||||
app.route('/resetPassword').post(resetPassword);
|
app.route('/resetPassword').post(resetPassword);
|
||||||
|
// SECURITY PLAN (point #1):
|
||||||
|
// Replace /resetPassword with request/confirm reset token endpoints:
|
||||||
|
// POST /password/request-reset
|
||||||
|
// POST /password/confirm-reset
|
||||||
|
|
||||||
// Routes
|
// Routes
|
||||||
const profileRoute = require('./routes/profile.js');
|
const profileRoute = require('./routes/profile.js');
|
||||||
|
|||||||
@@ -41,6 +41,10 @@ const getDB = new Promise((resolve, reject) => {
|
|||||||
DB.usersCol = db.db(DBName).collection("users");
|
DB.usersCol = db.db(DBName).collection("users");
|
||||||
DB.tokensCol = db.db(DBName).collection("tokens");
|
DB.tokensCol = db.db(DBName).collection("tokens");
|
||||||
DB.invitationCol = db.db(DBName).collection("invitation");
|
DB.invitationCol = db.db(DBName).collection("invitation");
|
||||||
|
// SECURITY PLAN (point #1):
|
||||||
|
// Add password reset token collection + TTL index, e.g.:
|
||||||
|
// DB.passwordResetTokensCol = db.db(DBName).collection("password_reset_tokens");
|
||||||
|
// DB.passwordResetTokensCol.createIndex({ expiresAt: 1 }, { expireAfterSeconds: 0 });
|
||||||
|
|
||||||
DB.checkSessionOnDB = async (session_id, user_sid)=>{
|
DB.checkSessionOnDB = async (session_id, user_sid)=>{
|
||||||
const temp_id = new mongo.ObjectID(session_id);
|
const temp_id = new mongo.ObjectID(session_id);
|
||||||
|
|||||||
Reference in New Issue
Block a user