diff --git a/src/CommandRegistration.cpp b/src/CommandRegistration.cpp index 242a4a2..16a4fa0 100644 --- a/src/CommandRegistration.cpp +++ b/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 diff --git a/src/Server.cpp b/src/Server.cpp index f4a224d..504f98c 100644 --- a/src/Server.cpp +++ b/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 diff --git a/tests/MANUAL-TESTING.md b/tests/MANUAL-TESTING.md index 84a0978..324d6b6 100644 --- a/tests/MANUAL-TESTING.md +++ b/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. \ No newline at end of file diff --git a/tests/test_conformance.cpp b/tests/test_conformance.cpp index 703f844..086f751 100644 --- a/tests/test_conformance.cpp +++ b/tests/test_conformance.cpp @@ -11,6 +11,12 @@ * - UserModeCase.* : MODE 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 @@ -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 * ════════════════════════════════════════════════════════════════════ */ diff --git a/tests/tools/mute_client.py b/tests/tools/mute_client.py new file mode 100644 index 0000000..9d223b3 --- /dev/null +++ b/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 ": 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()