diff options
| author | Jeff Halter <868228+jhalter@users.noreply.github.com> | 2026-05-30 16:16:32 -0700 |
|---|---|---|
| committer | Jeff Halter <868228+jhalter@users.noreply.github.com> | 2026-05-30 16:16:32 -0700 |
| commit | 588dce918aeda0efc4db80b30525d28943c029cd (patch) | |
| tree | c13a6b60e9a550f2b4b8aeb7ff3073a3ed6d6795 /hotline | |
| parent | 21cf5016d98ff568692e91751d021facbdfc6bb6 (diff) | |
Surface transaction handler errors instead of silently discarding them
Many handlers in internal/mobius/transaction_handlers.go returned an empty
transaction slice on error (return res / return nil), so the client received
no reply and the operation silently no-opped, with no log in most cases.
Convert these silent discards across the file to a consistent convention:
log the underlying cause via cc.Logger.Error and return cc.NewErrReply with a
user-facing message. Covers the file-operation, file-transfer, news,
user/account, and chat handler groups. Genuinely intentional no-reply paths
(target user offline, banned-nickname disconnect) are kept silent but now
carry an explanatory comment.
Also fixes adjacent defects found along the way:
- HandleSetFileInfo: a non-IsNotExist error from os.Rename during a directory
rename was swallowed; it now returns an error reply.
- HandleUploadFile: when a resume is requested but no .incomplete file exists,
fall back to a normal upload reply instead of discarding the reply.
- hotline.DecodeNewsPath: previously panicked on a 1-byte client-supplied
field (slice out of range) and silently produced empty path components on
truncated input. It now validates length and framing and returns an error,
which the handlers already surface.
Adds regression tests for the new error replies, the upload resume fallback,
and DecodeNewsPath's malformed-input handling.
Diffstat (limited to 'hotline')
| -rw-r--r-- | hotline/field.go | 14 | ||||
| -rw-r--r-- | hotline/field_test.go | 16 |
2 files changed, 28 insertions, 2 deletions
diff --git a/hotline/field.go b/hotline/field.go index 554f63d..99d0d35 100644 --- a/hotline/field.go +++ b/hotline/field.go @@ -5,6 +5,7 @@ import ( "bytes" "encoding/binary" "errors" + "fmt" "slices" ) @@ -137,19 +138,28 @@ func (f *Field) DecodeNewsPath() ([]string, error) { if len(f.Data) == 0 { return []string{}, nil } + if len(f.Data) < 2 { + return nil, fmt.Errorf("news path too short: %d bytes", len(f.Data)) + } pathCount := binary.BigEndian.Uint16(f.Data[0:2]) scanner := bufio.NewScanner(bytes.NewReader(f.Data[2:])) scanner.Split(newsPathScanner) - var paths []string + paths := make([]string, 0, pathCount) for i := uint16(0); i < pathCount; i++ { - scanner.Scan() + if !scanner.Scan() { + return nil, fmt.Errorf("news path truncated: declared %d items, found %d", pathCount, i) + } paths = append(paths, scanner.Text()) } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("scan news path: %w", err) + } + return paths, nil } diff --git a/hotline/field_test.go b/hotline/field_test.go index 6dff102..b8d909b 100644 --- a/hotline/field_test.go +++ b/hotline/field_test.go @@ -248,6 +248,22 @@ func TestField_DecodeNewsPath(t *testing.T) { wantErr: assert.NoError, }, { + name: "one byte of data returns an error instead of panicking", + fields: fields{Data: []byte{0x00}}, + want: nil, + wantErr: assert.Error, + }, + { + name: "declared path count exceeding available items returns an error", + fields: fields{Data: []byte{ + 0x00, 0x02, // path count = 2 + 0x00, 0x00, 0x05, // 2 bytes unused + 1 byte length (5) + 0x48, 0x65, 0x6c, 0x6c, 0x6f, // "Hello" (only 1 item present) + }}, + want: nil, + wantErr: assert.Error, + }, + { name: "single path", fields: fields{Data: []byte{ 0x00, 0x01, // path count = 1 |