mirror of
https://github.com/actualbudget/actual.git
synced 2026-03-21 15:36:50 -05:00
[AI] Fix privilege escalation in sync-server /change-password and getLoginMethod (#7155)
* [AI] Fix privilege escalation in sync-server /change-password and getLoginMethod Made-with: Cursor * Update upcoming-release-notes/7155.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Fix privilege escalation issue in change-password endpoint --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This commit is contained in:
committed by
GitHub
parent
60e2665fcc
commit
85e08b2e9e
@@ -59,7 +59,12 @@ export function getLoginMethod(req) {
|
|||||||
(req.body || { loginMethod: null }).loginMethod &&
|
(req.body || { loginMethod: null }).loginMethod &&
|
||||||
config.get('allowedLoginMethods').includes(req.body.loginMethod)
|
config.get('allowedLoginMethods').includes(req.body.loginMethod)
|
||||||
) {
|
) {
|
||||||
return req.body.loginMethod;
|
const accountDb = getAccountDb();
|
||||||
|
const activeRow = accountDb.first(
|
||||||
|
'SELECT method FROM auth WHERE method = ? AND active = 1',
|
||||||
|
[req.body.loginMethod],
|
||||||
|
);
|
||||||
|
if (activeRow) return req.body.loginMethod;
|
||||||
}
|
}
|
||||||
|
|
||||||
//BY-PASS ANY OTHER CONFIGURATION TO ENSURE HEADER AUTH
|
//BY-PASS ANY OTHER CONFIGURATION TO ENSURE HEADER AUTH
|
||||||
|
|||||||
@@ -121,6 +121,15 @@ app.post('/change-password', (req, res) => {
|
|||||||
const session = validateSession(req, res);
|
const session = validateSession(req, res);
|
||||||
if (!session) return;
|
if (!session) return;
|
||||||
|
|
||||||
|
if (getActiveLoginMethod() !== 'password') {
|
||||||
|
res.status(403).send({
|
||||||
|
status: 'error',
|
||||||
|
reason: 'forbidden',
|
||||||
|
details: 'password-auth-not-active',
|
||||||
|
});
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
const { error } = changePassword(req.body.password);
|
const { error } = changePassword(req.body.password);
|
||||||
|
|
||||||
if (error) {
|
if (error) {
|
||||||
|
|||||||
@@ -1,7 +1,8 @@
|
|||||||
import request from 'supertest';
|
import request from 'supertest';
|
||||||
import { v4 as uuidv4 } from 'uuid';
|
import { v4 as uuidv4 } from 'uuid';
|
||||||
|
|
||||||
import { getAccountDb, getServerPrefs } from './account-db';
|
import { getAccountDb, getLoginMethod, getServerPrefs } from './account-db';
|
||||||
|
import { bootstrapPassword } from './accounts/password';
|
||||||
import { handlers as app } from './app-account';
|
import { handlers as app } from './app-account';
|
||||||
|
|
||||||
const ADMIN_ROLE = 'ADMIN';
|
const ADMIN_ROLE = 'ADMIN';
|
||||||
@@ -33,6 +34,119 @@ const clearServerPrefs = () => {
|
|||||||
getAccountDb().mutate('DELETE FROM server_prefs');
|
getAccountDb().mutate('DELETE FROM server_prefs');
|
||||||
};
|
};
|
||||||
|
|
||||||
|
const insertAuthRow = (method, active, extraData = null) => {
|
||||||
|
getAccountDb().mutate(
|
||||||
|
'INSERT INTO auth (method, display_name, extra_data, active) VALUES (?, ?, ?, ?)',
|
||||||
|
[method, method, extraData, active],
|
||||||
|
);
|
||||||
|
};
|
||||||
|
|
||||||
|
const clearAuth = () => {
|
||||||
|
getAccountDb().mutate('DELETE FROM auth');
|
||||||
|
};
|
||||||
|
|
||||||
|
describe('/change-password', () => {
|
||||||
|
let userId, sessionToken;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
userId = uuidv4();
|
||||||
|
sessionToken = generateSessionToken();
|
||||||
|
createUser(userId, 'testuser', ADMIN_ROLE);
|
||||||
|
createSession(userId, sessionToken);
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
deleteUser(userId);
|
||||||
|
clearAuth();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should return 401 if no session token is provided', async () => {
|
||||||
|
const res = await request(app).post('/change-password').send({
|
||||||
|
password: 'newpassword',
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(res.statusCode).toEqual(401);
|
||||||
|
expect(res.body).toHaveProperty('status', 'error');
|
||||||
|
expect(res.body).toHaveProperty('reason', 'unauthorized');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should return 403 when active auth method is openid', async () => {
|
||||||
|
insertAuthRow('openid', 1);
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/change-password')
|
||||||
|
.set('x-actual-token', sessionToken)
|
||||||
|
.send({ password: 'newpassword' });
|
||||||
|
|
||||||
|
expect(res.statusCode).toEqual(403);
|
||||||
|
expect(res.body).toEqual({
|
||||||
|
status: 'error',
|
||||||
|
reason: 'forbidden',
|
||||||
|
details: 'password-auth-not-active',
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should return 400 when active method is password but password is empty', async () => {
|
||||||
|
bootstrapPassword('oldpassword');
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/change-password')
|
||||||
|
.set('x-actual-token', sessionToken)
|
||||||
|
.send({ password: '' });
|
||||||
|
|
||||||
|
expect(res.statusCode).toEqual(400);
|
||||||
|
expect(res.body).toEqual({ status: 'error', reason: 'invalid-password' });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should return 200 when active method is password and new password is valid', async () => {
|
||||||
|
bootstrapPassword('oldpassword');
|
||||||
|
|
||||||
|
const res = await request(app)
|
||||||
|
.post('/change-password')
|
||||||
|
.set('x-actual-token', sessionToken)
|
||||||
|
.send({ password: 'newpassword' });
|
||||||
|
|
||||||
|
expect(res.statusCode).toEqual(200);
|
||||||
|
expect(res.body).toEqual({ status: 'ok', data: {} });
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('getLoginMethod()', () => {
|
||||||
|
afterEach(() => {
|
||||||
|
clearAuth();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns the active DB method when no req is provided', () => {
|
||||||
|
insertAuthRow('password', 1);
|
||||||
|
expect(getLoginMethod(undefined)).toBe('password');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('honors a client-requested method when it is active in DB', () => {
|
||||||
|
insertAuthRow('openid', 1);
|
||||||
|
const req = { body: { loginMethod: 'openid' } };
|
||||||
|
expect(getLoginMethod(req)).toBe('openid');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('ignores a client-requested method that is inactive in DB', () => {
|
||||||
|
insertAuthRow('openid', 1);
|
||||||
|
insertAuthRow('password', 0);
|
||||||
|
const req = { body: { loginMethod: 'password' } };
|
||||||
|
expect(getLoginMethod(req)).toBe('openid');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('ignores a client-requested method that is not in DB', () => {
|
||||||
|
insertAuthRow('openid', 1);
|
||||||
|
const req = { body: { loginMethod: 'password' } };
|
||||||
|
expect(getLoginMethod(req)).toBe('openid');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('falls back to config default when auth table is empty and no req', () => {
|
||||||
|
// auth table is empty — getActiveLoginMethod() returns undefined
|
||||||
|
// config default for loginMethod is 'password'
|
||||||
|
expect(getLoginMethod(undefined)).toBe('password');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('/server-prefs', () => {
|
describe('/server-prefs', () => {
|
||||||
describe('POST /server-prefs', () => {
|
describe('POST /server-prefs', () => {
|
||||||
let adminUserId, basicUserId, adminSessionToken, basicSessionToken;
|
let adminUserId, basicUserId, adminSessionToken, basicSessionToken;
|
||||||
|
|||||||
6
upcoming-release-notes/7155.md
Normal file
6
upcoming-release-notes/7155.md
Normal file
@@ -0,0 +1,6 @@
|
|||||||
|
---
|
||||||
|
category: Bugfixes
|
||||||
|
authors: [MatissJanis]
|
||||||
|
---
|
||||||
|
|
||||||
|
Fixed a privilege escalation issue affecting password changes
|
||||||
Reference in New Issue
Block a user