Fix/nick truncate not reject

src/CommandRegistration.cpp

@@ -68,6 +68,17 @@ void Server::cmdNick(Client *client, const Message &msg)
 		return;
 	}
 
+	/* Truncate rather than reject past NICKLEN, like real ircds -- HexChat's
+	** own retry suffixes only make an over-long nick longer, so rejecting it
+	** leaves the client unable to connect at all. Character validation above
+	** already ran on the untruncated string, so a nick that's only invalid
+	** past position 9 still gets 432. This must happen before the collision
+	** check below: two different over-long nicks can truncate to the same
+	** name, and isNickInUse must compare the truncated form so the second
+	** one gets 433 instead of silently colliding. */
+	if (nick.size() > MAX_NICKLEN)
+		nick.erase(MAX_NICKLEN);
+
 	/* Check if the nickname is taken (CASEMAPPING=ascii: "Bob" collides
 	** "bob"). Uses isNickInUse(), not findClientByNick(): a connection that
 	** has sent NICK but not yet PASS/USER still owns the name, and must

src/Server.cpp

@@ -740,7 +740,7 @@ const std::string &Server::getServerName() const { return _serverName; }
 
 bool Server::isValidNickname(const std::string &nick) const
 {
-	if (nick.empty() || nick.size() > MAX_NICKLEN)
+	if (nick.empty())
 		return false;
 
 	// First char must be letter or special

tests/MANUAL-TESTING.md

@@ -67,8 +67,15 @@ each write:
 
 **Two traps worth knowing before you start:**
 
-1. `nc` stays connected after its stdin ends. A session's lifetime is *not* the sum of its
-   sleeps. When timing matters, print a timestamp per line:
+1. **`nc` only exits when its stdin is at EOF *and* the socket is closed.** This is the
+   single most misleading thing about using `nc` as a test client. If a `sleep` holds stdin
+   open, the server can close the connection and `nc` will keep running, waiting for input —
+   so the moment you observe the process end is the end of your `sleep`, not the server's
+   decision. This produced a false bug report that survived several rounds of review.
+
+   When the *timing* of a disconnect matters, do not use `nc`. Use a client that detects
+   `recv() == 0` and timestamps it (see 7.3). When timing does not matter, stamp each line
+   anyway:
    ```bash
    ... | nc 127.0.0.1 6667 | while IFS= read -r l; do echo "$(date +%T) $l"; done
    ```
@@ -132,25 +139,25 @@ Expect `451 :You have not registered` (or equivalent) rather than a successful j
 
 ## 2. Nickname handling
 
-### 2.1 Long nickname ⚠️ known bug
+### 2.1 Long nickname
 
-*Checks what happens when a nickname exceeds `NICKLEN`.*
+*Confirms a nickname exceeding `NICKLEN` truncates instead of being rejected.*
 
 ```bash
 { printf 'PASS test123\r\nNICK abcdefghij\r\nUSER x 0 * :X\r\n'; sleep 3; } | nc 127.0.0.1 6667
 ```
 
-**Current behaviour:** `432 Erroneous nickname`. **A real server truncates instead.**
+Expect a normal registration burst (`001`-`005`), with the nick truncated to
+`abcdefghi` (9 chars) -- silently, no notice or numeric about the truncation.
 
-This matters more than it looks. HexChat's fallback appends suffixes (`_a`, `_1`) which make
-the nickname *longer*, so all three retries fail and the connection dies. Any evaluator whose
-default nickname is 10+ characters cannot use the server at all.
-
-Workaround until fixed: set a nickname of 9 characters or fewer.
+This was previously a known bug: HexChat's own collision-retry suffixes (`_`, `_1`, ...) only
+make an over-long nick *longer*, so when the server rejected it with `432` instead of
+truncating, all three retries failed and the connection died. Fixed in T1 (truncate to
+NICKLEN, matching real ircds' behaviour).
 
 ### 2.2 Underscore in nickname
 
-*Confirms the rejection above is about length, not character set.*
+*Confirms the length truncation above is about length, not character set.*
 
 ```bash
 { printf 'PASS test123\r\nNICK bo_b\r\nUSER x 0 * :X\r\n'; sleep 3; } | nc 127.0.0.1 6667
@@ -467,31 +474,40 @@ every 30 seconds, so anything in the 120–150 s range is correct).
 The server's `PING` should arrive ~120 s after the **client's last line**, not 120 s after
 registration. The server must also answer the client's `PING` with `PONG ... :keep`.
 
-### 7.3 Timeout disconnect ⚠️ known bug
+### 7.3 Timeout disconnect
+
+*Checks that a registered client which stops responding is eventually dropped.*
 
-*Compare two clients that differ in exactly one thing: whether stdin stays open.*
+**Do not test this with `nc`** — see the warning in the setup section. `nc` will not exit
+when the server closes the connection if its stdin is still open, so you will measure your
+own `sleep` instead of the server. Use a client that reports `recv() == 0` with a timestamp:
 
 ```bash
-# A: stdin held open — client's socket is fully open
-{ printf 'PASS test123\r\nNICK w1\r\nUSER w1 0 * :W\r\n'; sleep 600; } \
-  | nc 127.0.0.1 6667 \
-  | { while IFS= read -r l; do echo "$(date +%T) $l"; done; \
-      echo "$(date +%T) --- socket closed ---"; }
+python3 mute_client.py 127.0.0.1 6667 test123 mute1 400
+```
 
-# B: stdin ends immediately — client half-closes its socket
-printf 'PASS test123\r\nNICK w2\r\nUSER w2 0 * :W\r\n' \
-  | nc 127.0.0.1 6667 \
-  | { while IFS= read -r l; do echo "$(date +%T) $l"; done; \
-      echo "$(date +%T) --- socket closed ---"; }
+`mute_client.py` performs exactly one `sendall` (the registration) and never writes again.
+The socket stays fully open — no `shutdown`, no `close` — so this is a genuinely silent
+client rather than one that has signalled it is going away.
+
+Expected output:
+
+```
+t+  0.0s  registration sent
+t+144.1s  :ft_irc PING :ft_irc
+t+264.3s  *** SERVER CLOSED THE CONNECTION ***
 ```
 
-**Correct behaviour:** both are dropped ~240 s after their last activity.
+Correct behaviour: a `PING` around 120 s after the last activity (130-150 s is fine, the
+idle sweep runs every 30 s), then a close 120 s after that `PING`.
 
-**Current behaviour:** only B is dropped. A receives the `PING`, never answers, and stays
-connected indefinitely — leaking a file descriptor. Both receive their `PING` in the same
-second, so detection works; the teardown is what does not complete.
+If you want to confirm the client really is silent, run it under `strace`:
 
----
+```bash
+strace -f -e trace=sendto,write,sendmsg,shutdown -tt -o /tmp/mute.trace \
+  python3 mute_client.py
+grep -c 'sendto\|sendmsg' /tmp/mute.trace
+```
 
 ## 8. Limits
 
@@ -597,12 +613,13 @@ swallows or rewrites input, and a reply to a command you did not send proves not
 
 | # | Issue | Severity | Test |
 |---|---|---|---|
-| 1 | Nicknames over 9 characters are rejected instead of truncated; HexChat cannot connect | **High** | 2.1 |
-| 2 | Idle timeout never drops a client whose socket is fully open | Medium | 7.3 |
-| 3 | `324`/`329` sent twice on join (HexChat de-duplicates them on screen) | Low | 3.1 |
-| 4 | `CHANMODES` puts `l` in the wrong group; should be `,k,l,it` | Cosmetic | 1.1 |
-| 5 | Channel name capitalisation differs between the `JOIN` echo and the numerics | Cosmetic | 3.2 |
-| 6 | Default `KICK` reason uses the kicker's nick; real servers use the kicked user's | Cosmetic | 3.5 |
-| 7 | No `~` prefix on the username when there is no ident response | Cosmetic | 1.1 |
-
-Everything else in this document has been verified as behaving correctly.
+| 1 | ~~Nicknames over 9 characters are rejected instead of truncated; HexChat cannot connect~~ — fixed: now truncated to NICKLEN (T1) | Fixed | 2.1 |
+| 2 | `324`/`329` sent twice on join (HexChat de-duplicates them on screen) | Low | 3.1 |
+| 3 | `CHANMODES` puts `l` in the wrong group; should be `,k,l,it` | Cosmetic | 1.1 |
+| 4 | Channel name capitalisation differs between the `JOIN` echo and the numerics | Cosmetic | 3.2 |
+| 5 | Default `KICK` reason uses the kicker's nick; real servers use the kicked user's | Cosmetic | 3.5 |
+| 6 | No `~` prefix on the username when there is no ident response | Cosmetic | 1.1 |
+
+Everything else in this document has been verified as behaving correctly — including the
+idle timeout, which was reported as broken for a while and turned out to be a measurement
+artifact of using `nc` as the test client.

tests/test_conformance.cpp

@@ -11,6 +11,12 @@
  *   - UserModeCase.*       : MODE <nick> compared the target with operator!=
  *                            instead of ircEquals, the one place in the tree
  *                            that bypassed CASEMAPPING=ascii.
+ *   - NickTruncation.*     : isValidNickname() rejected any nick over
+ *                            MAX_NICKLEN with 432 instead of truncating,
+ *                            and (if fixed naively, truncating after the
+ *                            isNickInUse check) two different over-long
+ *                            nicks could truncate to the same name and
+ *                            both register.
  */
 
 #include <gtest/gtest.h>
