Nachklang/API
2
0
Fork 0
Code Issues 2 Pull Requests Packages Projects Releases Wiki Activity

Harden the admin module after a fresh-context review

Browse Source 
Six defects found by an independent review of 7aac07a.

Environment handling now fails safe. NODE_ENV=production was gating the
signing key, the cookie domain, the CORS origin list and invitation-token
logging all at once, and it was documented nowhere - an unset value, which is
what a fresh Plesk vhost gives you, silently degraded all four. Only
'development' and 'test' relax anything now; everything else, unset included,
is strict. The hardcoded fallback secret is gone (dev gets a random
per-process one, so no committed value can ever sign a production cookie),
and invitation-link logging is an explicit ADMIN_LOG_INVITE_LINKS opt-in that
is refused in strict mode.

Rate limiting no longer collapses into a single global bucket. Without
trustedProxies, better-auth rejects a multi-value x-forwarded-for, resolves no
client IP, and keys every request to "no-trusted-ip" - where /sign-in/*
allows 3 requests per 10 seconds, so one noisy client could lock the whole
organisation out. CLIENT_IP_HEADERS and TRUSTED_PROXY_IPS make this explicit,
the unspecified x-forwarded-for fallback is gone, and strict mode warns at
boot when no trusted proxy is configured.

Invite acceptance is transactional. The user and its credential account go in
one runWithTransaction, as better-auth's own sign-up route does. A transaction
cannot span the permission and invitation writes - those use this module's own
pool - so a failure there is compensated: the user row is deleted and the
invitation un-marked, so the link works again instead of leaving the invitee
with a burnt token and an account no route can repair.

The last-admin guards were check-then-act. Two admins each removing the
other's admin permission could both pass the check and both commit, leaving
nobody able to administer anything. The count now runs inside the write
transaction under SELECT ... FOR UPDATE.

Also: lastSignInAt filtered expired sessions in the detail endpoint but not
the list, so the two disagreed; and the integration suite never reset
rateLimit, leaving it one added sign-in away from 429s that look like auth
bugs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Patrick Müller
2026-09-05 18:28:52 +02:00
parent 7aac07a013
commit dbcd5b56f6
15 changed files with 381 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
test/admin/users.admin.router.test.ts
+18 -18
View File
@@ -7,7 +7,9 @@ vi.mock('../../src/models/admin/users/users.admin.service.js', () => ({
getUserDetail: vi.fn(),
loadAccess: vi.fn(),
setPermissions: vi.fn(),
setPermissionsGuarded: vi.fn(),
disableUser: vi.fn(),
disableUserGuarded: vi.fn(),
enableUser: vi.fn(),
revokeSession: vi.fn(),
countActiveAdmins: vi.fn(),
@@ -40,6 +42,8 @@ beforeEach(() => {
}
service.getUserDetail.mockResolvedValue({id: 'other', apps: []});
service.userExists.mockResolvedValue(true);
service.setPermissionsGuarded.mockResolvedValue('ok');
service.disableUserGuarded.mockResolvedValue('ok');
});
describe('PUT /admin/users/:id/permissions', () => {
@@ -47,7 +51,7 @@ describe('PUT /admin/users/:id/permissions', () => {
const res = await request(makeApp()).put('/admin/users/other/permissions').send({apps: ['calendar', 'nope']});
expect(res.status).toBe(400);
expect(service.setPermissions).not.toHaveBeenCalled();
expect(service.setPermissionsGuarded).not.toHaveBeenCalled();
});
it('rejects a non-array body', async () => {
@@ -62,7 +66,7 @@ describe('PUT /admin/users/:id/permissions', () => {
const res = await request(makeApp()).put('/admin/users/ghost/permissions').send({apps: []});
expect(res.status).toBe(404);
expect(service.setPermissions).not.toHaveBeenCalled();
expect(service.setPermissionsGuarded).not.toHaveBeenCalled();
});
it('refuses to remove the caller\'s own admin permission', async () => {
@@ -72,30 +76,28 @@ describe('PUT /admin/users/:id/permissions', () => {
const res = await request(makeApp('me')).put('/admin/users/me/permissions').send({apps: ['feedback']});
expect(res.status).toBe(409);
expect(service.setPermissions).not.toHaveBeenCalled();
expect(service.setPermissionsGuarded).not.toHaveBeenCalled();
});
// Defence in depth: with the caller themselves being an active admin this
// count cannot actually reach 1 in production, but the guard is what makes
// that safe to rely on rather than to reason about.
it('refuses to remove the last remaining active admin', async () => {
// The last-admin decision is made inside the write transaction (so two
// admins acting at once cannot both pass a check-then-act); the router's
// job is only to turn that verdict into a 409.
it('answers 409 when the service reports the last admin would be removed', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']});
service.countActiveAdmins.mockResolvedValue(1);
service.setPermissionsGuarded.mockResolvedValue('last-admin');
const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: []});
expect(res.status).toBe(409);
expect(service.setPermissions).not.toHaveBeenCalled();
});
it('allows removing an admin while another active admin remains', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']});
service.countActiveAdmins.mockResolvedValue(2);
const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: ['tickets']});
expect(res.status).toBe(200);
expect(service.setPermissions).toHaveBeenCalledWith('other', ['tickets'], 'me');
expect(service.setPermissionsGuarded).toHaveBeenCalledWith('other', ['tickets'], 'me');
});
it('allows granting permissions to someone who has none', async () => {
@@ -104,8 +106,7 @@ describe('PUT /admin/users/:id/permissions', () => {
const res = await request(makeApp('me')).put('/admin/users/other/permissions').send({apps: ['feedback', 'tickets']});
expect(res.status).toBe(200);
// Nothing is being taken away, so the last-admin count is not consulted.
expect(service.countActiveAdmins).not.toHaveBeenCalled();
expect(service.setPermissionsGuarded).toHaveBeenCalledWith('other', ['feedback', 'tickets'], 'me');
});
});
@@ -114,17 +115,16 @@ describe('POST /admin/users/:id/disable', () => {
const res = await request(makeApp('me')).post('/admin/users/me/disable');
expect(res.status).toBe(409);
expect(service.disableUser).not.toHaveBeenCalled();
expect(service.disableUserGuarded).not.toHaveBeenCalled();
});
it('refuses to disable the last active admin', async () => {
it('answers 409 when the service reports the last active admin would be disabled', async () => {
service.loadAccess.mockResolvedValue({id: 'other', disabled: false, apps: ['admin']});
service.countActiveAdmins.mockResolvedValue(1);
service.disableUserGuarded.mockResolvedValue('last-admin');
const res = await request(makeApp('me')).post('/admin/users/other/disable');
expect(res.status).toBe(409);
expect(service.disableUser).not.toHaveBeenCalled();
});
it('disables a non-admin user', async () => {
@@ -133,7 +133,7 @@ describe('POST /admin/users/:id/disable', () => {
const res = await request(makeApp('me')).post('/admin/users/other/disable');
expect(res.status).toBe(200);
expect(service.disableUser).toHaveBeenCalledWith('other');
expect(service.disableUserGuarded).toHaveBeenCalledWith('other');
});
it('404s for an unknown user', async () => {