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}://[email protected]"
+ ))
+ .unwrap();
+ assert_eq!(
+ format!("{abfs:?}"),
+ format!(
+ "ObjectStoreUrl {{ url:
\"{scheme}://account.dfs.core.windows.net\" }}"
+ )
+ );
+ assert_eq!(
+ abfs.as_str(),
+ format!("{scheme}://[email protected]/")
+ );
+ }
+ }
+
+ #[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://[email protected]/path?token=secret",
+ "abfss://[email protected]/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://[email protected]/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}://[email protected]"
+ ))
+ .unwrap();
+ assert_eq!(
+ format!("{abfs:?}"),
+ format!(
+ "ObjectStoreUrl {{ url:
\"{scheme}://account.dfs.core.windows.net\" }}"
+ )
+ );
+ assert_eq!(
+ abfs.as_str(),
+ format!("{scheme}://[email protected]/")
+ );
+ }
+ }
+
+ #[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://[email protected]/path?token=secret",
+ "abfss://[email protected]/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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
