| Age | Commit message (Collapse) | Author |
|---|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
The seconds field of the 8-byte Hotline time was encoded as seconds since
the start of the current year. Clients that decode this field as a raw
classic Mac OS timestamp (e.g. Pitbull Pro) therefore always rendered the
year as 1904, since the value was under one year past the 1904 epoch.
Encode seconds from 1904-01-01 to match the original Hotline server, and
decode symmetrically. Fixes #166.
|
|
|
|
Build and push ghcr.io/jhalter/mobius-hotline-server:edge on every push
to master so unreleased builds can be tested without cutting a release.
Pin metadata-action tags explicitly (tag ref + edge on master) so the
existing latest/semver release tags are preserved, and make the Dockerfile
version stamp fall back to git describe --always so non-tag builds don't
produce an empty version.
|
|
Move the Redis/ban backend selection ahead of NewServer so the presence
tracker is supplied through the WithPresenceTracker option rather than a
direct field write, matching how the other server config flows through
options. The ban list shares the Redis client, so it is captured in the
same block and assigned after construction.
|
|
Cover the two biggest untested plumbing paths in the hotline package:
- handleNewConnection (server login sequence): 3.2% -> 79.6%, via a
scripted in-memory ReadWriteCloser that feeds handshake + login bytes
and captures replies. Covers successful 1.5+ and legacy 1.2.3 flows,
no-agreement access, incorrect login, banned username, and banned IP.
- Client Connect/HandleTransactions/keepalive: 0% -> 83-93%, via a
loopback TCP server for Connect and preloaded mock conns for the
transaction loop and keepalive shutdown.
|
|
UploadHandler and UploadFolderHandler opened the resource and info fork
writers but only ever closed the data fork writer. On the OS-backed file
store this leaked a descriptor per uploaded file; on a backend that only
commits on Close (such as an object store, modeled by MemFileStore) the fork
data was silently dropped entirely.
Track the fork writers as closers and close them after the transfer completes,
before renaming the data fork into place. Add a MemFileStore-backed regression
test that fails without the fix.
|
|
Cover UploadHandler, DownloadFolderHandler, and UploadFolderHandler, which
previously had 0% coverage, along with the fork writers they exercise
(rsrcForkWriter, InfoForkWriter, incFileWriter). Tests drive the real handlers
against an OSFileStore-backed temp dir and script the client side of the
interactive folder protocols over net.Pipe, including the resume and
skip-existing branches.
Raises hotline package coverage from 67.1% to 75.3%.
|
|
The manager mocks lived in production source so that internal/mobius
tests could import them (test files are not importable across
packages), which pulled testify into the production dependency graph
of hotline importers, counted the mocks against coverage, and put them
in the library's godoc. They now live in hotline/hltest, an
httptest-style test-support package.
The hotline package's own in-package tests cannot import hltest (import
cycle), so the mocks they use are duplicated in mocks_test.go;
conformance assertions in both files catch signature drift.
MockAccountManager was only ever used inside internal/mobius and moves
to a _test.go file there.
|
|
Add a protocol-level end-to-end suite (in-process fully wired server on
an ephemeral port pair, driven by hotline.Client over TCP) covering
handshake, login, public and private chat, message board, threaded
news, file list/download/upload, account admin, disconnect
notification, and shutdown broadcast. The harness retries on a fresh
port pair when another process steals a probed port before
ListenAndServe binds it, and Server gains WithConnectionRateLimit so
tests can disable the per-IP connection throttle.
Add native fuzz tests for Transaction, Field, and flattened file
object decoding, and fix the bugs the new tests surfaced:
- Transaction.Write panicked on out-of-range attacker-controlled size
fields, and transactionScanner's uint32 length addition could wrap
and yield a truncated token. The information fork size declared in
an untrusted fork header is now bounded too.
- Client keepalive read c.done unsynchronized while Disconnect
replaces it under the mutex.
- The shared Agreement's Seek+ReadAll login path raced concurrent
logins; the server now prefers an AgreementBytes() snapshot.
Fill unit-test gaps (main's config-copy helpers, file resume data,
ReloaderFunc, R2 error injection and env validation) and add a CI test
workflow (build/vet + race-enabled shuffled suite), fixed lint
workflow triggers with golangci-lint v2.6, and Makefile test/cover/
lint/fuzz targets.
|
|
SIGTERM and SIGINT canceled the server context directly, so clients
were force-closed without the TranDisconnectMsg broadcast that the API
shutdown endpoint sends. The signal handler now calls Server.Shutdown
with a goodbye message, giving both shutdown paths the same behavior:
broadcast, a brief flush window, then force-close and drain.
Default signal handling is restored before the graceful shutdown
begins, so a second signal still kills the process immediately rather
than waiting out the flush window.
|
|
Canceling the server context closed only the listeners: sessions and
file transfers never observed cancellation, so their goroutines kept
running after ListenAndServe returned, blocked in reads until the
remote side went away. Nothing joined them either, so shutdown raced
whatever work was still in flight.
The server now tracks every accepted connection (sessions and file
transfers) in a registry. After the serve loops stop, ListenAndServe
force-closes the tracked connections, which unblocks their read loops,
and waits on a WaitGroup covering every session, file transfer, and
client writer goroutine before returning. Connections that race in
after shutdown begins are closed on arrival. The registry initializes
lazily so test-constructed Servers keep working.
The 3-second Windows close workaround in handleFileTransfer is skipped
when the context is canceled so in-flight transfers do not delay
process exit, and the transfer goroutine no longer assigns its error
to the accept loop's captured variable.
|
|
NewClientConn added the connection to the ClientManager before Account
and Version were assigned, so any goroutine iterating ClientMgr.List()
during the login handshake could dereference a nil Account (e.g.
HandleSetUser reading c.Account.Login) and panic. Account was also read
and written across goroutines with no synchronization: HandleSetUser
wrote c.Account.Access on another client's connection while that
client's own transaction loop read it in Authorize.
The connection is now added to the manager in handleNewConnection only
once Account, Version, UserName, and Flags are initialized, so a
published client is always fully formed. Failed logins never publish
the connection at all; Disconnect's manager delete is a no-op for them.
Account joins the mutex-guarded session state with accessors in the
established style: SetAccount/GetAccount, SetAccountAccess for the one
post-login mutation, AccessBytes for building transaction fields, and
Authorize now takes the read lock. All cross-goroutine call sites go
through the accessors.
HandleSetUser previously recomputed the admin flag from the client's
stale access level and only converged on the following edit; the access
update now happens before the recompute.
|
|
LoadConfig unconditionally rewrote a relative FileRoot to an absolute
path under the config dir. R2FileStore.key() then embedded that local
path in every object key (e.g. Users/jhalter/.../config/Files/...), so
bucket contents were tied to the host's directory layout and orphaned
by moving the config dir or pointing another server at the bucket.
FileRoot is a path within the selected file store's namespace, so only
the backend selection in main knows how to resolve it: the os backend
resolves relative values against the config dir as before, while the
memory and r2 backends keep the configured value verbatim, yielding
portable keys like Files/foo.txt. An absolute FileRoot combined with an
object-store backend now logs a warning.
WithConfig copies the config struct, so the option is appended after
the backend selection mutates FileRoot rather than before.
|
|
Implement R2FileStore, a FileStore backed by Cloudflare R2 via its
S3-compatible API, selectable with --file-store r2 and configured through
R2_* environment variables.
The store follows the MemFileStore model: a flat keyspace where directories
are derived from key prefixes, with zero-byte marker objects so empty folders
persist, and errors.ErrUnsupported for symlink/alias operations. Because R2
has no append, in-progress .incomplete uploads are routed to a local staging
directory (real O_APPEND and size-based resume) and promoted to a finished R2
object on the terminal upload-commit Rename; all other paths live in R2.
A narrow s3API seam plus an s3Uploader interface make the backend unit-testable
against an in-memory fake without a network. Adds a user setup guide at
docs/r2-file-store.md.
|
|
Wire an os|memory selector mirroring the existing Redis-vs-file backend
selection. This marks the seam where a future object-store backend (e.g.
Cloudflare R2 / S3) slots in via hotline.WithFileStore.
|
|
Provide a reference non-OS FileStore that models storage as a flat
keyspace of cleaned paths, deriving directory structure from key prefixes
the way an object store does. It proves the FileStore abstraction is
genuinely backend-agnostic and serves as a hermetic test fixture needing
no temp directory. Symlink/ReadLink return errors.ErrUnsupported, matching
how an object-store backend would behave.
Tests drive a full upload -> list -> download round-trip and directory
listing entirely in memory.
|
|
The FileStore interface returned concrete *os.File from Open/Create/
OpenFile, which no non-filesystem backend (e.g. S3/R2) can produce, and
many file-library hot paths bypassed the interface entirely with direct
os.* / filepath.Walk calls.
Widen the interface to return io.ReadCloser / io.WriteCloser and add
ReadDir, ReadLink, and Walk so directory traversal no longer escapes the
abstraction. Route every file-library call site (fork writers, upload/
download handlers, GetFileNameList, CalcTotalSize/CalcItemCount, the set-
file-info folder rename) through the injected FileStore, and add a
WithFileStore option. OSFileStore keeps byte-identical behavior.
DownloadHandler now nil-guards the optional resource-fork reader instead
of relying on *os.File's nil-receiver tolerance, so a backend returning an
untyped-nil reader does not panic.
|
|
Replace the leaky *redis.Client field on Server with a PresenceTracker
interface that receives connect/rename/disconnect lifecycle events. This
removes Redis-specific set encoding from hotline/server.go and the session
handlers, and drops the redis dependency from the hotline package.
The Redis implementation moves to mobius.RedisPresenceTracker, which owns
the legacy "login::ip"/"login:nickname:ip" set encoding so existing
deployments keep working. The API server reads online users through a new
OnlineLister interface and falls back to the in-memory ClientMgr when no
presence tracker is configured. Startup clearing of stale online state now
happens unconditionally when Redis is configured, not only when the API
server is enabled.
|
|
main.go's reloadFunc reached through the server's interface fields with
concrete type assertions (srv.MessageBoard.(*mobius.FlatNews), etc.) to
trigger SIGHUP/API reloads, leaking storage implementation details past
the manager interfaces.
Storage backends now implement a one-method Reloader interface, with
compile-time assertions for FlatNews, BanFile, ThreadedNewsYAML, and
Agreement. BanFile.Load and ThreadedNewsYAML.Load are renamed Reload
for a uniform method set, matching FlatNews's existing convention of
using Reload for the initial load as well.
main.go registers each backend in a named reloader list as it is
constructed, and reloadFunc iterates the list. The banner reload is a
ReloaderFunc that also performs the initial load, and the Redis-backed
ban list simply registers no reloader, replacing the old type-switch
special case.
|
|
ClientConn's mutable session state (Flags, UserName, Icon, IdleTime,
AutoReply) was guarded inconsistently: two mutexes (FlagsMU and mu)
covered some paths while others mutated or read the fields with no
locking at all, including HandleSetClientUserInfo, HandleUpdateUser
(which writes other clients' admin flag), the login flow, the HTTP API
handlers, and the keepalive loop. Consolidate on a single mutex with
accessor methods (SetFlag/IsFlagSet/FlagBytes, SetUserName/GetUserName,
and so on) used by all production code; direct field access remains for
test construction. The idle/away logic moves into incrementIdleTime
and clearIdleAndAway helpers that report whether a notification is
needed, so SendAll is no longer called while holding the lock.
HandleRejectChatInvite also no longer appends to the username slice,
which could write past its length into the backing buffer.
The server banner is now behind Banner/SetBanner with an RWMutex: the
SIGHUP reload previously reassigned the field while banner download
goroutines read it, and nilled it when the file read failed. Reload
now keeps the previous banner on failure.
Per-IP rate limiter entries now record a last-seen time, and the
keepalive ticker evicts entries idle for over seven days, so the map
no longer grows unboundedly with each unique client IP.
|
|
ListenAndServe previously started each listener in a goroutine that
called log.Fatal on any error, which skipped deferred cleanup and made
errors unobservable to callers, and Server.Shutdown terminated the
process with os.Exit. Context cancellation was also ineffective:
Serve only checked ctx between Accept calls, which block indefinitely.
ListenAndServe now binds its listeners up front and returns bind
errors, closes every listener when the context is canceled so accept
loops unblock and return, and reports the first serve loop error to
the caller. Shutdown closes a lazily-initialized channel that cancels
ListenAndServe's context, so the shutdown API works race-free even
though it starts before ListenAndServe. "Server shutting down" is
logged once by ListenAndServe rather than per accept loop, which
produced duplicate or missing lines depending on scheduling.
main.go now treats context.Canceled as a clean exit, logs other server
errors and exits nonzero, and runs deferred cleanup (e.g. Bonjour
shutdown) on the way out.
|
|
The outbox channel spawned one goroutine per outbound transaction, so
concurrent sends to the same client could interleave bytes within the
transaction framing, per-client message ordering was not guaranteed,
and a slow client accumulated unbounded goroutines.
Each ClientConn now has a bounded send queue drained by a single writer
goroutine, which serializes writes and preserves enqueue order. Send
never blocks: if a client's queue overflows, its connection is closed
and the read loop performs the usual disconnect cleanup. Server.Send
routes transactions to the target client's queue, replacing
processOutbox and sendTransaction. Handler signatures are unchanged.
Disconnect now removes the client from the manager before notifying
peers so no new transactions are routed to a departing client, then
idempotently closes its send queue.
New tests cover write ordering, framing integrity under concurrent
senders, the slow-client disconnect policy, and a Send/Disconnect race
exercise (run with -race).
|
|
Break up the 2,349-line transaction_handlers.go and its 6,716-line test
file into per-domain files (chat, files, transfers, accounts, news,
session), moving code verbatim with no signature or behavior changes.
Error message constants move to errors.go and shared test fixtures to
helpers_test.go; transaction_handlers.go retains only RegisterHandlers.
|
|
Anchor the path returned by folderUpload.FormattedPath relative to the
upload root so item paths are resolved consistently, matching the
behavior of ReadPath. Resolve each destination path once per item in
UploadFolderHandler and use path.Join in place of manual string
concatenation.
Add test coverage for FormattedPath normalization.
|
|
Startup log:
- Add interface, port, and fileTransferPort fields to the "Hotline
server started" line so operators can see what the server bound to.
- Resolve an empty -interface flag to 0.0.0.0 for display.
Levels:
- Demote the two Redis startup messages (ban management, cleared online
users) from Info to Debug.
Consistency:
- Standardize the error field key to "err" (was "Err" in a few account
handlers) and lowercase "Account" -> "account".
- Standardize the remote-address key to "remoteAddr" (was "RemoteAddr").
- Replace fmt.Sprintf in the tracker-registration message and string
concatenation in the config-dir-init and ban-disconnect messages with
structured fields; use the standard "err" key.
- Give the two bare rLogger.Error(err.Error()) file-transfer calls a
descriptive message and an "err" field.
logger.go:
- Only attach the rotating lumberjack file writer when --log-file is set.
An empty Filename made lumberjack write to a temp file by default.
|
|
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.
|
|
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.
|
|
Replace two unchecked type assertions that could crash the server:
- server.go: the file-transfer logger built remoteAddr via a bare
ctx.Value(contextKeyReq).(requestCtx) assertion, which panics if the
context value is absent or the wrong type. Use the comma-ok form and
degrade to an empty string.
- access.go: the legacy (< v0.17.0) AccessBitmap YAML array path used
byte(v.(int)) with no element-type or bounds check, panicking on a
non-int element or an array longer than the [8]byte bitmap. Iterate the
slice from the type switch, bounds-check the index, and comma-ok each
element, returning a wrapped error so a malformed user file fails to
load loudly instead of crashing the server.
Add regression tests for the non-int and oversized legacy array cases.
|
|
Replace the map-backed Stats counter with a fixed [numStats]int array
indexed by a new StatKey enum, eliminating the hand-maintained map
initialization and the parallel string-keyed Values() map.
Fix a check-then-act race in the connection peak tracking: the old
Get-then-Set across two lock acquisitions could let concurrent
connections clobber each other's update. The new atomic Max method does
the compare-and-set under a single lock.
Values() now returns a typed StatValues struct whose JSON tags preserve
the existing /api/v1/stats wire format.
|
|
Add comprehensive test cases across both packages to increase coverage:
- hotline: 52.4% → ~55% (Disconnect, handleTransaction, SendAll,
sendBanMessage, MemClientMgr, and other tests)
- internal/mobius: 75.9% → ~80% (HandleUpdateUser, HandleDeleteUser,
HandleSetUser, HandleNewUser, HandleUserBroadcast success paths)
|
|
13 types implemented identical offset-based io.Reader patterns with
5-7 lines of copy-and-offset-tracking code each. Extract a shared
readFrom(p, offset, data) helper and reduce each Read() method to a
one-liner delegating to it.
|
|
Replace hardcoded Mac Roman encoding globals with a configurable Encoding
field in config.yaml. Servers that exclusively serve modern UTF-8 clients
can now set Encoding: utf8 to disable Mac Roman conversion. The default
remains "macintosh" for backward compatibility.
- Add Encoding field to Config struct (macintosh|utf8)
- Store TextDecoder/TextEncoder on Server, initialized from config
- Add TextDecoder()/TextEncoder() accessors on ClientConn
- Pass decoder/encoder explicitly to ReadPath and GetFileNameList
- Remove package-level txtEncoder/txtDecoder globals
- Add warning log when files are skipped due to encoding errors
- Log and return error replies in HandleGetFileNameList on failure
- Document the new config option in docs/text-encoding.md
|
|
ReadPath() and HandleNewFolder decoded the entire joined path from Mac
Roman, including the fileRoot prefix which is already UTF-8. This caused
file listing failures when config paths or FileRoot values contained
non-ASCII characters. Now only client-provided path components (subPath,
fileName/folderName) are decoded before joining with fileRoot.
|
|
Return errors to clients on write failures instead of silently succeeding.
Add rollback logic to BanFile and ThreadedNewsYAML mutations so in-memory
state is restored when persistence fails. Extract error message constants
and add comprehensive tests for error paths and rollback behavior.
|
|
Extract ban logic into a BanMgr interface with two implementations:
- BanFile: file-based YAML storage with support for IP, username, and
nickname bans (backwards-compatible with legacy format)
- RedisBanMgr: Redis-backed implementation with permanent and temporary
ban support, using fail-safe deny-on-error behavior
This replaces scattered Redis calls in API handlers, transaction
handlers, and server connection logic with unified interface calls,
removing the Redis dependency from the API server constructor and
enabling ban functionality for both file-only and Redis deployments.
|
|
|
|
|
|
|
|
- Add logging for unhandled transaction types in Client.HandleTransaction
- Fix race condition in Client.Disconnect by protecting done channel with mutex
- Add TranServerMsg to transaction type names map
- Use Time type instead of raw byte array in File.flattenedFileObject
- Improve error message in handleFileTransfer to include reference number
- Simplify return statement in HandleGetFileInfo
|
|
|
|
- Add mutex to protect activeTasks map and connection writes
- Fix nil pointer dereference when handling replies with unknown IDs
- Clean up activeTasks entries after processing replies (memory leak)
- Add done channel to allow keepalive goroutine to exit on Disconnect
- Use io.ReadFull in Handshake to ensure complete reads
- Fix misleading error message when handshake response is unexpected
|
|
|
|
|
