fix(lte,security): duplicate reset SMS, and three CodeQL findings (v0.24.7-beta)
Duplicate SMS. A user reported the manager sending two reset messages one
after another. It is a check-then-act race, and a real one.
Data-cap tracking runs from three paths — the fast poll, the slow poll and a
manual sync — and poll-fast and poll-slow are separate BullMQ queues that
overlap. Between reading last_sent_at and writing it back there are two
network round trips: fetching the interface counters, and sending the message.
Two overlapping polls both read the same stale last_sent_at, both clear the
cooldown, and both send.
The send is now claimed with a conditional UPDATE ... RETURNING before it is
made: whoever's row comes back won, everyone else stands down. The period_key
test mirrors the existing roll rule, so a new period is not bound by the
previous period's cooldown. A failed send releases the claim — holding it
would suppress a genuine retry for a full cooldown after a message that never
went, and being throttled costs the user more than a duplicate does.
Demonstrated against the real database with ten concurrent claims: the
previous shape had all ten sending; the claim admits exactly one, still allows
a fresh send after a period roll, and refuses a repeat inside the cooldown.
js/tainted-format-string (high, x2) in clients.ts. console.error( ... ${logSafe(mac)}:, err) puts an untrusted value in the format position
with a further argument, so a mac containing "%s" turns the message into a
format string and swallows err. Both now use a literal format string.
logSafe deliberately still leaves "%" alone: neutralising it would corrupt
every legitimate "90% full" in the logs to defend a case that only arises with
extra arguments. The rule is structural, and console-format.test.ts enforces
it — scoped to logSafe-wrapped values, since that wrapper is the author saying
"untrusted", which is exactly when the format position matters.
js/file-system-race (high) in backups.ts, introduced by me two days ago in
the backup preview. existsSync then statSync then readFileSync resolves the
path three times; a delete landing mid-sequence turned a clean 404 into an
unhandled throw. Now one handle, stat and read on it.
Noted, not fixed: the wider scan found ~40 call sites interpolating a device
name into a log line with no sanitising at all. That is a real but separate
concern — they need logSafe, not a different format string — and it deserves
its own change rather than being smuggled into this one.
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Container image: ghcr.io/2gt-media-group-llc/mikrotik-manager:0.24.7-beta
Documentation: https://2gt-media-group-llc.github.io/mikrotik-manager/