chore(auth): remove security plan doc and marker comments
This commit is contained in:
@@ -1,77 +0,0 @@
|
|||||||
# 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 single-use token login
|
|
||||||
- Problem:
|
|
||||||
- Current flow resets by username and emails a plaintext temporary password.
|
|
||||||
- Implementation:
|
|
||||||
- Keep `POST /resetPassword` as token request endpoint:
|
|
||||||
- Accept identifier (email/username).
|
|
||||||
- Always return generic success response.
|
|
||||||
- If account exists, create one-time login token with short TTL (15-30 min), store hashed token, email link.
|
|
||||||
- Add `POST /password/token-login`:
|
|
||||||
- Accept token.
|
|
||||||
- Validate token (exists, not expired, unused), mark used atomically, then create normal auth session cookies.
|
|
||||||
- Data model:
|
|
||||||
- New collection `password_login_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 login flow (new endpoint + 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:
|
|
||||||
- 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 token cannot be reused after first successful consumption.
|
|
||||||
@@ -47,7 +47,6 @@ const createSessionFromUser = async ({ DB, user, req, res }) => {
|
|||||||
// 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 FIX (#2): only accept credentials from request body.
|
|
||||||
const username = (req.body.username || "").trim().toLowerCase();
|
const username = (req.body.username || "").trim().toLowerCase();
|
||||||
const password = req.body.password;
|
const password = req.body.password;
|
||||||
const email = (req.body.email || "").trim().toLowerCase();
|
const email = (req.body.email || "").trim().toLowerCase();
|
||||||
@@ -68,9 +67,6 @@ 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" });
|
||||||
// SECURITY PLAN (point #5):
|
|
||||||
// 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,
|
||||||
@@ -115,7 +111,6 @@ const login = async function (req, res) {
|
|||||||
if (userInfo) return res.redirect('/');
|
if (userInfo) return res.redirect('/');
|
||||||
}
|
}
|
||||||
const invalidCredentials = () => res.status(401).json({ status: "Invalid credentials" });
|
const invalidCredentials = () => res.status(401).json({ status: "Invalid credentials" });
|
||||||
// SECURITY FIX (#2): only accept credentials from request body.
|
|
||||||
const username = (req.body.username || req.body.email || "").trim().toLowerCase();
|
const username = (req.body.username || req.body.email || "").trim().toLowerCase();
|
||||||
const password = req.body.password || "";
|
const password = req.body.password || "";
|
||||||
if (!username || !password) return invalidCredentials();
|
if (!username || !password) return invalidCredentials();
|
||||||
@@ -128,11 +123,7 @@ const login = async function (req, res) {
|
|||||||
properties: { username },
|
properties: { username },
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
// SECURITY PLAN (point #5):
|
|
||||||
// bcrypt.compare validates salted hashes directly; no manual salt parameter is needed.
|
|
||||||
// SECURITY FIX (#4): compare against dummy hash when user doesn't exist to reduce timing side-channel.
|
|
||||||
const isSamePassword = await bcrypt.compare(password, user?.password || DUMMY_BCRYPT_HASH);
|
const isSamePassword = await bcrypt.compare(password, user?.password || DUMMY_BCRYPT_HASH);
|
||||||
// SECURITY FIX (#4): same response for non-existing user and wrong password.
|
|
||||||
if (!user || !isSamePassword) return invalidCredentials();
|
if (!user || !isSamePassword) return invalidCredentials();
|
||||||
try {
|
try {
|
||||||
return res.json(await createSessionFromUser({ DB, user, req, res }));
|
return res.json(await createSessionFromUser({ DB, user, req, res }));
|
||||||
@@ -170,7 +161,6 @@ const logout = async function (req, res) {
|
|||||||
const resetPassword = async function (req, res) {
|
const resetPassword = async function (req, res) {
|
||||||
const DB = await MongoDB.getDB;
|
const DB = await MongoDB.getDB;
|
||||||
|
|
||||||
// SECURITY FIX (#1): issue a single-use token instead of sending/changing passwords.
|
|
||||||
const genericResetResponse = {
|
const genericResetResponse = {
|
||||||
status: "ok",
|
status: "ok",
|
||||||
details: "If the account exists, check your email for next steps"
|
details: "If the account exists, check your email for next steps"
|
||||||
|
|||||||
4
index.js
4
index.js
@@ -72,7 +72,6 @@ const { authRateLimiter } = require('./middleware/authRateLimiter');
|
|||||||
* 400:
|
* 400:
|
||||||
* description: Bad request.
|
* description: Bad request.
|
||||||
*/
|
*/
|
||||||
// SECURITY FIX (#2): POST-only signup to avoid query-string credential leakage.
|
|
||||||
app.post('/signup', signup);
|
app.post('/signup', signup);
|
||||||
/**
|
/**
|
||||||
* @swagger
|
* @swagger
|
||||||
@@ -106,7 +105,6 @@ app.post('/signup', signup);
|
|||||||
* 401:
|
* 401:
|
||||||
* description: Invalid credentials.
|
* description: Invalid credentials.
|
||||||
*/
|
*/
|
||||||
// SECURITY FIX (#2): POST-only login to avoid query-string credential leakage.
|
|
||||||
app.post('/login', authRateLimiter('login'), login);
|
app.post('/login', authRateLimiter('login'), login);
|
||||||
/**
|
/**
|
||||||
* @swagger
|
* @swagger
|
||||||
@@ -156,8 +154,6 @@ app.get('/logout', logout);
|
|||||||
* description: Bad request.
|
* description: Bad request.
|
||||||
*/
|
*/
|
||||||
app.route('/resetPassword').post(authRateLimiter('reset'), resetPassword);
|
app.route('/resetPassword').post(authRateLimiter('reset'), resetPassword);
|
||||||
// SECURITY FIX (#1):
|
|
||||||
// Single-use token login endpoint for password recovery flow.
|
|
||||||
/**
|
/**
|
||||||
* @swagger
|
* @swagger
|
||||||
* /password/token-login:
|
* /password/token-login:
|
||||||
|
|||||||
Reference in New Issue
Block a user