← back to Commercialrealestate
CRCP auth hardening from security review (TK-10301)
c95386d1b0ca33b28caac04aa2360a1b08735a6c · 2026-08-06 12:05:43 -0700 · Steve Abrams
- HIGH-2 fix: admin :key routes now use the LITERAL stored key (rawKey), not
uname() — so email/magic-link-keyed accounts are manageable and the
self-delete guard compares real keys (was comparing mangled strings).
- MEDIUM-1 fix: serialize accounts read-modify-write via withAcct() queue so
the last-admin / dup-username guards are atomic (no TOCTOU lock-out race).
- HIGH-1 backstop: in NODE_ENV=production, if CRCP_ADMIN_PASS is unset, generate
a random admin password + log it once instead of shipping the house DW2024!.
- MEDIUM-2: same-origin Origin/Referer CSRF check (denyCsrf) on all admin
mutations, defense-in-depth over SameSite=Lax.
Re-verified: email-keyed promote/demote/pw-reset work; cross-site Origin -> 403;
last-admin guard still fires; per-user notes intact. Restored real accounts file
(removed test pollution); local :9911 on fixed code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Files touched
M scripts/crcp-accounts.js
Diff
commit c95386d1b0ca33b28caac04aa2360a1b08735a6c
Author: Steve Abrams <steve@designerwallcoverings.com>
Date: Thu Aug 6 12:05:43 2026 -0700
CRCP auth hardening from security review (TK-10301)
- HIGH-2 fix: admin :key routes now use the LITERAL stored key (rawKey), not
uname() — so email/magic-link-keyed accounts are manageable and the
self-delete guard compares real keys (was comparing mangled strings).
- MEDIUM-1 fix: serialize accounts read-modify-write via withAcct() queue so
the last-admin / dup-username guards are atomic (no TOCTOU lock-out race).
- HIGH-1 backstop: in NODE_ENV=production, if CRCP_ADMIN_PASS is unset, generate
a random admin password + log it once instead of shipping the house DW2024!.
- MEDIUM-2: same-origin Origin/Referer CSRF check (denyCsrf) on all admin
mutations, defense-in-depth over SameSite=Lax.
Re-verified: email-keyed promote/demote/pw-reset work; cross-site Origin -> 403;
last-admin guard still fires; per-user notes intact. Restored real accounts file
(removed test pollution); local :9911 on fixed code.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---
scripts/crcp-accounts.js | 128 ++++++++++++++++++++++++++++++++---------------
1 file changed, 88 insertions(+), 40 deletions(-)
diff --git a/scripts/crcp-accounts.js b/scripts/crcp-accounts.js
index 8bd4ae1..843d43b 100644
--- a/scripts/crcp-accounts.js
+++ b/scripts/crcp-accounts.js
@@ -186,11 +186,25 @@ module.exports = function mountAccounts(app, ROOT) {
const hasAdmin = Object.values(db.users).some(u => u && u.perm === 'admin');
if (!hasAdmin) {
const akey = uname(process.env.CRCP_ADMIN_USER || 'admin');
+ // Password policy (security review HIGH-1): honor CRCP_ADMIN_PASS if set. If it is NOT set,
+ // NEVER ship the shared house default to a customer-facing prod deploy — generate a random
+ // one and print it once so a forgotten env can't silently hand out `DW2024!` (which is also
+ // the site basic-auth default, so it wouldn't be an independent second factor). Local/dev
+ // keeps the convenient default.
+ let adminPass = process.env.CRCP_ADMIN_PASS;
+ if (!adminPass) {
+ if (process.env.NODE_ENV === 'production') {
+ adminPass = crypto.randomBytes(12).toString('base64url');
+ console.log(`[crcp-accounts] PROD: CRCP_ADMIN_PASS unset — generated a random admin password for "${akey}": ${adminPass} (set CRCP_ADMIN_PASS in the env to control it)`);
+ } else {
+ adminPass = 'DW2024!';
+ }
+ }
if (!db.users[akey]) {
const salt = rid();
db.users[akey] = {
tier: 'pro', perm: 'admin', username: akey, name: 'Admin', role: 'other', industry: ROLES.other,
- salt, passhash: hashPw(process.env.CRCP_ADMIN_PASS || 'DW2024!', salt),
+ salt, passhash: hashPw(adminPass, salt),
created: new Date().toISOString(), seeded: true,
};
dirty = true;
@@ -277,6 +291,38 @@ module.exports = function mountAccounts(app, ROOT) {
has_password: !!u.passhash, magic_only: !u.passhash,
});
const countAdmins = db => Object.values(db.users).filter(u => u && u.perm === 'admin').length;
+ // The :key route param IS the stored account key verbatim (a username for password accounts, a
+ // real email — with '@' — for magic-link accounts). It must NOT be run through uname() (which
+ // strips '@'), or email-keyed accounts become unmanageable and the self-delete guard compares
+ // two mangled strings (security review HIGH-2). Only bound length + strip control chars.
+ const rawKey = s => String(s == null ? '' : s).replace(/[\x00-\x1f\x7f]/g, '').trim().slice(0, 120);
+
+ // Serialized read-modify-write for the accounts store, mirroring crcp-notes' _q queue. The admin
+ // mutations do check-then-write on shared state (e.g. "≤1 admin left?" then delete); without a
+ // queue two concurrent deletes could both pass the last-admin guard and zero out all admins
+ // (security review MEDIUM-1). A mutator may `throw { status, error }` to abort WITHOUT saving.
+ let _aq = Promise.resolve();
+ function withAcct(mutator) {
+ const run = _aq.then(() => {
+ const db = load(); ['users', 'tokens', 'sessions', 'saved', 'watch'].forEach(k => db[k] = db[k] || blank()[k]);
+ const out = mutator(db); // throws to abort (no save); returns { status, body } on success
+ save(db);
+ return out;
+ });
+ _aq = run.then(() => {}, () => {}); // swallow so a rejected mutation never wedges the queue
+ return run;
+ }
+ const sendAcct = (res) => (r) => res.status(r.status || 200).json(r.body);
+ const acctErr = (res) => (e) => (e && e.status) ? res.status(e.status).json({ error: e.error })
+ : res.status(500).json({ error: String(e && e.message || e) });
+ // Same-origin CSRF guard for state-changing routes (defense-in-depth beyond SameSite=Lax, since
+ // sibling *.agentabrams.com pages are same-site for the cookie). A same-origin fetch always sends
+ // a matching Origin on POST/DELETE; we only reject a PRESENT, mismatched Origin (security MEDIUM-2).
+ function csrfOk(req) {
+ const o = req.headers.origin; if (!o) return true;
+ try { return new URL(o).host === req.headers.host; } catch { return false; }
+ }
+ const denyCsrf = (req, res) => { if (csrfOk(req)) return false; res.status(403).json({ error: 'bad origin' }); return true; };
// List every account (admin only).
app.get('/api/admin/users', (req, res) => {
@@ -289,64 +335,66 @@ module.exports = function mountAccounts(app, ROOT) {
// Create a new account (admin only). Defaults to a standard user; perm can be set to admin.
app.post('/api/admin/users', (req, res) => {
- if (!requireAdmin(req, res)) return;
+ const me = requireAdmin(req, res); if (!me) return; if (denyCsrf(req, res)) return;
const b = req.body || {};
- const key = uname(b.username);
+ const key = uname(b.username); // a NEW password account is keyed by its sanitized username
if (!key) return res.status(400).json({ error: 'username required (letters, digits, . _ -)' });
if (!b.password || String(b.password).length < 6) return res.status(400).json({ error: 'password required (min 6 chars)' });
- const db = load(); ['users', 'tokens', 'sessions', 'saved', 'watch'].forEach(k => db[k] = db[k] || blank()[k]);
- if (db.users[key]) return res.status(409).json({ error: 'username already exists' });
const perm = b.perm === 'admin' ? 'admin' : 'standard';
const role = ROLES[b.role] ? b.role : 'loan_officer';
- const salt = rid();
- db.users[key] = {
- tier: b.tier === 'pro' ? 'pro' : (perm === 'admin' ? 'pro' : 'free'),
- perm, username: key, name: b.name ? clean(b.name) : key, role, industry: ROLES[role],
- salt, passhash: hashPw(String(b.password), salt),
- created: new Date().toISOString(), created_by: userOf(req).email,
- };
- save(db);
- res.json({ ok: true, user: pubUser(key, db.users[key]) });
+ withAcct(db => {
+ if (db.users[key]) throw { status: 409, error: 'username already exists' }; // atomic dup-check
+ const salt = rid();
+ db.users[key] = {
+ tier: b.tier === 'pro' ? 'pro' : (perm === 'admin' ? 'pro' : 'free'),
+ perm, username: key, name: b.name ? clean(b.name) : key, role, industry: ROLES[role],
+ salt, passhash: hashPw(String(b.password), salt),
+ created: new Date().toISOString(), created_by: me.email,
+ };
+ return { status: 200, body: { ok: true, user: pubUser(key, db.users[key]) } };
+ }).then(sendAcct(res)).catch(acctErr(res));
});
// Reset a user's password (admin only).
app.post('/api/admin/users/:key/password', (req, res) => {
- if (!requireAdmin(req, res)) return;
- const key = uname(req.params.key);
+ if (!requireAdmin(req, res)) return; if (denyCsrf(req, res)) return;
+ const key = rawKey(req.params.key);
const pw = String((req.body || {}).password || '');
if (pw.length < 6) return res.status(400).json({ error: 'password required (min 6 chars)' });
- const db = load(); const u = db.users[key]; if (!u) return res.status(404).json({ error: 'no such user' });
- const salt = rid(); u.salt = salt; u.passhash = hashPw(pw, salt); u.pw_reset_at = new Date().toISOString();
- save(db);
- res.json({ ok: true, user: pubUser(key, u) });
+ withAcct(db => {
+ const u = db.users[key]; if (!u) throw { status: 404, error: 'no such user' };
+ const salt = rid(); u.salt = salt; u.passhash = hashPw(pw, salt); u.pw_reset_at = new Date().toISOString();
+ return { status: 200, body: { ok: true, user: pubUser(key, u) } };
+ }).then(sendAcct(res)).catch(acctErr(res));
});
// Change a user's permission level (admin only). Guarded so the LAST admin can never be demoted
- // (which would lock everyone out of the console).
+ // (which would lock everyone out of the console) — checked atomically inside withAcct.
app.post('/api/admin/users/:key/perm', (req, res) => {
- const me = requireAdmin(req, res); if (!me) return;
- const key = uname(req.params.key);
+ const me = requireAdmin(req, res); if (!me) return; if (denyCsrf(req, res)) return;
+ const key = rawKey(req.params.key);
const perm = (req.body || {}).perm === 'admin' ? 'admin' : 'standard';
- const db = load(); const u = db.users[key]; if (!u) return res.status(404).json({ error: 'no such user' });
- if (u.perm === 'admin' && perm === 'standard' && countAdmins(db) <= 1) {
- return res.status(409).json({ error: 'cannot demote the last admin' });
- }
- u.perm = perm; save(db);
- res.json({ ok: true, user: pubUser(key, u) });
+ withAcct(db => {
+ const u = db.users[key]; if (!u) throw { status: 404, error: 'no such user' };
+ if (u.perm === 'admin' && perm === 'standard' && countAdmins(db) <= 1) throw { status: 409, error: 'cannot demote the last admin' };
+ u.perm = perm;
+ return { status: 200, body: { ok: true, user: pubUser(key, u) } };
+ }).then(sendAcct(res)).catch(acctErr(res));
});
- // Delete an account (admin only). Can't delete yourself or the last admin.
+ // Delete an account (admin only). Can't delete yourself or the last admin — checked atomically.
app.delete('/api/admin/users/:key', (req, res) => {
- const me = requireAdmin(req, res); if (!me) return;
- const key = uname(req.params.key);
- const db = load(); const u = db.users[key]; if (!u) return res.status(404).json({ error: 'no such user' });
- if (key === uname(me.email)) return res.status(409).json({ error: 'cannot delete your own account' });
- if (u.perm === 'admin' && countAdmins(db) <= 1) return res.status(409).json({ error: 'cannot delete the last admin' });
- delete db.users[key]; delete db.saved[key]; delete db.watch[key];
- // Revoke any live sessions for the deleted user so a signed-in tab loses access immediately.
- for (const [sid, s] of Object.entries(db.sessions)) if (s && s.email === key) delete db.sessions[sid];
- save(db);
- res.json({ ok: true, deleted: key });
+ const me = requireAdmin(req, res); if (!me) return; if (denyCsrf(req, res)) return;
+ const key = rawKey(req.params.key);
+ withAcct(db => {
+ const u = db.users[key]; if (!u) throw { status: 404, error: 'no such user' };
+ if (key === me.email) throw { status: 409, error: 'cannot delete your own account' }; // literal-key compare
+ if (u.perm === 'admin' && countAdmins(db) <= 1) throw { status: 409, error: 'cannot delete the last admin' };
+ delete db.users[key]; delete db.saved[key]; delete db.watch[key];
+ // Revoke any live sessions for the deleted user so a signed-in tab loses access immediately.
+ for (const [sid, s] of Object.entries(db.sessions)) if (s && s.email === key) delete db.sessions[sid];
+ return { status: 200, body: { ok: true, deleted: key } };
+ }).then(sendAcct(res)).catch(acctErr(res));
});
module.exports.userOf = userOf; // expose for the alert job
← 1934c92 CRCP full login: admin+standard roles, per-user private note
·
back to Commercialrealestate
·
auto-data-snapshot: 2026-08-06T12:25:51 (3 data files) — dat 3e01427 →