codex/password-security-plan-comments #2
@@ -9,18 +9,17 @@ const Notifications = require("../notifications");
|
|||||||
// Object Definitions
|
// Object Definitions
|
||||||
const Post = require("../def/post.js")
|
const Post = require("../def/post.js")
|
||||||
const Profile = require("../def/profile.js");
|
const Profile = require("../def/profile.js");
|
||||||
|
const DUMMY_BCRYPT_HASH = '$2b$10$2zQfAaxK0cN13N7V2Q5hAOL3wxY5E9OQj1YxDCEV4VpWw2X2gYd6C';
|
||||||
|
|
||||||
// Function to Singup new users. An user is a combination of a user obj and a profile.
|
// Function to Singup new users. An user is a combination of a user obj and a profile.
|
||||||
// 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):
|
// SECURITY FIX (#2): only accept credentials from request body.
|
||||||
// Stop accepting credentials from query params and enforce POST body only.
|
const username = (req.body.username || "").trim().toLowerCase();
|
||||||
// Query-string credentials can leak through logs/history/referers.
|
const password = req.body.password;
|
||||||
const username = req.query.username || req.body.username;
|
const email = (req.body.email || "").trim().toLowerCase();
|
||||||
const password = req.query.password || req.body.password;
|
const profile = req.body.profile;
|
||||||
const email = req.query.email || req.body.email;
|
|
||||||
const profile = req.query.profile || req.body.profile;
|
|
||||||
if (!username || !password || !email) return res.json({ status: "Incomplete information!" });
|
if (!username || !password || !email) return res.json({ status: "Incomplete information!" });
|
||||||
// Check if the new user has an invitation.
|
// Check if the new user has an invitation.
|
||||||
const DB = await MongoDB.getDB;
|
const DB = await MongoDB.getDB;
|
||||||
@@ -42,8 +41,8 @@ const signup = async function (req, res) {
|
|||||||
// Future hardening: centralize cost factor policy (and consider rehash-on-login).
|
// 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,
|
||||||
email: email.toLowerCase(),
|
email,
|
||||||
password: hashedPassword
|
password: hashedPassword
|
||||||
});
|
});
|
||||||
// If newUserObject it's an error message, we check by looking toLowerCase function
|
// If newUserObject it's an error message, we check by looking toLowerCase function
|
||||||
@@ -83,29 +82,26 @@ 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):
|
const invalidCredentials = () => res.status(401).json({ status: "Invalid credentials" });
|
||||||
// Migrate to req.body-only credentials after route-level POST-only enforcement.
|
// SECURITY FIX (#2): only accept credentials from request body.
|
||||||
const username = req.body.username || req.query.username;
|
const username = (req.body.username || req.body.email || "").trim().toLowerCase();
|
||||||
const password = req.body.password || req.query.password || "";
|
const password = req.body.password || "";
|
||||||
|
if (!username || !password) return invalidCredentials();
|
||||||
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',
|
||||||
event: 'server@' + req.method + '@' + req.originalUrl + '@userNotFound',
|
event: 'server@' + req.method + '@' + req.originalUrl + '@invalidCredentials',
|
||||||
properties: {
|
properties: { username },
|
||||||
username: username,
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
return res.json({ status: "user not founded" });
|
|
||||||
}
|
}
|
||||||
// SECURITY PLAN (point #5):
|
// SECURITY PLAN (point #5):
|
||||||
// bcrypt.compare validates salted hashes directly; no manual salt parameter is needed.
|
// bcrypt.compare validates salted hashes directly; no manual salt parameter is needed.
|
||||||
const isSamePassword = await bcrypt.compare(password, user.password);
|
// SECURITY FIX (#4): compare against dummy hash when user doesn't exist to reduce timing side-channel.
|
||||||
// SECURITY PLAN (point #4):
|
const isSamePassword = await bcrypt.compare(password, user?.password || DUMMY_BCRYPT_HASH);
|
||||||
// Use the same generic auth error response for wrong password and unknown user.
|
// SECURITY FIX (#4): same response for non-existing user and wrong password.
|
||||||
if (!isSamePassword) return res.json({ status: "incorrect password" });
|
if (!user || !isSamePassword) return invalidCredentials();
|
||||||
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
|
||||||
const sessionObj = await DB.newSession(user._id);
|
const sessionObj = await DB.newSession(user._id);
|
||||||
@@ -193,12 +189,23 @@ const resetPassword = async function (req, res) {
|
|||||||
// Replace this entire non-authenticated branch with reset-token flow:
|
// Replace this entire non-authenticated branch with reset-token flow:
|
||||||
// request-reset(email) -> email link -> confirm-reset(token, newPassword).
|
// request-reset(email) -> email link -> confirm-reset(token, newPassword).
|
||||||
// Do not generate/send plaintext passwords by email.
|
// Do not generate/send plaintext passwords by email.
|
||||||
// SECURITY PLAN (point #4):
|
// SECURITY FIX (#4): return generic response regardless of account existence.
|
||||||
// Return generic response regardless of account existence to avoid enumeration.
|
const genericResetResponse = {
|
||||||
|
status: "ok",
|
||||||
|
details: "If the account exists, check your email for next steps"
|
||||||
|
};
|
||||||
// Logic for non-logged in users.
|
// Logic for non-logged in users.
|
||||||
const username = req.body.username;
|
const username = (req.body.username || req.body.email || "").trim().toLowerCase();
|
||||||
|
if (!username) return res.json(genericResetResponse);
|
||||||
const user = await DB.getUser(username);
|
const user = await DB.getUser(username);
|
||||||
if (!user) return res.json({ status: "user not founded" });
|
if (!user) {
|
||||||
|
client_logger.capture({
|
||||||
|
distinctId: 'app_level',
|
||||||
|
event: 'server@' + req.method + '@' + req.originalUrl + '@resetRequestedUnknownUser',
|
||||||
|
properties: { username }
|
||||||
|
});
|
||||||
|
return res.json(genericResetResponse);
|
||||||
|
}
|
||||||
const password = generatePassword();
|
const password = generatePassword();
|
||||||
const hashedPassword = await bcrypt.hash(password, 10);
|
const hashedPassword = await bcrypt.hash(password, 10);
|
||||||
// TODO: Add salt to password here as well.
|
// TODO: Add salt to password here as well.
|
||||||
@@ -220,10 +227,7 @@ const resetPassword = async function (req, res) {
|
|||||||
username: username,
|
username: username,
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
return res.json({
|
return res.json(genericResetResponse);
|
||||||
status: "ok",
|
|
||||||
details: 'Check your email for new password' // Enum of details?
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
8
index.js
8
index.js
@@ -75,9 +75,8 @@ const { signup, login, logout, resetPassword } = require('./auth/authEmail.js');
|
|||||||
* 400:
|
* 400:
|
||||||
* description: Bad request.
|
* description: Bad request.
|
||||||
*/
|
*/
|
||||||
// SECURITY PLAN (point #2):
|
// SECURITY FIX (#2): POST-only signup to avoid query-string credential leakage.
|
||||||
// Make signup/login POST-only once clients are aligned.
|
app.post('/signup', signup);
|
||||||
app.route('/signup').get(signup).post(signup);
|
|
||||||
/**
|
/**
|
||||||
* @swagger
|
* @swagger
|
||||||
* /login:
|
* /login:
|
||||||
@@ -110,7 +109,8 @@ app.route('/signup').get(signup).post(signup);
|
|||||||
* 401:
|
* 401:
|
||||||
* description: Invalid credentials.
|
* description: Invalid credentials.
|
||||||
*/
|
*/
|
||||||
app.route('/login').get(login).post(login);
|
// SECURITY FIX (#2): POST-only login to avoid query-string credential leakage.
|
||||||
|
app.post('/login', login);
|
||||||
/**
|
/**
|
||||||
* @swagger
|
* @swagger
|
||||||
* /logout:
|
* /logout:
|
||||||
|
|||||||
Reference in New Issue
Block a user