Fix 10 security and correctness bugs (auth bypass, SQL injection, TOCTOU, panics, negative uptime)#21
Merged
Conversation
- db: now_ts() uses unwrap_or_default() instead of panicking on pre-epoch clock - db: stream_find_by() validates column name against allowlist to prevent SQL injection - db: stream_delete() returns Option<bool> (Some(true)=deleted, Some(false)=not found, None=error) so callers can distinguish not-found from failure - db: stream_list/publisher_list/player_list/stat_recent use let-else on prepare() and .unwrap_or_default() on query_map() instead of panicking, preventing mutex poisoning - http: now_ts() uses unwrap_or_default() for same reason as db - http: bearer_ok() returns false (not true) when api_token is empty, closing auth bypass - http: uptime calculations clamp to .max(0) to prevent negative values when connected_at is slightly in the future due to clock skew - http: handle_stream_delete uses single stream_delete() call, eliminating TOCTOU race between stream_get() and stream_delete() across two separate mutex acquisitions - keygen: keygen_secret returns Result<String,String> instead of panicking on RNG failure - rtmp_bridge: gen_id uses 64-bit random alone (drops timestamp) to eliminate collision risk under concurrent connections sharing the same second - rtmp_bridge: on_close logs a warning for untracked connections instead of silently returning and leaking ghost rows - server: default DB path changed from /tmp (world-writable, symlink attack) to ./ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QjjkQ1isQqDyBxn6E8eFWq
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces panics and unwraps with fallible handling across key generation, database access, HTTP stream creation/deletion, and runtime helpers. It also tightens bearer auth, clamps uptime values, changes stream ID formatting, and requires an explicit SQLite database path. ChangesError Hardening and Runtime Behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/http.rs (2)
116-119: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd an empty-token auth regression test.
This is the bypass being fixed; the visible test covers a missing header with a non-empty token, but not
api_token == "". A request with any/missing bearer token should still return401.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http.rs` around lines 116 - 119, The auth check in bearer_ok should be covered with a regression test for the empty api_token bypass: add a test case around the bearer auth path in src/http.rs that sets AppState.config.api_token to an empty string and verifies requests with any bearer token and with no Authorization header both return 401. Reuse the existing bearer_ok behavior and the HTTP request handling/tests around it so the new test clearly exercises the empty-token branch.
418-430: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog key generation failures without exposing secrets.
The
_branch returns500but drops the RNG error, leaving operators no diagnostic. Capture eachErr(e)individually and log only the error/prefix context, not any successfully generated keys.Example refactor
- let (publish_key, play_key, stats_key) = match ( - keygen_secret("pub_"), - keygen_secret("pl_"), - keygen_secret("st_"), - ) { - (Ok(a), Ok(b), Ok(c)) => (a, b, c), - _ => { - return err_json( - StatusCode::INTERNAL_SERVER_ERROR, - "INTERNAL_ERROR", - "Key generation failed", - ) - } - }; + let publish_key = match keygen_secret("pub_") { + Ok(key) => key, + Err(e) => { + crate::log_error!("Publish key generation failed: {e}"); + return err_json(StatusCode::INTERNAL_SERVER_ERROR, "INTERNAL_ERROR", "Key generation failed"); + } + };Repeat for
play_keyandstats_key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http.rs` around lines 418 - 430, The key generation path in the match over keygen_secret("pub_"), keygen_secret("pl_"), and keygen_secret("st_") currently hides the underlying RNG failure, so update this branch to capture each Err(e) separately and log the failure with only the prefix/error context. Keep the existing err_json response from the surrounding logic, but use the local symbols publish_key, play_key, and stats_key handling to ensure no successfully generated key values are ever logged or exposed. Repeat the same error logging pattern for the play_key and stats_key branches so operators get diagnostics without secrets.src/db.rs (1)
282-288: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog DB failures before returning hardened fallbacks.
These paths now avoid panics, but commit/prepare/query/row-mapping errors are silently collapsed to
NoneorVec::new(). That can make a DB outage or schema mismatch look like “no streams/clients” in stats.Example pattern
- if tx.commit().is_ok() { - Some(rows > 0) - } else { - None - } + match tx.commit() { + Ok(()) => Some(rows > 0), + Err(e) => { + crate::log_error!("DB cascade delete commit error for {id}: {e}"); + None + } + }Apply the same pattern to
prepare,query_map, and row conversion failures before returning empty vectors.Also applies to: 298-308, 396-415, 507-526, 562-582
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db.rs` around lines 282 - 288, Log database failures before returning fallback values in the affected query paths: in the logic around the existing result handling, plus the `prepare`, `query_map`, and row-conversion code paths, capture and report the actual error before collapsing to `None` or `Vec::new()`. Use the existing DB helper methods and match arms in `src/db.rs` as the insertion points so commit/prepare/query/row-mapping failures are visible in logs while still returning hardened fallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server.rs`:
- Around line 26-29: The database path fallback in the server startup logic is
cwd-relative, which makes persistence depend on the working directory. Update
the db path handling in the server initialization code around the existing
LRTMP2_DB lookup so that it either requires LRTMP2_DB to be set and fails fast
when missing, or resolves to a stable absolute data directory instead of using
"./librtmp2-server.db". Keep the change localized to the db_path setup in
src/server.rs so the rest of the server and database initialization paths
continue to use the resolved path unchanged.
---
Nitpick comments:
In `@src/db.rs`:
- Around line 282-288: Log database failures before returning fallback values in
the affected query paths: in the logic around the existing result handling, plus
the `prepare`, `query_map`, and row-conversion code paths, capture and report
the actual error before collapsing to `None` or `Vec::new()`. Use the existing
DB helper methods and match arms in `src/db.rs` as the insertion points so
commit/prepare/query/row-mapping failures are visible in logs while still
returning hardened fallbacks.
In `@src/http.rs`:
- Around line 116-119: The auth check in bearer_ok should be covered with a
regression test for the empty api_token bypass: add a test case around the
bearer auth path in src/http.rs that sets AppState.config.api_token to an empty
string and verifies requests with any bearer token and with no Authorization
header both return 401. Reuse the existing bearer_ok behavior and the HTTP
request handling/tests around it so the new test clearly exercises the
empty-token branch.
- Around line 418-430: The key generation path in the match over
keygen_secret("pub_"), keygen_secret("pl_"), and keygen_secret("st_") currently
hides the underlying RNG failure, so update this branch to capture each Err(e)
separately and log the failure with only the prefix/error context. Keep the
existing err_json response from the surrounding logic, but use the local symbols
publish_key, play_key, and stats_key handling to ensure no successfully
generated key values are ever logged or exposed. Repeat the same error logging
pattern for the play_key and stats_key branches so operators get diagnostics
without secrets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 294180e1-3dfb-4012-88be-aa9923c703e3
📒 Files selected for processing (5)
src/db.rssrc/http.rssrc/keygen.rssrc/rtmp_bridge.rssrc/server.rs
- server: fail fast with a clear error when LRTMP2_DB is unset instead of falling back to a cwd-relative path that silently opens a different database each time the process is started from a different directory - http: log each keygen failure individually (publish/play/stats prefix + error) before returning 500, so operators have diagnostics without any key material being logged - http: add empty_api_token_always_denies test covering the api_token=="" bypass both with no Authorization header and with a bearer token — both must 401 - db: log before returning silent fallbacks; stream_delete commit failure, and prepare() failures in stream_list/publisher_list/player_list/stat_recent all now emit log_error! so a DB outage is visible in logs rather than appearing as empty results Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QjjkQ1isQqDyBxn6E8eFWq
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QjjkQ1isQqDyBxn6E8eFWq
Stream keys (publish_key, play_key, stats_key) stored in SQLite are sensitive; on multi-user hosts the default umask leaves the DB file world-readable (0644). Added restrict_db_file_permissions() which sets the DB file and its WAL/SHM siblings to owner-only (0600) immediately after Connection::open(). Added a test to verify the mode. Addresses the fix described in PR #20 applied on top of the latest main (which already has the bearer_ok empty-token fix from our PR #21). Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QjjkQ1isQqDyBxn6E8eFWq
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/db.rs`:
- Around line 145-161: `restrict_db_file_permissions` is trying to chmod every
database path, including special non-file values like `:memory:` that `Db::open`
can pass in during tests and runtime. Update `restrict_db_file_permissions` to
detect and skip non-files/URIs/empty paths before calling `fs::set_permissions`,
so `log_warn!` only runs for real on-disk databases when the main candidate
actually exists. Keep the existing WAL/SHM handling for valid file-backed paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
:memory: and file: URI paths have no backing file. Calling fs::set_permissions on them fails with NotFound and incorrectly triggered log_warn! during every unit test open, masking genuine permission failures on real database files. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QjjkQ1isQqDyBxn6E8eFWq
4 tasks
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security and correctness review of
librtmp2-server. Ten confirmed bugs fixed acrossdb.rs,http.rs,keygen.rs,rtmp_bridge.rs, andserver.rs.Security fixes
Auth bypass (
http.rsbearer_ok): Whenapi_tokenwas empty the function returnedtrue, making the entire REST API publicly accessible without a token. Changed toreturn false— an unconfigured token denies all access, forcing operators to set a real secret.SQL injection (
db.rsstream_find_by): Thecolumnparameter was interpolated directly into the SQL string viaformat!. Although all three call sites pass literal column names today, the function was private with no guard against future misuse. Added an explicit allowlist ("publish_key" | "play_key" | "stats_key") that returnsNoneand logs an error for any other value.World-writable default DB path (
server.rs): The fallback database path was/tmp/librtmp2-server.db, a world-writable directory where a local attacker could plant a symlink before first launch. Changed to./librtmp2-server.db.Correctness fixes
TOCTOU on stream delete (
http.rshandle_stream_delete): Existence was checked viastream_get()(lock Update to Mongoose 7.14 API and fix build configuration #1), then the record was deleted viastream_delete()(lock fix: build fixes for server, add docs #2). A concurrent delete between those two acquisitions could succeed silently. Fixed by extendingstream_delete()to returnOption<bool>(Some(true)= deleted,Some(false)= not found,None= DB error) so the handler makes a single atomic decision.Negative uptime (
http.rsbuild_json_stats/build_nginx_xml):now - p.connected_atcould be negative if the system clock moved backward orconnected_atwas stored slightly in the future. Added.max(0)on all four uptime calculations.gen_idcollision risk (rtmp_bridge.rs): IDs were{timestamp}_{32-bit random}, so two connections arriving in the same second shared the upper bits and had only 2³² possible IDs to avoid each other. Replaced with a single 64-bit random value ({:016x}) — no timestamp, 2⁶⁴ space.Untracked
on_close(rtmp_bridge.rs): Whenon_closefired for aConnIdnot in the map (e.g.on_connectwas never called, or the connection was already cleaned up), it silently returned. Added alog_warn!so ghost-connection scenarios are visible in logs.Panic prevention
now_ts()panics before UNIX_EPOCH (db.rs,http.rs):duration_since(UNIX_EPOCH).unwrap()panics if the system clock is before 1970. Changed to.unwrap_or_default()(returns 0).keygen_secretpanics on RNG failure (keygen.rs):.expect("OS RNG failure")would panic inside an async handler, crashing the Tokio worker. Changed the return type toResult<String, String>; callers inhandle_stream_createnow return HTTP 500 on failure..unwrap()on DBprepare()/query_map()(db.rsstream_list,publisher_list,player_list,stat_recent): A panic on prepare/query would poison theMutex<Connection>, making the database permanently inaccessible for the lifetime of the process. Replaced withlet Ok(mut stmt) = … else { return Vec::new(); }and.unwrap_or_default().Test plan
cargo test)stream_delete_cascades_active_publishersupdated toassert!(matches!(…, Some(true)))keygen::tests::keys_are_unique_and_well_formedupdated to unwrapResultGenerated by Claude Code
Summary by CodeRabbit
Bug Fixes / Security
0600(and sidecars) on Unix.Maintenance / Tests
LRTMP2_DB.