Repository navigation
fix: redact credentials in object-store URL diagnostics - #25927
goutamadwant wants to merge 2 commits into
Conversation
github-actions
Bot
added
execution
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25927 +/- ##
==========================================
+ Coverage 82.58% 82.90% +0.31%
==========================================
Files 1144 1154 +10
Lines 441510 458114 +16604
Branches 441510 458114 +16604
==========================================
+ Hits 364615 379786 +15171
- Misses 54844 55290 +446
- Partials 22051 23038 +987 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Toby1009
left a comment
Toby1009
left a comment
There was a problem hiding this comment.
Thanks for working on this, Goutam! This looks good overall and matches the direction discussed in #25632: keep redaction in diagnostics while preserving the underlying URL and existing lookup/serialization behavior.
I especially appreciate the explicit URL assertions in the protobuf round-trip test. Since Debug now hides userinfo, comparing Debug output alone could miss data loss; I verified that the new test catches a deliberately introduced Display-redaction regression.
I reviewed the formatting, registry, and serialization paths. The six object-store unit tests, the new protobuf test, and four additional public-API checks all passed locally on this commit.
I have a few non-blocking suggestions below to clarify the comments and strengthen the regression coverage. These are about guarding against future regressions; I did not find those regressions in the current implementation.
| } | ||
|
|
||
| /// 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. |
There was a problem hiding this 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.
| let message = err.err().unwrap().strip_backtrace(); | ||
| assert!(message.contains(&expected), "{message}"); | ||
| for secret in | ||
| ["u%40ser", "p%40ss", "container", "token=secret", "fragment"] |
There was a problem hiding this 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.
| 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)); |
There was a problem hiding this 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.
3 participants
Which issue does this PR close?
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?
ObjectStoreUrland registry Debug output and in missing-store errors.What is the testing strategy for this PR?
cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo 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.