@@ -491,6 +497,86 @@ TEST_F(ConformanceTest, UnregisteredConnectionStillReservesItsNick)
 		<< "a nick claimed by an unregistered connection was handed out twice";
 }
 
+/* ════════════════════════════════════════════════════════════════════════
+ * Suite: NickTruncation — over-long nicks truncate to NICKLEN, not reject
+ * ════════════════════════════════════════════════════════════════════ */
+
+TEST_F(ConformanceTest, OverlongNickTruncatesInsteadOfRejecting)
+{
+	/* HexChat's own collision retries (_ , _1, ...) only make an over-long
+	 * nick longer, so rejecting it with 432 leaves the client unable to
+	 * connect at all. A real ircd truncates to NICKLEN silently. */
+	TestClient tc;
+	ASSERT_TRUE(tc.connect(serverPort));
+
+	tc.registerClient("testpass", "abcdefghij", "tuser");
+	std::string reply = tc.recvAll(300);
+
+	EXPECT_TRUE(tc.hasNumeric(reply, "001"))
+		<< "a 10-char valid nick must register, truncated, not be rejected";
+	EXPECT_EQ(reply.find("abcdefghij"), std::string::npos)
+		<< "the untruncated 10-char nick must never appear in a reply";
+	EXPECT_NE(reply.find("abcdefghi!"), std::string::npos)
+		<< "the welcome prefix must carry the nick truncated to 9 chars";
+}
+
+TEST_F(ConformanceTest, InvalidCharacterPastNicklenStillRejects)
+{
+	/* Character validation must run on the full, untruncated nick: if
+	 * truncation happened first, an invalid char past position 9 would
+	 * never be seen and the nick would wrongly register. */
+	TestClient tc;
+	ASSERT_TRUE(tc.connect(serverPort));
+
+	tc.registerClient("testpass", "abcdefghi#junk", "tuser");
+	std::string reply = tc.recvAll(300);
+
+	EXPECT_TRUE(tc.hasNumeric(reply, ERR_ERRONEUSNICKNAME))
+		<< "an invalid char at position 10 must still be rejected, not "
+		   "silently dropped by truncation";
+	EXPECT_FALSE(tc.hasNumeric(reply, "001"))
+		<< "a nick invalid past NICKLEN must not register";
+}
+
+TEST_F(ConformanceTest, TwoNicksTruncatingToTheSameNameCollide)
+{
+	/* The core risk from the audit: if truncation happened after the
+	 * collision check, two different over-long nicks that truncate to the
+	 * same 9-char name would both register, breaking nick uniqueness. */
+	TestClient first, second;
+	ASSERT_TRUE(first.connect(serverPort));
+	ASSERT_TRUE(second.connect(serverPort));
+
+	first.registerClient("testpass", "abcdefghiONE", "u1");
+	EXPECT_TRUE(first.hasNumeric(first.recvAll(300), "001"));
+
+	second.registerClient("testpass", "abcdefghiTWO", "u2");
+	std::string reply = second.recvAll(300);
+
+	EXPECT_TRUE(second.hasNumeric(reply, ERR_NICKNAMEINUSE))
+		<< "two nicks truncating to the same 9-char name must collide, "
+		   "not silently both register";
+	EXPECT_FALSE(second.hasNumeric(reply, "001"));
+}
+
+TEST_F(ConformanceTest, TruncatedNickNeverExceedsAdvertisedNicklen)
+{
+	/* Cross-check against the 005 NICKLEN token the server itself
+	 * advertises -- the truncated nick must actually honour it. */
+	TestClient tc;
+	ASSERT_TRUE(tc.connect(serverPort));
+
+	tc.registerClient("testpass", "abcdefghijklmno", "tuser");
+	std::string reply = tc.recvAll(300);
+
+	ASSERT_NE(reply.find("NICKLEN=9"), std::string::npos)
+		<< "005 must still advertise NICKLEN=9";
+	EXPECT_NE(reply.find("abcdefghi!"), std::string::npos)
+		<< "the registered nick must be truncated to exactly NICKLEN chars";
+	EXPECT_EQ(reply.find("abcdefghij"), std::string::npos)
+		<< "no more than NICKLEN characters of the requested nick may survive";
+}
+
 /* ════════════════════════════════════════════════════════════════════════
  * Suite: ReplyWellFormedness — required numerics and canonical names
  * ════════════════════════════════════════════════════════════════════ */

