fix(server): validate packet length header before parsing

[AshleyAHuang]

Closes #155.

Root cause

_ThreadedTCPRequestHandler.handle() parses the 10-byte length header with a bare int(header.strip()). If the TCP stream ever gets desynced (as reported, apparently from connection instability over VPN/multi-network client setups), the bytes read as "header" aren't a valid integer, and the unhandled ValueError kills that client's handler thread with an unstructured traceback — matching the exact error in the issue (invalid literal for int() with base 10: 'alive>12').

Fix

Wrapped the header parse in try/except ValueError, logging a clear error identifying the client and the malformed header content, then closing just that connection cleanly — instead of an unhandled exception.

Testing

Added test_serverInvalidHeaderClosesConnectionOnly: opens a raw socket, sends a 10-byte non-integer header (b"bad_head!!"), and asserts the server (1) logs a clear error, (2) closes that connection, (3) keeps working normally for other clients afterward.

Verified non-vacuously: reverting just the fix reproduces the exact reported traceback and the test fails; restoring it, the test passes. Full suite: 65 passed / 4 skipped (one unrelated broadcast-socket test is flaky in this sandboxed environment specifically — confirmed pre-existing by re-running in isolation and re-running the full suite clean, unrelated to this change). flake8 clean on both changed files.

Note on a related, separate finding

While investigating, the linked comment thread surfaced a SyntaxError crash in packet.py, which parses incoming packet data via eval(str(bwPacket.strip())) — i.e. eval() on raw network input. That's a distinct, more serious concern (arbitrary code execution risk from untrusted socket data) than the header-validation bug here, so I've deliberately left it out of this PR. Happy to open a separate issue/PR for it if useful.

[Schrolli91]

@KoenigMjr Can you take a look on this PR please.
How was your solution on this problem?

[Schrolli91]

My main question regarding this error is:

Is this a general issue or a temporary one? If so, does it affect the entire TCP socket asynchronously, or is only that specific packet "cut off"? Does the connection need to be closed, or is it sufficient to simply skip the faulty packet?

After all, as far as I know, this only happens with keep-alive heartbeat packets, right?
That point is still a bit unclear to me.

[KoenigMjr]

It's fantastic to see multiple perspectives on this.

@AshleyAHuang did an amazing job pinpointing the exact symptom (the ValueError crash) and providing a top-tier regression test, which we should absolutely adopt into the final codebase regardless of which implementation we choose.

To answer your questions about the root cause and compare our approaches, here is a breakdown of what happens IMHO:

- Is it a general or temporary issue?
It is a general and inherent trait of TCP streaming over unstable connections (like WAN, VPN, or mobile networks).

- Does it affect the entire socket or just one packet?
TCP is a byte-stream, not a packet-stream. When the network hiccups, packets get fragmented at the MTU level. If self.request.recv(HEADERSIZE) or recv(length) is called on a standard socket, it reads whatever bytes are currently in the OS buffer. If a payload is truncated, the remaining bytes of that same message are left behind. In the next iteration, the server reads those remaining payload bytes and misinterprets them as the next 10-byte header.

- Does it only happen to keep-alive?
No, it can happen to any packet. However, because is sent constantly every few seconds/milliseconds, the probability of it hitting a network hiccup is statistically much higher.

- Can we just skip the packet, or must the connection close?
With standard recv(), once a desync happens, the stream is corrupted. Skipping is impossible because the server no longer knows where the next true header starts. The connection must be closed to reset the framing, leading to data loss for any pending alarms.

The approach in this PR wraps the header parsing in a try/except ValueError. (Approach A)

• Pros: Highly local, clean, prevents the server thread from throwing an unstructured traceback, and cleanly disconnects the corrupted client.

• Cons: It treats the symptom, not the cause. If a 1000-byte alarm payload arrives fragmented (e.g., only 500 bytes arrive in the first TCP window), recv(1000) will return 500 bytes. The next loop will parse the rest of the alarm as a header, fail, and drop the connection, losing the alarm.

In my parallel overhaul, we approached this from a "Zero Packet Loss" perspective. (Approach B)

• TCP Reassembly (recvall): We replaced raw recv() with a loop-based recvall(socket, length) helper. If a header or payload is fragmented, recvall waits and reconstructs the chunk until exactly length bytes are read (or a timeout hits). This prevents desyncs entirely.
• Symmetric Architecture: We fixed a hidden loop issue where the client kept reconnecting because the server swallowed without sending an [ack]. Now, heartbeats are properly acknowledged.
• Client Buffer: The client retains packets in a pending queue until the server explicitly returns [ack].

Proposal for Moving Forward

Both solutions agree on the vulnerability, but Approach B rewrites the network I/O layer to prevent data loss over shaky VPNs entirely, rather than just handling the disconnect gracefully.

However, @AshleyAHuang regression test is excellent. It elegantly validates that a bad header won't crash the server.

My recommendation: I would love to see us combine the best of both worlds. We could adapt @AshleyAHuang test suite to ensure it guards our recvall() implementation as well.

I leave the final architectural decision to you, @Schrolli91. Let me know if you would like me to link our branch or push our recvall + [ack] implementation here for comparison.

Approach B ist beta-testing atm. Its running local at my server, but i dont use any VPN-connection, but I've teamed up with a beta-tester running a VPN client setup. It seems to run smoothly so far.

[Schrolli91]

@KoenigMjr Your solution sounds great—it tackles the problem at its root rather than just reacting with basic error handling (like dropping frames).

Let's go with the recvall solution.

If that works for you, I’ll merge this now as a temporary hotfix. Any merge conflicts on your end should be easy to fix since the changes are minimal. Once that's done, you can adapt the test for your approach

[Schrolli91]

LGTM

[KoenigMjr]

LGTM aswell :) Good to Go! :)

[Schrolli91]

@AshleyAHuang Thank you for the contribution :)