Harden server auth, deletes, and rate limiting#56
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates token seeding and documentation, changes health/stats/deletion HTTP behavior, adds stream-delete recovery state, alters rate-limiter eviction, adds RTMP auth-failure limiting, and changes publisher count-query failure handling. ChangesServer behavior updates
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 595a97e26a
ℹ️ 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".
| result | ||
| } | ||
|
|
||
| fn try_authorize_publish(&self, conn: ConnId, app: &str, stream_key: &str) -> Result<(), ()> { |
There was a problem hiding this comment.
This helper is declared inside impl RtmpEventHandler for DbRtmpBridge, but RtmpEventHandler only defines on_connect, authorize_publish, authorize_play, on_frame, and on_close. Rust rejects extra methods in a trait impl, so this module will not compile when the crate is built; make this an inherent impl DbRtmpBridge helper (and do the same for try_authorize_play) or inline it.
Useful? React with 👍 / 👎.
| fn evict_oldest_client(&self, guard: &mut HashMap<String, Vec<Instant>>) { | ||
| let Some(oldest_key) = guard | ||
| .iter() | ||
| .min_by_key(|(_, entries)| entries.first().copied().unwrap_or(Instant::now())) |
There was a problem hiding this comment.
When MAX_TRACKED_KEYS is full, this chooses the bucket with the oldest first request, not the least-recently-seen bucket. Because buckets are stored oldest-to-newest, a client with recent traffic but one older in-window entry can be evicted and have its counter reset when an attacker fills the client map, weakening the per-IP/path rate limit; use the newest retained timestamp instead.
Useful? React with 👍 / 👎.
| {}\n\ | ||
| Set LRTMP2_API_TOKEN in the panel .env to this value.\n\ | ||
| ============================================================", | ||
| mask_api_token(&candidate) |
There was a problem hiding this comment.
When LRTMP2_API_TOKEN is not provided, this masks the freshly generated API token before printing it. That token is immediately required for every admin endpoint and there is no unauthenticated retrieval path, so a first-time operator using the default startup path cannot configure the panel or make API calls without manually opening SQLite; keep the one-time generated secret visible or provide another retrieval mechanism.
Useful? React with 👍 / 👎.
|
|
||
| # Optional shared API bearer token (recommended for Docker). When set, the server | ||
| # stores this value in SQLite on first startup instead of generating a new one. | ||
| # LRTMP2_API_TOKEN=<generate-with-openssl-rand-hex-32> |
There was a problem hiding this comment.
Uncommenting this line in the copied .env will not seed the database: the config loader applies only the unprefixed server keys and resolve_api_token reads std::env::var("LRTMP2_API_TOKEN") instead of values parsed from .env. In the normal librtmp2-server startup that parses .env itself, the server will generate a different token and a panel configured with this value will receive 401s.
Useful? React with 👍 / 👎.
| db: Arc<Db>, | ||
| conns: Mutex<HashMap<ConnId, ConnState>>, | ||
| deleted_streams: Arc<Mutex<HashSet<String>>>, | ||
| auth_failures: Mutex<HashMap<ConnId, Vec<Instant>>>, |
There was a problem hiding this comment.
The new failure bucket is keyed only by ConnId, and on_close clears that bucket, so a client can avoid the RTMP auth limit by reconnecting between failed publish/play attempts. Since failed auth attempts are tied to short-lived RTMP connections, this does not slow brute-force attempts across sessions; use a stable peer/key scope if this is meant to protect RTMP credentials.
Useful? React with 👍 / 👎.
| std::env::set_var("LRTMP2_API_TOKEN", env_token); | ||
| let app = ServerApp::bootstrap(ServerConfig::default(), db_path_str).expect("bootstrap"); | ||
| std::env::remove_var("LRTMP2_API_TOKEN"); |
There was a problem hiding this comment.
This crate is edition 2024, where std::env::set_var and std::env::remove_var are unsafe APIs. This new test uses both as safe calls, so cargo test will fail to compile this target until the environment mutation is wrapped appropriately, ideally with serialization around the global process environment.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/db.rs (1)
828-834: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider applying the same explicit error logging to
player_try_acquire's COUNT query for consistency.This query still relies on a silent
unwrap_orfallback (falling back to the cap so it fails closed), unlike the newly-hardenedpublisher_try_acquireabove, which now logs and returnsfalseexplicitly on query failure. Not a correctness issue since both fail closed, but the inconsistency in error observability between the two acquire paths could be worth aligning.🤖 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 828 - 834, The COUNT query in player_try_acquire still uses a silent unwrap_or fallback, unlike publisher_try_acquire which now logs and returns false on query failure. Update player_try_acquire to handle the query_row error explicitly: log the database error with clear context using the same error-reporting style as publisher_try_acquire, then fail closed by returning false or the capped result as appropriate. Keep the change localized around the active count query and its surrounding logic so the two acquire paths behave consistently.src/http.rs (1)
773-787: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffTimeout path silently re-enables a stream after a
202 "deleting"was returned.If live connections don't drain within 30s,
wait_and_finalize_stream_deletere-enables the stream and drops it fromdeleted_streams. The client already received202 {status:"deleting"}and has no signal that the delete was abandoned—polling will show the stream alive again, which is surprising. Separately, thetokio::spawned finalizer is in-memory only: a process restart while a delete is pending leaves the stream disabled and stuck indeleted_streamswith no task to finalize or revert it. Consider a durable/reconciled path or at least documenting the timeout-revert contract.🤖 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 773 - 787, The timeout branch in wait_and_finalize_stream_delete currently re-enables the stream and removes it from deleted_streams after returning 202 deleting, which makes the delete appear to vanish; change the flow so a timed-out delete remains in a clear terminal/pending-failed state or is explicitly reconciled, rather than silently restoring the stream. Update finalize_stream_delete, deleted_streams handling, and the delete path in src/http.rs so clients can observe the abandoned delete consistently. Also address the in-memory tokio::spawn finalizer limitation by persisting enough delete state in AppState/db to recover after restart or document and enforce the timeout-revert contract explicitly.src/server.rs (1)
726-740: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGlobal env var mutation in a parallel test can leak into sibling tests.
std::env::set_var/remove_varmutate process-wide state;cargo testruns tests in parallel by default, and the siblingcreate_generates_api_token_on_first_starttest also callsServerApp::bootstrap, which readsLRTMP2_API_TOKEN. A concurrent run risks flaky cross-test contamination, and a panic betweenset_var/remove_var(e.g. a failedassert_eq!) leaves the var set for the rest of the process.Consider guarding these tests with a shared mutex/
serial_test, or refactoringresolve_api_tokento accept an injectable token source for testing instead of readingstd::envdirectly.🤖 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/server.rs` around lines 726 - 740, The `bootstrap_seeds_api_token_from_env_on_first_start` test mutates the process-wide `LRTMP2_API_TOKEN` env var, which can leak into other parallel tests that also call `ServerApp::bootstrap`. Update the test setup around `std::env::set_var`/`remove_var` to run serially or under a shared lock, or refactor `resolve_api_token`/`ServerApp::bootstrap` to accept an injectable token source so tests do not depend on global env state.
🤖 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/http.rs`:
- Around line 504-519: The unauthenticated health check in handle_health now
returns only the minimal status payload, so the e2e test
health_endpoint_reports_rtmp_port is asserting fields that are no longer
present. Update the test to either include Bearer authorization when calling
/api/v1/health so it can assert rtmp_port and the other detailed fields, or
change its expectation to match the unauthenticated {"status":"ok"} response.
In `@src/rate_limit.rs`:
- Around line 193-201: The rate limit eviction test is flaky because
`limited_key` ties with every other bucket on the same seed timestamp, and
`min_by_key` can evict it nondeterministically based on `HashMap` iteration
order. Update the setup in the `rate_limit` test so `limited_key` is clearly the
newest entry (for example by giving it a later timestamp than the others) before
calling `limiter.check`. Keep the existing assertions, but ensure the eviction
target is deterministic so `check(limited_key, max)` always remains rejected
after the full map eviction.
- Around line 45-55: The eviction logic in evict_oldest_client currently uses
entries.first(), which treats the first retained request as the activity
timestamp and can evict an actively used client bucket incorrectly. Update the
min_by_key comparison to use entries.last() so the HashMap<String, Vec<Instant>>
is ordered by each bucket’s most recent activity, matching the
least-recently-seen policy.
In `@src/rtmp_bridge.rs`:
- Around line 116-145: The per-connection auth throttling in rtmp_bridge.rs is
reset too easily because auth_failures is keyed by ConnId and cleared by
clear_auth_failures/on_close, allowing reconnects to bypass the window. Update
is_auth_rate_limited, record_auth_failure, and clear_auth_failures to track
failures by a stable client identity such as remote_addr (or another persistent
key) instead of ConnId, or otherwise keep the failure history alive for the full
RTMP_AUTH_FAILURE_WINDOW after disconnect so reconnects do not reset the limit.
In `@src/server.rs`:
- Around line 329-333: The validation error message in the
`is_valid_env_api_token` check is incomplete and should match the actual token
rules. Update the `Err(...)` text in `src/server.rs` where the token is rejected
so it explicitly says the token may contain ASCII alphanumeric characters plus
`-` and `_`, keeping the wording aligned with `is_valid_env_api_token` and the
surrounding API token validation logic.
- Around line 345-355: The bootstrap banner in key generation is currently
printing a masked value, which prevents the operator from copying the newly
generated API token. In the token setup flow around keygen_api_token() and
db.token_set(), either log the actual candidate once in this eprintln! path or
update the message to direct the operator to a different retrieval method, and
keep mask_api_token() only for non-sensitive display paths.
---
Nitpick comments:
In `@src/db.rs`:
- Around line 828-834: The COUNT query in player_try_acquire still uses a silent
unwrap_or fallback, unlike publisher_try_acquire which now logs and returns
false on query failure. Update player_try_acquire to handle the query_row error
explicitly: log the database error with clear context using the same
error-reporting style as publisher_try_acquire, then fail closed by returning
false or the capped result as appropriate. Keep the change localized around the
active count query and its surrounding logic so the two acquire paths behave
consistently.
In `@src/http.rs`:
- Around line 773-787: The timeout branch in wait_and_finalize_stream_delete
currently re-enables the stream and removes it from deleted_streams after
returning 202 deleting, which makes the delete appear to vanish; change the flow
so a timed-out delete remains in a clear terminal/pending-failed state or is
explicitly reconciled, rather than silently restoring the stream. Update
finalize_stream_delete, deleted_streams handling, and the delete path in
src/http.rs so clients can observe the abandoned delete consistently. Also
address the in-memory tokio::spawn finalizer limitation by persisting enough
delete state in AppState/db to recover after restart or document and enforce the
timeout-revert contract explicitly.
In `@src/server.rs`:
- Around line 726-740: The `bootstrap_seeds_api_token_from_env_on_first_start`
test mutates the process-wide `LRTMP2_API_TOKEN` env var, which can leak into
other parallel tests that also call `ServerApp::bootstrap`. Update the test
setup around `std::env::set_var`/`remove_var` to run serially or under a shared
lock, or refactor `resolve_api_token`/`ServerApp::bootstrap` to accept an
injectable token source so tests do not depend on global env state.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58144bda-e657-4442-890a-80cddf240a9e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.env.examplesrc/config.rssrc/db.rssrc/http.rssrc/rate_limit.rssrc/rtmp_bridge.rssrc/server.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 723d696cdc
ℹ️ 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".
| tokio::spawn(async move { | ||
| wait_and_finalize_stream_delete(state_bg, id_bg).await; | ||
| }); | ||
| (StatusCode::ACCEPTED, Json(json!({"status": "deleting"}))).into_response() |
There was a problem hiding this comment.
When deleting a stream with active RTMP sessions, the handler has already persisted enabled=0 and then returns 202 while relying on this detached task to either delete the row or re-enable it. If the process is shut down, redeployed, or crashes before the spawned task completes, Tokio drops the task and the next startup only clears active publisher/player rows, leaving the stream disabled in SQLite but not deleted; make the delete/re-enable outcome durable or recover pending deletes on startup before acknowledging the request.
Useful? React with 👍 / 👎.
- 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.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/server.rs (1)
763-770: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSerialize this test and make env cleanup panic-safe.
std::env::set_var/remove_varare process-global, and other tests insrc/server.rsalso callServerApp::bootstrapin the same test binary. Use the existingserial_testcrate (or a sharedMutex) and restore the prior value with an RAII guard so a panic doesn’t leakLRTMP2_API_TOKEN.🤖 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/server.rs` around lines 763 - 770, This test in ServerApp::bootstrap mutates the process-global LRTMP2_API_TOKEN without isolation or panic-safe cleanup. Serialize the test using the existing serial_test crate (or a shared Mutex) so it cannot run alongside other src/server.rs tests, and replace the manual set_var/remove_var pair with an RAII guard that captures and restores the previous env var value even if the test panics.
🤖 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/rtmp_bridge.rs`:
- Around line 615-650: The brute-force tracking in authorize_publish and
authorize_play is too broad because every Err(()) from
try_authorize_publish/try_authorize_play is treated as an auth failure. Update
the authorization flow to distinguish credential/app mismatches from operational
or capacity errors (such as deleted streams, capacity limits, keygen, or DB
acquisition failures), and only call record_auth_failure for true auth-related
rejections while still clearing failures on success.
- Around line 615-650: In authorize_publish and authorize_play, avoid using an
empty string as the fallback remote IP before on_connect has populated
ConnState. The shared "" bucket causes unrelated connections to share
auth-failure/rate-limit state and clear each other’s failures, so update the
remote_ip lookup from self.conns to use a per-connection placeholder or bypass
auth throttling until the real IP is available. Keep the fix localized around
authorize_publish, authorize_play, and the auth-failure helpers they call.
- Around line 136-165: The auth-failure cache in
rtmp_bridge::is_auth_rate_limited and record_auth_failure only removes expired
entries for the current IP, so unused buckets in auth_failures can accumulate
indefinitely. Add a global cleanup/prune step around the auth_failures lock
before inserting or when checking failures, and consider enforcing a maximum
bucket count so stale remote_ip entries are evicted even if they are never
revisited. Keep the existing per-IP expiry logic in is_auth_rate_limited, but
make the pruning apply across the whole auth_failures map.
- Around line 734-765: The test auth_rate_limit_survives_reconnect_from_same_ip
currently only checks authorize_publish returning Err, which does not prove
throttling because bogus keys fail for other reasons too. Update the test to
assert the actual rate-limit state via is_auth_rate_limited using the remote IP
for the relevant connection, or switch the final checks to a valid key so the
same IP is blocked while a different IP is not. Use the existing test_bridge,
authorize_publish, on_connect, and on_close flow to keep the setup the same.
---
Nitpick comments:
In `@src/server.rs`:
- Around line 763-770: This test in ServerApp::bootstrap mutates the
process-global LRTMP2_API_TOKEN without isolation or panic-safe cleanup.
Serialize the test using the existing serial_test crate (or a shared Mutex) so
it cannot run alongside other src/server.rs tests, and replace the manual
set_var/remove_var pair with an RAII guard that captures and restores the
previous env var value even if the test panics.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db3ff218-5646-43ed-b510-69e63c705cf8
📒 Files selected for processing (4)
.env.examplesrc/rate_limit.rssrc/rtmp_bridge.rssrc/server.rs
✅ Files skipped from review due to trivial changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- src/rate_limit.rs
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".
| let remote_ip = self | ||
| .conns | ||
| .lock() | ||
| .get(&conn) | ||
| .map(|cs| cs.remote_ip.clone()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
When the RTMP publish/play callback fires before on_connect has populated ConnState for this connection, this falls back to an empty peer key and records failures under "". Because this ordering is explicitly supported by the bridge, ten invalid early callbacks from any clients make is_auth_rate_limited("") reject subsequent early publish/play attempts from unrelated IPs for the 60-second window, turning the intended per-peer throttle into a global RTMP auth denial; skip accounting until the remote IP is known or attach the peer address before authorization.
Useful? React with 👍 / 👎.
| fn record_auth_failure(&self, remote_ip: &str) { | ||
| let mut guard = self.auth_failures.lock(); | ||
| let now = Instant::now(); | ||
| let entries = guard.entry(remote_ip.to_string()).or_default(); |
There was a problem hiding this comment.
Each failing RTMP peer creates an auth_failures bucket, but stale buckets are only pruned when that same peer fails or is checked again, and there is no global cap like the HTTP limiter has. On a long-lived public RTMP server, one-off failed auth attempts from many client IPs will leave permanent HashMap entries after their 60-second windows expire, so a scan can grow memory without bound; periodically purge all buckets or evict old peers before inserting a new key.
Useful? React with 👍 / 👎.
| for stream in db.stream_list() { | ||
| if stream.enabled { | ||
| continue; | ||
| } | ||
| match db.stream_delete(&stream.id) { |
There was a problem hiding this comment.
This recovery treats every persisted enabled=0 stream as an abandoned delete and removes it. If an operator or future admin path uses the existing enabled column to keep a stream configured but disabled (the API serializes this field and RTMP auth already rejects disabled streams without deleting them), the next server restart will silently cascade-delete that stream and its keys; track pending deletes separately instead of inferring them from enabled=false.
Useful? React with 👍 / 👎.
- Distinguish credential/app-mismatch auth failures (brute-forceable) from operational rejections (deleted/disabled stream, capacity, DB/keygen errors) so only the former count against the RTMP auth-failure rate limit. - Skip auth-failure throttling entirely when authorize_publish/play runs before on_connect has recorded a remote IP, instead of sharing a "" bucket across unrelated connections. - Bound auth_failures with a global prune + LRU eviction cap (MAX_TRACKED_AUTH_FAILURE_KEYS), mirroring the HTTP rate limiter. - Track pending stream deletes with a dedicated `pending_delete` column instead of inferring them from `enabled=0`, so an administratively-disabled-but-not-deleted stream is never cascade-deleted by the startup recovery pass. - Tighten the reconnect-throttle regression test to assert is_auth_rate_limited directly instead of only checking for an Err that a bogus key would produce either way.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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/rtmp_bridge.rs`:
- Around line 462-465: The publish rejection path in rtmp_bridge.rs is treating
disabled or delete-drain streams as Credential failures, which wrongly consumes
the shared auth-failure budget. Update the stream authorization logic around
stream_find_by_publish_key() to distinguish invalid keys from
disabled/pending-delete streams, and mirror try_authorize_play() by returning
AuthFailureKind::Operational for the disabled/delete-drain cases while keeping
true bad-key cases as Credential.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d1bbac6-1964-4812-ad91-76795f57d422
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/db.rssrc/rtmp_bridge.rssrc/server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server.rs
stream_find_by_publish_key() filters WHERE enabled=1, so a publisher retrying with a still-valid key against its own just-disabled or pending-delete stream fell into the "invalid publish_key" branch and was counted as a Credential auth failure — burning the shared per-IP brute-force budget for a rejection that has nothing to do with guessing a key. Add stream_find_by_publish_key_any() (same query without the enabled filter) and check `enabled` explicitly in try_authorize_publish, mirroring the existing check already present in try_authorize_play.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
2 participants
Summary
LRTMP2_API_TOKENon first startup and mask generated tokens in logs202), and safer viewer revocation/healthand stats responses; improve HTTP rate-limit LRU eviction and DB error handlingTest plan
cargo testin librtmp2-serverLRTMP2_API_TOKEN202then background cleanup)Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
/api/v1/healthnow returns a minimal unauthenticated response, and a richer Bearer-auth response with timestamps and RTMP/RTMPS port details./statsendpoints now include consistent response pacing and improved stats key handling.202with{status:"deleting"}and finalizes in the background)..env.exampleand clarifiedLRTMP2_API_TOKENseeding/persistence behavior; health/auth E2E tests updated accordingly.