From cb83a8e870f821e15fcf24c895a16ae4e05b46cf Mon Sep 17 00:00:00 2001 From: Justin Bull Date: Mon, 15 Nov 2021 03:53:52 -0500 Subject: [PATCH] Properly format and safely escape to CSV in DecodeEventLogger (#1092) * Add failing test for toCSV() * Add missing " to timeslot value * Add failing test for quotes and commas in details * Separate comma and quote tests * Escape toCSV cells using utility * Use CSVFormatter instead of escape utils Allows us to control exactly how we want to format a CSV-valid row. * Never have a null cell Instead of using a stream map, opting for an explicit handling so the logger can explode when an unexpected null shows up. Prevents the situation where things are mysteriously blank, forcing developers to handle known null scenarios. --- build.gradle | 2 + .../module/log/DecodeEventLogger.java | 54 ++++---- .../module/log/DecodeEventLoggerTest.java | 121 ++++++++++++++++++ 3 files changed, 155 insertions(+), 22 deletions(-) create mode 100644 src/test/java/io/github/dsheirer/module/log/DecodeEventLoggerTest.java diff --git a/build.gradle b/build.gradle index a2e72723..507a7f4a 100644 --- a/build.gradle +++ b/build.gradle @@ -62,6 +62,7 @@ dependencies { testImplementation 'junit:junit:4.12' testImplementation 'org.assertj:assertj-core:3.8.0' testImplementation("org.junit.jupiter:junit-jupiter-api:5.7.0") + testImplementation 'org.mockito:mockito-core:3.+' testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine' @@ -92,6 +93,7 @@ dependencies { implementation 'org.apache.commons:commons-compress:1.20' implementation 'org.apache.commons:commons-lang3:3.8.1' implementation 'org.apache.commons:commons-math3:3.6.1' + implementation 'org.apache.commons:commons-csv:1.9.0' implementation 'org.apache.mina:mina-core:2.1.3' implementation 'org.apache.mina:mina-http:2.1.3' implementation 'org.controlsfx:controlsfx:11.1.0' diff --git a/src/main/java/io/github/dsheirer/module/log/DecodeEventLogger.java b/src/main/java/io/github/dsheirer/module/log/DecodeEventLogger.java index 566aa9ca..129dda56 100644 --- a/src/main/java/io/github/dsheirer/module/log/DecodeEventLogger.java +++ b/src/main/java/io/github/dsheirer/module/log/DecodeEventLogger.java @@ -33,10 +33,13 @@ import io.github.dsheirer.module.decode.event.IDecodeEvent; import io.github.dsheirer.module.decode.event.IDecodeEventListener; import io.github.dsheirer.preference.TimestampFormat; import io.github.dsheirer.sample.Listener; +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.QuoteMode; import java.nio.file.Path; import java.text.DecimalFormat; import java.text.SimpleDateFormat; +import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -47,6 +50,16 @@ public class DecodeEventLogger extends EventLogger implements IDecodeEventListen private AliasList mAliasList; private AliasModel mAliasModel; + /** + * The CSV format that SDR Trunk will use to write decode event logs + *

+ * It uses the standard, default CSV format (RFC 4180 and permitting empty lines) but *always* quoting cells since + * that's what SDR Trunk has done previously when hand-crafting CSV rows. + */ + private final CSVFormat mCsvFormat = CSVFormat.Builder.create(CSVFormat.DEFAULT) + .setQuoteMode(QuoteMode.ALL) + .build(); + public DecodeEventLogger(AliasModel aliasModel, Path logDirectory, String fileNameSuffix, long frequency) { super(logDirectory, fileNameSuffix, frequency); @@ -83,25 +96,23 @@ public class DecodeEventLogger extends EventLogger implements IDecodeEventListen private String toCSV(IDecodeEvent event) { + List cells = new ArrayList<>(); - StringBuilder sb = new StringBuilder(); - - sb.append("\"").append(mTimestampFormat.format(new Date(event.getTimeStart()))).append("\""); - sb.append(",\"").append(event.getDuration() > 0 ? event.getDuration() : "").append("\""); - sb.append(",\"").append(event.getProtocol()).append("\""); + cells.add(mTimestampFormat.format(new Date(event.getTimeStart()))); + cells.add(event.getDuration() > 0 ? event.getDuration() : ""); + cells.add(event.getProtocol()); String description = event.getEventDescription(); - - sb.append(",\"").append(description != null ? description : "").append("\""); + cells.add(description != null ? description : ""); List fromIdentifiers = event.getIdentifierCollection().getIdentifiers(Role.FROM); if(fromIdentifiers != null && !fromIdentifiers.isEmpty()) { - sb.append(",\"").append(fromIdentifiers.get(0)).append("\""); + cells.add(fromIdentifiers.get(0)); } else { - sb.append(",\"\""); + cells.add(""); } List toIdentifiers = event.getIdentifierCollection().getIdentifiers(Role.TO); @@ -116,48 +127,47 @@ public class DecodeEventLogger extends EventLogger implements IDecodeEventListen { String mystring = (!mAliasList.getAliases(toIdentifiers.get(0)).isEmpty()) ? mAliasList.getAliases(toIdentifiers.get(0)).get(0).toString() : ""; - sb.append(",\"").append(mystring).append(" (").append(toIdentifiers.get(0)).append(")\""); + cells.add(mystring + " (" + toIdentifiers.get(0) + ")"); } else { - sb.append(",\"\""); + cells.add(""); } } else { - sb.append(",\"\""); + cells.add(""); } IChannelDescriptor descriptor = event.getChannelDescriptor(); - - sb.append(",\"").append(descriptor != null ? descriptor : "").append("\""); + cells.add(descriptor != null ? descriptor : ""); Identifier frequency = event.getIdentifierCollection() .getIdentifier(IdentifierClass.CONFIGURATION, Form.CHANNEL_FREQUENCY, Role.ANY); if(frequency instanceof FrequencyConfigurationIdentifier) { - sb.append(",\"").append(mFrequencyFormat - .format(((FrequencyConfigurationIdentifier)frequency).getValue() / 1e6d)).append("\""); + cells.add(mFrequencyFormat + .format(((FrequencyConfigurationIdentifier)frequency).getValue() / 1e6d)); + } else { - sb.append(",\"\""); + cells.add(""); } if(event.hasTimeslot()) { - sb.append(",\"TS:").append(event.getTimeslot()); + cells.add("TS:" + event.getTimeslot()); } else { - sb.append(",\"\""); + cells.add(""); } String details = event.getDetails(); + cells.add(details != null ? details : ""); - sb.append(",\"").append(details != null ? details : "").append("\""); - - return sb.toString(); + return mCsvFormat.format(cells.toArray()); } } diff --git a/src/test/java/io/github/dsheirer/module/log/DecodeEventLoggerTest.java b/src/test/java/io/github/dsheirer/module/log/DecodeEventLoggerTest.java new file mode 100644 index 00000000..d2df5fd7 --- /dev/null +++ b/src/test/java/io/github/dsheirer/module/log/DecodeEventLoggerTest.java @@ -0,0 +1,121 @@ +package io.github.dsheirer.module.log; + +import io.github.dsheirer.alias.Alias; +import io.github.dsheirer.alias.AliasModel; +import io.github.dsheirer.channel.IChannelDescriptor; +import io.github.dsheirer.identifier.IdentifierCollection; +import io.github.dsheirer.identifier.Role; +import io.github.dsheirer.identifier.configuration.AliasListConfigurationIdentifier; +import io.github.dsheirer.identifier.configuration.DecoderTypeConfigurationIdentifier; +import io.github.dsheirer.identifier.configuration.FrequencyConfigurationIdentifier; +import io.github.dsheirer.module.decode.DecoderType; +import io.github.dsheirer.module.decode.event.DecodeEvent; +import io.github.dsheirer.module.decode.event.IDecodeEvent; +import io.github.dsheirer.module.decode.p25.identifier.channel.APCO25Channel; +import io.github.dsheirer.module.decode.p25.identifier.talkgroup.APCO25Talkgroup; +import io.github.dsheirer.protocol.Protocol; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.Arrays; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.*; + +class DecodeEventLoggerTest { + IChannelDescriptor channelDescriptor = APCO25Channel.create(98765, 1); + + APCO25Talkgroup fromIdentifier = new APCO25Talkgroup(123, Role.FROM); + APCO25Talkgroup toIdentifier = new APCO25Talkgroup(456, Role.TO); + FrequencyConfigurationIdentifier freqIdentifier = new FrequencyConfigurationIdentifier(859562500L); + DecoderTypeConfigurationIdentifier decoderIdentifier = new DecoderTypeConfigurationIdentifier(DecoderType.P25_PHASE1); + AliasListConfigurationIdentifier aliasListIdentifier = new AliasListConfigurationIdentifier("My Alias List"); + AliasModel aliasModel = new AliasModel(); + + Path logDirectory = Paths.get(System.getProperty("java.io.tmpdir"), "sdr_trunk_tests"); + + @BeforeEach + void setUp() { + aliasModel.addAliasList(aliasListIdentifier.getValue()); + } + + @AfterEach + void tearDown() { + } + + IdentifierCollection buildIdentifierCollection() { + return new IdentifierCollection(Arrays.asList( + fromIdentifier, + toIdentifier, + decoderIdentifier, + freqIdentifier, + aliasListIdentifier + )); + } + + DecodeEvent.DecodeEventBuilder decodeEventBuilder() { + return DecodeEvent.builder(1634428994000L) + .channel(channelDescriptor) + .identifiers(buildIdentifierCollection()) + .duration(111) + .protocol(Protocol.APCO25) + .eventDescription("DATA_PACKET") + .timeslot(2) + .details("Some details"); + } + + @Test + void test_receive_writesToCsv() { + IDecodeEvent decodeEvent = decodeEventBuilder().build(); + + DecodeEventLogger decodeEventLogger = new DecodeEventLogger(aliasModel, logDirectory, "_foo.txt", 859562500); + DecodeEventLogger spy = spy(decodeEventLogger); + + doNothing().when(spy).write(anyString()); + + spy.receive(decodeEvent); + + String expectedToCsvString = + "\"2021:10:16:20:03:14\",\"111\",\"APCO-25\",\"DATA_PACKET\",\"123\",\" (456)\",\"98765-1\",\"859.562500\",\"TS:2\",\"Some details\""; + verify(spy).write(expectedToCsvString); + } + + @Test + void test_receive_withQuotesInDetails_writesToCsv() { + IDecodeEvent decodeEvent = decodeEventBuilder() + .details("Some details now with \"quotes\"!") + .build(); + + DecodeEventLogger decodeEventLogger = new DecodeEventLogger(aliasModel, logDirectory, "_foo.txt", 859562500); + DecodeEventLogger spy = spy(decodeEventLogger); + + doNothing().when(spy).write(anyString()); + + spy.receive(decodeEvent); + + String expectedToCsvString = + "\"2021:10:16:20:03:14\",\"111\",\"APCO-25\",\"DATA_PACKET\",\"123\",\" (456)\",\"98765-1\",\"859.562500\",\"TS:2\",\"Some details now with \"\"quotes\"\"!\""; + verify(spy).write(expectedToCsvString); + } + + @Test + void test_receive_withCommasInDetails_writesToCsv() { + IDecodeEvent decodeEvent = decodeEventBuilder() + .details("Some details now with, to an extent, commas!") + .build(); + + DecodeEventLogger decodeEventLogger = new DecodeEventLogger(aliasModel, logDirectory, "_foo.txt", 859562500); + DecodeEventLogger spy = spy(decodeEventLogger); + + doNothing().when(spy).write(anyString()); + + spy.receive(decodeEvent); + + String expectedToCsvString = + "\"2021:10:16:20:03:14\",\"111\",\"APCO-25\",\"DATA_PACKET\",\"123\",\" (456)\",\"98765-1\",\"859.562500\",\"TS:2\",\"Some details now with, to an extent, commas!\""; + verify(spy).write(expectedToCsvString); + } +} \ No newline at end of file