You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "goutamadwant (via GitHub)" <gi...@apache.org> on 2026/10/01 05:15:52 UTC
[PR] fix: redact credentials in object-store URL diagnostics [datafusion]
goutamadwant opened a new pull request, #25927:
URL: https://github.com/apache/datafusion/pull/25927
## Which issue does this PR close?
- Closes #25632.
## Rationale for this change
Object-store URLs can contain credentials, ABFS container identifiers, and query tokens. Debug output and missing-store errors currently expose parts of those URLs in diagnostics.
## What changes are included in this PR?
- Show only the scheme, host, and port in `ObjectStoreUrl` and registry Debug output and in missing-store errors.
- Keep URL parsing, display, registry lookup keys, and serialized scan configurations unchanged.
- Add regression tests for credential redaction, ABFS/ABFSS URLs, registry behavior, and protobuf round-trip preservation.
## What is the testing strategy for this PR?
- `cargo fmt --all -- --check`
- `cargo clippy --all-targets --all-features -- -D warnings`
- `cargo test -p datafusion-execution` (124 unit tests and 19 doc tests passed)
- `cargo test -p datafusion-proto --test proto_integration` (266 tests passed)
## Are there any user-facing changes?
Debug output and missing-store error messages now omit URL userinfo, paths, queries, and fragments. Object-store lookup and serialized URL behavior are unchanged.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: redact credentials in object-store URL diagnostics [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25927:
URL: https://github.com/apache/datafusion/pull/25927#issuecomment-5925508188
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25927?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `98.64865%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.61%. Comparing base ([`1d9be2e`](https://app.codecov.io/gh/apache/datafusion/commit/1d9be2e10794994fc1495c35363658dc7db0adf7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`50a4a35`](https://app.codecov.io/gh/apache/datafusion/commit/50a4a350ee5722631f56a363fdbddc6f8911ba2b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 19 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25927?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/execution/src/object\_store.rs](https://app.codecov.io/gh/apache/datafusion/pull/25927?src=pr&el=tree&filepath=datafusion%2Fexecution%2Fsrc%2Fobject_store.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leGVjdXRpb24vc3JjL29iamVjdF9zdG9yZS5ycw==) | 98.64% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25927?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25927 +/- ##
==========================================
+ Coverage 82.58% 82.61% +0.03%
==========================================
Files 1144 1144
Lines 441510 442975 +1465
Branches 441510 442975 +1465
==========================================
+ Hits 364615 365962 +1347
- Misses 54844 54857 +13
- Partials 22051 22156 +105
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25927?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: redact credentials in object-store URL diagnostics [datafusion]
Posted by "Toby1009 (via GitHub)" <gi...@apache.org>.
Toby1009 commented on code in PR #25927:
URL: https://github.com/apache/datafusion/pull/25927#discussion_r4152591649
##########
datafusion/execution/src/object_store.rs:
##########
@@ -331,4 +357,86 @@ mod tests {
let key = get_url_key(&url.url);
assert_eq!(key.as_str(), "s3://host:123");
}
+
+ #[test]
+ fn test_object_store_url_debug_redacts_userinfo() {
+ let url = ObjectStoreUrl::parse("s3://u%40ser:p%40ss@host:123").unwrap();
+ let debug = format!("{url:?}");
+ assert_eq!(debug, "ObjectStoreUrl { url: \"s3://host:123\" }");
+ assert_eq!(url.as_str(), "s3://u%40ser:p%40ss@host:123/");
+ assert_eq!(url.to_string(), url.as_str());
+
+ let plain = ObjectStoreUrl::parse("s3://bucket").unwrap();
+ assert_eq!(
+ format!("{plain:?}"),
+ "ObjectStoreUrl { url: \"s3://bucket\" }"
+ );
+
+ for scheme in ["abfs", "abfss"] {
+ let abfs = ObjectStoreUrl::parse(format!(
+ "{scheme}://container@account.dfs.core.windows.net"
+ ))
+ .unwrap();
+ assert_eq!(
+ format!("{abfs:?}"),
+ format!(
+ "ObjectStoreUrl {{ url: \"{scheme}://account.dfs.core.windows.net\" }}"
+ )
+ );
+ assert_eq!(
+ abfs.as_str(),
+ format!("{scheme}://container@account.dfs.core.windows.net/")
+ );
+ }
+ }
+
+ #[test]
+ fn test_registry_errors_redact_userinfo_and_query() {
+ let registry = DefaultObjectStoreRegistry::new();
+ for url in [
+ "s3://u%40ser:p%40ss@host:123/path?token=secret#fragment",
+ "abfs://container@account.dfs.core.windows.net/path?token=secret",
+ "abfss://container@account.dfs.core.windows.net/path?token=secret",
+ ] {
+ let url = Url::parse(url).unwrap();
+ let expected = format!(
+ "{}://{}",
+ url.scheme(),
+ &url[url::Position::BeforeHost..url::Position::AfterPort]
+ );
+ for err in [registry.get_store(&url), registry.deregister_store(&url)] {
+ let message = err.err().unwrap().strip_backtrace();
+ assert!(message.contains(&expected), "{message}");
+ for secret in
+ ["u%40ser", "p%40ss", "container", "token=secret", "fragment"]
+ {
+ assert!(!message.contains(secret), "{message}");
+ }
+ }
+ }
+ }
+
+ #[test]
+ fn test_registry_lookup_preserves_original_url() {
+ let registry = DefaultObjectStoreRegistry::new();
+ let url =
+ Url::parse("abfss://container@account.dfs.core.windows.net/path").unwrap();
+ let store: Arc<dyn ObjectStore> = Arc::new(InMemory::new());
+ registry.register_store(&url, Arc::clone(&store));
+ assert!(Arc::ptr_eq(®istry.get_store(&url).unwrap(), &store));
Review Comment:
Would it be useful to extend this with a lookup/deregistration using a different URL that currently resolves to the same key? For example, keep the ABFSS scheme/host/port the same while changing the password and path/query.
Registering and looking up the exact same URL can still pass if both operations change their key construction together. I checked this by temporarily including userinfo in ABFS/ABFSS keys: all six existing object-store tests remained green, while an additional public-API compatibility check failed as expected.
A case with distinct input URLs would give stronger coverage for the promise that registry lookup behavior remains unchanged.
##########
datafusion/execution/src/object_store.rs:
##########
@@ -258,11 +272,22 @@ impl ObjectStoreRegistry for DefaultObjectStoreRegistry {
.get(&s)
.map(|o| Arc::clone(o.value()))
.ok_or_else(|| {
- internal_datafusion_err!("No suitable object store found for {url}. See `RuntimeEnv::register_object_store`")
+ let diagnostic_url = diagnostic_url(url);
+ internal_datafusion_err!("No suitable object store found for {diagnostic_url}. See `RuntimeEnv::register_object_store`")
})
}
}
+/// Format only the scheme, host, and port for diagnostics. Registry keys may
+/// include userinfo (for example, an ABFS container), so do not reuse them here.
Review Comment:
Small documentation suggestion: could we clarify the rationale here? `get_url_key()` currently removes userinfo for every scheme, including ABFS/ABFSS, so keys inserted through `register_store()` do not contain the container username. The new registry Debug test creates that state by inserting directly into the private map.
Keeping diagnostic formatting separate from lookup-key construction makes sense. Perhaps the comment could explain that this keeps the redaction policy independent of future changes to registry identity, and describe the registry Debug handling/test as defensive. The similar comment in `test_registry_debug_redacts_userinfo_in_keys` could use the same clarification.
##########
datafusion/execution/src/object_store.rs:
##########
@@ -331,4 +357,86 @@ mod tests {
let key = get_url_key(&url.url);
assert_eq!(key.as_str(), "s3://host:123");
}
+
+ #[test]
+ fn test_object_store_url_debug_redacts_userinfo() {
+ let url = ObjectStoreUrl::parse("s3://u%40ser:p%40ss@host:123").unwrap();
+ let debug = format!("{url:?}");
+ assert_eq!(debug, "ObjectStoreUrl { url: \"s3://host:123\" }");
+ assert_eq!(url.as_str(), "s3://u%40ser:p%40ss@host:123/");
+ assert_eq!(url.to_string(), url.as_str());
+
+ let plain = ObjectStoreUrl::parse("s3://bucket").unwrap();
+ assert_eq!(
+ format!("{plain:?}"),
+ "ObjectStoreUrl { url: \"s3://bucket\" }"
+ );
+
+ for scheme in ["abfs", "abfss"] {
+ let abfs = ObjectStoreUrl::parse(format!(
+ "{scheme}://container@account.dfs.core.windows.net"
+ ))
+ .unwrap();
+ assert_eq!(
+ format!("{abfs:?}"),
+ format!(
+ "ObjectStoreUrl {{ url: \"{scheme}://account.dfs.core.windows.net\" }}"
+ )
+ );
+ assert_eq!(
+ abfs.as_str(),
+ format!("{scheme}://container@account.dfs.core.windows.net/")
+ );
+ }
+ }
+
+ #[test]
+ fn test_registry_errors_redact_userinfo_and_query() {
+ let registry = DefaultObjectStoreRegistry::new();
+ for url in [
+ "s3://u%40ser:p%40ss@host:123/path?token=secret#fragment",
+ "abfs://container@account.dfs.core.windows.net/path?token=secret",
+ "abfss://container@account.dfs.core.windows.net/path?token=secret",
+ ] {
+ let url = Url::parse(url).unwrap();
+ let expected = format!(
+ "{}://{}",
+ url.scheme(),
+ &url[url::Position::BeforeHost..url::Position::AfterPort]
+ );
+ for err in [registry.get_store(&url), registry.deregister_store(&url)] {
+ let message = err.err().unwrap().strip_backtrace();
+ assert!(message.contains(&expected), "{message}");
+ for secret in
+ ["u%40ser", "p%40ss", "container", "token=secret", "fragment"]
Review Comment:
Could we also assert that the path is omitted? The inputs contain `/path`, but the negative assertions do not cover it, and `message.contains(&expected)` still passes if the path is appended to the safe URL.
As a local experiment, I changed only the two error-formatting calls to append `url.path()`; all six existing object-store tests still passed. The current implementation does remove the path correctly, so this is a coverage suggestion.
Using a distinctive path and asserting that it is absent, or checking a literal expected URL followed by `. See`, would protect the path-redaction behavior described in the PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org