[dwsmith1983]

Which issue does this PR close?

Closes #6605.

Items 1 to 5 of that issue describe the native key lookup that #6059 replaced before it merged. The driver now resolves the mechanism through hadoop-azure's own AbfsConfiguration, and the native store reads only the global keys it is sent. This comment maps each item to the parity cases that cover it.

This PR covers item 6, and the same leak in the code around it. The S3 store's own messages are tracked in #7128.

Rationale for this change

A password in an ABFS URL's userinfo, or a SAS token in its query, could reach errors, logs and plan text:

• The native Azure store printed whole URLs in its errors and debug lines, and handed them to object_store, whose errors quote the URL. MicrosoftAzureBuilder::build does this for a host outside the public and Fabric endpoints (a sovereign cloud, a private endpoint, Azurite).
• object_store_authority kept the ABFS password, so it reached the store cache key, the registration URL, the two debug lines in prepare_object_store_with_config_hash, and the deregister_store error.
• object_store_backend ran ObjectStoreScheme::parse on the raw URL before any store was built, and normalize_object_store_url echoed an input it could not parse.
• On the driver, AbfsAuthResolver put the URI into messages that are forwarded to the native error, and into a warning. The decline reason for mixed ABFS stores, which shows in EXPLAIN, and objectStoreKey, printed by the multi-store fallback reasons, kept the raw authority.

A URL from a Spark file listing doesn't normally carry a password or a SAS query, since Hadoop's ABFS reads the part before @ as the container. This is defense in depth.

What changes are included in this PR?

• Native: object_store gets a copy of the URL without the password, query and fragment; the container, which object_store reads from the username, is kept. object_store_authority drops the password, so the cache key and registration URL do too. Messages name a URL by scheme://[container@]host[:port], or by its scheme alone when it can't be parsed. The helpers live in objectstore/url_redaction.rs.
• JVM: NativeConfig.displayAuthority gives the same form, and AbfsAuthResolver's messages and warning, the mixed-store decline reason and objectStoreKey use it. objectStoreKey matches the native key again. The resolver also scrubs the URI's password and query from Hadoop messages it forwards.
• Two ABFS URLs that differ only by password still fall back to Spark: Hadoop reads the whole userinfo as the container, so they may resolve differently. Credentials only ever come from configuration, so dropping the password from the store identity never makes two credentials share a store.

How are these changes tested?

Unit tests in azure.rs, parquet_support.rs, s3_blob_fs_support.rs, NativeConfigSuite and AbfsAuthResolverSuite:

• Errors from the Azure store, from prepare_object_store_with_configs (rejected at the Azure builder, at ObjectStoreScheme::parse and at Url::parse), from deregister_store and from a parse failure carry no username, password or query.
• Two ABFS URLs that differ only by password register under one password-free URL, share one store and leave one cache key. object_store_authority and objectStoreKey drop only the ABFS password, with the container and port kept, and the shared key fixture covers those rows on both sides.
• The resolver's messages, the forwarded error and the mixed-store decline reason name the store without its password, path or query.

Each test fails without its change. The native parquet tests, NativeConfigSuite, AbfsAuthResolverSuite, AbfsAuthParitySuite and the scan fallback suites pass, with cargo fmt, cargo clippy and the strict Scala build.

[sunchao]

Reviewed the full diff from 39380cca813bc0b861353de2cb53af960dd2a87f to c447c1ecf2a7117c22a613dbd1fbcade0787d632: one commit changing native/core/src/parquet/objectstore/azure.rs. The PR is not a draft. Read AGENTS.md and applied .ai/skills/review-comet-pr/SKILL.md. No sibling skill applies. The supplied discussion snapshot and live checks contained no existing reviews or comments.

No introduced P1/P2 issues found within this review.

Summary

• Prior state and problem: Azure store errors and debug messages could include complete URLs, exposing passwords or SAS query strings. Errors generated inside object_store could also echo those fields.
• Design approach: Sanitize URLs before passing them to dependency code, and format diagnostic messages using only the relevant authority components.
• Correctness: Verified ObjectStoreScheme::parse and MicrosoftAzureBuilder::parse_url in pinned object_store 0.13.2. Scheme classification does not require userinfo. The Azure builder requires the username as the container but does not consume URL passwords, queries, or fragments for authentication. The change preserves those existing semantics. All 16 Azure tests and a 32-case base/head comparison passed.
• Compatibility analysis: Checked Spark's InMemoryFileIndex in 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0, which delegates file discovery to Hadoop. Hadoop ABFS 3.3.4, 3.4.1, 3.4.2, and 3.5.0 consistently split the raw authority at @. Normal container/account addressing and driver-selected authentication remain unchanged. Compared with branch-1.1 at 89c729a9697467e8f8d89e3eadf1797a75e2328c, the broader driver-resolution, environment-precedence, and fixed-SAS-key changes already exist in the base. This PR adds intended diagnostic redaction.
• Key design decisions: Keep the ABFS container when constructing the Azure store, but remove usernames before generic scheme validation because another scheme's username may be a credential. Preserve object paths and configured authentication values.
• Implementation sketch: Four private helpers separate URL sanitization from authority formatting. create_store sanitizes scheme validation, both successful builder_for branches sanitize their builder URLs, and diagnostics use restricted authority displays. Three new tests and an extended driver-error test cover the changed behavior.
• Performance: Added URL copies occur during store construction behind the existing store cache. Debug-message formatting remains conditional on logging. No additional network request or work in object-data reads was introduced. No benchmark was run, and no evidence-backed P1/P2 performance regression was identified.
• Design: Sanitizing before the dependency receives the URL addresses dependency-generated errors without relying on matching or rewriting their text. Authentication policy remains in the existing resolver.
• Abstraction & complexity: The helpers are local and distinguish two necessary policies: retaining the Azure container and removing potentially credential-bearing usernames. No broader abstraction or configuration surface is introduced.
• Behavioral changes worth calling out: Diagnostic URLs omit passwords, queries, and fragments. Comet's authority-only messages also omit paths, while dependency errors may still include the preserved path. Sovereign-cloud and other unsupported endpoints remain unsupported.
• Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified.

