Validate client input to prevent panics from malformed transactions

A malicious or buggy client could send transaction fields with the wrong
length and trigger runtime panics (slice/index out of range, slice-to-array
conversion, nil deref) in the parsing and handler code. These were caught by
the connection-level recover, so they dropped the client connection and dumped
a stack trace to stdout rather than crashing the process, but they are still
incorrect behavior, a log-flood vector, and a latent crash if the recover
scope ever changes.

Fix at the source and harden the safety net:

- Add ClientIDFromBytes / ChatIDFromBytes helpers that return ok=false on a
  length mismatch, and use them in the transaction handlers instead of direct
  [2]byte(...) / [4]byte(...) conversions on field data. Nil-check ClientMgr.Get
  results, and length-guard FieldOptions and the HandleUpdateUser sub-field
  header. Malformed input now yields an error reply (or a clean no-op for
  reply-less handlers) instead of panicking.
- Bounds-check FileResumeData.UnmarshalBinary (header length and fork count)
  and guard the ForkInfoList[0] accesses against an empty list.
- Bounds-check FlatFileInformationFork parsing (reachable on upload): validate
  the fixed header, name, and comment lengths, and fix a latent 72+nameSize
  uint16 overflow. Route Write through UnmarshalBinary.
- panic.go: stop printing stack traces to stdout (keep structured logging) so a
  client cannot flood stdout by repeatedly triggering a panic.
- handleTransaction: recover per-transaction so one malformed request no longer
  tears down the whole client connection.

Adds tests for the new helpers, the hardened resume-data and flat-file-object
decoders, and handler-level malformed-ID handling.

8 files changed, 186 insertions, 40 deletions

