Fix VNC robustness gap: unannounced encodings now fail clearly, not silently

Previously an unrecognized rectangle encoding in a FramebufferUpdate
aborted the connection with a generic message, and there was nothing
tying the set of encodings we announce via SetEncodings to the set we
actually know how to decode. Introduces kAnnouncedEncodings as the
single source of truth for both, converts the rectangle dispatch to a
switch keyed off it, and gives the (still intentionally fatal --
there's no safe way to skip an unknown-length payload) fallback a
message that identifies it as a protocol violation rather than "not
supported". Adds a regression test asserting every announced encoding
has a working dispatch case, so future encodings (Hextile/ZRLE/Tight/
Cursor) can't be added to the announced list without matching decode
support. Also replaces scattered inline magic numbers for RFB
message-type constants with named constants, in prep for the
clipboard/cursor/compression work that follows.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

src/vnc_session_backend.cpp

@@ -8,10 +8,33 @@
 
 #include <openssl/des.h>
 
+#include <array>
 #include <cstring>
 #include <limits>
 
 namespace {
+// RFB (RFC 6143) message-type constants, client -> server.
+constexpr char kMsgSetPixelFormat = 0;
+constexpr char kMsgSetEncodings = 2;
+constexpr char kMsgFramebufferUpdateRequest = 3;
+constexpr char kMsgKeyEvent = 4;
+constexpr char kMsgPointerEvent = 5;
+constexpr char kMsgClientCutText = 6;
+
+// RFB message-type constants, server -> client.
+constexpr quint8 kServerMsgFramebufferUpdate = 0;
+constexpr quint8 kServerMsgSetColourMapEntries = 1;
+constexpr quint8 kServerMsgBell = 2;
+constexpr quint8 kServerMsgServerCutText = 3;
+
+// Encoding types. This list grows as new FramebufferUpdate rectangle
+// encodings are supported; kAnnouncedEncodings below is the single source
+// of truth for what we tell the server we can decode via SetEncodings.
+constexpr qint32 kEncRaw = 0;
+constexpr qint32 kEncCopyRect = 1;
+
+constexpr std::array<qint32, 2> kAnnouncedEncodings = { kEncRaw, kEncCopyRect };
+
 quint16 readU16BE(const QByteArray& buf, int offset)
 {
     const auto* p = reinterpret_cast<const uchar*>(buf.constData()) + offset;
@@ -173,7 +196,7 @@ void VncSessionBackend::sendKeyEvent(int key,
     }
 
     QByteArray msg;
-    msg.append(char(4)); // message-type: KeyEvent
+    msg.append(kMsgKeyEvent);
     msg.append(pressed ? char(1) : char(0));
     appendU16BE(msg, 0); // padding
     appendU32BE(msg, keysym);
@@ -368,7 +391,7 @@ void VncSessionBackend::sendSetPixelFormatAndEncodings()
     // wrapped with zero conversion -- the same trick
     // RdpSessionBackend::orbitEndPaint uses for FreeRDP's GDI buffer.
     QByteArray setPixelFormat;
-    setPixelFormat.append(char(0)); // message-type: SetPixelFormat
+    setPixelFormat.append(kMsgSetPixelFormat);
     setPixelFormat.append(3, char(0)); // padding
     setPixelFormat.append(char(32)); // bits-per-pixel
     setPixelFormat.append(char(24)); // depth
@@ -384,18 +407,19 @@ void VncSessionBackend::sendSetPixelFormatAndEncodings()
     m_socket->write(setPixelFormat);
 
     QByteArray setEncodings;
-    setEncodings.append(char(2)); // message-type: SetEncodings
+    setEncodings.append(kMsgSetEncodings);
     setEncodings.append(char(0)); // padding
-    appendU16BE(setEncodings, 2); // number-of-encodings
+    appendU16BE(setEncodings, static_cast<quint16>(kAnnouncedEncodings.size()));
-    appendU32BE(setEncodings, 0); // Raw
+    for (qint32 encoding : kAnnouncedEncodings) {
-    appendU32BE(setEncodings, 1); // CopyRect
+        appendU32BE(setEncodings, static_cast<quint32>(encoding));
+    }
     m_socket->write(setEncodings);
 }
 
 void VncSessionBackend::requestFramebufferUpdate(bool incremental)
 {
     QByteArray msg;
-    msg.append(char(3)); // message-type: FramebufferUpdateRequest
+    msg.append(kMsgFramebufferUpdateRequest);
     msg.append(incremental ? char(1) : char(0));
     appendU16BE(msg, 0); // x
     appendU16BE(msg, 0); // y
@@ -410,7 +434,7 @@ void VncSessionBackend::sendPointerEvent()
         return;
     }
     QByteArray msg;
-    msg.append(char(5)); // message-type: PointerEvent
+    msg.append(kMsgPointerEvent);
     msg.append(static_cast<char>(m_pointerButtonMask));
     appendU16BE(msg, static_cast<quint16>(qBound(0, m_lastPointerX, 65535)));
     appendU16BE(msg, static_cast<quint16>(qBound(0, m_lastPointerY, 65535)));
@@ -427,14 +451,14 @@ void VncSessionBackend::sendWheelClick(quint8 wheelBit)
     const quint16 y = static_cast<quint16>(qBound(0, m_lastPointerY, 65535));
 
     QByteArray press;
-    press.append(char(5));
+    press.append(kMsgPointerEvent);
     press.append(static_cast<char>(m_pointerButtonMask | wheelBit));
     appendU16BE(press, x);
     appendU16BE(press, y);
     m_socket->write(press);
 
     QByteArray release;
-    release.append(char(5));
+    release.append(kMsgPointerEvent);
     release.append(static_cast<char>(m_pointerButtonMask));
     appendU16BE(release, x);
     appendU16BE(release, y);
@@ -706,17 +730,17 @@ void VncSessionBackend::processReceiveBuffer()
             const quint8 messageType = static_cast<quint8>(m_recvBuffer.at(0));
             m_recvBuffer.remove(0, 1);
             switch (messageType) {
-            case 0:
+            case kServerMsgFramebufferUpdate:
                 m_rfbState = RfbState::WaitingFramebufferUpdateHeader;
                 break;
-            case 1:
+            case kServerMsgSetColourMapEntries:
                 m_rfbState = RfbState::WaitingSetColourMapHeader;
                 break;
-            case 2:
+            case kServerMsgBell:
                 // Bell -- nothing to render; stay in the same state.
                 emit eventLogged(QStringLiteral("Remote bell."));
                 break;
-            case 3:
+            case kServerMsgServerCutText:
                 m_rfbState = RfbState::WaitingServerCutTextHeader;
                 break;
             default:
@@ -756,16 +780,36 @@ void VncSessionBackend::processReceiveBuffer()
             m_currentRectangle.encoding = static_cast<qint32>(readU32BE(m_recvBuffer, 8));
             m_recvBuffer.remove(0, 12);
 
-            if (m_currentRectangle.encoding == 0) {
+            switch (m_currentRectangle.encoding) {
+            case kEncRaw:
                 m_rfbState = RfbState::WaitingRawPixelData;
-            } else if (m_currentRectangle.encoding == 1) {
+                break;
+            case kEncCopyRect:
                 m_rfbState = RfbState::WaitingCopyRectSource;
-            } else {
+                break;
+            default: {
+                // SetEncodings (see kAnnouncedEncodings) is entirely
+                // client-controlled, so a spec-compliant server will never
+                // send an encoding we didn't announce. Reaching here means
+                // either a non-compliant server, or -- more likely in
+                // practice -- an OrbitHub bug where an encoding was added to
+                // kAnnouncedEncodings without a matching case above. There's
+                // no safe way to skip an unrecognized rectangle's payload
+                // (its length depends on decoding it), so this must stay
+                // fatal rather than attempt to guess and resync.
+                QStringList announced;
+                for (qint32 encoding : kAnnouncedEncodings) {
+                    announced.push_back(QString::number(encoding));
+                }
                 failConnection(
-                    QStringLiteral("The VNC server sent an encoding OrbitHub doesn't support."),
+                    QStringLiteral("The VNC server sent an encoding it was never offered -- "
-                    QStringLiteral("Encoding type %1").arg(m_currentRectangle.encoding));
+                                   "this indicates a non-compliant server."),
+                    QStringLiteral("Encoding type %1 (announced: %2)")
+                        .arg(m_currentRectangle.encoding)
+                        .arg(announced.join(QStringLiteral(", "))));
                 return;
             }
+            }
             break;
         }
 

tests/test_vnc_session_backend.cpp

@@ -108,6 +108,8 @@ private slots:
     void unsupportedSecurityTypeReachesFailedState();
     void rfb33ServerWithNoAuthConnectsWithoutSecurityResult();
     void rawFramebufferUpdateProducesExpectedPixels();
+    void copyRectEncodingDoesNotFailConnection();
+    void unannouncedEncodingFailsConnectionWithClearMessage();
 
 private:
     std::unique_ptr<FakeVncServer> m_server;
@@ -491,5 +493,132 @@ void TestVncSessionBackend::rawFramebufferUpdateProducesExpectedPixels()
     QCOMPARE(m_lastFrame.pixelColor(1, 0), QColor(0, 255, 0));
 }
 
+// Regression guard for the WaitingRectangleHeader dispatch: every encoding
+// this backend announces via SetEncodings must have a working decode path.
+// Raw is already exercised by rawFramebufferUpdateProducesExpectedPixels();
+// this covers CopyRect. Whenever a new encoding is added to
+// kAnnouncedEncodings in vnc_session_backend.cpp, a matching test belongs
+// here (or nearby) so the announced list and the dispatch switch can never
+// silently drift apart.
+void TestVncSessionBackend::copyRectEncodingDoesNotFailConnection()
+{
+    int frameCount = 0;
+    connect(m_backend.get(), &SessionBackend::frameUpdated, this,
+            [&frameCount](const QImage&) { ++frameCount; });
+
+    connect(m_server.get(), &FakeVncServer::clientConnected, this, [this]() {
+        m_server->sendWhenConnected(QByteArray("RFB 003.008\n"));
+    });
+    connect(m_server.get(), &FakeVncServer::dataReceived, this, [this]() {
+        m_server->received.clear();
+        switch (m_server->nextStep()) {
+        case 0: {
+            QByteArray securityTypes;
+            securityTypes.append(char(1));
+            securityTypes.append(char(1));
+            m_server->sendWhenConnected(securityTypes);
+            break;
+        }
+        case 1:
+            m_server->sendWhenConnected(QByteArray(4, char(0))); // SecurityResult: OK
+            break;
+        case 2: { // ClientInit
+            QByteArray serverInit;
+            serverInit.append(char(0)); serverInit.append(char(2)); // width = 2
+            serverInit.append(char(0)); serverInit.append(char(2)); // height = 2
+            serverInit.append(QByteArray(16, char(0)));
+            serverInit.append(QByteArray(4, char(0)));
+            m_server->sendWhenConnected(serverInit);
+            break;
+        }
+        case 3: { // initial non-incremental request -> seed the framebuffer with Raw
+            QByteArray update;
+            update.append(char(0)); update.append(char(0));
+            update.append(char(0)); update.append(char(1)); // 1 rectangle
+            update.append(char(0)); update.append(char(0)); // x
+            update.append(char(0)); update.append(char(0)); // y
+            update.append(char(0)); update.append(char(2)); // width
+            update.append(char(0)); update.append(char(2)); // height
+            update.append(char(0)); update.append(char(0)); update.append(char(0));
+            update.append(char(0)); // encoding = Raw
+            update.append(QByteArray(2 * 2 * 4, char(0x11)));
+            m_server->sendWhenConnected(update);
+            break;
+        }
+        case 4: { // next incremental request -> a CopyRect rectangle
+            QByteArray update;
+            update.append(char(0)); update.append(char(0));
+            update.append(char(0)); update.append(char(1)); // 1 rectangle
+            update.append(char(0)); update.append(char(0)); // x
+            update.append(char(0)); update.append(char(0)); // y
+            update.append(char(0)); update.append(char(2)); // width
+            update.append(char(0)); update.append(char(2)); // height
+            update.append(char(0)); update.append(char(0)); update.append(char(0));
+            update.append(char(1)); // encoding = CopyRect
+            update.append(char(0)); update.append(char(0)); // src x = 0
+            update.append(char(0)); update.append(char(0)); // src y = 0
+            m_server->sendWhenConnected(update);
+            break;
+        }
+        default:
+            break;
+        }
+    });
+
+    m_backend->connectSession(makeOptions());
+    QTRY_VERIFY(frameCount >= 2);
+    QCOMPARE(m_lastState, SessionState::Connected);
+}
+
+void TestVncSessionBackend::unannouncedEncodingFailsConnectionWithClearMessage()
+{
+    connect(m_server.get(), &FakeVncServer::clientConnected, this, [this]() {
+        m_server->sendWhenConnected(QByteArray("RFB 003.008\n"));
+    });
+    connect(m_server.get(), &FakeVncServer::dataReceived, this, [this]() {
+        m_server->received.clear();
+        switch (m_server->nextStep()) {
+        case 0: {
+            QByteArray securityTypes;
+            securityTypes.append(char(1));
+            securityTypes.append(char(1));
+            m_server->sendWhenConnected(securityTypes);
+            break;
+        }
+        case 1:
+            m_server->sendWhenConnected(QByteArray(4, char(0))); // SecurityResult: OK
+            break;
+        case 2: { // ClientInit
+            QByteArray serverInit;
+            serverInit.append(char(0)); serverInit.append(char(1)); // width = 1
+            serverInit.append(char(0)); serverInit.append(char(1)); // height = 1
+            serverInit.append(QByteArray(16, char(0)));
+            serverInit.append(QByteArray(4, char(0)));
+            m_server->sendWhenConnected(serverInit);
+            break;
+        }
+        case 3: { // a rectangle whose encoding was never announced via SetEncodings
+            QByteArray update;
+            update.append(char(0)); update.append(char(0));
+            update.append(char(0)); update.append(char(1)); // 1 rectangle
+            update.append(char(0)); update.append(char(0)); // x
+            update.append(char(0)); update.append(char(0)); // y
+            update.append(char(0)); update.append(char(1)); // width
+            update.append(char(0)); update.append(char(1)); // height
+            update.append(char(0)); update.append(char(0)); update.append(char(0));
+            update.append(char(99)); // encoding = 99, never announced
+            m_server->sendWhenConnected(update);
+            break;
+        }
+        default:
+            break;
+        }
+    });
+
+    m_backend->connectSession(makeOptions());
+    QTRY_COMPARE(m_lastState, SessionState::Failed);
+    QVERIFY(m_lastErrorDisplay.contains(QStringLiteral("non-compliant")));
+}
+
 QTEST_GUILESS_MAIN(TestVncSessionBackend)
 #include "test_vnc_session_backend.moc"