tests/tools/mute_client.py

@@ -0,0 +1,109 @@
+#!/usr/bin/env python3
+"""
+T2 — Silent client with a fully open socket.
+
+Purpose: decide whether a registered client that sends nothing is dropped by the
+server's idle timeout, without any possibility of "phantom input" from the test tool.
+
+This client performs EXACTLY ONE sendall() and never writes again. The socket stays
+fully open (no shutdown, no close) until the server closes it or the run ends.
+
+Usage:
+    python3 mute_client.py [host] [port] [password] [nick] [seconds]
+Defaults:
+    127.0.0.1 6667 test123 mute1 400
+
+What to look for:
+  - Timestamped server lines, in particular how many PINGs arrive.
+  - Whether "SERVER CLOSED THE CONNECTION" appears, and at what elapsed time.
+
+Interpretation:
+  - Closed at ~240s (+/- the 30s sweep)  -> timeout works; T2 was an nc artifact.
+  - One PING, never closed               -> server bug: detection fires, teardown
+                                            never completes for silent open sockets.
+  - Repeated PINGs, never closed         -> something IS resetting the activity
+                                            timer (but it is not this client).
+"""
+
+import socket
+import sys
+import time
+
+host = sys.argv[1] if len(sys.argv) > 1 else "127.0.0.1"
+port = int(sys.argv[2]) if len(sys.argv) > 2 else 6667
+password = sys.argv[3] if len(sys.argv) > 3 else "test123"
+nick = sys.argv[4] if len(sys.argv) > 4 else "mute1"
+duration = float(sys.argv[5]) if len(sys.argv) > 5 else 400.0
+
+registration = (
+    "PASS {p}\r\n"
+    "NICK {n}\r\n"
+    "USER {n} 0 * :M\r\n"
+).format(p=password, n=nick).encode()
+
+
+def stamp(elapsed):
+    return "{clock}  t+{el:6.1f}s".format(
+        clock=time.strftime("%H:%M:%S"), el=elapsed
+    )
+
+
+sock = socket.create_connection((host, port))
+start = time.time()
+
+# The one and only write this process will ever perform on this socket.
+sock.sendall(registration)
+bytes_sent = len(registration)
+print("{s}  >>> sent registration ({b} bytes). This client will not write again."
+      .format(s=stamp(0.0), b=bytes_sent), flush=True)
+
+sock.settimeout(1.0)
+ping_count = 0
+closed_at = None
+
+try:
+    while True:
+        elapsed = time.time() - start
+        if elapsed >= duration:
+            print("{s}  --- run finished, still connected ---".format(s=stamp(elapsed)),
+                  flush=True)
+            break
+        try:
+            data = sock.recv(4096)
+        except socket.timeout:
+            continue
+        except OSError as exc:
+            print("{s}  --- socket error: {e} ---".format(s=stamp(elapsed), e=exc),
+                  flush=True)
+            closed_at = elapsed
+            break
+
+        elapsed = time.time() - start
+        if not data:
+            print("{s}  *** SERVER CLOSED THE CONNECTION ***".format(s=stamp(elapsed)),
+                  flush=True)
+            closed_at = elapsed
+            break
+
+        for line in data.decode("utf-8", "replace").split("\r\n"):
+            if not line:
+                continue
+            print("{s}  {l}".format(s=stamp(elapsed), l=line), flush=True)
+            # A server PING starts with "PING" or ":<server> PING"
+            parts = line.split()
+            if parts and (parts[0].upper() == "PING"
+                          or (len(parts) > 1 and parts[1].upper() == "PING")):
+                ping_count += 1
+finally:
+    print("", flush=True)
+    print("=== SUMMARY ===", flush=True)
+    print("bytes written by this client after connect: {b} (registration only)"
+          .format(b=bytes_sent), flush=True)
+    print("server PINGs received: {c}".format(c=ping_count), flush=True)
+    if closed_at is None:
+        print("server closed connection: NO (ran for {d:.0f}s)".format(d=duration),
+              flush=True)
+    else:
+        print("server closed connection: YES, at t+{c:.1f}s".format(c=closed_at),
+              flush=True)
+    sock.close()