Rework AOPacket (#24)

core/include/aopacket.h

@@ -41,35 +41,115 @@ class AOPacket
     AOPacket(QString p_header, QStringList p_contents);
 
     /**
-     * @brief AOPacket Interprets a string of a full (header + content) packet into an AOPacket.
+     * @brief Create an AOPacket from an incoming network message.
      *
-     * @param packet The string to interpret.
+     * @param f_packet An escaped string with header and content.
      */
-    AOPacket(QString packet);
+    AOPacket(QString f_packet);
 
     /**
-     * @brief Returns the string representation of the packet.
+     * @brief Destructor for the AOPacket
+     */
+    ~AOPacket(){};
+
+    /**
+     * @brief Returns the current content of the packet
      *
-     * @return See brief description.
+     * @return The content of the packet.
+     */
+    const QStringList getContent();
+
+    /**
+     * @brief Returns the header of the packet.
+     *
+     * @return The packets header.
+     */
+    QString getHeader();
+
+    /**
+     * @brief Converts the header and content into a single string.
+     *
+     * @return String converted packet.
      */
     QString toString();
 
     /**
-     * @brief Convenience function over AOPacket::toString() + QString::toUtf8().
+     * @brief Converts the entire packet, header and content, to a UTF8 formatted ByteArray.
      *
      * @return A UTF-8 representation of the packet.
      */
     QByteArray toUtf8();
 
     /**
-     * @brief The string that indentifies the type of the packet.
+     * @brief Allows editing of the content inside the packet on a per-field basis.
      */
-    QString header;
+    void setContentField(int f_content_index, QString f_content_data);
 
     /**
-     * @brief The list of parameters for the packet. Can be empty.
+     * @brief Escapes the content of the packet using AO2's escape codes.
+     *
+     * @see https://github.com/AttorneyOnline/docs/blob/master/AO%20Documentation/docs/development/network.md#escape-codes
      */
-    QStringList contents;
+    void escapeContent();
+
+    /**
+     * @brief Unescapes the content of the packet using AO2's escape codes.
+     *
+     * @see https://github.com/AttorneyOnline/docs/blob/master/AO%20Documentation/docs/development/network.md#escape-codes
+     */
+    void unescapeContent();
+
+    /**
+     * @brief Due to the way AO's netcode actively fights you, you have to do some specific considerations when escaping evidence.
+     */
+    void escapeEvidence();
+
+    /**
+     * @brief Sets the state if a packet has already been escaped or not.
+     *
+     * @details This is partially a workaround to make edge case behaviour possible while maintaining a
+     * mostly unified escape/unescape path.
+     *
+     * @param Boolean value of the current state.
+     *
+     */
+    void setPacketEscaped(bool f_packet_state);
+
+    /**
+     * @brief Returns if the packet is currently escaped or not.
+     *
+     * @details If a packet is escaped, it likely has either just been received by the server or is about to be written
+     * to a network socket. There should **NEVER** be an instance where an unescaped packet is processed inside the server.
+     *
+     * @return If true, the packet is escaped. If false, it is unescaped and plain text.
+     */
+    bool isPacketEscaped();
+
+  private:
+    /**
+     * @brief The header of the packet.
+     *
+     * @see https://github.com/AttorneyOnline/docs/blob/master/AO%20Documentation/docs/development/network.md#network-protocol
+     * for a general explanation on Attorney Online 2's network protocl.
+     */
+    QString m_header;
+
+    /**
+     * @brief The contents of the packet.
+     */
+    QStringList m_content;
+
+    /**
+     * @brief Wether the packet is currently escaped or not. If false, the packet is unescaped.
+     */
+    bool m_escaped;
+
+    /**
+     * @brief According to AO documentation a complete packet is finished using the percent symbol.
+     *
+     * @details Note : This is due to AOs inability to determine the packet length, making it read forever otherwise.
+     */
+    const QString packetFinished = "%";
 };
 
 #endif // PACKET_MANAGER_H

core/src/aoclient.cpp

