Fix PR #56 review findings (rate limiting, auth throttling, token bootstrap, delete recovery)#57
Conversation
- Rate limiter LRU eviction now keys on the most recent request per bucket (entries.last()) instead of the oldest, so active clients can't be evicted ahead of stale ones; fix the matching test to give the protected bucket an unambiguously newer timestamp. - Print the freshly generated API token in the first-startup banner instead of the masked form, since there is no other way for an operator to retrieve it. - Clarify that LRTMP2_API_TOKEN in .env.example is not read from the file by the config loader and must be a real process env var. - Key RTMP auth-failure rate limiting by client IP instead of ConnId, and stop clearing the failure history on connection close, so reconnecting no longer resets the brute-force window. - Recover streams left disabled-but-undeleted by a crash mid-delete (the async 202 delete path) by finishing the delete on startup. - Fix the LRTMP2_API_TOKEN validation error message to mention the allowed '-'/'_' characters.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
AlexanderWagnerDev
merged commit 06176b0
into
cursor/security-review-fixes
![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: 06176b0bf8
ℹ️ 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".
| .lock() | ||
| .get(&conn) | ||
| .map(|cs| cs.remote_ip.clone()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
When librtmp2 invokes a publish/play callback during server.poll() before process_server_connections has called on_connect (an ordering this file already accounts for in on_connect_preserves_prior_authorize_publish_state), this fallback uses "" as the remote IP. Failed first-poll auth attempts are then recorded under the empty key, so ten bad attempts can cause is_auth_rate_limited("") to reject unrelated clients whose first auth callback arrives before their address is recorded, while the failures are not attributed to the attacker's actual IP.
Useful? React with 👍 / 👎.
| let mut guard = self.auth_failures.lock(); | ||
| let now = Instant::now(); | ||
| let entries = guard.entry(conn).or_default(); | ||
| let entries = guard.entry(remote_ip.to_string()).or_default(); |
There was a problem hiding this comment.
For an internet-exposed RTMP listener that receives failed auth attempts from many unique client IPs, each first failure inserts a new auth_failures key, but expired keys are only removed when that same IP is checked again and on_close no longer clears them. A scan or botnet can therefore grow this map indefinitely with one failed connection per address, unlike the previous per-connection state that was removed on close.
Useful? React with 👍 / 👎.
| if stream.enabled { | ||
| continue; | ||
| } | ||
| match db.stream_delete(&stream.id) { |
There was a problem hiding this comment.
If a database contains streams intentionally stored with enabled=0 (the schema/API model exposes enabled as the stream's active/inactive state), this recovery path treats them all as abandoned async deletes and calls the cascading stream_delete, removing the stream plus dependent stats/session rows on the next server restart. Because enabled is not a dedicated pending-delete marker, startup recovery should only finalize rows known to be mid-delete rather than all inactive streams.
Useful? React with 👍 / 👎.
2 participants
Summary
Addresses the outstanding review findings left on #56 by
chatgpt-codex-connectorandcoderabbitai:src/rate_limit.rs): eviction now keys on the most recent request per client bucket (entries.last()) instead of the oldest, so an actively-used bucket can't be evicted ahead of a genuinely stale one. Fixed the matching test (active_clients_are_evicted_via_lru_when_map_is_at_capacity) to give the protected bucket an unambiguously newer timestamp so it isn't flaky underHashMapiteration-order ties.src/rtmp_bridge.rs): failures are now tracked per client IP instead of perConnId, and the history is no longer cleared onon_close. Previously a client could reconnect (newConnId) between failed publish/play attempts and reset the brute-force window entirely. Added a regression test (auth_rate_limit_survives_reconnect_from_same_ip).src/server.rs): the first-startup banner now prints the actual generated API token instead of a masked value — there was no other way for an operator to retrieve it. Also fixed theLRTMP2_API_TOKENvalidation error message to mention the allowed'-'/'_'characters..env.example: clarified thatLRTMP2_API_TOKENis intentionally not read from the.envfile by the config loader and must be set as a real process environment variable.src/server.rs): if the process crashes/restarts while a202background delete is still draining RTMP sessions, the stream row was leftenabled=0forever. Startup now finishes any such pending deletes, since a fresh process has no surviving sessions from the prior run.Test plan
cargo test --lib(66 passed)cargo clippy --all-features --all-targets(clean)cargo fmt --checkGenerated by Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.