hubcio commented on code in PR #3152:
URL: https://github.com/apache/iggy/pull/3152#discussion_r3160631229
##########
core/common/src/utils/net.rs:
##########
@@ -122,6 +123,42 @@ fn is_valid_host(host: &str) -> bool {
is_valid_hostname(host)
}
+/// Validates that `addr` is a strict HTTP(S) API base URL in the format
+/// `scheme://host:port`.
+///
+/// Accepted formats:
+/// - `http://hostname:port` / `https://hostname:port`
+/// - `http://ipv4:port` / `https://ipv4:port`
+/// - `http://[ipv6]:port` / `https://[ipv6]:port`
+///
+/// Rejected formats:
+/// - Schemes other than `http` and `https`
+/// - Missing host or missing explicit port
+/// - URLs with additional components beyond `scheme://host:port`,
+/// such as userinfo (`user:pass@`), path, query string, or fragment.
+pub fn validate_api_url(addr: &str) -> Result<(), IggyError> {
+ let api_url = Url::parse(addr).map_err(|_| IggyError::CannotParseUrl)?;
+ match api_url.scheme() {
+ "http" | "https" => {}
+ _ => return Err(IggyError::InvalidApiUrl(addr.to_string())),
+ }
+
+ if api_url.host_str().is_none() || api_url.port().is_none() {
Review 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.
##########
core/common/src/utils/net.rs:
##########
@@ -122,6 +123,42 @@ fn is_valid_host(host: &str) -> bool {
is_valid_hostname(host)
}
+/// Validates that `addr` is a strict HTTP(S) API base URL in the format
+/// `scheme://host:port`.
+///
+/// Accepted formats:
+/// - `http://hostname:port` / `https://hostname:port`
+/// - `http://ipv4:port` / `https://ipv4:port`
+/// - `http://[ipv6]:port` / `https://[ipv6]:port`
+///
+/// Rejected formats:
+/// - Schemes other than `http` and `https`
+/// - Missing host or missing explicit port
+/// - URLs with additional components beyond `scheme://host:port`,
+/// such as userinfo (`user:pass@`), path, query string, or fragment.
+pub fn validate_api_url(addr: &str) -> Result<(), IggyError> {
+ let api_url = Url::parse(addr).map_err(|_| IggyError::CannotParseUrl)?;
+ match api_url.scheme() {
+ "http" | "https" => {}
+ _ => return Err(IggyError::InvalidApiUrl(addr.to_string())),
+ }
+
+ if api_url.host_str().is_none() || api_url.port().is_none() {
+ return Err(IggyError::InvalidApiUrl(addr.to_string()));
+ }
+
+ if !api_url.username().is_empty()
+ || api_url.password().is_some()
+ || api_url.path() != "/"
+ || api_url.query().is_some()
+ || api_url.fragment().is_some()
+ {
+ return Err(IggyError::InvalidApiUrl(addr.to_string()));
Review 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.
##########
core/sdk/src/http/http_client.rs:
##########
@@ -266,11 +266,7 @@ impl HttpClient {
/// Create a new HTTP client for interacting with the Iggy API using the
provided configuration.
pub fn create(config: Arc<HttpClientConfig>) -> Result<Self, IggyError> {
- let api_url = Url::parse(&config.api_url);
- if api_url.is_err() {
- return Err(IggyError::CannotParseUrl);
- }
- let api_url = api_url.unwrap();
+ let api_url = Url::parse(&config.api_url).map_err(|_|
IggyError::CannotParseUrl)?;
Review 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`.
##########
core/common/Cargo.toml:
##########
@@ -53,6 +53,7 @@ moka = { workspace = true }
once_cell = { workspace = true }
papaya = { workspace = true }
rcgen = { workspace = true }
+reqwest = { workspace = true }
Review 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`.
##########
core/common/src/types/configuration/quic_config/quic_client_config_builder.rs:
##########
@@ -154,7 +154,10 @@ impl QuicClientConfigBuilder {
}
/// Finalizes the builder and returns the `QuicClientConfig`.
- 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();
Review 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.
##########
core/common/src/types/configuration/quic_config/quic_client_config_builder.rs:
##########
@@ -154,7 +154,10 @@ impl QuicClientConfigBuilder {
}
/// Finalizes the builder and returns the `QuicClientConfig`.
- 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();
+ validate_server_address(&self.config.server_address)?;
+
+ Ok(self.config)
}
Review 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.
##########
core/common/Cargo.toml:
##########
@@ -67,7 +68,6 @@ tungstenite = { workspace = true }
twox-hash = { workspace = true }
ulid = { workspace = true }
uuid = { workspace = true }
-
[target.'cfg(unix)'.dependencies]
Review Comment:
blank line before `[target.'cfg(unix)'.dependencies]` was removed. restore
it for consistency with the rest of the file.
##########
core/common/src/error/iggy_error.rs:
##########
@@ -86,6 +86,8 @@ pub enum IggyError {
InvalidIpAddress(String, String) = 35,
#[error("Http error {0}")]
HttpError(String) = 36,
+ #[error("Invalid Api Url: {0}")]
Review 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}"`.
##########
core/common/src/types/configuration/http_config/http_client_config_builder.rs:
##########
@@ -52,7 +52,10 @@ impl HttpClientConfigBuilder {
}
/// Builds the `HttpClientConfig` instance.
- pub fn build(self) -> HttpClientConfig {
- self.config
+ pub fn build(mut self) -> Result<HttpClientConfig, IggyError> {
+ self.config.api_url = self.config.api_url.trim().to_owned();
+ validate_api_url(&self.config.api_url)?;
+
+ Ok(self.config)
}
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
