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:
📝 WalkthroughWalkthroughAdded TCP-proxy routing and HTTP-first-with-TCP-fallback control flow in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as Request Handler
participant HTTP as HTTP Layer
participant TCP as TCP Proxy / Rendezvous
participant Final as Response Finalizer
Client->>Handler: post_request(url, body, headers)
alt Direct TCP-proxy target (flag + host match & WS disabled)
Handler->>TCP: post_request_via_tcp_proxy(url, body, headers)
TCP-->>Handler: HttpProxyResponse -> JSON
Handler->>Final: return proxy JSON result
else Not direct TCP-proxy target
Handler->>HTTP: post_request_http(url, body, headers)
HTTP-->>Handler: (status_code, body_text)
alt 2xx/3xx
Handler->>Final: return HTTP success
else 4xx
Handler->>Final: return HTTP body (no TCP fallback)
else 5xx or HTTP connection error (and fallback allowed)
Handler->>TCP: post_request_via_tcp_proxy(...)
TCP-->>Handler: proxy response (or error)
alt proxy success
Handler->>Final: return proxy JSON result
else proxy fails
Handler->>Final: return original HTTP result
end
end
end
Final-->>Client: Final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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)
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 |
There was a problem hiding this comment.
Pull request overview
Adds a raw TCP proxy path for API-bound HTTP requests by tunneling them through the rendezvous server, with opt-in “direct TCP” mode and automatic fallback from HTTP on failures/5xx responses.
Changes:
- Introduces TCP-proxy request helpers and API-target matching logic to decide when to proxy.
- Updates
post_requestandhttp_request_syncto support direct TCP proxy and HTTP→TCP fallback behavior. - Adds unit tests for API host matching and
HttpProxyResponse→ JSON serialization.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 51b562c6c8
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common.rs`:
- Around line 1313-1325: The current logic in post_request and http_request_sync
uses Err(_) or any 5xx to trigger a TCP proxy fallback (via
can_fallback_to_raw_tcp and post_request_via_tcp_proxy), which can replay
non-idempotent mutations; update both post_request and http_request_sync to only
perform the automatic TCP fallback when the HTTP method is safe/idempotent (GET,
HEAD, OPTIONS, PUT when proven idempotent) or when the caller provides an
explicit idempotency key header; otherwise return the original Err/5xx to the
caller and do not call post_request_via_tcp_proxy. Ensure you check the request
method (method param) and presence of an idempotency header before calling
can_fallback_to_raw_tcp/post_request_via_tcp_proxy and document the new
requirement in the function comments.
- Around line 1628-1647: The current raw-TCP header handling silently ignores
malformed or non-object JSON and unconditionally appends Content-Type, causing
behavior divergence from the HTTP branch; change the logic around the header
variable so that if header is non-empty you strictly parse it with
serde_json::from_str and if parsing fails or the parsed value is not an object
return or propagate an error (matching the HTTP path) instead of ignoring it;
when parsing succeeds iterate the object and push HeaderEntry (name/value) into
headers as before, and only inject a default Content-Type HeaderEntry if the
parsed headers did not already contain one (to mirror parse_simple_header
behavior).
- Around line 1143-1155: The function should_use_tcp_proxy_for_api_url currently
calls is_public(api_url) on the raw string; instead parse both url and api_url
first (use url::Url::parse) and run is_public on the parsed api host (or its
registrable domain like "rustdesk.com"/"*.rustdesk.com") so URLs with explicit
ports (e.g. "https://admin.rustdesk.com:21114") are correctly treated as public
and not proxied; adjust the logic in should_use_tcp_proxy_for_api_url to check
parsed api_host via is_public(parsed_api_host) and then compare parsed host
strings for equality, and add a regression test that asserts a URL with an
explicit port on the public api host is not proxied.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f590a07b5
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/common.rs (2)
1649-1656:⚠️ Potential issue | 🟠 MajorKeep proxy-mode headers identical to the HTTP branch.
http_request_httpforwards only the caller’s headers, but this path always appendsContent-Type: application/json. Once proxy mode or fallback kicks in, the same logical request can go out with a different header set, and a caller-suppliedContent-Typebecomes duplicated.💡 Proposed fix
async fn http_request_via_tcp_proxy( url: &str, method: &str, body: Option<&str>, header: &str, ) -> ResultType<String> { - let mut headers = parse_json_header_entries(header)?; + let headers = parse_json_header_entries(header)?; let body_bytes = body.unwrap_or("").as_bytes(); - // Always include Content-Type for consistency with parse_simple_header - headers.push(HeaderEntry { - name: "Content-Type".into(), - value: "application/json".into(), - ..Default::default() - }); let resp = tcp_proxy_request(method, url, body_bytes, headers).await?; http_proxy_response_to_json(resp) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1649 - 1656, The proxy/fallback path is unconditionally appending a Content-Type header which can duplicate caller-supplied headers and diverge from the http_request_http behavior; update the logic in the block that calls parse_json_header_entries(header) (and builds headers/Body bytes) to only push the HeaderEntry { name: "Content-Type", value: "application/json", .. } when the parsed headers do not already contain a Content-Type (use a case-insensitive check on HeaderEntry.name), so the header set remains identical to the HTTP branch and avoids duplicate Content-Type entries.
1333-1346:⚠️ Potential issue | 🔴 CriticalAvoid automatic TCP fallback for non-idempotent calls.
These wrappers retry on any transport error or
5xx. Forpost_request, and for non-idempotenthttp_request_synccalls, the upstream may already have applied the write before the client observes that failure, so the TCP retry can duplicate side effects.src/hbbs_http/sync.rs:210already sends a mutating request throughpost_request.Also applies to: 1617-1630
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1333 - 1346, The TCP-proxy fallback currently runs for any transport error or 5xx, which can duplicate side-effects for non-idempotent requests; update the fallback guard so post_request and non-idempotent http_request_sync calls never fall back to post_request_via_tcp_proxy. Concretely, in the block that calls post_request_http and then maybe post_request_via_tcp_proxy, remove or short-circuit the fallback when the request is non-idempotent (e.g., HTTP POST) by checking the request method (or add an is_idempotent_method helper) before evaluating can_fallback_to_raw_tcp(&url); apply the same guard for the other occurrence around the http_request_sync path so only idempotent methods (GET/HEAD/PUT/DELETE as appropriate) use the TCP fallback. Ensure you reference and modify the logic around post_request_http, post_request_via_tcp_proxy, can_fallback_to_raw_tcp, post_request, and http_request_sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/common.rs`:
- Around line 2788-2809: The test
test_get_tcp_proxy_addr_normalizes_bare_ipv6_host mutates process-global Config
(via Config::set_option/get_option and keys::OPTION_CUSTOM_RENDEZVOUS_SERVER)
and only restores it on drop, which can race with other tests; serialize this by
acquiring a global test mutex (or applying a serial-test attribute) around the
test body so the temporary Config change is exclusive while calling
get_tcp_proxy_addr() (and still restore the original value in
RestoreCustomRendezvousServer on drop); reference the test name,
RestoreCustomRendezvousServer, Config::set_option/get_option,
keys::OPTION_CUSTOM_RENDEZVOUS_SERVER, get_tcp_proxy_addr, and RENDEZVOUS_PORT
when making the change.
- Around line 1276-1290: The parser parse_json_header_entries currently fails on
an empty or whitespace-only header string; change it to treat trimmed empty
input as "no headers" by returning Ok(vec![]) early (instead of attempting to
parse), and keep existing behavior for non-empty input (parsing to
Vec<HeaderEntry>); update callers get_http_response_async and
http_request_via_tcp_proxy tests by adding a regression test that passes an
empty string (and a whitespace string) to ensure they succeed and no
request-level error occurs.
---
Duplicate comments:
In `@src/common.rs`:
- Around line 1649-1656: The proxy/fallback path is unconditionally appending a
Content-Type header which can duplicate caller-supplied headers and diverge from
the http_request_http behavior; update the logic in the block that calls
parse_json_header_entries(header) (and builds headers/Body bytes) to only push
the HeaderEntry { name: "Content-Type", value: "application/json", .. } when the
parsed headers do not already contain a Content-Type (use a case-insensitive
check on HeaderEntry.name), so the header set remains identical to the HTTP
branch and avoids duplicate Content-Type entries.
- Around line 1333-1346: The TCP-proxy fallback currently runs for any transport
error or 5xx, which can duplicate side-effects for non-idempotent requests;
update the fallback guard so post_request and non-idempotent http_request_sync
calls never fall back to post_request_via_tcp_proxy. Concretely, in the block
that calls post_request_http and then maybe post_request_via_tcp_proxy, remove
or short-circuit the fallback when the request is non-idempotent (e.g., HTTP
POST) by checking the request method (or add an is_idempotent_method helper)
before evaluating can_fallback_to_raw_tcp(&url); apply the same guard for the
other occurrence around the http_request_sync path so only idempotent methods
(GET/HEAD/PUT/DELETE as appropriate) use the TCP fallback. Ensure you reference
and modify the logic around post_request_http, post_request_via_tcp_proxy,
can_fallback_to_raw_tcp, post_request, and http_request_sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/common.rs (4)
1276-1289:⚠️ Potential issue | 🟠 MajorTreat empty header JSON as “no headers.”
serde_json::from_str("")fails, but Lines 1474-1476 and Line 1649 call this helper unconditionally. Any caller that passes""or whitespace now errors locally before any request is sent. ReturnOk(vec![])for trimmed-empty input and add regression coverage for both empty and whitespace-only strings.Suggested fix
fn parse_json_header_entries(header: &str) -> ResultType<Vec<HeaderEntry>> { + if header.trim().is_empty() { + return Ok(Vec::new()); + } let v: Value = serde_json::from_str(header)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1276 - 1289, The parse_json_header_entries helper should treat trimmed-empty input as "no headers": in parse_json_header_entries(header: &str) return Ok(vec![]) immediately when header.trim().is_empty() instead of calling serde_json::from_str; otherwise proceed with the existing serde_json parsing and construction of Vec<HeaderEntry>. Update tests to add regression coverage calling parse_json_header_entries with "" and with a whitespace-only string to assert they both return Ok(empty vec). Ensure the function still returns the same ResultType and preserves the existing error path for non-object JSON.
1143-1155:⚠️ Potential issue | 🟠 MajorBase the public-host exclusion on the parsed host.
Lines 1056-1060 only strip
:21114whenOPTION_ALLOW_HTTPS_21114 != "Y". With that builtin enabled,https://admin.rustdesk.com:21114can still reach this helper,is_public(api_url)returns false on the raw string, and the public admin host becomes proxyable once the parsed hosts match. Checkis_publicagainstapi_hostinstead, and add a regression case for the explicit-port public URL.Suggested fix
fn should_use_tcp_proxy_for_api_url(url: &str, api_url: &str) -> bool { - if api_url.is_empty() || is_public(api_url) { + if api_url.is_empty() { return false; } @@ - matches!((target_host, api_host), (Some(target), Some(api)) if target == api) + matches!( + (target_host, api_host), + (Some(target), Some(api)) if !is_public(&api) && target == api + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1143 - 1155, The initial public-host check uses the raw api_url string which can be non-public only due to an explicit port (e.g. ":21114"); update should_use_tcp_proxy_for_api_url to parse api_url first and call is_public on the parsed host (api_host) rather than the raw api_url, keeping the fallback behavior when parsing fails; ensure the function still extracts target_host and api_host via url::Url::parse and then performs is_public(&api_host) before deciding to return false, and add a regression test that verifies a public host with an explicit port (e.g. "https://admin.rustdesk.com:21114") is treated as public and not proxyable.
2782-2803:⚠️ Potential issue | 🟡 MinorSerialize this test's global
Configmutation.This test temporarily rewrites
OPTION_CUSTOM_RENDEZVOUS_SERVERand only restores it on drop.cargo testruns tests in parallel by default, so another test can observe the temporary rendezvous value and fail nondeterministically. Guard the body with a shared test mutex or a serial-test style attribute.Suggested fix
+ static TEST_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); + #[test] fn test_get_tcp_proxy_addr_normalizes_bare_ipv6_host() { + let _guard = TEST_MUTEX.lock().unwrap(); struct RestoreCustomRendezvousServer(String);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 2782 - 2803, The test test_get_tcp_proxy_addr_normalizes_bare_ipv6_host mutates global Config via Config::set_option/Config::get_option for OPTION_CUSTOM_RENDEZVOUS_SERVER and must be serialized to avoid races; wrap the test body with a global test mutex or add a serial-test style attribute so only one test manipulates Config at a time, keeping the RestoreCustomRendezvousServer drop-based restore and using the same get_tcp_proxy_addr assertion unchanged.
1334-1353:⚠️ Potential issue | 🔴 CriticalDon't auto-fallback mutating requests after ambiguous failures.
Err(_)here is broader than transport failures, andpost_requestis always replaying aPOST. Retrying through the TCP proxy after a timeout or5xxcan duplicate server-side side effects; inhttp_request_syncthe same block also affectsPOST/PUT/DELETE. Restrict automatic fallback to verified transport failures on idempotent requests, or require an idempotency guarantee from the caller.Also applies to: 1618-1636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1334 - 1353, The current fallback logic uses Err(_) and 5xx to trigger TCP proxy retries in post_request (and http_request_sync), which can replay mutating POST/PUT/DELETE requests; change it to only fall back for verified transport failures and idempotent requests: inspect the HTTP method (or require an idempotency guarantee header) and only allow fallback when method is idempotent (e.g., GET/HEAD/OPTIONS) or an Idempotency-Key is present, and change the Err(_) check to detect transport-level errors (e.g., connection refused, timeout, or reqwest::Error::is_connect()/is_timeout()) instead of matching all errors; update should_fallback calculation (the match on http_result), the log call using tcp_proxy_fallback_log_condition(), and the fallback call site post_request_via_tcp_proxy so non-idempotent requests are not retried automatically (and mirror the same change in http_request_sync).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/common.rs`:
- Around line 1276-1289: The parse_json_header_entries helper should treat
trimmed-empty input as "no headers": in parse_json_header_entries(header: &str)
return Ok(vec![]) immediately when header.trim().is_empty() instead of calling
serde_json::from_str; otherwise proceed with the existing serde_json parsing and
construction of Vec<HeaderEntry>. Update tests to add regression coverage
calling parse_json_header_entries with "" and with a whitespace-only string to
assert they both return Ok(empty vec). Ensure the function still returns the
same ResultType and preserves the existing error path for non-object JSON.
- Around line 1143-1155: The initial public-host check uses the raw api_url
string which can be non-public only due to an explicit port (e.g. ":21114");
update should_use_tcp_proxy_for_api_url to parse api_url first and call
is_public on the parsed host (api_host) rather than the raw api_url, keeping the
fallback behavior when parsing fails; ensure the function still extracts
target_host and api_host via url::Url::parse and then performs
is_public(&api_host) before deciding to return false, and add a regression test
that verifies a public host with an explicit port (e.g.
"https://admin.rustdesk.com:21114") is treated as public and not proxyable.
- Around line 2782-2803: The test
test_get_tcp_proxy_addr_normalizes_bare_ipv6_host mutates global Config via
Config::set_option/Config::get_option for OPTION_CUSTOM_RENDEZVOUS_SERVER and
must be serialized to avoid races; wrap the test body with a global test mutex
or add a serial-test style attribute so only one test manipulates Config at a
time, keeping the RestoreCustomRendezvousServer drop-based restore and using the
same get_tcp_proxy_addr assertion unchanged.
- Around line 1334-1353: The current fallback logic uses Err(_) and 5xx to
trigger TCP proxy retries in post_request (and http_request_sync), which can
replay mutating POST/PUT/DELETE requests; change it to only fall back for
verified transport failures and idempotent requests: inspect the HTTP method (or
require an idempotency guarantee header) and only allow fallback when method is
idempotent (e.g., GET/HEAD/OPTIONS) or an Idempotency-Key is present, and change
the Err(_) check to detect transport-level errors (e.g., connection refused,
timeout, or reqwest::Error::is_connect()/is_timeout()) instead of matching all
errors; update should_fallback calculation (the match on http_result), the log
call using tcp_proxy_fallback_log_condition(), and the fallback call site
post_request_via_tcp_proxy so non-idempotent requests are not retried
automatically (and mirror the same change in http_request_sync).
Signed-off-by: 21pages <sunboeasy@gmail.com>
|
Tested:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/common.rs (3)
1276-1290:⚠️ Potential issue | 🟠 MajorTreat blank header payloads as “no headers”.
http_request_sync()now routes every header string through this parser, so""/ whitespace fail locally with an EOF parse error before either transport is attempted. ReturnOk(vec![])for trimmed-empty input and add a regression test next to the new header-parser tests.💡 Proposed fix
fn parse_json_header_entries(header: &str) -> ResultType<Vec<HeaderEntry>> { + if header.trim().is_empty() { + return Ok(Vec::new()); + } let v: Value = serde_json::from_str(header)?; if let Value::Object(obj) = v { Ok(obj .iter() .map(|(key, value)| HeaderEntry {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1276 - 1290, parse_json_header_entries currently fails on empty/whitespace header strings because serde_json::from_str is called unconditionally; change parse_json_header_entries to trim the header argument and return Ok(vec![]) when trimmed header is empty, otherwise proceed to parse as JSON as before (keep producing Vec<HeaderEntry> on success and the existing error on non-object JSON). Also add a regression test alongside the existing header-parser tests that calls http_request_sync() (or directly parse_json_header_entries) with "" and whitespace-only strings to assert Ok(vec![]) is returned.
2792-2813:⚠️ Potential issue | 🟡 MinorSerialize this test’s global
Configmutation.This test rewrites process-global rendezvous state and restores it only on drop.
cargo testruns tests in parallel by default, so another test can observe the temporary"1:2"value and fail nondeterministically. Guard the body with a shared test mutex before callingConfig::set_option()/get_tcp_proxy_addr().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 2792 - 2813, The test test_get_tcp_proxy_addr_normalizes_bare_ipv6_host mutates global rendezvous state via Config::set_option/get_option and must be serialized; wrap the body that sets/restores keys::OPTION_CUSTOM_RENDEZVOUS_SERVER and calls get_tcp_proxy_addr() in the project-wide test mutex (the shared test lock used across tests) so the global Config mutation cannot run concurrently with other tests—acquire the mutex at the start of the test, perform the Config::set_option, call get_tcp_proxy_addr(), assert the value, then drop/release the mutex after restoring state via RestoreCustomRendezvousServer.
1328-1358:⚠️ Potential issue | 🔴 CriticalLimit TCP fallback to retry-safe cases.
Both wrappers still retry on every
Err(_)and every5xx. That can replay mutations after the upstream already applied them, and inhttp_request_sync()it also treats local failures like unsupported methods or bad header JSON as retryable. Only fall back on proven transport/connect failures, and only for idempotent requests or when the caller supplies an idempotency guard.Also applies to: 1607-1636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1328 - 1358, post_request currently falls back to TCP for any Err(_) or any 5xx, which can replay non-idempotent mutations; change the fallback logic in post_request (and the same pattern in http_request_sync) so we only attempt post_request_via_tcp_proxy when the HTTP error is a proven transport/connect failure (e.g., timeout, connection reset, DNS/connection refused) OR when the response is 5xx and the request is idempotent or the caller provided an idempotency guard/header. Concretely, replace the broad Err(_) / status>=500 check with: (1) inspect the Err variant from post_request_http to detect transport/connect errors and only treat those as retryable; (2) for Ok((status, _)) only allow fallback for >=500 when either the request is idempotent (detect via method/flags you have) or an idempotency header/guard is present; and (3) keep can_fallback_to_raw_tcp(&url) gating. Apply the same stricter check in http_request_sync where it currently treats local failures as retryable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/common.rs`:
- Around line 1276-1290: parse_json_header_entries currently fails on
empty/whitespace header strings because serde_json::from_str is called
unconditionally; change parse_json_header_entries to trim the header argument
and return Ok(vec![]) when trimmed header is empty, otherwise proceed to parse
as JSON as before (keep producing Vec<HeaderEntry> on success and the existing
error on non-object JSON). Also add a regression test alongside the existing
header-parser tests that calls http_request_sync() (or directly
parse_json_header_entries) with "" and whitespace-only strings to assert
Ok(vec![]) is returned.
- Around line 2792-2813: The test
test_get_tcp_proxy_addr_normalizes_bare_ipv6_host mutates global rendezvous
state via Config::set_option/get_option and must be serialized; wrap the body
that sets/restores keys::OPTION_CUSTOM_RENDEZVOUS_SERVER and calls
get_tcp_proxy_addr() in the project-wide test mutex (the shared test lock used
across tests) so the global Config mutation cannot run concurrently with other
tests—acquire the mutex at the start of the test, perform the
Config::set_option, call get_tcp_proxy_addr(), assert the value, then
drop/release the mutex after restoring state via RestoreCustomRendezvousServer.
- Around line 1328-1358: post_request currently falls back to TCP for any Err(_)
or any 5xx, which can replay non-idempotent mutations; change the fallback logic
in post_request (and the same pattern in http_request_sync) so we only attempt
post_request_via_tcp_proxy when the HTTP error is a proven transport/connect
failure (e.g., timeout, connection reset, DNS/connection refused) OR when the
response is 5xx and the request is idempotent or the caller provided an
idempotency guard/header. Concretely, replace the broad Err(_) / status>=500
check with: (1) inspect the Err variant from post_request_http to detect
transport/connect errors and only treat those as retryable; (2) for Ok((status,
_)) only allow fallback for >=500 when either the request is idempotent (detect
via method/flags you have) or an idempotency header/guard is present; and (3)
keep can_fallback_to_raw_tcp(&url) gating. Apply the same stricter check in
http_request_sync where it currently treats local failures as retryable.
… clone Signed-off-by: 21pages <sunboeasy@gmail.com>
Signed-off-by: 21pages <sunboeasy@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: 21pages <sunboeasy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/common.rs (3)
1350-1368:⚠️ Potential issue | 🔴 CriticalDon’t replay mutating requests after ambiguous failures.
These branches retry on every transport error and every
5xx. Forpost_request, and forhttp_request_syncwhenmethodispost/put/delete, the upstream may already have applied the mutation before the client observes the failure, so the TCP fallback can duplicate side effects.Also applies to: 1631-1646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1350 - 1368, The code currently retries via TCP proxy on any transport error or 5xx for post_request and http_request_sync; prevent replaying mutating requests by restricting the TCP fallback to safe/idempotent methods only (e.g., allow fallback for GET, HEAD, OPTIONS, PUT when known idempotent, but disallow for POST/PUT/DELETE that can cause side effects) and by checking the HTTP method before calling can_fallback_to_raw_tcp/post_request_via_tcp_proxy; update the logic in post_request and http_request_sync to consult the request method (and any idempotency markers) and skip the TCP fallback branch for non-idempotent methods so mutating requests are not retried.
1296-1310:⚠️ Potential issue | 🟠 MajorTreat empty header JSON as “no headers”.
parse_json_header_entries("")now errors, but both callers invoke it even when the request has no headers. That turns a header-lesshttp_request_sync/ TCP-proxy call into a local parse failure.💡 Proposed fix
fn parse_json_header_entries(header: &str) -> ResultType<Vec<HeaderEntry>> { + if header.trim().is_empty() { + return Ok(Vec::new()); + } let v: Value = serde_json::from_str(header)?; if let Value::Object(obj) = v { Ok(obj .iter() .map(|(key, value)| HeaderEntry {Please add a regression test for both
""and whitespace-only input.Also applies to: 1493-1495, 1662-1663
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 1296 - 1310, The function parse_json_header_entries currently fails on empty or whitespace-only input and should treat these cases as "no headers"; modify parse_json_header_entries to trim the header string and if it is empty return Ok(vec![]) before attempting serde_json::from_str, keeping the rest of the logic unchanged (use the existing HeaderEntry construction and error path for non-object JSON). Add regression tests that call parse_json_header_entries with "" and with a whitespace-only string and assert an empty Vec is returned, and also add integration tests exercising the callers (http_request_sync and the TCP-proxy path) to ensure they no longer error when headers are absent.
2805-2827:⚠️ Potential issue | 🟡 MinorSerialize this test’s process-global
Configmutation.This test rewrites
keys::OPTION_CUSTOM_RENDEZVOUS_SERVERand only restores it on drop.cargo testruns tests in parallel by default, so another test can observe the temporary value and fail nondeterministically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/common.rs` around lines 2805 - 2827, The test test_get_tcp_proxy_addr_normalizes_bare_ipv6_host mutates the global Config via Config::set_option/get_option which can race with other tests; serialize this mutation by running the test under a test-level lock or serial test attribute (e.g. use serial_test::serial or your project’s test serialization utility) so no other test can observe the temporary keys::OPTION_CUSTOM_RENDEZVOUS_SERVER value; keep the RestoreCustomRendezvousServer drop-based restore and wrap or annotate the test (or surrounding setup) to acquire the global test lock before calling Config::set_option and release it on drop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/common.rs`:
- Around line 1350-1368: The code currently retries via TCP proxy on any
transport error or 5xx for post_request and http_request_sync; prevent replaying
mutating requests by restricting the TCP fallback to safe/idempotent methods
only (e.g., allow fallback for GET, HEAD, OPTIONS, PUT when known idempotent,
but disallow for POST/PUT/DELETE that can cause side effects) and by checking
the HTTP method before calling
can_fallback_to_raw_tcp/post_request_via_tcp_proxy; update the logic in
post_request and http_request_sync to consult the request method (and any
idempotency markers) and skip the TCP fallback branch for non-idempotent methods
so mutating requests are not retried.
- Around line 1296-1310: The function parse_json_header_entries currently fails
on empty or whitespace-only input and should treat these cases as "no headers";
modify parse_json_header_entries to trim the header string and if it is empty
return Ok(vec![]) before attempting serde_json::from_str, keeping the rest of
the logic unchanged (use the existing HeaderEntry construction and error path
for non-object JSON). Add regression tests that call parse_json_header_entries
with "" and with a whitespace-only string and assert an empty Vec is returned,
and also add integration tests exercising the callers (http_request_sync and the
TCP-proxy path) to ensure they no longer error when headers are absent.
- Around line 2805-2827: The test
test_get_tcp_proxy_addr_normalizes_bare_ipv6_host mutates the global Config via
Config::set_option/get_option which can race with other tests; serialize this
mutation by running the test under a test-level lock or serial test attribute
(e.g. use serial_test::serial or your project’s test serialization utility) so
no other test can observe the temporary keys::OPTION_CUSTOM_RENDEZVOUS_SERVER
value; keep the RestoreCustomRendezvousServer drop-based restore and wrap or
annotate the test (or surrounding setup) to acquire the global test lock before
calling Config::set_option and release it on drop.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: 21pages <sunboeasy@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: 21pages <sunboeasy@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ll timeout - Extract duplicated TCP proxy fallback logic into generic `with_tcp_proxy_fallback` helper used by both `post_request` and `http_request_sync`, eliminating code drift risk - Allow caller-supplied Content-Type to override the default in `parse_simple_header` instead of silently dropping it - Take body by reference in `post_request_http` to avoid eager clone when no fallback is needed - Wrap entire `tcp_proxy_request` flow (connect + handshake + send + receive) in an overall timeout to prevent indefinite stalls Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e7484c51b
ℹ️ 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".
… URLs Signed-off-by: 21pages <sunboeasy@gmail.com>
…eping TLS warmup Signed-off-by: 21pages <sunboeasy@gmail.com>
Changes:
Tested:
Note:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e method Remove TryFrom<Response> impl that was never called and replace the private parse_hbb_http_response helper in account.rs with a public parse() method on HbbHttpResponse, eliminating code duplication. Signed-off-by: 21pages <sunboeasy@gmail.com>
Summary
This PR adds TCP proxy support for API requests in
src/common.rs.post_requestandhttp_request_syncHttpProxyResponseJSON serializationWhy
In some self-hosted/private deployments, the API server is not directly reachable from the client over normal HTTP, while the rendezvous server is reachable. This change allows those API requests to go
through the existing secure TCP channel instead of failing.
Notes
use-raw-tcp-for-apioption and disabled when WebSocket mode is in useSummary by CodeRabbit
New Features
Improvements
Tests