diff --git a/AGENTS.md b/AGENTS.md index c315cf9..9cc318a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -63,6 +63,14 @@ Recent history favors short, descriptive commit messages, often in Swedish imper - All schema changes must go through migrations in `app/src/main/resources/db/migrations/`, and new migrations must be registered in `DatabaseService.MIGRATIONS`. - Do not edit generated build output under `app/build/`, root `build/`, `.gradle/`, `.gradle-user/`, `dist/`, or `releases/`. +## Data Integrity & Corrections +`audit_log` is an append-only, SHA-256 hash-chained ledger (see `AuditLogService`): each row's `entry_hash` commits to its own content plus the previous row's hash, so altering anything after the fact breaks the chain in a way `validateIntegrity()` is specifically designed to catch. This is intentional tamper-evidence, not an implementation detail to work around. + +- **Never directly `UPDATE`/`DELETE` rows in `audit_log`, `voucher`, `voucher_line`, `opening_balance`, or other ledger tables via raw SQL to fix bad data** - not even when the fix is obviously correct and the user asks for it directly. This is exactly what caused a real incident: a prior session patched incorrect opening balances straight in the database, which silently broke `entry_hash` for the affected rows without anyone noticing until much later. +- Data corrections must go through the application's own domain operations, not raw SQL, so the correction itself produces a new, properly-chained audit entry describing what changed and why. `VoucherService`'s correction-voucher pattern (`CORRECTION_VOUCHER`, `recordCorrectionVoucher`) is the model to follow: never edit a wrong voucher in place, post a new one that references and reverses it. Extend that same append-only pattern to other data types rather than reaching for an `UPDATE` statement. +- If you discover a hash-chain integrity violation - from a bug, a previous manual patch, or anything else - do not "fix" it by rewriting `entry_hash`/`previous_hash` on the broken row, and do not call `AuditLogService.rebuildIntegrityChain`/`repairIntegrityForAllCompanies` casually. Those exist solely as one-time, migration-triggered repair tools for specific historical bugs (see `V27__audit_log_decouple_references.sql` and `DatabaseService.initialize()`), gated to run exactly once, never as general-purpose "make the warning go away" utilities - doing so would silently mask real tampering going forward. Use `AuditLogService.recordIntegrityRemediation(companyId, auditLogId, reason)` instead: it appends a new, normally-chained entry documenting *why* a specific row's mismatch is a known, explained anomaly, which moves it from `validateIntegrity()`'s blocking critical list to `listDocumentedExceptions()` - visible and no longer blocking, without ever touching the original row. +- If you are unsure whether something needs a proper code fix versus a one-off manual correction, stop and ask the user before running SQL against a live database - do not decide unilaterally. + ## Agent-Specific Notes - Always ask before creating a new git branch. - Never push directly to `main` unless the user explicitly says to push `main`, or you ask for and receive confirmation immediately before pushing. diff --git a/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy b/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy index ef42173..0b50ae1 100644 --- a/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy +++ b/app/src/main/groovy/se/alipsa/accounting/service/AuditLogService.groovy @@ -41,6 +41,7 @@ final class AuditLogService { static final String DELETE_FISCAL_YEAR = 'DELETE_FISCAL_YEAR' static final String ARCHIVE_COMPANY = 'ARCHIVE_COMPANY' static final String UNARCHIVE_COMPANY = 'UNARCHIVE_COMPANY' + static final String INTEGRITY_REMEDIATION = 'INTEGRITY_REMEDIATION' private static final String DEFAULT_ACTOR = 'desktop-app' private static final DateTimeFormatter HASH_TIMESTAMP_FORMAT = DateTimeFormatter.ofPattern("uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSS") @@ -137,12 +138,19 @@ final class AuditLogService { } } + /** + * Critical, unexplained integrity problems only. A row with a hash mismatch that has a + * matching {@link #recordIntegrityRemediation} entry is excluded here - see + * {@link #listDocumentedExceptions(long)} for those. This is the list report exports and + * other operations gate on (see ReportIntegrityService); a documented exception must not + * keep blocking business operations forever once it has been explained. + */ List validateIntegrity() { databaseService.withSql { Sql sql -> List problems = [] sql.rows('select distinct company_id as companyId from audit_log').each { GroovyRowResult companyRow -> long cid = ((Number) companyRow.get('companyId')).longValue() - problems.addAll(validateIntegrityForCompany(sql, cid)) + problems.addAll(checkIntegrityForCompany(sql, cid).criticalProblems) } problems } @@ -151,12 +159,89 @@ final class AuditLogService { List validateIntegrity(long companyId) { CompanyService.requireValidCompanyId(companyId) databaseService.withSql { Sql sql -> - validateIntegrityForCompany(sql, companyId) + checkIntegrityForCompany(sql, companyId).criticalProblems + } + } + + /** + * Hash mismatches that have a matching {@link #recordIntegrityRemediation} entry + * explaining them - visible for transparency, but excluded from + * {@link #validateIntegrity(long)} and therefore no longer blocking. + */ + List listDocumentedExceptions(long companyId) { + CompanyService.requireValidCompanyId(companyId) + databaseService.withSql { Sql sql -> + checkIntegrityForCompany(sql, companyId).documentedExceptions + } + } + + /** + * Documents that a hash mismatch on {@code remediatedAuditLogId} is a known, explained + * historical anomaly rather than undetected tampering - e.g. data corrupted by a bug that + * has since been fixed, where the underlying record was already corrected through its + * normal domain operation (which itself produced its own, properly-chained audit entry). + * + * This deliberately never touches the broken row's own entry_hash/previous_hash: doing so + * would mean the chain no longer commits to what actually happened, which is the entire + * point of keeping it append-only. It appends a new, normally-chained + * {@link #INTEGRITY_REMEDIATION} entry instead, capturing the row's exact anomalous state + * (its stored entry_hash and previous_hash, plus what entry_hash should be given its + * current content) at the moment it is explained. Suppression in + * {@link #checkIntegrityForCompany} only ever applies while the row still matches that + * exact captured state - if the row changes again afterwards, by any means, none of those + * three values will still match and it reverts to critical. Without this, one remediation + * would permanently silence *any* future tampering of the same row, not just the anomaly + * it was written to explain. + * + * The target row must currently have an integrity problem - this is for documenting real, + * already-diagnosed anomalies, not for pre-emptively annotating healthy rows. + */ + AuditLogEntry recordIntegrityRemediation(long companyId, long remediatedAuditLogId, String reason) { + CompanyService.requireValidCompanyId(companyId) + String safeReason = requireText(reason, 'Anledning') + databaseService.withTransaction { Sql sql -> + GroovyRowResult target = sql.firstRow(''' + select id, + event_type as eventType, + voucher_id as voucherId, + attachment_id as attachmentId, + fiscal_year_id as fiscalYearId, + accounting_period_id as accountingPeriodId, + vat_period_id as vatPeriodId, + actor, + summary, + details, + previous_hash as previousHash, + entry_hash as entryHash, + created_at as createdAt + from audit_log + where id = ? + and company_id = ? + ''', [remediatedAuditLogId, companyId]) as GroovyRowResult + if (target == null) { + throw new IllegalArgumentException("Auditlogg ${remediatedAuditLogId} finns inte för detta företag.") + } + IntegrityCheckResult current = checkIntegrityForCompany(sql, companyId) + if (!current.criticalAuditLogIds.contains(remediatedAuditLogId)) { + throw new IllegalStateException( + "Auditlogg ${remediatedAuditLogId} har inget odokumenterat integritetsproblem att förklara." + ) + } + recordEvent(sql, INTEGRITY_REMEDIATION, AuditReferences.EMPTY, + "Manuell rättelse dokumenterad för auditlogg ${remediatedAuditLogId}", + formatDetails([ + remediatesAuditLogId : remediatedAuditLogId, + remediatedEntryHash : target.get('entryHash'), + remediatedPreviousHash : target.get('previousHash'), + remediatedContentHash : calculatedHashOfRow(target), + reason : safeReason + ]), companyId) } } - private static List validateIntegrityForCompany(Sql sql, long companyId) { - List problems = [] + private static IntegrityCheckResult checkIntegrityForCompany(Sql sql, long companyId) { + List rowProblems = [] + List structuralProblems = [] // Archiving a fiscal year (see FiscalYearReplacementService.archiveFiscalYearAuditLogRows) // resets the chain head to the latest *live* row so the next insert skips over the rows // being archived, rather than chaining off them. So the row immediately following an @@ -187,9 +272,6 @@ final class AuditLogService { ''', [companyId]).each { GroovyRowResult row -> AuditLogEntry entry = mapEntry(row) boolean archived = row.get('archived') as Boolean - if (entry.previousHash != expectedPreviousHash && entry.previousHash != expectedLastLiveHash) { - problems << ("Auditlogg ${entry.id} har fel föregående hash." as String) - } String calculated = calculateHash(new AuditEntrySeed( eventType: entry.eventType, voucherId: entry.voucherId, @@ -203,8 +285,12 @@ final class AuditLogService { previousHash: entry.previousHash, createdAt: entry.createdAt )) + String signature = integritySignature(entry.entryHash, entry.previousHash, calculated) + if (entry.previousHash != expectedPreviousHash && entry.previousHash != expectedLastLiveHash) { + rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har fel föregående hash." as String, signature) + } if (entry.entryHash != calculated) { - problems << ("Auditlogg ${entry.id} har ogiltig hash." as String) + rowProblems << new RowProblem(entry.id, "Auditlogg ${entry.id} har ogiltig hash." as String, signature) } expectedPreviousHash = entry.entryHash if (!archived) { @@ -214,11 +300,73 @@ final class AuditLogService { } AuditChainHead chainHead = loadChainHead(sql, companyId) if (chainHead == null) { - problems << 'Auditloggkedjans huvud saknas.' + structuralProblems << 'Auditloggkedjans huvud saknas.' } else if (chainHead.lastEntryHash != actualLastLiveHash) { - problems << 'Auditloggkedjans huvud pekar inte på sista auditraden.' + structuralProblems << 'Auditloggkedjans huvud pekar inte på sista auditraden.' + } + + Map> remediatedSignatures = loadRemediationSignatures(sql, companyId) + List critical = [] + List documented = [] + Set criticalIds = [] as Set + rowProblems.each { RowProblem problem -> + Set signaturesForRow = remediatedSignatures[problem.auditLogId] + if (signaturesForRow?.contains(problem.signature)) { + documented << problem.message + } else { + critical << problem.message + criticalIds << problem.auditLogId + } } - problems + // Structural problems (a missing/misdirected chain head) aren't tied to one explainable + // row, so a documented remediation can never resolve them - they always stay critical. + critical.addAll(structuralProblems) + new IntegrityCheckResult(critical, documented, criticalIds) + } + + /** + * For each remediated row id, the set of exact anomalous states (entry_hash|previous_hash| + * calculated-content-hash) that have been explained for it. A row problem only gets + * suppressed when its *current* state matches one of these exactly - see + * {@link #recordIntegrityRemediation} for why. + */ + private static Map> loadRemediationSignatures(Sql sql, long companyId) { + Map> signatures = [:] + sql.rows( + 'select details from audit_log where company_id = ? and event_type = ?', + [companyId, INTEGRITY_REMEDIATION] + ).each { GroovyRowResult row -> + Map parsed = parseDetails(SqlValueMapper.toClob(row.get('details'))) + Long remediatedId = longOrNull(parsed.remediatesAuditLogId) + if (remediatedId == null) { + return + } + String signature = integritySignature( + parsed.remediatedEntryHash, parsed.remediatedPreviousHash, parsed.remediatedContentHash + ) + signatures.computeIfAbsent(remediatedId) { [] as Set } << signature + } + signatures + } + + private static String integritySignature(String storedEntryHash, String storedPreviousHash, String calculatedContentHash) { + "${storedEntryHash ?: ''}|${storedPreviousHash ?: ''}|${calculatedContentHash ?: ''}" + } + + private static String calculatedHashOfRow(GroovyRowResult row) { + calculateHash(new AuditEntrySeed( + eventType: row.get('eventType') as String, + voucherId: longOrNull(row.get('voucherId')), + attachmentId: longOrNull(row.get('attachmentId')), + fiscalYearId: longOrNull(row.get('fiscalYearId')), + accountingPeriodId: longOrNull(row.get('accountingPeriodId')), + vatPeriodId: longOrNull(row.get('vatPeriodId')), + actor: row.get('actor') as String, + summary: row.get('summary') as String, + details: SqlValueMapper.toClob(row.get('details')), + previousHash: row.get('previousHash') as String, + createdAt: SqlValueMapper.toLocalDateTime(row.get('createdAt')) + )) } AuditLogEntry logImport(String summary, String details = null, long companyId = CompanyService.LEGACY_COMPANY_ID) { @@ -537,7 +685,7 @@ final class AuditLogService { repairArchivedReferencesFromDetails(sql) sql.rows('select distinct company_id as companyId from audit_log').each { GroovyRowResult row -> long companyId = ((Number) row.get('companyId')).longValue() - if (!validateIntegrityForCompany(sql, companyId).isEmpty()) { + if (!checkIntegrityForCompany(sql, companyId).criticalProblems.isEmpty()) { rebuildIntegrityChain(sql, companyId) } } @@ -801,4 +949,30 @@ final class AuditLogService { this.lastEntryHash = lastEntryHash } } + + private static final class RowProblem { + + final long auditLogId + final String message + final String signature + + private RowProblem(long auditLogId, String message, String signature) { + this.auditLogId = auditLogId + this.message = message + this.signature = signature + } + } + + private static final class IntegrityCheckResult { + + final List criticalProblems + final List documentedExceptions + final Set criticalAuditLogIds + + private IntegrityCheckResult(List criticalProblems, List documentedExceptions, Set criticalAuditLogIds) { + this.criticalProblems = criticalProblems + this.documentedExceptions = documentedExceptions + this.criticalAuditLogIds = criticalAuditLogIds + } + } } diff --git a/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy b/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy index 8783747..b95bf49 100644 --- a/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy +++ b/app/src/test/groovy/integration/se/alipsa/accounting/service/AuditLogServiceTest.groovy @@ -2,6 +2,7 @@ package se.alipsa.accounting.service import static org.junit.jupiter.api.Assertions.assertEquals import static org.junit.jupiter.api.Assertions.assertFalse +import static org.junit.jupiter.api.Assertions.assertThrows import static org.junit.jupiter.api.Assertions.assertTrue import groovy.sql.GroovyRowResult @@ -60,9 +61,10 @@ class AuditLogServiceTest { @Test void validateIntegrityDetectsTamperedAuditRow() { auditLogService.logImport('Importerade SIE', 'jobId=1') + long rowId = idOfRow(databaseService, "summary = 'Importerade SIE'") databaseService.withTransaction { Sql sql -> - sql.executeUpdate("update audit_log set summary = 'tampered' where id = 1") + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered', rowId]) } List problems = auditLogService.validateIntegrity() @@ -71,6 +73,92 @@ class AuditLogServiceTest { assertTrue(problems.any { String problem -> problem.contains('ogiltig hash') }) } + @Test + void recordIntegrityRemediationMovesRowFromCriticalToDocumented() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long tamperedId = idOfRow(databaseService, "summary = 'Importerade SIE'") + + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered', tamperedId]) + } + assertTrue(auditLogService.validateIntegrity(companyId).any { String problem -> problem.contains("Auditlogg ${tamperedId} ") }) + + auditLogService.recordIntegrityRemediation( + companyId, tamperedId, 'Rättad efter felsökning: fel bugg orsakade nollställd summary.' + ) + + List critical = auditLogService.validateIntegrity(companyId) + assertFalse(critical.any { String problem -> problem.contains("Auditlogg ${tamperedId} ") }, + 'A documented, explained mismatch must no longer block operations as a critical problem') + List documented = auditLogService.listDocumentedExceptions(companyId) + assertTrue(documented.any { String problem -> problem.contains("Auditlogg ${tamperedId} ") }, + 'The documented exception must still be visible, just not critical') + assertTrue(critical.isEmpty(), + 'The remediation entry itself, and everything else, must still form a valid chain') + } + + @Test + void recordIntegrityRemediationDoesNotMaskALaterDifferentTamperOfTheSameRow() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long rowId = idOfRow(databaseService, "summary = 'Importerade SIE'") + + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered once', rowId]) + } + auditLogService.recordIntegrityRemediation(companyId, rowId, 'Rättad efter felsökning av det första felet.') + assertTrue(auditLogService.validateIntegrity(companyId).isEmpty(), + 'Sanity check: the first anomaly must be documented before the real assertion below') + + // A second, unrelated change to the very same row - the remediation above explained the + // *first* anomaly, not a blanket exemption for this row forever. + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered twice, differently', rowId]) + } + + List critical = auditLogService.validateIntegrity(companyId) + assertTrue(critical.any { String problem -> problem.contains("Auditlogg ${rowId} ") }, + 'A second, different tamper of an already-remediated row must NOT be silently masked by the earlier remediation') + } + + @Test + void recordIntegrityRemediationRejectsARowWithNoIntegrityProblem() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long healthyId = idOfRow(databaseService, "summary = 'Importerade SIE'") + + IllegalStateException exception = assertThrows(IllegalStateException) { + auditLogService.recordIntegrityRemediation(companyId, healthyId, 'Ingen anledning egentligen') + } + assertTrue(exception.message.contains('inget odokumenterat integritetsproblem')) + } + + @Test + void recordIntegrityRemediationRejectsUnknownAuditLogId() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + + IllegalArgumentException exception = assertThrows(IllegalArgumentException) { + auditLogService.recordIntegrityRemediation(companyId, 999999L, 'Motivering') + } + assertTrue(exception.message.contains('finns inte')) + } + + @Test + void recordIntegrityRemediationRequiresAReason() { + long companyId = CompanyService.LEGACY_COMPANY_ID + auditLogService.logImport('Importerade SIE', 'jobId=1') + long tamperedId = idOfRow(databaseService, "summary = 'Importerade SIE'") + databaseService.withTransaction { Sql sql -> + sql.executeUpdate('update audit_log set summary = ? where id = ?', ['tampered', tamperedId]) + } + + assertThrows(IllegalArgumentException) { + auditLogService.recordIntegrityRemediation(companyId, tamperedId, ' ') + } + } + @Test void rebuildIntegrityChainMakesLiveEntriesSkipArchivedOnesInBetween() { long companyId = CompanyService.LEGACY_COMPANY_ID @@ -169,6 +257,15 @@ class AuditLogServiceTest { row.get('columnValue') as String } + private static long idOfRow(DatabaseService databaseService, String whereClause) { + databaseService.withSql { Sql sql -> + GroovyRowResult row = sql.firstRow( + "select id from audit_log where ${whereClause}".toString() + ) as GroovyRowResult + ((Number) row.get('id')).longValue() + } + } + private static void restoreProperty(String name, String value) { if (value == null) { System.clearProperty(name)