carverauto/serviceradar

Fork 0

edge onboarding improvements #2535

Merged

mfreeman451 merged 3 commits from refs/pull/2535/head into main

2025-12-10 05:11:24 +00:00

mfreeman451 commented

2025-12-10 04:47:50 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2094
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2094
Original created: 2025-12-10T04:47:50Z
Original updated: 2025-12-10T05:11:24Z
Original head: carverauto/serviceradar:update/rust_edge_onboarding
Original base: main
Original merged: 2025-12-10T05:11:24Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Enhancement

Description

Add host override functionality to token parsing for flexible deployment
Implement apply_host_override() function to replace hostnames while preserving scheme and port
Add comprehensive test coverage for host override scenarios
Document service integration requirements and future enhancements in proposal

Diagram Walkthrough

flowchart LR
  A["Token with localhost:8090"] -->|parse_token with override| B["apply_host_override()"]
  B -->|preserve scheme/port| C["http://192.168.2.235:8090"]
  D["Full URL override"] -->|detect scheme| C
  E["No original URL"] -->|create default| F["http://host:8090"]

File Walkthrough

Relevant files

Enhancement

token.rs

Implement host override logic for token URLs

rust/edge-onboarding/src/token.rs

Changed token parsing to override (not fallback) the core_url when
--host flag is provided
Added apply_host_override() function that intelligently handles
hostname replacement while preserving scheme and port
Function supports full URL overrides, partial hostname overrides, and
default port assignment
Updated comment to clarify override behavior for localhost and
internal hostnames

+70/-3

Tests

token_tests.rs

Add comprehensive host override test coverage

rust/edge-onboarding/tests/token_tests.rs

Added 5 new test cases covering host override scenarios
Tests verify localhost replacement with IP addresses
Tests validate full URL override behavior and HTTPS scheme
preservation
Tests confirm explicit port handling and default port assignment
Tests ensure proper behavior when original token has no URL

+71/-0

Documentation

proposal.md

Document service integration proposal and requirements

openspec/changes/add-rust-edge-onboarding-service-integration/proposal.md

New proposal document outlining service integration enhancements for
Rust edge-onboarding
Describes --mtls-bootstrap-only mode for zero-touch deployment with
systemd/launchd support
Details platform-aware service restart logic and configurable paths
for systemd integration
Outlines RPM/DEB packaging updates and future auto-configuration of
pollers

+54/-0