diff --git a/hotline/client_conn.go b/hotline/client_conn.go
index 5428ea5..9ca93a7 100644
--- a/hotline/client_conn.go
+++ b/hotline/client_conn.go
@@ -114,6 +114,10 @@ func (cc *ClientConn) SendAll(t [2]byte, fields ...Field) {
 }
 
 func (cc *ClientConn) handleTransaction(transaction Transaction) {
+	// Contain panics to the individual transaction so a single malformed request
+	// cannot tear down the whole client connection.
+	defer dontPanic(cc.Logger)
+
 	if handler, ok := cc.Server.handlers[transaction.Type]; ok {
 		if transaction.Type != TranKeepAlive {
 			cc.Logger.Info(tranTypeNames[transaction.Type])
diff --git a/hotline/decode.go b/hotline/decode.go
new file mode 100644
index 0000000..385bc07
--- /dev/null
+++ b/hotline/decode.go
@@ -0,0 +1,22 @@
+package hotline
+
+// ClientIDFromBytes converts client-supplied bytes to a ClientID, returning
+// ok=false if the input is not exactly the expected length. Use this instead of
+// a direct [2]byte(...) conversion on field data, which panics on a length
+// mismatch.
+func ClientIDFromBytes(b []byte) (id ClientID, ok bool) {
+	if len(b) != len(id) {
+		return id, false
+	}
+	return ClientID(b), true
+}
+
+// ChatIDFromBytes converts client-supplied bytes to a ChatID, returning ok=false
+// if the input is not exactly the expected length. Use this instead of a direct
+// [4]byte(...) conversion on field data, which panics on a length mismatch.
+func ChatIDFromBytes(b []byte) (id ChatID, ok bool) {
+	if len(b) != len(id) {
+		return id, false
+	}
+	return ChatID(b), true
+}
diff --git a/hotline/decode_test.go b/hotline/decode_test.go
new file mode 100644
index 0000000..663f7ce
--- /dev/null
+++ b/hotline/decode_test.go
@@ -0,0 +1,85 @@
+package hotline
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestClientIDFromBytes(t *testing.T) {
+	tests := []struct {
+		name   string
+		b      []byte
+		want   ClientID
+		wantOk bool
+	}{
+		{name: "valid 2 bytes", b: []byte{0x00, 0x05}, want: ClientID{0x00, 0x05}, wantOk: true},
+		{name: "nil", b: nil, wantOk: false},
+		{name: "too short", b: []byte{0x01}, wantOk: false},
+		{name: "too long", b: []byte{0x01, 0x02, 0x03}, wantOk: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, ok := ClientIDFromBytes(tt.b)
+			assert.Equal(t, tt.wantOk, ok)
+			if tt.wantOk {
+				assert.Equal(t, tt.want, got)
+			}
+		})
+	}
+}
+
+func TestChatIDFromBytes(t *testing.T) {
+	tests := []struct {
+		name   string
+		b      []byte
+		want   ChatID
+		wantOk bool
+	}{
+		{name: "valid 4 bytes", b: []byte{0x00, 0x00, 0x00, 0x09}, want: ChatID{0x00, 0x00, 0x00, 0x09}, wantOk: true},
+		{name: "nil", b: nil, wantOk: false},
+		{name: "too short", b: []byte{0x01, 0x02, 0x03}, wantOk: false},
+		{name: "too long", b: []byte{0x01, 0x02, 0x03, 0x04, 0x05}, wantOk: false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, ok := ChatIDFromBytes(tt.b)
+			assert.Equal(t, tt.wantOk, ok)
+			if tt.wantOk {
+				assert.Equal(t, tt.want, got)
+			}
+		})
+	}
+}
+
+func TestFileResumeData_UnmarshalBinary(t *testing.T) {
+	// A well-formed buffer: 42-byte header with ForkCount=1, plus one 16-byte fork.
+	valid := make([]byte, resumeDataHeaderLen+forkInfoLen)
+	valid[41] = 1 // ForkCount low byte
+
+	t.Run("valid single-fork buffer parses", func(t *testing.T) {
+		var frd FileResumeData
+		require.NoError(t, frd.UnmarshalBinary(valid))
+		assert.Len(t, frd.ForkInfoList, 1)
+	})
+
+	t.Run("buffer shorter than header returns error, no panic", func(t *testing.T) {
+		var frd FileResumeData
+		assert.Error(t, frd.UnmarshalBinary(make([]byte, 10)))
+	})
+
+	t.Run("fork count overrunning the buffer returns error, no panic", func(t *testing.T) {
+		b := make([]byte, resumeDataHeaderLen) // header only, no fork bytes
+		b[41] = 3                              // claims 3 forks that aren't present
+		var frd FileResumeData
+		assert.Error(t, frd.UnmarshalBinary(b))
+	})
+
+	t.Run("zero fork count yields an empty list", func(t *testing.T) {
+		b := make([]byte, resumeDataHeaderLen)
+		var frd FileResumeData
+		require.NoError(t, frd.UnmarshalBinary(b))
+		assert.Empty(t, frd.ForkInfoList)
+	})
+}
diff --git a/hotline/file_resume_data.go b/hotline/file_resume_data.go
index 1926bc6..7cbfb17 100644
--- a/hotline/file_resume_data.go
+++ b/hotline/file_resume_data.go
@@ -76,15 +76,31 @@ func (frd *FileResumeData) BinaryMarshal() ([]byte, error) {
 	return buf.Bytes(), nil
 }
 
+// resumeDataHeaderLen is the fixed-size header (Format, Version, RSVD, ForkCount)
+// that precedes the variable-length fork info list.
+const resumeDataHeaderLen = 42
+
+// forkInfoLen is the size of a single ForkInfoList entry.
+const forkInfoLen = 16
+
 func (frd *FileResumeData) UnmarshalBinary(b []byte) error {
+	if len(b) < resumeDataHeaderLen {
+		return fmt.Errorf("file resume data too short: %d bytes, need at least %d", len(b), resumeDataHeaderLen)
+	}
+
 	frd.Format = [4]byte{b[0], b[1], b[2], b[3]}
 	frd.Version = [2]byte{b[4], b[5]}
 	frd.ForkCount = [2]byte{b[40], b[41]}
 
-	for i := 0; i < int(frd.ForkCount[1]); i++ {
+	forkCount := int(frd.ForkCount[1])
+	if need := resumeDataHeaderLen + forkCount*forkInfoLen; len(b) < need {
+		return fmt.Errorf("file resume data truncated: %d bytes, need %d for %d forks", len(b), need, forkCount)
+	}
+
+	for i := 0; i < forkCount; i++ {
 		var fil ForkInfoList
-		start := 42 + i*16
-		end := start + 16
+		start := resumeDataHeaderLen + i*forkInfoLen
+		end := start + forkInfoLen
 
 		r := bytes.NewReader(b[start:end])
 		if err := binary.Read(r, binary.BigEndian, &fil); err != nil {
diff --git a/hotline/file_transfer.go b/hotline/file_transfer.go
index e5898db..98999f1 100644
--- a/hotline/file_transfer.go
+++ b/hotline/file_transfer.go
@@ -246,7 +246,7 @@ func (fh *FileHeader) Read(p []byte) (int, error) {
 
 func DownloadHandler(w io.Writer, fullPath string, fileTransfer *FileTransfer, fs FileStore, rLogger *slog.Logger, preserveForks bool) error {
 	var dataOffset int64
-	if fileTransfer.FileResumeData != nil {
+	if fileTransfer.FileResumeData != nil && len(fileTransfer.FileResumeData.ForkInfoList) > 0 {
 		dataOffset = int64(binary.BigEndian.Uint32(fileTransfer.FileResumeData.ForkInfoList[0].DataSize[:]))
 	}
 
@@ -458,7 +458,9 @@ func DownloadFolderHandler(rwc io.ReadWriter, fullPath string, fileTransfer *Fil
 			if err := frd.UnmarshalBinary(resumeDataBytes); err != nil {
 				return err
 			}
-			dataOffset = int64(binary.BigEndian.Uint32(frd.ForkInfoList[0].DataSize[:]))
+			if len(frd.ForkInfoList) > 0 {
+				dataOffset = int64(binary.BigEndian.Uint32(frd.ForkInfoList[0].DataSize[:]))
+			}
 		case DlFldrActionNextFile:
 			// client asked to skip this file
 			return nil
diff --git a/hotline/flattened_file_object.go b/hotline/flattened_file_object.go
index 130e917..9a4fd85 100644
--- a/hotline/flattened_file_object.go
+++ b/hotline/flattened_file_object.go
@@ -3,10 +3,15 @@ package hotline
 import (
 	"bytes"
 	"encoding/binary"
+	"fmt"
 	"io"
 	"slices"
 )
 
+// flatFileInfoForkMinLen is the fixed-size portion of a FlatFileInformationFork
+// that precedes the variable-length name (and optional comment).
+const flatFileInfoForkMinLen = 72
+
 type flattenedFileObject struct {
 	FlatFileHeader                FlatFileHeader
 	FlatFileInformationForkHeader FlatFileForkHeader
@@ -156,40 +161,22 @@ func (ffif *FlatFileInformationFork) Read(p []byte) (int, error) {
 
 // Write implements the io.Writer interface for FlatFileInformationFork
 func (ffif *FlatFileInformationFork) Write(p []byte) (int, error) {
-	nameSize := p[70:72]
-	bs := binary.BigEndian.Uint16(nameSize)
-	total := 72 + bs
-
-	ffif.Platform = [4]byte(p[0:4])
-	ffif.TypeSignature = [4]byte(p[4:8])
-	ffif.CreatorSignature = [4]byte(p[8:12])
-	ffif.Flags = [4]byte(p[12:16])
-	ffif.PlatformFlags = [4]byte(p[16:20])
-	ffif.RSVD = [32]byte(p[20:52])
-	ffif.CreateDate = [8]byte(p[52:60])
-	ffif.ModifyDate = [8]byte(p[60:68])
-	ffif.NameScript = [2]byte(p[68:70])
-	ffif.NameSize = [2]byte(p[70:72])
-	ffif.Name = p[72:total]
-
-	if len(p) > int(total) {
-		ffif.CommentSize = [2]byte(p[total : total+2])
-		commentLen := binary.BigEndian.Uint16(ffif.CommentSize[:])
-		commentStartPos := int(total) + 2
-		commentEndPos := int(total) + 2 + int(commentLen)
-
-		ffif.Comment = p[commentStartPos:commentEndPos]
-
-		//total = uint16(commentEndPos)
+	if err := ffif.UnmarshalBinary(p); err != nil {
+		return 0, err
 	}
-
 	return len(p), nil
 }
 
 func (ffif *FlatFileInformationFork) UnmarshalBinary(b []byte) error {
-	nameSize := b[70:72]
-	bs := binary.BigEndian.Uint16(nameSize)
-	nameEnd := 72 + bs
+	if len(b) < flatFileInfoForkMinLen {
+		return fmt.Errorf("flat file information fork too short: %d bytes, need at least %d", len(b), flatFileInfoForkMinLen)
+	}
+
+	bs := binary.BigEndian.Uint16(b[70:72])
+	nameEnd := flatFileInfoForkMinLen + int(bs)
+	if len(b) < nameEnd {
+		return fmt.Errorf("flat file information fork name overruns buffer: need %d bytes, have %d", nameEnd, len(b))
+	}
 
 	ffif.Platform = [4]byte(b[0:4])
 	ffif.TypeSignature = [4]byte(b[4:8])
@@ -203,12 +190,18 @@ func (ffif *FlatFileInformationFork) UnmarshalBinary(b []byte) error {
 	ffif.NameSize = [2]byte(b[70:72])
 	ffif.Name = b[72:nameEnd]
 
-	if len(b) > int(nameEnd) {
+	if len(b) > nameEnd {
+		if len(b) < nameEnd+2 {
+			return fmt.Errorf("flat file information fork comment size overruns buffer")
+		}
 		ffif.CommentSize = [2]byte(b[nameEnd : nameEnd+2])
 		commentLen := binary.BigEndian.Uint16(ffif.CommentSize[:])
 
-		commentStartPos := int(nameEnd) + 2
-		commentEndPos := int(nameEnd) + 2 + int(commentLen)
+		commentStartPos := nameEnd + 2
+		commentEndPos := nameEnd + 2 + int(commentLen)
+		if len(b) < commentEndPos {
+			return fmt.Errorf("flat file information fork comment overruns buffer: need %d bytes, have %d", commentEndPos, len(b))
+		}
 
 		ffif.Comment = b[commentStartPos:commentEndPos]
 	}
diff --git a/hotline/flattened_file_object_test.go b/hotline/flattened_file_object_test.go
index 6279157..6c525c4 100644
--- a/hotline/flattened_file_object_test.go
+++ b/hotline/flattened_file_object_test.go
@@ -35,6 +35,29 @@ func TestFlatFileInformationFork_UnmarshalBinary(t *testing.T) {
 			},
 			wantErr: assert.NoError,
 		},
+		{
+			name:    "when the buffer is shorter than the fixed header returns an error instead of panicking",
+			args:    args{b: make([]byte, 10)},
+			wantErr: assert.Error,
+		},
+		{
+			name: "when the declared name size overruns the buffer returns an error",
+			args: args{b: func() []byte {
+				b := make([]byte, 72)
+				binary.BigEndian.PutUint16(b[70:72], 50) // claims a 50-byte name that isn't present
+				return b
+			}()},
+			wantErr: assert.Error,
+		},
+		{
+			name: "when the declared comment size overruns the buffer returns an error",
+			args: args{b: func() []byte {
+				b := make([]byte, 74)            // 72 header + 2 comment-size bytes, no name
+				binary.BigEndian.PutUint16(b[72:74], 50) // claims a 50-byte comment that isn't present
+				return b
+			}()},
+			wantErr: assert.Error,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/hotline/panic.go b/hotline/panic.go
index d7376db..2207911 100644
--- a/hotline/panic.go
+++ b/hotline/panic.go
@@ -1,15 +1,16 @@
 package hotline
 
 import (
-	"fmt"
 	"log/slog"
 	"runtime/debug"
 )
 
-// dontPanic logs panics instead of crashing
+// dontPanic recovers from a panic and logs it (with a stack trace) instead of
+// letting it crash the goroutine. The trace is recorded via the structured
+// logger only; it is intentionally not written to stdout, so a client that can
+// repeatedly trigger a panic cannot flood stdout.
 func dontPanic(logger *slog.Logger) {
 	if r := recover(); r != nil {
-		fmt.Println("stacktrace from panic: \n" + string(debug.Stack()))
 		logger.Error("PANIC", "err", r, "trace", string(debug.Stack()))
 	}
 }