diff --git a/backend/src/entities/table-logs/repository/table-logs-custom-repository-extension.ts b/backend/src/entities/table-logs/repository/table-logs-custom-repository-extension.ts index 760908dee..355d0d110 100644 --- a/backend/src/entities/table-logs/repository/table-logs-custom-repository-extension.ts +++ b/backend/src/entities/table-logs/repository/table-logs-custom-repository-extension.ts @@ -57,9 +57,11 @@ export const tableLogsCustomRepositoryExtension: ITableLogsRepository = { if (!userConnectionEdit) { if (userInGroupsIds.length > 0) { - for (const userId of userInGroupsIds) { - qb.orWhere('tableLogs.cognitoUserName = :userIdInAdminGroup', { userIdInAdminGroup: userId }); - } + // An IN list, not a loop of orWhere. orWhere appends a top-level OR and drops the + // connection filter above. That leaked the logs of every connection. + qb.andWhere('tableLogs.cognitoUserName IN (:...userIdsInAdminGroup)', { + userIdsInAdminGroup: userInGroupsIds, + }); } else { qb.andWhere('tableLogs.cognitoUserName = :currentUserId', { currentUserId: currentUserId }); } @@ -76,9 +78,7 @@ export const tableLogsCustomRepositoryExtension: ITableLogsRepository = { qb.andWhere('tableLogs.operationType = :logOperationType', { logOperationType: logOperationType }); } if (logOperationTypes.length > 0) { - for (const type of logOperationTypes) { - qb.orWhere('tableLogs.operationType = :type', { type: type }); - } + qb.andWhere('tableLogs.operationType IN (:...logOperationTypes)', { logOperationTypes }); } const rowsCount = await qb.getCount(); const lastPage = Math.ceil(rowsCount / perPage); @@ -126,9 +126,11 @@ export const tableLogsCustomRepositoryExtension: ITableLogsRepository = { } if (!userConnectionEdit) { if (userInGroupsIds.length > 0) { - for (const userId of userInGroupsIds) { - qb.orWhere('tableLogs.cognitoUserName = :userIdInAdminGroup', { userIdInAdminGroup: userId }); - } + // An IN list, not a loop of orWhere. orWhere appends a top-level OR and drops the + // connection filter above. That leaked the logs of every connection. + qb.andWhere('tableLogs.cognitoUserName IN (:...userIdsInAdminGroup)', { + userIdsInAdminGroup: userInGroupsIds, + }); } else { qb.andWhere('tableLogs.cognitoUserName = :currentUserId', { currentUserId: currentUserId }); } @@ -145,9 +147,7 @@ export const tableLogsCustomRepositoryExtension: ITableLogsRepository = { qb.andWhere('tableLogs.operationType = :logOperationType', { logOperationType: logOperationType }); } if (logOperationTypes.length > 0) { - for (const type of logOperationTypes) { - qb.orWhere('tableLogs.operationType = :type', { type: type }); - } + qb.andWhere('tableLogs.operationType IN (:...logOperationTypes)', { logOperationTypes }); } const offset = (page - 1) * perPage; diff --git a/backend/src/entities/user/enums/jwt-scopes.enum.ts b/backend/src/entities/user/enums/jwt-scopes.enum.ts index 4ee7d5a3e..1d9d6c425 100644 --- a/backend/src/entities/user/enums/jwt-scopes.enum.ts +++ b/backend/src/entities/user/enums/jwt-scopes.enum.ts @@ -4,4 +4,8 @@ export enum JwtScopesEnum { // unconfirmed user) in rocketadmin-saas issues a token carrying this, and it is refused // everywhere except the routes that complete the flow — see assertTokenScopeAllowed. EMAIL_VERIFY = 'email_verify', + // rocketadmin-saas issues it for a support session that acts as a user. The core accepts it + // as a normal session today. The enum entry makes the scope visible to the core. A route + // can refuse it later. + IMPERSONATED = 'impersonated', } diff --git a/backend/src/entities/user/user.entity.ts b/backend/src/entities/user/user.entity.ts index eb785ac83..ff0de8cc2 100644 --- a/backend/src/entities/user/user.entity.ts +++ b/backend/src/entities/user/user.entity.ts @@ -55,9 +55,12 @@ export class UserEntity { this.emailToLowerCase(); } + // The hook encrypts the OTP seed whenever it is set, not only after enrollment. Before this rule + // a user who never completed verify-otp kept a clear-text seed at rest (ADR 01 §5.1). + @BeforeInsert() @BeforeUpdate() encryptOtpSecretKey() { - if (this.isOTPEnabled && this.otpSecretKey) { + if (this.otpSecretKey && !Encryptor.isEncrypted(this.otpSecretKey)) { this.otpSecretKey = Encryptor.encryptData(this.otpSecretKey); } this.emailToLowerCase(); @@ -65,7 +68,7 @@ export class UserEntity { @AfterLoad() decryptOtpSecretKey() { - if (this.isOTPEnabled && this.otpSecretKey) { + if (this.otpSecretKey && Encryptor.isEncrypted(this.otpSecretKey)) { this.otpSecretKey = Encryptor.decryptData(this.otpSecretKey); } this.emailToLowerCase(); diff --git a/backend/src/helpers/encryption/encryptor.ts b/backend/src/helpers/encryption/encryptor.ts index 9c2127b08..83fdf4e20 100644 --- a/backend/src/helpers/encryption/encryptor.ts +++ b/backend/src/helpers/encryption/encryptor.ts @@ -105,15 +105,22 @@ export class Encryptor { if (data === null || data === undefined) { return data; } + // Fail closed. A return of the plaintext here stores a secret in clear text and the caller + // cannot tell. decryptData throws in the same case. try { const privateKey = Encryptor.getPrivateKey(); return Encryptor.encryptDataV2(data, privateKey); } catch (e) { - console.log('-> Encryption error', e); - return data; + throw new Error('Data encryption failed with error: ' + e); } } + // True when the value already carries the V2 envelope. The user entity hooks use it to + // encrypt a value once and to skip a value that is still in clear text on load. + static isEncrypted(value: string): boolean { + return Encryptor.isV2Format(value); + } + static decryptData(encryptedData: string): string; static decryptData(encryptedData: string | null | undefined): string | null | undefined; static decryptData(encryptedData: string | null | undefined): string | null | undefined { diff --git a/backend/src/microservices/saas-microservice/use-cases/saas-otp-login.use.case.ts b/backend/src/microservices/saas-microservice/use-cases/saas-otp-login.use.case.ts index 9bc6179a6..c0578174d 100644 --- a/backend/src/microservices/saas-microservice/use-cases/saas-otp-login.use.case.ts +++ b/backend/src/microservices/saas-microservice/use-cases/saas-otp-login.use.case.ts @@ -7,6 +7,7 @@ import { FoundUserDto } from '../../../entities/user/dto/found-user.dto.js'; import { IOtpLogin } from '../../../entities/user/use-cases/user-use-cases.interfaces.js'; import { UserHelperService } from '../../../entities/user/user-helper.service.js'; import { InTransactionEnum } from '../../../enums/in-transaction.enum.js'; +import { Messages } from '../../../exceptions/text/messages.js'; import { appConfig } from '../../../shared/config/app-config.js'; import { SaasOtpLoginDs } from '../data-structures/saas-otp-login.ds.js'; import { ISaasOtpLogin } from './saas-use-cases.interface.js'; @@ -68,6 +69,9 @@ export class SaasOtpLoginUseCase extends AbstractUseCase { + const { appConfig } = awaitedAppConfig; + const savedKey = appConfig.auth.privateKey; + appConfig.auth.privateKey = null; + try { + const error = t.throws(() => Encryptor.encryptData(TEST_DATA)); + t.true(error.message.includes('Data encryption failed')); + t.false(error.message.includes(TEST_DATA)); + } finally { + appConfig.auth.privateKey = savedKey; + } +}); + +test('isEncrypted: true only for the V2 envelope', (t) => { + t.true(Encryptor.isEncrypted(Encryptor.encryptData(TEST_DATA))); + t.false(Encryptor.isEncrypted(TEST_DATA)); +}); diff --git a/backend/test/ava-tests/non-saas-tests/non-saas-table-postgres-bulk-delete-e2e.test.ts b/backend/test/ava-tests/non-saas-tests/non-saas-table-postgres-bulk-delete-e2e.test.ts index cfba1fcf5..8b4a95507 100644 --- a/backend/test/ava-tests/non-saas-tests/non-saas-table-postgres-bulk-delete-e2e.test.ts +++ b/backend/test/ava-tests/non-saas-tests/non-saas-table-postgres-bulk-delete-e2e.test.ts @@ -143,6 +143,39 @@ test.serial(`${currentTest} should delete more rows than fit one statement and l } }); +test.serial(`GET /logs/:connectionId with operationTypes stays inside the connection`, async (t) => { + // Regression for the orWhere leak: an operationTypes filter used to drop the connection filter + // and return the logs of every connection. The first test above left 1500 deleteRow logs on + // another connection of another user; this connection must report only its own. + const connectionToTestDB = getTestData(mockFactory).connectionToPostgres; + const secondUserToken = (await registerUserAndReturnUserInfo(app)).token; + const { testTableName } = await createTestTable(connectionToTestDB, 5); + const connectionId = await createConnection(secondUserToken); + + const deleteResponse = await request(app.getHttpServer()) + .put(`/table/rows/delete/${connectionId}?tableName=${testTableName}`) + .send([{ id: 1 }, { id: 2 }]) + .set('Cookie', secondUserToken) + .set('Content-Type', 'application/json') + .set('Accept', 'application/json'); + t.is(deleteResponse.status, 200, deleteResponse.text); + + for (const query of ['operationTypes=deleteRow', 'operationTypes=deleteRow,addRow', '']) { + const logsResponse = await request(app.getHttpServer()) + .get(`/logs/${connectionId}?page=1&perPage=50&${query}`) + .set('Cookie', secondUserToken) + .set('Content-Type', 'application/json') + .set('Accept', 'application/json'); + t.is(logsResponse.status, 200, logsResponse.text); + const logsRO = JSON.parse(logsResponse.text); + t.is(logsRO.pagination.total, 2, `query "${query}" must see only this connection`); + t.is(logsRO.logs.length, 2); + for (const log of logsRO.logs) { + t.is(log.table_name, testTableName); + } + } +}); + test.serial(`${currentTest} should delete rows of a table with a composite primary key`, async (t) => { const connectionToTestDB = getTestData(mockFactory).connectionToPostgres; const firstUserToken = (await registerUserAndReturnUserInfo(app)).token; diff --git a/backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts b/backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts index df0020001..145b3770f 100644 --- a/backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts +++ b/backend/test/ava-tests/saas-tests/saas-user-login-e2e.test.ts @@ -181,3 +181,38 @@ test.serial(`${currentTest} rejects a request without a microservice JWT`, async t.is(result.status, 401); t.pass(); }); + +test.serial(`${currentTest} refuses a suspended user with 401`, async (t) => { + const { email, password, companyId, userId } = await createCoreUser(); + const dataSource = app.get(BaseType.DATA_SOURCE); + await dataSource.getRepository(UserEntity).update({ id: userId }, { suspended: true }); + + const result = await request(app.getHttpServer()) + .post('/saas/user/login') + .set('Authorization', microserviceAuthHeader()) + .set('Content-Type', 'application/json') + .set('Accept', 'application/json') + .send({ email, password, companyId, request_domain: '127.0.0.1' }); + + t.is(result.status, 401, result.text); + t.is(Object.hasOwn(JSON.parse(result.text), 'id'), false); +}); + +test.serial(`${currentTest} stores the OTP seed encrypted before enrollment completes`, async (t) => { + const { userId } = await createCoreUser(); + const dataSource = app.get(BaseType.DATA_SOURCE); + const userRepository = dataSource.getRepository(UserEntity); + const seed = 'JBSWY3DPEHPK3PXP'; + + // The same path as generate-otp: a save with isOTPEnabled still false. + const user = await userRepository.findOneByOrFail({ id: userId }); + user.otpSecretKey = seed; + await userRepository.save(user); + + const [raw] = await dataSource.query('SELECT "otpSecretKey" FROM "user" WHERE id = $1', [userId]); + t.true(String(raw.otpSecretKey).startsWith('$v2:'), 'seed at rest must carry the V2 envelope'); + t.not(raw.otpSecretKey, seed); + + const reloaded = await userRepository.findOneByOrFail({ id: userId }); + t.is(reloaded.otpSecretKey, seed, 'the entity hook decrypts the seed on load'); +});