Imported from GitHub pull request. Original GitHub pull request: #2094 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2094 Original created: 2025-12-10T04:47:50Z Original updated: 2025-12-10T05:11:24Z Original head: carverauto/serviceradar:update/rust_edge_onboarding Original base: main Original merged: 2025-12-10T05:11:24Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Enhancement ___ ### **Description** - Add host override functionality to token parsing for flexible deployment - Implement `apply_host_override()` function to replace hostnames while preserving scheme and port - Add comprehensive test coverage for host override scenarios - Document service integration requirements and future enhancements in proposal ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Token with localhost:8090"] -->|parse_token with override| B["apply_host_override()"] B -->|preserve scheme/port| C["http://192.168.2.235:8090"] D["Full URL override"] -->|detect scheme| C E["No original URL"] -->|create default| F["http://host:8090"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><table> <tr> <td> <details> <summary>token.rs<dd><code>Implement host override logic for token URLs</code>                          </dd></summary> <hr> rust/edge-onboarding/src/token.rs <ul><li>Changed token parsing to override (not fallback) the core_url when <code>--host</code> flag is provided <li> Added <code>apply_host_override()</code> function that intelligently handles hostname replacement while preserving scheme and port <li> Function supports full URL overrides, partial hostname overrides, and default port assignment <li> Updated comment to clarify override behavior for localhost and internal hostnames</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cd">+70/-3</a>    </td> </tr> </table></td></tr><tr><td>Tests</td><td><table> <tr> <td> <details> <summary>token_tests.rs<dd><code>Add comprehensive host override test coverage</code>                        </dd></summary> <hr> rust/edge-onboarding/tests/token_tests.rs <ul><li>Added 5 new test cases covering host override scenarios <li> Tests verify localhost replacement with IP addresses <li> Tests validate full URL override behavior and HTTPS scheme preservation <li> Tests confirm explicit port handling and default port assignment <li> Tests ensure proper behavior when original token has no URL</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2094/files#diff-5c3192711c7e59713634180745b00fa6ac48caff6d9fbe5392b357362ace1761">+71/-0</a>    </td> </tr> </table></td></tr><tr><td>Documentation</td><td><table> <tr> <td> <details> <summary>proposal.md<dd><code>Document service integration proposal and requirements</code>      </dd></summary> <hr> openspec/changes/add-rust-edge-onboarding-service-integration/proposal.md <ul><li>New proposal document outlining service integration enhancements for Rust edge-onboarding <li> Describes <code>--mtls-bootstrap-only</code> mode for zero-touch deployment with systemd/launchd support <li> Details platform-aware service restart logic and configurable paths for systemd integration <li> Outlines RPM/DEB packaging updates and future auto-configuration of pollers</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2094/files#diff-82ba6af87d12c498440eca2fd214393657a458157f941b76be99deed9ecd1c4b">+54/-0</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-10 04:48:20 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2094#issuecomment-3635368488
Original created: 2025-12-10T04:48:20Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🔴	IPv6/port mishandling Description: Port detection uses a simplistic `rsplit(':')` on the authority which misinterprets IPv6 addresses (e.g., `[2001:db8::1]:8090`) and any colon in the hostname, potentially extracting an incorrect port and constructing a wrong target URL, enabling SSRF-style redirection to unintended hosts/ports. token.rs [168-213] Referred Code let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ("http://", &original[7..]) ... (clipped 25 lines)
⚪	URL parsing/validation Description: The custom URL construction in `apply_host_override()` lacks robust parsing and normalization (e.g., no RFC-compliant URL parser, no validation/encoding), which can allow crafted inputs like malformed hosts or embedded credentials (`user@host`, path/query fragments, or IPv6 literals with colons) to produce unintended endpoints or bypass expected port/scheme handling. token.rs [167-215] Referred Code fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ... (clipped 28 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No auditing: New logic overriding token core_url lacks any audit/log trail of the critical action applied, making it unclear who/when/how overrides occurred. Referred Code `// --host flag should OVERRIDE the token's API URL, not just be a fallback // This allows users to specify the actual reachable host when the token // contains localhost or an internal hostname if let Some(override_host) = fallback_core_url { payload.core_url = Some(apply_host_override( payload.core_url.as_deref(), override_host, )); }` Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: URL parsing edgecases: The manual URL parsing in apply_host_override may mishandle edge cases (IPv6, usernames, paths, query/fragment) without validation or error handling. Referred Code fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ... (clipped 28 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Unvalidated override: The host override string is accepted and used to construct URLs without normalization or validation, potentially enabling SSRF or invalid URL injection. Referred Code fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ... (clipped 28 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2094#issuecomment-3635368488 Original created: 2025-12-10T04:48:20Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=1>🔴</td> <td><details><summary>IPv6/port mishandling </summary> Description: Port detection uses a simplistic <code>rsplit(':')</code> on the authority which misinterprets IPv6 addresses (e.g., <code>[2001:db8::1]:8090</code>) and any colon in the hostname, potentially extracting an incorrect port and constructing a wrong target URL, enabling SSRF-style redirection to unintended hosts/ports. <a href='https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cdR168-R213'>token.rs [168-213]</a> <details open><summary>Referred Code</summary> ```rust let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ("http://", &original[7..]) ... (clipped 25 lines) ``` </details></details></td></tr> <tr><td rowspan=1>⚪</td> <td><details><summary>URL parsing/validation </summary> Description: The custom URL construction in <code>apply_host_override()</code> lacks robust parsing and normalization (e.g., no RFC-compliant URL parser, no validation/encoding), which can allow crafted inputs like malformed hosts or embedded credentials (<code>user@host</code>, path/query fragments, or IPv6 literals with colons) to produce unintended endpoints or bypass expected port/scheme handling. <a href='https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cdR167-R215'>token.rs [167-215]</a> <details open><summary>Referred Code</summary> ```rust fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ... (clipped 28 lines) ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=3>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cdR88-R96'>No auditing</a>: New logic overriding token core_url lacks any audit/log trail of the critical action applied, making it unclear who/when/how overrides occurred. <details open><summary>Referred Code</summary> ```rust // --host flag should OVERRIDE the token's API URL, not just be a fallback // This allows users to specify the actual reachable host when the token // contains localhost or an internal hostname if let Some(override_host) = fallback_core_url { payload.core_url = Some(apply_host_override( payload.core_url.as_deref(), override_host, )); } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cdR167-R215'>URL parsing edgecases</a>: The manual URL parsing in apply_host_override may mishandle edge cases (IPv6, usernames, paths, query/fragment) without validation or error handling. <details open><summary>Referred Code</summary> ```rust fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ... (clipped 28 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cdR167-R215'>Unvalidated override</a>: The host override string is accepted and used to construct URLs without normalization or validation, potentially enabling SSRF or invalid URL injection. <details open><summary>Referred Code</summary> ```rust fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { // Check if override already has a port if override_host.contains(':') { return format!("http://{}", override_host); } return format!("http://{}:8090", override_host); }; // Parse the original URL to extract scheme and port let (scheme, rest) = if original.starts_with("https://") { ("https://", &original[8..]) } else if original.starts_with("http://") { ... (clipped 28 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-12-10 04:49:21 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2094#issuecomment-3635370795
Original created: 2025-12-10T04:49:21Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Preserve URL path during host override Update the host override logic to preserve the path and query string from the original URL. The current implementation incorrectly drops them, which can break API requests. rust/edge-onboarding/src/token.rs [193-214] -// Extract port from original URL (if any) -// Format: host:port or host:port/path -let port = rest - .split('/') +// Extract authority and path from original URL +let (original_authority, original_path) = rest.split_once('/').unwrap_or((rest, "")); + +// Extract port from original authority (if any) +let port = original_authority + .rsplit(':') .next() - .and_then(\|host_port\| { - host_port - .rsplit(':') - .next() - .filter(\|p\| p.chars().all(\|c\| c.is_ascii_digit())) - }); + .filter(\|p\| p.chars().all(\|c\| c.is_ascii_digit())); // If override already has a port, use it as-is if override_host.contains(':') { - return format!("{}{}", scheme, override_host); + if original_path.is_empty() { + return format!("{}{}", scheme, override_host); + } + return format!("{}{}/{}", scheme, override_host, original_path); } -// Construct new URL with override host and original port -match port { - Some(p) => format!("{}{}:{}", scheme, override_host, p), - None => format!("{}{}", scheme, override_host), +// Construct new URL with override host, original port, and original path +let new_authority = match port { + Some(p) => format!("{}:{}", override_host, p), + None => override_host.to_string(), +}; + +if original_path.is_empty() { + format!("{}{}", scheme, new_authority) +} else { + format!("{}{}/{}", scheme, new_authority, original_path) } Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a significant bug in the new `apply_host_override` function where the URL path is discarded, which would likely cause API requests to fail.	High
High-level	Use a URL parsing library Replace the manual string-based URL parsing in `apply_host_override` with a dedicated URL parsing library like the `url` crate. This will make the implementation more robust and handle edge cases correctly. Examples: rust/edge-onboarding/src/token.rs [167-215] `fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { ... (clipped 39 lines)` Solution Walkthrough: Before: fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { // ... manual checks for "http://" or "https:// if looks_like_url(override_host) { return override_host.to_string(); } let (scheme, rest) = if original.starts_with("https://") { ... } else { ... }; // ... manual string splitting to find port let port = rest .split('/') .next() .and_then(\|host_port\| host_port.rsplit(':').next()...); // ... manual string formatting to build new URL match port { Some(p) => format!("{}{}:{}", scheme, override_host, p), None => format!("{}{}", scheme, override_host), } } After: use url::Url; fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { // If override is a full URL, parse and return it. if let Ok(url) = Url::parse(override_host) { if url.scheme().starts_with("http") { return url.into(); } } // Parse original URL or use a default base. let mut url = original_url .and_then(\|s\| Url::parse(s).ok()) .unwrap_or_else(\|\| Url::parse("http://dummy:8090").unwrap()); // Use URL setters to replace host and port, which is more robust. url.set_host(Some(override_host)).unwrap(); return url.into(); } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the manual URL parsing in `apply_host_override` is fragile and proposes using a dedicated library, which would significantly improve the robustness and correctness of this core functionality.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2094#issuecomment-3635370795 Original created: 2025-12-10T04:49:21Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Preserve URL path during host override</summary> ___ **Update the host override logic to preserve the path and query string from the original URL. The current implementation incorrectly drops them, which can break API requests.** [rust/edge-onboarding/src/token.rs [193-214]](https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cdR193-R214) ```diff -// Extract port from original URL (if any) -// Format: host:port or host:port/path -let port = rest - .split('/') +// Extract authority and path from original URL +let (original_authority, original_path) = rest.split_once('/').unwrap_or((rest, "")); + +// Extract port from original authority (if any) +let port = original_authority + .rsplit(':') .next() - .and_then(|host_port| { - host_port - .rsplit(':') - .next() - .filter(|p| p.chars().all(|c| c.is_ascii_digit())) - }); + .filter(|p| p.chars().all(|c| c.is_ascii_digit())); // If override already has a port, use it as-is if override_host.contains(':') { - return format!("{}{}", scheme, override_host); + if original_path.is_empty() { + return format!("{}{}", scheme, override_host); + } + return format!("{}{}/{}", scheme, override_host, original_path); } -// Construct new URL with override host and original port -match port { - Some(p) => format!("{}{}:{}", scheme, override_host, p), - None => format!("{}{}", scheme, override_host), +// Construct new URL with override host, original port, and original path +let new_authority = match port { + Some(p) => format!("{}:{}", override_host, p), + None => override_host.to_string(), +}; + +if original_path.is_empty() { + format!("{}{}", scheme, new_authority) +} else { + format!("{}{}/{}", scheme, new_authority, original_path) } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a significant bug in the new `apply_host_override` function where the URL path is discarded, which would likely cause API requests to fail. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Use a URL parsing library</summary> ___ **Replace the manual string-based URL parsing in <code>apply_host_override</code> with a dedicated URL parsing library like the <code>url</code> crate. This will make the implementation more robust and handle edge cases correctly.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2094/files#diff-522ddb82ad9919bd416394e34bbfd4ce3e8793723b260a08843d7500ae0b98cdR167-R215">rust/edge-onboarding/src/token.rs [167-215]</a> </summary> ```rust fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { let override_host = override_host.trim(); // If override is already a full URL, use it directly if looks_like_url(override_host) { return override_host.to_string(); } // If no original URL, construct a new one with default port let Some(original) = original_url else { ... (clipped 39 lines) ``` </details> ### Solution Walkthrough: #### Before: ```rust fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { // ... manual checks for "http://" or "https:// if looks_like_url(override_host) { return override_host.to_string(); } let (scheme, rest) = if original.starts_with("https://") { ... } else { ... }; // ... manual string splitting to find port let port = rest .split('/') .next() .and_then(|host_port| host_port.rsplit(':').next()...); // ... manual string formatting to build new URL match port { Some(p) => format!("{}{}:{}", scheme, override_host, p), None => format!("{}{}", scheme, override_host), } } ``` #### After: ```rust use url::Url; fn apply_host_override(original_url: Option<&str>, override_host: &str) -> String { // If override is a full URL, parse and return it. if let Ok(url) = Url::parse(override_host) { if url.scheme().starts_with("http") { return url.into(); } } // Parse original URL or use a default base. let mut url = original_url .and_then(|s| Url::parse(s).ok()) .unwrap_or_else(|| Url::parse("http://dummy:8090").unwrap()); // Use URL setters to replace host and port, which is more robust. url.set_host(Some(override_host)).unwrap(); return url.into(); } ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that the manual URL parsing in `apply_host_override` is fragile and proposes using a dedicated library, which would significantly improve the robustness and correctness of this core functionality. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>