Conversation
📝 WalkthroughWalkthroughAdds three GitHub Actions workflows (HBBS Windows build, CodeX code review automation, and build triggers) and enhances the Rust peer server with a socket address lookup method and outbound peer whitelist validation during punch-hole requests. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
🧹 Nitpick comments (2)
src/peer.rs (1)
181-189: Avoid O(n) reverse lookups on the punch-hole hot path.This linearly scans every in-memory peer and keeps the map read lock alive while awaiting each peer lock. Now that whitelist enforcement calls it on the punch-hole path, lookup cost and writer contention scale with peer count. A reverse
SocketAddr -> idindex updated alongsidesocket_addrwould keep this O(1).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/peer.rs` around lines 181 - 189, get_id_by_socket_addr is doing an O(n) scan holding map.read() and awaiting peer.read(), which is too slow on the punch-hole path; add a reverse index (e.g. a HashMap<SocketAddr, String> field on the same struct) and use it in get_id_by_socket_addr to return the id in O(1) without iterating. Update that reverse index whenever a peer's socket_addr is set/changed/removed (the same places that mutate peer.socket_addr or modify self.map) under the same write lock or transaction so the index stays consistent with the existing map; then change get_id_by_socket_addr to read the reverse index (under a read lock) instead of iterating over map and awaiting peer locks..github/workflows/HBBS.yaml (1)
16-32: Avoid duplicating the Windows hbbs build.
.github/workflows/build.yamlalready has abuild-winjob that produceshbbs.exe(among other artifacts). Running a second Windows build here with different runner and toolchain pins (windows-latest+ stable vswindows-2022+ 1.90) doubles CI time on every push/PR to master and makes it easy for the two build paths to drift. Consolidate to a single Windows build workflow or ensure both use the same runner and toolchain pins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/HBBS.yaml around lines 16 - 32, The workflow defines a duplicate Windows build job (job id build-windows / name "Build hbbs.exe (Windows)" that runs cargo build --release --target x86_64-pc-windows-msvc --bin hbbs) which duplicates the existing build-win job in .github/workflows/build.yaml; either remove this job from HBBS.yaml so the single build-win job produces hbbs.exe, or change build-windows to match the same runner and toolchain pins as build-win (runner, toolchain version) and ensure only one job uploads the hbbs.exe artifact to avoid doubling CI time and drift between the two Windows paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yaml:
- Around line 16-21: The workflow currently triggers publish-related jobs on
pull_request which causes docker/build-push-action to run without credentials;
for each publish job (docker, docker-manifest, docker-classic,
docker-manifest-classic, release, deb-package) add an explicit job-level
condition if: github.event_name != 'pull_request' (or remove them from this
workflow and put them into a separate workflow that only triggers on
push/release) so the docker/build-push-action steps never execute during PR
events.
In @.github/workflows/codex.yml:
- Around line 10-29: The workflow should skip runs from forked PRs and reduce
permissions: in the codex job (job id "codex") add a guard such as if: ${{
github.event.pull_request.head.repo.fork == false }} or if: ${{
secrets.OPENAI_API_KEY != '' }} to prevent runs when the secret is unavailable,
and change the job permissions to only contents: read (remove contents: write)
and drop pull-requests: write; keep the step using openai/codex-action@v1
unchanged but rely on the guard to prevent failing on forked PRs.
In `@src/rendezvous_server.rs`:
- Around line 133-143: The current use of
fs::read_to_string("whitelist.txt").unwrap_or_default() hides missing/unreadable
whitelist errors and creates an empty allowlist; change this to fail fast:
replace the unwrap_or_default usage around fs::read_to_string with explicit
error handling that logs a clear error including the filename and the io::Error
and then exits/returns an Err (e.g., log::error!("Failed to read whitelist.txt:
{}", err); std::process::exit(1)) so outbound_whitelist is only constructed when
the file is successfully read; reference the outbound_whitelist variable and the
read_to_string call when making this change.
- Around line 717-746: The current socket-address based auth using
get_id_by_socket_addr(addr) rejects legitimate TCP/WS callers because addr is a
transient transport endpoint; instead retrieve the authenticated peer/session ID
from the request context/session (the value used for authentication earlier in
your connection handling) and use that ID for whitelist checks against
outbound_whitelist and for constructing the PunchHoleResponse; replace the
get_id_by_socket_addr(addr) call and the addr-based log messages with logic that
reads the session/authenticated peer ID (e.g. a variable like session_id or
auth_peer_id produced during handshake) and if missing return the same
PunchHoleResponse but with logs and failure reason keyed to
missing/unauthenticated session rather than socket addr.
---
Nitpick comments:
In @.github/workflows/HBBS.yaml:
- Around line 16-32: The workflow defines a duplicate Windows build job (job id
build-windows / name "Build hbbs.exe (Windows)" that runs cargo build --release
--target x86_64-pc-windows-msvc --bin hbbs) which duplicates the existing
build-win job in .github/workflows/build.yaml; either remove this job from
HBBS.yaml so the single build-win job produces hbbs.exe, or change build-windows
to match the same runner and toolchain pins as build-win (runner, toolchain
version) and ensure only one job uploads the hbbs.exe artifact to avoid doubling
CI time and drift between the two Windows paths.
In `@src/peer.rs`:
- Around line 181-189: get_id_by_socket_addr is doing an O(n) scan holding
map.read() and awaiting peer.read(), which is too slow on the punch-hole path;
add a reverse index (e.g. a HashMap<SocketAddr, String> field on the same
struct) and use it in get_id_by_socket_addr to return the id in O(1) without
iterating. Update that reverse index whenever a peer's socket_addr is
set/changed/removed (the same places that mutate peer.socket_addr or modify
self.map) under the same write lock or transaction so the index stays consistent
with the existing map; then change get_id_by_socket_addr to read the reverse
index (under a read lock) instead of iterating over map and awaiting peer locks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 517547a8-1f63-4cb0-8828-43b92460f5c2
📒 Files selected for processing (5)
.github/workflows/HBBS.yaml.github/workflows/build.yaml.github/workflows/codex.ymlsrc/peer.rssrc/rendezvous_server.rs
| pull_request: | ||
| branches: | ||
| - master | ||
| push: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect: pull_request is enabled, login steps are PR-gated, but publish steps still push.
echo "Trigger section:"
nl -ba .github/workflows/build.yaml | sed -n '14,25p'
echo
echo "Publish-related jobs/steps:"
rg -n -C2 "github\.event_name != 'pull_request'|push:\s*true|action-gh-release|docker-manifest-action" .github/workflows/build.yamlRepository: rustdesk/rustdesk-server
Length of output: 8425
🏁 Script executed:
#!/bin/bash
# Get the exact trigger section
echo "=== Trigger section (lines 14-25) ==="
sed -n '14,25p' .github/workflows/build.yaml
echo -e "\n=== Job definitions (first job to understand structure) ==="
# Find job names and their triggers/gates
rg -n "^ [a-z-]+:" .github/workflows/build.yaml | head -15Repository: rustdesk/rustdesk-server
Length of output: 589
🏁 Script executed:
#!/bin/bash
# Check job-level conditions for publish jobs
echo "=== Job-level conditions for docker/manifest/release/deb jobs ==="
for job in "docker:" "docker-manifest:" "docker-classic:" "docker-manifest-classic:" "release:" "deb-package:"; do
echo "Job: $job"
rg -A 5 "^ $job" .github/workflows/build.yaml | head -7
echo
doneRepository: rustdesk/rustdesk-server
Length of output: 889
🏁 Script executed:
#!/bin/bash
# Check if docker build-push steps have conditions
echo "=== Docker job build step (around line 285-295) ==="
sed -n '280,300p' .github/workflows/build.yaml
echo -e "\n=== Docker-classic job build step (around line 450-465) ==="
sed -n '450,465p' .github/workflows/build.yamlRepository: rustdesk/rustdesk-server
Length of output: 1687
🏁 Script executed:
#!/bin/bash
# Final verification: confirm there are no job-level "if" conditions on publish jobs
echo "=== Checking for job-level if conditions on publish jobs ==="
rg -B 2 "^ (docker|release|deb-package):" .github/workflows/build.yaml | grep -E "^\s*(if:|docker:|release:|deb-package:)" | head -20Repository: rustdesk/rustdesk-server
Length of output: 164
Gate publish jobs on non-PR events.
Adding pull_request here enables the docker, docker-manifest, docker-classic, docker-manifest-classic, release, and deb-package jobs to run on PRs. The login steps correctly skip on PRs (gated with if: github.event_name != 'pull_request'), but the docker/build-push-action steps still execute with push: true without credentials, causing authentication failures. Either add if: github.event_name != 'pull_request' to these jobs or move them to a separate workflow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build.yaml around lines 16 - 21, The workflow currently
triggers publish-related jobs on pull_request which causes
docker/build-push-action to run without credentials; for each publish job
(docker, docker-manifest, docker-classic, docker-manifest-classic, release,
deb-package) add an explicit job-level condition if: github.event_name !=
'pull_request' (or remove them from this workflow and put them into a separate
workflow that only triggers on push/release) so the docker/build-push-action
steps never execute during PR events.
| pull_request: | ||
| types: [opened, synchronize, reopened] | ||
|
|
||
| jobs: | ||
| codex: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: write | ||
| pull-requests: write | ||
| steps: | ||
| - name: Checkout repo | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| submodules: recursive | ||
|
|
||
| - name: Run Codex | ||
| uses: openai/codex-action@v1 | ||
| with: | ||
| openai-api-key: ${{ secrets.OPENAI_API_KEY }} | ||
| prompt: ${{ github.event.inputs.prompt || 'Review this PR and suggest fixes focused on rustdesk-server hbbs changes.' }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GitHub Actions workflows triggered by pull_request, are repository secrets exposed to PRs from forks, and what minimum GITHUB_TOKENpermissions doesopenai/codex-action@v1 require?
💡 Result:
-
Are repository secrets exposed to
pull_requestruns from forks? No. For workflows triggered bypull_requestfrom a fork, GitHub does not pass repository/org/environment secrets to the runner (the documented exception is the automatically-providedGITHUB_TOKEN). [1] -
Minimum
GITHUB_TOKENpermissions foropenai/codex-action@v1: The action’s own README example runsopenai/codex-action@v1with only:permissions: contents: read
i.e.,
contents: readis sufficient/minimal for the job running Codex (the example uses separate, more-privileged permissions only for a later step that posts PR feedback). [2]
Sources: [1] GitHub Docs – “Use secrets” (note about forks) (docs.github.com), [2] openai/codex-action README example (github.com)
Citations:
- 1: https://docs.github.com/actions/security-guides/encrypted-secrets
- 2: https://github.com/openai/codex-action
Guard fork PRs and trim the token scope.
This runs on every pull_request, but forked PRs will not receive OPENAI_API_KEY, causing the action to fail with a noisy red check. Gate the job on secret availability or limit to internal PRs. Additionally, openai/codex-action@v1 requires only contents: read and does not need write permissions; remove contents: write and pull-requests: write.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/codex.yml around lines 10 - 29, The workflow should skip
runs from forked PRs and reduce permissions: in the codex job (job id "codex")
add a guard such as if: ${{ github.event.pull_request.head.repo.fork == false }}
or if: ${{ secrets.OPENAI_API_KEY != '' }} to prevent runs when the secret is
unavailable, and change the job permissions to only contents: read (remove
contents: write) and drop pull-requests: write; keep the step using
openai/codex-action@v1 unchanged but rely on the guard to prevent failing on
forked PRs.
| let outbound_whitelist: HashSet<String> = fs::read_to_string("whitelist.txt") | ||
| .unwrap_or_default() | ||
| .lines() | ||
| .map(|l| l.trim().to_string()) | ||
| .filter(|l| !l.is_empty() && !l.starts_with('#')) | ||
| .collect(); | ||
|
|
||
| log::info!( | ||
| "Loaded outbound whitelist entries: {}", | ||
| outbound_whitelist.len() | ||
| ); |
There was a problem hiding this comment.
Fail fast when whitelist.txt is missing.
read_to_string(...).unwrap_or_default() makes a missing or unreadable whitelist.txt look like an empty allowlist. Since the punch-hole path now rejects anything not in this set, a deployment without that file will break outbound connections with only a log entry. Please either fail fast on configuration errors or treat a missing file as “whitelist disabled”. The relative path makes this especially easy to hit under services and containers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rendezvous_server.rs` around lines 133 - 143, The current use of
fs::read_to_string("whitelist.txt").unwrap_or_default() hides missing/unreadable
whitelist errors and creates an empty allowlist; change this to fail fast:
replace the unwrap_or_default usage around fs::read_to_string with explicit
error handling that logs a clear error including the filename and the io::Error
and then exits/returns an Err (e.g., log::error!("Failed to read whitelist.txt:
{}", err); std::process::exit(1)) so outbound_whitelist is only constructed when
the file is successfully read; reference the outbound_whitelist variable and the
read_to_string call when making this change.
| let source_id = self.pm.get_id_by_socket_addr(addr).await; | ||
| match source_id { | ||
| Some(src_id) => { | ||
| if !self.outbound_whitelist.contains(&src_id) { | ||
| log::warn!( | ||
| "Outbound connection rejected by whitelist: source_id={} remote_addr={}", | ||
| src_id, | ||
| addr | ||
| ); | ||
| let mut msg_out = RendezvousMessage::new(); | ||
| msg_out.set_punch_hole_response(PunchHoleResponse { | ||
| failure: punch_hole_response::Failure::ID_NOT_EXIST.into(), | ||
| ..Default::default() | ||
| }); | ||
| return Ok((msg_out, None)); | ||
| } | ||
| } | ||
| None => { | ||
| log::warn!( | ||
| "Outbound connection rejected: unable to resolve source_id for remote_addr={}", | ||
| addr | ||
| ); | ||
| let mut msg_out = RendezvousMessage::new(); | ||
| msg_out.set_punch_hole_response(PunchHoleResponse { | ||
| failure: punch_hole_response::Failure::ID_NOT_EXIST.into(), | ||
| ..Default::default() | ||
| }); | ||
| return Ok((msg_out, None)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Socket-address auth will reject TCP/WebSocket callers.
addr here is the current transport endpoint, not a stable peer identity. TCP punch-hole requests arrive from a different socket than the UDP registration address, and in the websocket path Line 1219 and Line 1221 explicitly rewrite it to ip:0 / [ip]:0. That makes get_id_by_socket_addr return None for legitimate clients, so outbound punch-hole requests over TCP/WS get rejected. This check needs to use an authenticated peer/session ID instead of reverse-matching the socket address.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rendezvous_server.rs` around lines 717 - 746, The current socket-address
based auth using get_id_by_socket_addr(addr) rejects legitimate TCP/WS callers
because addr is a transient transport endpoint; instead retrieve the
authenticated peer/session ID from the request context/session (the value used
for authentication earlier in your connection handling) and use that ID for
whitelist checks against outbound_whitelist and for constructing the
PunchHoleResponse; replace the get_id_by_socket_addr(addr) call and the
addr-based log messages with logic that reads the session/authenticated peer ID
(e.g. a variable like session_id or auth_peer_id produced during handshake) and
if missing return the same PunchHoleResponse but with logs and failure reason
keyed to missing/unauthenticated session rather than socket addr.
1 participant
Summary by CodeRabbit
New Features
Chores