You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "Standing-Man (via GitHub)" <gi...@apache.org> on 2026/04/22 11:43:22 UTC
[PR] feat(config): add address validation for `QUIC/HTTP` builders [iggy]
Standing-Man opened a new pull request, #3152:
URL: https://github.com/apache/iggy/pull/3152
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #3075.
## Rationale
<!--
Why is this change needed? If the issue explains it well, a one-liner is fine.
-->
## What changed?
<!--
2-4 sentences. Problem first (before), then solution (after).
GOOD:
"Messages were unavailable when background message_saver committed the
journal and started async disk I/O before completion. Polling during
this window found neither journal nor disk data.
The fix freezes journal batches in the in-flight buffer before async persist."
GOOD:
"When many small messages accumulate in the journal, the flush passes
thousands of IO vectors to writev(), exceeding IOV_MAX (1024 on Linux)."
BAD:
- Walls of text
- "This PR adds..." (we can see the diff)
-->
## Local Execution
- Passed / not passed
Passed
- Pre-commit hooks ran / not ran
Ran
<!--
You must run your code locally before submitting.
"Relying on CI" is not acceptable - PRs from authors who haven't run the code will be closed.
Did you have `prek` installed? It runs automatically on commit and covers all project languages. See [CONTRIBUTING.md](https://github.com/apache/iggy/blob/master/CONTRIBUTING.md).
-->
## AI Usage
<!--
If AI tools were used, please answer:
1. Which tools? (e.g., GitHub Copilot, Claude, ChatGPT)
2. Scope of usage? (e.g., autocomplete, generated functions, entire implementation)
3. How did you verify the generated code works correctly?
4. Can you explain every line of the code if asked?
If no AI tools were used, write "None" or delete this section.
-->
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "Standing-Man (via GitHub)" <gi...@apache.org>.
Standing-Man commented on PR #3152:
URL: https://github.com/apache/iggy/pull/3152#issuecomment-4387616521
Hi @hubcio, thank you very much for your detailed review. I’ve addressed the following issues in the latest commit:
1. Strengthened `validate_api_url` behavior
* Switched the port check to port_or_known_default() so default ports are treated as valid
* Added explicit rejection for `:0` `(api_url.port() == Some(0))`
* Kept strict rejection for `userinfo/path/query/fragment` components
2. Added URL validation not only in the builders, but also in the constructors.
3. Added corresponding tests for the `TCP` and `QUIC` builders.
4. Replaced `reqwest::Url` usage with `url::Url`.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on PR #3152:
URL: https://github.com/apache/iggy/pull/3152#issuecomment-4295874968
looks good, but next time please comment under the issue before implementation and wait till we give a green light.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "Standing-Man (via GitHub)" <gi...@apache.org>.
Standing-Man commented on PR #3152:
URL: https://github.com/apache/iggy/pull/3152#issuecomment-4296735500
Hi @hubcio and @spetz - Please review at your convinience.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #3152:
URL: https://github.com/apache/iggy/pull/3152#issuecomment-4295872799
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/3152?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 72.16%. Comparing base ([`9b05400`](https://app.codecov.io/gh/apache/iggy/commit/9b0540038533acbfc2eea4d4f4d06dca4f37dd5b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b4efaa8`](https://app.codecov.io/gh/apache/iggy/commit/b4efaa8b0bc0b14ee908aeda570d41506090b272?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 3 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #3152 +/- ##
============================================
- Coverage 73.27% 72.16% -1.12%
Complexity 943 943
============================================
Files 1126 1063 -63
Lines 98435 93617 -4818
Branches 75611 75511 -100
============================================
- Hits 72132 67561 -4571
+ Misses 23696 23447 -249
- Partials 2607 2609 +2
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/3152/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/3152/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.16% <ø> (+<0.01%)` | :arrow_up: |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/3152/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.30% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/3152/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.40% <ø> (ø)` | |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/3152/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.43% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/3152/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.29% <ø> (-4.24%)` | :arrow_down: |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/3152/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.41% <ø> (ø)` | |
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/3152?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [core/common/src/error/iggy\_error.rs](https://app.codecov.io/gh/apache/iggy/pull/3152?src=pr&el=tree&filepath=core%2Fcommon%2Fsrc%2Ferror%2Figgy_error.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jb21tb24vc3JjL2Vycm9yL2lnZ3lfZXJyb3IucnM=) | `100.00% <ø> (ø)` | |
| [...guration/http\_config/http\_client\_config\_builder.rs](https://app.codecov.io/gh/apache/iggy/pull/3152?src=pr&el=tree&filepath=core%2Fcommon%2Fsrc%2Ftypes%2Fconfiguration%2Fhttp_config%2Fhttp_client_config_builder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jb21tb24vc3JjL3R5cGVzL2NvbmZpZ3VyYXRpb24vaHR0cF9jb25maWcvaHR0cF9jbGllbnRfY29uZmlnX2J1aWxkZXIucnM=) | `56.25% <ø> (-4.87%)` | :arrow_down: |
| [...guration/quic\_config/quic\_client\_config\_builder.rs](https://app.codecov.io/gh/apache/iggy/pull/3152?src=pr&el=tree&filepath=core%2Fcommon%2Fsrc%2Ftypes%2Fconfiguration%2Fquic_config%2Fquic_client_config_builder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jb21tb24vc3JjL3R5cGVzL2NvbmZpZ3VyYXRpb24vcXVpY19jb25maWcvcXVpY19jbGllbnRfY29uZmlnX2J1aWxkZXIucnM=) | `0.00% <ø> (ø)` | |
| [core/common/src/utils/net.rs](https://app.codecov.io/gh/apache/iggy/pull/3152?src=pr&el=tree&filepath=core%2Fcommon%2Fsrc%2Futils%2Fnet.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jb21tb24vc3JjL3V0aWxzL25ldC5ycw==) | `99.47% <ø> (ø)` | |
| [core/sdk/src/clients/client\_builder.rs](https://app.codecov.io/gh/apache/iggy/pull/3152?src=pr&el=tree&filepath=core%2Fsdk%2Fsrc%2Fclients%2Fclient_builder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZGsvc3JjL2NsaWVudHMvY2xpZW50X2J1aWxkZXIucnM=) | `60.22% <ø> (+0.34%)` | :arrow_up: |
| [core/sdk/src/http/http\_client.rs](https://app.codecov.io/gh/apache/iggy/pull/3152?src=pr&el=tree&filepath=core%2Fsdk%2Fsrc%2Fhttp%2Fhttp_client.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZGsvc3JjL2h0dHAvaHR0cF9jbGllbnQucnM=) | `92.17% <ø> (+0.25%)` | :arrow_up: |
... and [81 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/3152/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on PR #3152:
URL: https://github.com/apache/iggy/pull/3152#issuecomment-4369847920
@Standing-Man do you have plans to continue? this is good change, just needs a little bit more polishing
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio merged PR #3152:
URL: https://github.com/apache/iggy/pull/3152
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "Standing-Man (via GitHub)" <gi...@apache.org>.
Standing-Man commented on PR #3152:
URL: https://github.com/apache/iggy/pull/3152#issuecomment-4370499319
> @Standing-Man do you have plans to continue? this is good change, just needs a little bit more polishing
@hubcio, I’ll continue polishing this PR — I’ve been a bit busy recently.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(configs): add address validation for `QUIC/HTTP` builders [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on code in PR #3152:
URL: https://github.com/apache/iggy/pull/3152#discussion_r3160631248
##########
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.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org