Repository navigation
Migrate line protocol and nickname support - #11
TonyTown6033 wants to merge 2 commits into
Conversation
…patcher Migrate minimal client command dispatcher, good job
Port the stable text protocol from feature to main: - line-based client commands MSG and NICK - server responses SYS, MSG, NICK, YOU and ERR - access-token authentication on connect - server-side nickname validation (length, charset, case-insensitive uniqueness) - per-client rate limiting and temporary IP bans - safe rejection of malformed input (empty messages, unknown commands, oversized lines) - parser/validation tests and PROTOCOL.md documentation File transfer, remote execution, LLM and chat-history changes are intentionally excluded.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37717ea2e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| )?; | ||
|
|
||
| println!("get buffer {buffer}"); | ||
| let (outbound_tx, outbound_rx) = mpsc::channel::<Outbound>(); |
There was a problem hiding this comment.
Once an authenticated peer stops reading, its writer thread can block on the TCP send buffer while send_to continues to enqueue every broadcast on this unbounded channel. A client that authenticates and then does not read can therefore make normal chat traffic retain an unbounded number of Strings and exhaust server memory; use a bounded queue and disconnect or drop output for lagging peers.
Useful? React with 👍 / 👎.
| return Err(()); | ||
| loop { | ||
| let mut line = String::new(); | ||
| match reader.read_line(&mut line) { |
There was a problem hiding this comment.
read_line appends until it sees a newline before the following branch tests line.len(), so a client can send an arbitrarily large line, or continuously omit its newline, and force this connection thread to allocate it. The advertised 4 KiB limit therefore does not protect server memory; enforce the byte limit while reading, before extending the String.
Useful? React with 👍 / 👎.
| ctx.stream = None; | ||
| pending.clear(); | ||
| } | ||
| Ok(n) => match from_utf8(&buffer[..n]) { |
There was a problem hiding this comment.
TCP can split a valid UTF-8 MSG or SYS frame at any byte. When a read ends inside a multibyte character, this conversion fails and the error path discards the entire chunk rather than retaining it for the next read, so ordinary Unicode chat messages can lose their prefix or never render; buffer raw bytes through a newline before decoding a complete frame.
Useful? React with 👍 / 👎.
| client.last_message_at = Some(now); | ||
| if too_quick { | ||
| client.rate_limit_violations += 1; | ||
| send_to(client, "ERR you are sending messages too quickly"); |
There was a problem hiding this comment.
The limiter runs before command parsing, so a user can be rate-limited and eventually banned merely by rapidly retrying /nickname, an invalid request, or an unknown command, even though none is a chat MSG. This contradicts the documented per-message limit and makes nickname correction fail immediately after another command; parse first and rate-limit only message sends.
Useful? React with 👍 / 👎.
1875b35 to
a220c83
Compare
2 participants
Summary
featuretomainMSG,NICKSYS,MSG,NICK,YOU,ERRERR/connect <ip> <port> [token]), add/nickname, and parse line-based server responsesPROTOCOL.mdOut of scope
PUT/GET/LS), remote execution (EXEC) and LLM (LLM)Validation
cargo fmt --all -- --checkcargo check --all-targetscargo test --all-targets(9 tests)Closes #4