@@ -171,6 +171,10 @@ void AOClient::clientData()
     QStringList l_all_packets = l_data.split("%");
     l_all_packets.removeLast(); // Remove the entry after the last delimiter
 
+    if (l_all_packets.value(0).startsWith("MC", Qt::CaseInsensitive)) {
+        l_all_packets = QStringList{l_all_packets.value(0)};
+    }
+
     for (const QString &l_single_packet : qAsConst(l_all_packets)) {
         AOPacket l_packet(l_single_packet);
         handlePacket(l_packet);
@@ -180,7 +184,7 @@ void AOClient::clientData()
 void AOClient::clientDisconnected()
 {
 #ifdef NET_DEBUG
-    qDebug() << remote_ip.toString() << "disconnected";
+    qDebug() << m_remote_ip.toString() << "disconnected";
 #endif
     if (m_joined) {
         server->getAreaById(m_current_area)->clientLeftArea(server->getCharID(m_current_char), m_id);
@@ -208,12 +212,12 @@ void AOClient::clientDisconnected()
 void AOClient::handlePacket(AOPacket packet)
 {
 #ifdef NET_DEBUG
-    qDebug() << "Received packet:" << packet.header << ":" << packet.contents << "args length:" << packet.contents.length();
+    qDebug() << "Received packet:" << packet.getHeader() << ":" << packet.getContent() << "args length:" << packet.getContent().length();
 #endif
     AreaData *l_area = server->getAreaById(m_current_area);
-    PacketInfo l_info = packets.value(packet.header, {ACLRole::NONE, 0, &AOClient::pktDefault});
+    PacketInfo l_info = packets.value(packet.getHeader(), {ACLRole::NONE, 0, &AOClient::pktDefault});
 
-    if (packet.contents.join("").size() > 16384) {
+    if (packet.getContent().join("").size() > 16384) {
         return;
     }
 
@@ -221,21 +225,21 @@ void AOClient::handlePacket(AOPacket packet)
         return;
     }
 
-    if (packet.header != "CH") {
+    if (packet.getHeader() != "CH") {
         if (m_is_afk)
             sendServerMessage("You are no longer AFK.");
         m_is_afk = false;
         m_afk_timer->start(ConfigManager::afkTimeout() * 1000);
     }
 
-    if (packet.contents.length() < l_info.minArgs) {
+    if (packet.getContent().length() < l_info.minArgs) {
 #ifdef NET_DEBUG
-        qDebug() << "Invalid packet args length. Minimum is" << info.minArgs << "but only" << packet.contents.length() << "were given.";
+        qDebug() << "Invalid packet args length. Minimum is" << l_info.minArgs << "but only" << packet.getContent().length() << "were given.";
 #endif
         return;
     }
 
-    (this->*(l_info.action))(l_area, packet.contents.length(), packet.contents, packet);
+    (this->*(l_info.action))(l_area, packet.getContent().length(), packet.getContent(), packet);
 }
 
 void AOClient::changeArea(int new_area)
@@ -430,13 +434,8 @@ void AOClient::fullArup()
 void AOClient::sendPacket(AOPacket packet)
 {
 #ifdef NET_DEBUG
-    qDebug() << "Sent packet:" << packet.header << ":" << packet.contents;
+    qDebug() << "Sent packet:" << packet.getHeader() << ":" << packet.getContent();
 #endif
-    packet.contents.replaceInStrings("#", "<num>")
-        .replaceInStrings("%", "<percent>")
-        .replaceInStrings("$", "<dollar>");
-    if (packet.header != "LE")
-        packet.contents.replaceInStrings("&", "<and>");
     m_socket->write(packet.toUtf8());
     m_socket->flush();
 }

core/src/aopacket.cpp

@@ -17,47 +17,103 @@
 //////////////////////////////////////////////////////////////////////////////////////
 #include "include/aopacket.h"
 
-AOPacket::AOPacket(QString p_header, QStringList p_contents)
+AOPacket::AOPacket(QString p_header, QStringList p_contents) :
+    m_header(p_header),
+    m_content(p_contents),
+    m_escaped(false)
 {
-    header = p_header;
-    contents = p_contents;
 }
 
-AOPacket::AOPacket(QString p_packet)
+AOPacket::AOPacket(QString f_packet)
 {
-    if (p_packet.isEmpty())
+    QString l_packet = f_packet;
+    if (l_packet.isEmpty() || l_packet.at(0) == '#' || l_packet.contains("%")) {
+#if NET_DEBUG
+        qDebug() << "Invalid or fantacrypt packet received.";
+#endif
+        m_header = "Unknown";
+        m_content = QStringList{"Unknown"};
         return;
+    }
 
-    QStringList packet_contents = p_packet.split("#");
+    QStringList l_split_packet = l_packet.split("#");
-    if (p_packet.at(0) == '#') {
+    m_header = l_split_packet.value(0);
-        // The header is encrypted with FantaCrypt
+
-        // This should never happen with AO2 2.4.3 or newer
+    // Remove header and trailing packetFinished
-        qDebug() << "FantaCrypt packet received";
+    l_split_packet.removeFirst();
-        header = "Unknown";
+    l_split_packet.removeLast();
-        packet_contents.append("Unknown");
+    m_content = l_split_packet;
-        return;
+
-    }
+    // All incoming data has to be escaped after being split.
-    else {
+    this->unescapeContent();
-        header = packet_contents[0];
+}
-    }
+
-    packet_contents.removeFirst(); // Remove header
+const QStringList AOPacket::getContent()
-    packet_contents.removeLast();  // Remove anything trailing after delimiter
+{
-    contents = packet_contents;
+    return m_content;
+}
+
+QString AOPacket::getHeader()
+{
+    return m_header;
 }
 
 QString AOPacket::toString()
 {
-    QString ao_packet = header;
+    if (!isPacketEscaped() && !(m_header == "LE")) {
-    for (int i = 0; i < contents.length(); i++) {
+        // We will never send unescaped data to a client, unless its evidence.
-        ao_packet += "#" + contents[i];
+        this->escapeContent();
     }
-    ao_packet += "#%";
+    else {
-
+        // Of course AO has SOME expection to the rule.
-    return ao_packet;
+        this->escapeEvidence();
+    }
+    return QString("%1#%2#%3").arg(m_header, m_content.join("#"), packetFinished);
 }
 
 QByteArray AOPacket::toUtf8()
 {
-    QString packet_string = toString();
+    QString l_packet = this->toString();
-    return packet_string.toUtf8();
+    return l_packet.toUtf8();
+}
+
+void AOPacket::setContentField(int f_content_index, QString f_content_data)
+{
+    m_content[f_content_index] = f_content_data;
+}
+
+void AOPacket::escapeContent()
+{
+    m_content.replaceInStrings("#", "<num>")
+        .replaceInStrings("%", "<percent>")
+        .replaceInStrings("$", "<dollar>")
+        .replaceInStrings("&", "<and>");
+    this->setPacketEscaped(true);
+}
+
+void AOPacket::unescapeContent()
+{
+    m_content.replaceInStrings("<num>", "#")
+        .replaceInStrings("<percent>", "%")
+        .replaceInStrings("<dollar>", "$")
+        .replaceInStrings("<and>", "&");
+    this->setPacketEscaped(false);
+}
+
+void AOPacket::escapeEvidence()
+{
+    m_content.replaceInStrings("#", "<num>")
+        .replaceInStrings("%", "<percent>")
+        .replaceInStrings("$", "<dollar>");
+    this->setPacketEscaped(true);
+}
+
+void AOPacket::setPacketEscaped(bool f_packet_state)
+{
+    m_escaped = f_packet_state;
+}
+
+bool AOPacket::isPacketEscaped()
+{
+    return m_escaped;
 }

core/src/packets.cpp

@@ -33,7 +33,7 @@ void AOClient::pktDefault(AreaData *area, int argc, QStringList argv, AOPacket p
     Q_UNUSED(argc);
     Q_UNUSED(argv);
 #ifdef NET_DEBUG
-    qDebug() << "Unimplemented packet:" << packet.header << packet.contents;
+    qDebug() << "Unimplemented packet:" << packet.getHeader() << packet.getContent();
 #else
     Q_UNUSED(packet);
 #endif
@@ -104,7 +104,7 @@ void AOClient::pktSoftwareId(AreaData *area, int argc, QStringList argv, AOPacke
 
     if (m_version.release != 2) {
         // No valid ID packet resolution.
-        sendPacket(AOPacket("BD", {"A protocol error has been encountered. Packet : ID"}));
+        sendPacket(AOPacket("BD", {"A protocol error has been encountered. Packet : ID\nMajor version not recognised."}));
         m_socket->close();
         return;
     }
@@ -224,6 +224,11 @@ void AOClient::pktSelectChar(AreaData *area, int argc, QStringList argv, AOPacke
         l_selected_char_id = SPECTATOR_ID;
     }
 
+    if (l_selected_char_id < -1 || l_selected_char_id > server->getCharacters().size() - 1) {
+        sendPacket(AOPacket("KK", {"A protocol error has been encountered.Packet : CC\nCharacter ID out of range."}));
+        m_socket->close();
+    }
+
     if (changeCharacter(l_selected_char_id))
         m_char_id = l_selected_char_id;
 
@@ -247,15 +252,15 @@ void AOClient::pktIcChat(AreaData *area, int argc, QStringList argv, AOPacket pa
     }
 
     AOPacket validated_packet = validateIcPacket(packet);
-    if (validated_packet.header == "INVALID")
+    if (validated_packet.getHeader() == "INVALID")
         return;
 
     if (m_pos != "")
-        validated_packet.contents[5] = m_pos;
+        validated_packet.setContentField(5, m_pos);
 
     server->broadcast(validated_packet, m_current_area);
     emit logIC((m_current_char + " " + m_showname), m_ooc_name, m_ipid, server->getAreaById(m_current_area)->name(), m_last_message);
-    area->updateLastICMessage(validated_packet.contents);
+    area->updateLastICMessage(validated_packet.getContent());
 
     area->startMessageFloodguard(ConfigManager::messageFloodguard());
     server->startMessageFloodguard(ConfigManager::globalMessageFloodguard());
@@ -502,8 +507,8 @@ void AOClient::pktModCall(AreaData *area, int argc, QStringList argv, AOPacket p
 
     QString l_modcallNotice = "!!!MODCALL!!!\nArea: " + l_areaName + "\nCaller: " + l_name + "\n";
 
-    if (!packet.contents[0].isEmpty())
+    if (!packet.getContent()[0].isEmpty())
-        l_modcallNotice.append("Reason: " + packet.contents[0]);
+        l_modcallNotice.append("Reason: " + packet.getContent()[0]);
     else
         l_modcallNotice.append("No reason given.");
 
@@ -520,7 +525,7 @@ void AOClient::pktModCall(AreaData *area, int argc, QStringList argv, AOPacket p
             l_name = m_current_char;
 
         QString l_areaName = area->name();
-        emit server->modcallWebhookRequest(l_name, l_areaName, packet.contents[0], server->getAreaBuffer(l_areaName));
+        emit server->modcallWebhookRequest(l_name, l_areaName, packet.getContent().value(0), server->getAreaBuffer(l_areaName));
     }
 }
 
@@ -694,7 +699,7 @@ AOPacket AOClient::validateIcPacket(AOPacket packet)
         return l_invalid;
 
     QList<QVariant> l_incoming_args;
-    for (const QString &l_arg : qAsConst(packet.contents)) {
+    for (const QString &l_arg : packet.getContent()) {
         l_incoming_args.append(QVariant(l_arg));
     }
 
@@ -882,7 +887,7 @@ AOPacket AOClient::validateIcPacket(AOPacket packet)
         QString l_other_offset = "0";
         QString l_other_flip = "0";
         for (int l_client_id : area->joinedIDs()) {
-        AOClient* l_client = server->getClientByID(l_client_id);
+            AOClient *l_client = server->getClientByID(l_client_id);
             if (l_client->m_pairing_with == m_char_id && l_other_charid != m_char_id && l_client->m_char_id == m_pairing_with && l_client->m_pos == m_pos) {
                 l_other_name = l_client->m_current_iniswap;
                 l_other_emote = l_client->m_emote;

core/src/server.cpp

@@ -221,7 +221,7 @@ void Server::clientConnected()
     client->sendPacket(decryptor);
     hookupAOClient(client);
 #ifdef NET_DEBUG
-    qDebug() << client->remote_ip.toString() << "connected";
+    qDebug() << client->m_remote_ip.toString() << "connected";
 #endif
 }
 

tests/tests.pro

@@ -4,4 +4,5 @@ SUBDIRS += \
     unittest_area \
     unittest_music_manager \
     unittest_acl_roles_handler \
-    unittest_command_extension
+    unittest_command_extension \
+    unittest_aopacket

tests/unittest_aopacket/tst_unittest_aopacket.cpp

@@ -0,0 +1,86 @@
+#include <QObject>
+#include <QTest>
+
+#include "include/aopacket.h"
+
+namespace tests {
+namespace unittests {
+
+/**
+ * @brief Unit Tester class for the area-related functions.
+ */
+class Packet : public QObject
+{
+    Q_OBJECT
+
+  public:
+    AOPacket m_packet = AOPacket{"", {}};
+
+  private slots:
+    /**
+     * @brief Creates a packet from a defined header and content.
+     */
+    void createPacket();
+
+    /**
+     * @brief The data function for createPacketFromString();
+     */
+    void createPacketFromString_data();
+
+    /**
+     * @brief Tests the creation of AOPackets from incoming string formatted packets.
+     */
+    void createPacketFromString();
+};
+
+void Packet::createPacket()
+{
+    AOPacket packet = AOPacket("HI", {"HDID"});
+    QCOMPARE(packet.getHeader(), "HI");
+    QCOMPARE(packet.getContent(), {"HDID"});
+}
+
+void Packet::createPacketFromString_data()
+{
+    QTest::addColumn<QString>("incoming_packet");
+    QTest::addColumn<QString>("expected_header");
+    QTest::addColumn<QStringList>("expected_content");
+
+    QTest::newRow("No Escaped fields") << "HI#1234#"
+                                       << "HI"
+                                       << QStringList{"1234"};
+
+    QTest::newRow("Multiple fields") << "ID#34#Akashi#"
+                                     << "ID"
+                                     << QStringList{"34", "Akashi"};
+
+    QTest::newRow("Encoded fields") << "MC#[T<and>T]Objection.opus#0#oldmud0#-1#0#0#"
+                                    << "MC"
+                                    << QStringList{"[T&T]Objection.opus", "0", "oldmud0", "-1", "0", "0"};
+
+    QTest::newRow("Sequence of encoded characters") << "UNIT#<and><and><percent><num><percent><dollar>#"
+                                                    << "UNIT"
+                                                    << QStringList{"&&%#%$"};
+
+    QTest::newRow("Unescaped characters") << "MC#20% Cooler#"
+                                          << "Unknown"
+                                          << QStringList{"Unknown"}; // This should be impossible.
+}
+
+void Packet::createPacketFromString()
+{
+    QFETCH(QString, incoming_packet);
+    QFETCH(QString, expected_header);
+    QFETCH(QStringList, expected_content);
+
+    AOPacket packet = AOPacket(incoming_packet);
+    QCOMPARE(packet.getHeader(), expected_header);
+    QCOMPARE(packet.getContent(), expected_content);
+}
+
+}
+}
+
+QTEST_APPLESS_MAIN(tests::unittests::Packet)
+
+#include "tst_unittest_aopacket.moc"

tests/unittest_aopacket/unittest_aopacket.pro

@@ -0,0 +1,5 @@
+QT -= gui
+
+include(../tests_common.pri)
+
+SOURCES +=  tst_unittest_aopacket.cpp