Exact-head CI: label passed. Comet CI, CodeQL, and Check PR Title report action_required. There is no substantive CI build/test verdict for this head yet.

Validation: From native, cargo test --locked --offline -p datafusion-comet parquet::objectstore::azure::tests --no-default-features passed all 16 tests. An isolated harness using the unchanged source and pinned dependency versions also passed those tests. A disposable 32-case comparison against the pinned base preserved store identity and encoded paths across both ABFS schemes, public/Fabric hosts, explicit ports, queries, and fragments. rustfmt --check and diff whitespace checks passed. The checkout remains unchanged.

Validation limits: The native run disabled default HDFS/Delta features. No full Rust suite, Spark SQL/JVM integration suite, live Azure I/O, or performance benchmark was run.

[andygrove]

Thanks for working on this. The helpers in azure.rs look right to me, and I like that the container stays in the URL handed to object_store. I compared the touched code against branch-1.1 and see no behavior change other than the message text and the stripped password, query and fragment on the builder URL.

I'm wondering how much of the leak is closed in practice, because the callers in parquet_support.rs still echo the raw URL.

For ABFS, object_store_authority slices from BeforeUsername, so url_key in prepare_object_store_with_config_hash includes the password. That string is printed by debug!("Creating new object store for {url_key}") and debug!("Reusing cached object store for {url_key}"). It also goes into the cache key and the registration URL, and deregister_store formats {url} into an error. The PR title and rationale both promise that logs are covered. Could we strip the password there too, or log display_authority-style output instead?

I also think the unrecognised-scheme test covers a path that production doesn't reach. create_store is only called when is_azure_scheme(url.scheme()) holds, so the "Scheme of URL is not Azure" branch is effectively unreachable from there. A URL like foo://alice:pw@host/x?sig=... fails earlier at object_store_backend(&normalized.url, ...). That runs ObjectStoreScheme::parse on the raw URL and returns e.to_string(), which contains Unable to recognise URL "{url}" with the userinfo and query. The Error parsing URL {url_str} message in normalize_object_store_url has the same problem. Could we sanitize at those two sites? A test that goes through prepare_object_store_with_configs would show whether the credentials actually stay out of the final error.

The description says #6059 covered items 1 to 5 of #6605. Could you note on the issue which PR or test covers each item, so it can close cleanly with this PR?

[dwsmith1983]

Could we strip the password there too, or log display_authority-style output instead?

Done in 65b5ad2. object_store_authority drops the ABFS password and keeps the container, so url_key, both debug lines, the cache key and the registration URL are password-free, and deregister_store names the store by its registry key. Credentials only come from configuration, so two URLs that differ only by password now share one store, which a test pins. The JVM side had the same leak: AbfsAuthResolver put the URI into messages that are forwarded to this native error and into a warning, and the mixed-store decline reason and objectStoreKey kept the raw authority. Those now use a password-free form as well, and objectStoreKey matches the native key again.

Could we sanitize at those two sites? A test that goes through prepare_object_store_with_configs would show whether the credentials actually stay out of the final error.

Both are in. object_store_backend parses a copy without userinfo and query, and normalize_object_store_url names only the scheme. prepare_errors_do_not_echo_userinfo_or_query goes through prepare_object_store_with_configs with URLs rejected at each of the three points: the Azure builder, ObjectStoreScheme::parse and Url::parse.

Could you note on the issue which PR or test covers each item, so it can close cleanly with this PR?

Done on the issue.

The S3 store's own messages are left to #7128, which I've narrowed to what remains. The title and description now cover the wider change.

[comphead]

The url-crate based redaction is sound and the cache key stays unique, because credentials enter it through the resolved configs and not the URL. andygrove's points are addressed in the head. One note outside the diff. This PR treats the query as carrying a credential, but redacted_url_display in native/core/src/execution/planner/delta_spark_scan.rs returns a URL without userinfo unchanged and puts the query back after masking userinfo, and DeltaScanSupport.objectStoreRejectedPathReason prints uri.toString when there is no userinfo. Issue #7128 does not mention the Delta helper, so it may be worth moving redacted_url_display next to the new helpers in url_redaction.rs and dropping the query there too, here or in a follow-up. One inline comment.

[comphead]

create_store is only called from parquet_support.rs after is_azure_scheme, and ObjectStoreScheme::parse returns MicrosoftAzure for abfs and abfss URLs with a host, so I could not find a way for this Scheme of URL is not Azure branch to fire from that caller. The redacted helper exists only for it, along with the s3:// and foo:// rows of create_store_errors_do_not_echo_userinfo_or_query and part of create_store_checks_the_scheme_and_returns_the_container_path. Is the branch kept because create_store is pub? If not, naming only the scheme here would let redacted go.

[dwsmith1983]

It was there before this PR and mirrors the Scheme of URL is not S3 check in s3.rs, so I kept the branch since create_store is pub. It now names only the scheme in a3275e6, and redacted is gone.

[dwsmith1983]

@comphead

Issue #7128 does not mention the Delta helper, so it may be worth moving redacted_url_display next to the new helpers in url_redaction.rs and dropping the query there too, here or in a follow-up.

Thanks for checking the cache key. I left the Delta helper out of this one to keep it to the Azure and object store paths, and added redacted_url_display and objectStoreRejectedPathReason to #7128 so they're handled with the other sites.