comphead commented on code in PR #7556:
URL: https://github.com/apache/datafusion-comet/pull/7556#discussion_r4532818588
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -69,11 +70,12 @@ pub fn create_store(
configs: &HashMap<String, String>,
min_ttl: Duration,
) -> Result<(Box<dyn ObjectStore>, Path), object_store::Error> {
- let (scheme, path) = ObjectStoreScheme::parse(url)?;
+ // The parser quotes the URL it rejects.
+ let (scheme, path) = ObjectStoreScheme::parse(&without_credentials(url))?;
Review Comment:
`create_store` strips the URL here for the parser and then passes the
original `url` to `S3StoreTemplate::new` (both call sites below), which strips
it again at line 186. Could `create_store` start with `let url =
&without_credentials(url);` and use it throughout, with `S3StoreTemplate::new`
storing `url.to_string()` as before? `host_str()` and `path()` are unchanged by
the stripping, and `create_store` is the only non-test caller of
`S3StoreTemplate::new`, so one strip would cover the parser, the error and the
template.
##########
native/core/src/parquet/objectstore/s3_blob_fs_support.rs:
##########
@@ -523,4 +525,30 @@ mod tests {
// than opening a bucketless URL. (The JVM gate already declines
these; defense in depth.)
assert!(promote_hostless_alias_url("blob:///").is_err());
}
+
+ #[test]
+ fn promote_errors_do_not_echo_the_url() {
+ let err =
+
promote_hostless_alias_url("blob://alice:SECRET@bucket:notaport/x?sig=SAS-MARKER")
+ .unwrap_err()
+ .to_string();
+ for leaked in ["alice", "SECRET", "SAS-MARKER"] {
+ assert!(!err.contains(leaked), "{leaked} echoed: {err}");
+ }
+ assert!(
+ err.contains("Error parsing blob URL: invalid port number"),
+ "{err}"
+ );
+ }
Review Comment:
This is the same check as the existing
`parse_errors_name_only_a_valid_scheme` above, with the same `bucket:notaport`
URL shape, only going through `promote_hostless_alias_url`. Could it be one
more case in that test instead of a new one?
--
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]
