https://gitlab.synchro.net/main/sbbs/-/merge_requests/711#note_9794
## Review
I read the full diff (7 commits, 414 changed lines across 7 files) and checked each
hunk against master. There are a lot of genuinely good bug fixes here, several of
which have clearly never worked. There are also five things I'd want fixed before
merge, and a handful of smaller ones.
One process note up front: the MR description covers only commit `165e9d64` (the
I:line port work). The other six commits, which carry essentially all of the risk
(TS enforcement, SJOIN origin validation, SendQ enforcement, the RBL change), are
undescribed.
### Bugs this fixes correctly - please keep these
- `channel.js`: `this.mode&USERMODE_ADMIN` -> `this.user.mode&...`. `this` is the
`ChanMode`, so `this.mode` was `undefined`; admins were being blocked from mode
changes.
- `channel.js`: `c.list[CHANMODE_BAN][add][i]` -> `[true][i]` (stale loop variable).
- `channel.js`: `this.modelist[modebit].nick` -> `[i].nick` in
`is_str_member_of_chanmode_list()`.
- `server.js:973`: `modelist[CHANMODE_OP][n.id] = n.id` -> `= n`. Confirmed correct:
`ChanMode_affect_mode_list()` and `do_join` both store the user *object*; SJOIN was
the outlier, and that is what broke the function above.
- `user.js`: `!this.mode&USERMODE_OPER` -> `!(this.mode&USERMODE_OPER)` (precedence).
- `core.js`: `format("%s :...")` with the argument outside the call.
- `server.js` INVITE: `p[2]` -> `p[1]`, and the inverted `!j.channels[...]` test.
- `user.js` WHOIS: the `p[0]`/`p[1]` mixup for `WHOIS <server> <nick>`.
- `server.js`: `typeof p[1] === undefined` (a string is never `=== undefined`). - The nick-collision KILL back toward the source. This fixes a real desync where the
originating server still believed the nick existed.
### Blocking
**1. `ts_rejected` conflates "no mode change" with "lost the TS race"**
`set_chanmode()` already had a pre-existing `return 0` at `exec/load/ircd/channel.js:448`:
```js
if (!c.addmodes && !c.delmodes)
return 0;
```
So `ts_rejected = (this.set_chanmode(...) === 0)` is true whenever the SJOIN modeline
produced *no net change*. That is the common case on a re-burst or netjoin into an
existing channel whose modes already match, and for the bare `SJOIN <ts> #chan + :@nick`
form. The result is that `k.isop` / `k.isvoice` get cleared for every member in the
burst: a mass deop, on exactly the netjoin path this commit is meant to make safe.
This needs a distinct sentinel return value (or an out-parameter), not `0`.
**2. RBL `GoodResponses` now denies every unlisted client**
`check_dnsbl()` returns `resolve_ip(qstr)`, which is falsy for the normal not-listed
case - that is what the existing `else if (dns_reply)` branch relies on. The added
`return true` fires there, so any `[RBL:x]` section with `GoodResponses=` set now
blocks essentially everyone with "Your IP address is on an RBL." `ctrl/ircd.ini:151`
ships that key as a documented example. The old code was dead (a `good` list could
never block), so a fix is warranted - it just needs to bail out on `!dns_reply` first.
**3. The KICK ops check dereferences a null `origin`**
The new block uses `origin.id` and `origin.nick`, and sits *above* the pre-existing
guard:
```js
if (!origin) {
log(LOG_ERR, format("KICK: origin is null for %s from %s", p[1], this.nick));
break;
}
```
The very next line the MR pushes down writes `origin ? origin.nick : "(null)"`, so the
surrounding code already treats origin as nullable. The new check needs to move below
the guard.
**4. The KICK check has no U:line exemption**
`!this.hub && !tmp.modelist[CHANMODE_OP][origin.id]` - a U:lined services server is a
leaf, not a hub, and is not opped. Its KICKs will now be rejected with an oper notice.
Every other trust gate in this file checks `origin.uline` (`server.js:184`, `server.js:813`).
**5. The leaf-TS override produces the desync it is meant to prevent**
`var sjoin_ts = this.hub ? parseInt(p[0]) : Epoch();` - hub-ness is not symmetric. When
our leaf regards *us* as its hub, it trusts our TS and correctly keeps its own older
ops, while we rewrite its TS to `now` and strip them. The two servers then disagree
about who is opped. The precedent being followed (`server.js:1161`, TOPIC) is safe
because a topic is idempotent state; channel ops are not. A sanity clamp - reject a
future-dated or absurdly old TS - would get the anti-spoofing benefit without discarding legitimate history.
### Non-blocking, but worth fixing before merge
- **`Queue_Send` can now busy-spin.** `js.setImmediate(Process_Sendq, this)` is now
reached when `sent <= 0`. That `setImmediate` is the only re-trigger for a partial
sendq - there is no writable-socket poll; `core.js:425/453/476` are the sole other
call sites. Worse, `js_send()` in `src/sbbs3/js_socket.cpp` leaves the return value
as `JSVAL_VOID` on error, so `sent` is `undefined`, `sent > 0` is false,
`_send_bytes` is untouched, and the `setImmediate` re-enters immediately. That is a
genuine tight loop on a failing socket, not a theoretical one. The old code stalled
forever instead, which is also a bug; the `sent <= 0` path wants a `setTimeout`
backoff.
- **SendQ enforcement goes live with no clamp on the legacy conf path.** `ini_Class()`
floors SendQ at 2048, but `config.js:1134` (`case "Y"`) does a bare
`parseInt(arg[5])`. A small or `NaN` value now disconnects clients that worked
yesterday. Worth a release note either way.
- **`[Allow] Port=` is read but never written.** `Write_Config_File()`
(`config.js:447-455`) emits only `Mask=` and `Class=`, so the new port is silently
dropped on any config rewrite. `[Capab]` got a writer; this did not.
- **The `setusermode` fix is half-applied.** Case `"A"` got the
`Servers[this.parent...] &&` guard; case `"o"` at `core.js:2622-2623` has the
identical unguarded deref and was left alone.
- **The NICK TOCTOU re-check in `User_Work` is dead code.** `check_nickname()` already
does `Users[newnick.toUpperCase()]` and emits 433, and nothing suspends between the
two calls. The `Unregistered_Welcome` one *is* legitimate, since DNS resolution is
async there - but the identical comment on both is misleading.
- **`/stats L` is unprivileged.** `do_stats()` has no oper gate anywhere, so this newly
exposes every local user's `uprefix@hostname` plus traffic counters to any client.
Pre-existing looseness, but this is the first entry that leaks per-user data. - **Socket leak in `Automatic_Server_Connect`.** `var sock = new Socket()` is created
before the guards, so the new "already connected" early return leaks it - as did the
branch it replaced. It now fires every `connfreq` for every linked C:line. Moving
`new Socket()` below the guards fixes both.
### SM128 compatibility
Checked, since the ircd JS has to keep working on both engines. All seven changed
files compile cleanly under SpiderMonkey 1.8.5 (Synchronet's `jsexec`) and under
SpiderMonkey 78 (`js78`, the nearest forward proxy available - there is no js128
shell here). A scan against the forbidden-construct list in `exec/CLAUDE.md` is also
clean: no `let`/`const`, arrow functions, template literals, `for each`, `.toSource()`,
E4X, expression closures, `for...of`, `Object.keys/values/entries`, `Map`/`Set`/
`Promise`/`class`, spread, or regex lookbehind. `Array.prototype.indexOf`, new here for
`their_capab`, is ES5 and present in both.
Two engine-sensitive things the MR does touch:
**(a) The non-ASCII literals behave differently per engine.** These files are 100%
ASCII today; the MR introduces 12 UTF-8 em dashes, 4 of them inside `log()` / `umode_notice()` format strings, one of which goes over the wire to opers. Measured:
`"a<emdash>b"` is `length` 5 under SM1.8.5 (raw bytes 226,128,148) and `length` 3 under
SM78/128 (a single U+2014). So those strings have a different value depending on the
engine, and will emit different bytes. Keeping the files ASCII avoids this entirely,
and also avoids mojibake on CP437 and latin-1 IRC clients.
**(b) The new SendQ enforcement is byte-vs-character confused, and that only bites on
SM128.** `Queue_Send` does `sent = sock.send(this._send_bytes)` and then `this._send_bytes.substr(sent)`. `js_send()` returns **bytes** written from the `JSSTRING_TO_MSTRING` buffer, while `substr()` indexes **characters**. Under SM1.8.5
that buffer is one byte per character, so it lines up by accident; under SM128 it is
UTF-8, so any non-ASCII in the stream (normal for IRC) misaligns the queue and corrupts
the output. The pre-existing `substr` is the root of it, but the new `this._send_bytes.length > limit` check layers a second byte-vs-character confusion on
top and turns the mismatch into disconnects - "Max SendQ exceeded" computed from a
character count that is not the byte count. Worth fixing while this code is open.
### Style and conventions
- The em dashes above (see SM128 note (a)).
- `var` declared mid-function inside switch cases (`join_ts`, `sjoin_ts`,
`ts_rejected`, `yline`); `Server_Work` declares at the top.
- `ctrl/ircd.ini` is not updated to document `[Capab] Enforce=` or `[Allow] Port=`. The
description says `[Allow:N]`, but the shipped default section is a bare `[Allow]`.
### On the two open questions
**Tests** - agreed, and worth pressing on, because findings 1 and 2 above are exactly
what a test network would have caught. `exec/tests/` has a runner but no ircd harness
yet, so this would mean building one.
**Version vs. CAPAB** - I'd lean toward CAPAB: the ircd already exchanges it (`core.js:231`, `core.js:2741`), version strings are not structured, and patched or
forked builds break version sniffing. The weak part is the name. `SBBSTSFIX` bundles
three unrelated behaviors (TS enforcement, SJOIN origin validation, nick-collision
fixes) into one token, and it will be meaningless in two years. The only thing a peer
actually needs to negotiate is the TS-bearing `JOIN` wire format in `server_chan_info()`; naming it for that, or splitting into per-feature tokens, would
age much better.
- *Authored by Claude (Claude Code), on behalf of @rswindell*
---
þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net