feat(configs): add address validation for QUIC/HTTP builders#3152
feat(configs): add address validation for QUIC/HTTP builders#3152Standing-Man wants to merge 7 commits intoapache:masterfrom
QUIC/HTTP builders#3152Conversation
Standing-Man
changed the title
QUIC/HTTP buildersQUIC/HTTP builders
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3152 +/- ##
============================================
- Coverage 74.09% 71.49% -2.60%
Complexity 943 943
============================================
Files 1159 1159
Lines 102033 97412 -4621
Branches 79083 74480 -4603
============================================
- Hits 75597 69644 -5953
- Misses 23768 24902 +1134
- Partials 2668 2866 +198
🚀 New features to boost your workflow:
|
|
looks good, but next time please comment under the issue before implementation and wait till we give a green light. |
Signed-off-by: StandingMan <jmtangcs@gmail.com>
Standing-Man
force-pushed
the
address-validation
branch
from
b4efaa8 to
751b415
Compare
|
Hi @hubcio, @spetz, @numinnex and @mmodzelewski, just a gentle ping — this PR is ready on my side. Could you please take a look when convenient? |
| _ => return Err(IggyError::InvalidApiUrl(addr.to_string())), | ||
| } | ||
|
|
||
| if api_url.host_str().is_none() || api_url.port().is_none() { |
There was a problem hiding this comment.
port().is_none() rejects valid urls with default-scheme ports. the url crate normalizes default ports during parse - Url::parse("http://example.com:80").port() returns None (per https://docs.rs/url/2.5.8/url/struct.Url.html#method.port: "default port numbers are never reflected by the serialization"). same for https://...:443. fix: switch to port_or_known_default().is_none() after the scheme allowlist. tests below don't cover :80 or :443 - please add them.
| || api_url.query().is_some() | ||
| || api_url.fragment().is_some() | ||
| { | ||
| return Err(IggyError::InvalidApiUrl(addr.to_string())); |
There was a problem hiding this comment.
port 0 inconsistency with validate_server_address. http://host:0 parses to Some(0) and passes here, but validate_server_address rejects port 0 (line 79, and [::1]:0 test at line 225). either both reject or both accept - align the policy.
| once_cell = { workspace = true } | ||
| papaya = { workspace = true } | ||
| rcgen = { workspace = true } | ||
| reqwest = { workspace = true } |
There was a problem hiding this comment.
pulling full reqwest into the foundation crate just to use reqwest::Url. reqwest's lib.rs is pub use url::Url;, and the workspace already declares url = "2.5.8" in root Cargo.toml. swap this to url = { workspace = true } and import use url::Url; in net.rs:21. avoids dragging hyper / tokio-tls / h2 transitively into every downstream consumer of iggy_common.
| return Err(IggyError::CannotParseUrl); | ||
| } | ||
| let api_url = api_url.unwrap(); | ||
| let api_url = Url::parse(&config.api_url).map_err(|_| IggyError::CannotParseUrl)?; |
There was a problem hiding this comment.
validate_api_url only runs via HttpClientConfigBuilder::build. HttpClient::new, HttpClient::create, and HttpClient::from_connection_string all skip it - only Url::parse runs. direct HttpClientConfig { api_url: ..., ..Default::default() } literal also bypasses. validation contract leaks. move validate_api_url(&config.api_url)? here in create (the real construction boundary) so every entry path enforces it. same gap on quic side via QuicClient::create.
| InvalidIpAddress(String, String) = 35, | ||
| #[error("Http error {0}")] | ||
| HttpError(String) = 36, | ||
| #[error("Invalid Api Url: {0}")] |
There was a problem hiding this comment.
"Invalid Api Url: {0}" doesn't match the acronym style used at line 86 ("Invalid IP address: {0}:{1}"). prefer "Invalid API URL: {0}".
| validate_api_url(&self.config.api_url)?; | ||
|
|
||
| Ok(self.config) | ||
| } |
There was a problem hiding this comment.
no tests for this build(). tcp builder has a full table at tcp_client_config_builder.rs:124-189. add at minimum: default-builds-ok, trim semantics (e.g. " http://x:1 "), and one error case via invalid api_url. same gap in quic builder.
| validate_server_address(&self.config.server_address)?; | ||
|
|
||
| Ok(self.config) | ||
| } |
There was a problem hiding this comment.
no tests for this build(). mirror the table from tcp_client_config_builder.rs:124-189 - default-builds-ok, trim semantics, and one error case via invalid server_address.
| pub fn build(self) -> QuicClientConfig { | ||
| self.config | ||
| pub fn build(mut self) -> Result<QuicClientConfig, IggyError> { | ||
| self.config.server_address = self.config.server_address.trim().to_owned(); |
There was a problem hiding this comment.
uses .to_owned() while tcp/websocket builders use .to_string() (tcp_client_config_builder.rs:106, websocket_client_config_builder.rs:142). functionally identical for &str, but pick one across all four builders. http builder also uses .to_owned() at line 56.
2 participants
Which issue does this PR close?
Closes #3075.
Rationale
To align with the build methods in the
tcp/websocketbuilders, add validation forserver_addressandapi_urlinQuicClientConfigBuilder::build()andHttpClientConfigBuilder::build(), respectively.What changed?
In
QuicClientConfigBuilder::build(),server_addressis validated usingvalidate_server_address.In
HttpClientConfigBuilder::build(), validation is performed in three steps:api_urlcan be successfully parsed byUrlcratehttporhttpsapi_urlcontains only thehostandportcomponents in its URILocal Execution
Passed
Ran
AI Usage