[#345] CSV audit: compare the header lengths when resuming a CSV file

[vharseko]

Fixes #345.

StandardCsvWriter compared the header of the CSV file it resumes with the headers of the topic schema in a loop over the header of the file; the length check before the loop was commented out.

• A file with more columns than the schema, whose first columns match it (after the last field was removed from the topic schema), threw ArrayIndexOutOfBoundsException. CsvAuditEventHandler.startup() logs an IOException and goes on with the next topic, but this exception escaped it, so the handler failed to start.
• A file with fewer columns than the schema (after a field was added to the end of the topic schema) was resumed, and the rows appended to it had more cells than its header, which Super CSV rejects when the topic is read or queried.
• A header with as many columns as the schema but an empty column name threw NullPointerException, which escaped startup() too: Super CSV reads an empty column name as null, and the loop called equals() on the header of the file.
• null headers threw a NullPointerException without the message of checkNotNull, which ran after the comparison.

StandardCsvWriter now compares the lengths and throws "Resuming an existing CSV file but the headers do not match." when they differ, as SecureCsvWriter does, calls equals() on the headers of the topic schema, which hold no null, and checks the headers for null before it reads the file.

SecureCsvWriter had the same loop. A tamper-evident file passes the verification with an emptied column name when that column is empty in every row, since the row HMAC covers the values only; it now gets the same IOException instead of the NullPointerException, which the constructor wraps in its RuntimeException until #341.

Behavior change

A plain CSV file with fewer columns than the topic schema is no longer resumed. As with a file whose columns differ in name or order, the handler logs the error at startup and rejects the events of the topic with BadRequestException until the file is moved away; the writer then starts a new file with the current header.

Tests

• CsvWriterTest.shouldNotResumeCsvFileWithHeadersThatDoNotMatch, with one column more and one column less than the schema, and with an empty column name in the middle and at the end (child1,,child3, child1,child2,): the writer throws the IOException and leaves the file as it was.
• CsvWriterTest.shouldRejectNullHeadersWhenResumingCsvFile.
• CsvWriterTest.shouldNotResumeCsvSecureFileWithEmptyColumnName: a tamper-evident file whose header lost the name of a column that is empty in every row. The test accepts the IOException of the header check or of the verification: once [#344] Tamper-evident CSV audit: sign the column names and the value lengths in the row HMAC #369 covers the column names in the row HMAC, the same file fails the verification.
• CsvAuditEventHandlerTest.testStartupWithAuditFileOfMoreColumnsThanTopicSchema: the handler starts, and the events of the topic are rejected with BadRequestException.

Each case fails without the fix. audit/core and audit/handler-csv pass locally.

#341, #347, #348 and #369 change the same files; each merges with this one without conflicts.

[maximthomas]

praise: The length check closes both resume failures where they start, and each new test is shown to fail without it.

• StandardCsvWriter.java:65-67 rejects a length mismatch before the per-column loop, so the more-columns file no longer throws ArrayIndexOutOfBoundsException out of CsvAuditEventHandler.startup(); checkNotNull(headers, ...) now runs before the file is read (:56).
• With StandardCsvWriter.java reverted to the base, all four new cases fail: both rows of CsvWriterTest.shouldNotResumeCsvFileWithHeadersOfOtherLength, shouldRejectNullHeadersWhenResumingCsvFile, and CsvAuditEventHandlerTest.testStartupWithAuditFileOfMoreColumnsThanTopicSchema.

---

issue (non-blocking): A resumed header with the right number of columns but an empty column name still throws NullPointerException out of startup.

commons/audit/handler-csv/src/main/java/org/forgerock/audit/handlers/csv/StandardCsvWriter.java:69

super-csv 2.4.0 getHeader(true) reads an empty header cell (_id,,transactionId or "_id","","transactionId") as null, checked with a probe. That header passes the new length check, and actualHeaders[idx].equals(headers[idx]) throws an NPE. CsvAuditEventHandler.startup() catches only IOException (:222), and AuditServiceImpl.startup() catches only ResourceException (AuditServiceImpl.java:399-400), so the whole audit service fails to start instead of the one topic. This is the same kind of escape the PR closes for the length mismatch, reached through a different input. The base has the same behaviour, so it can be fixed here or in a follow-up.

for (int idx = 0; idx < actualHeaders.length; idx++) {
    if (!headers[idx].equals(actualHeaders[idx])) {
        throw new IOException("Resuming an existing CSV file but the headers do not match.");
    }
}

Pin: a CsvWriterTest case that resumes a file with the header child1,,child3 against the headers {child1, child2, child3} and expects the IOException. At the head it fails with the NPE.

[vharseko]

Fixed in 4ed28b856: both writers now call equals() on the headers of the topic schema (headers[idx].equals(actualHeaders[idx])), which hold no null, so a header with an empty column name gets the IOException. CsvWriterTest.shouldNotResumeCsvFileWithHeadersThatDoNotMatch (renamed from …WithHeadersOfOtherLength) has two more rows, child1,,child3 and child1,child2,; at 99f3a5de1 both fail with the NPE.

SecureCsvWriter had the same loop. A tamper-evident file passes the verification with an emptied column name when that column is empty in every row, since the row HMAC covers the values only. CsvWriterTest.shouldNotResumeCsvSecureFileWithEmptyColumnName pins it, and fails at 99f3a5de1 with the NPE wrapped in the RuntimeException of the constructor. Once #369 covers the column names in the row HMAC, the same file fails the verification instead, so the test accepts either IOException.

[maximthomas]

praise: The round-2 commit fixes the empty-column-name NPE where it starts, and the new test is measured to pin it.

• headers[idx].equals(actualHeaders[idx]) at StandardCsvWriter.java:70 and SecureCsvWriter.java:146 puts the topic-schema header, which cannot be null, on the receiver side; for every non-null pair the result is unchanged.
• CsvWriterTest.shouldNotResumeCsvSecureFileWithEmptyColumnName fails 1/10 with the NPE against a reverted SecureCsvWriter.java:146, with every sibling module built from this head's sources (-am, JDK 26).