SEZ9 commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-5825759532
Following up on the previous review round against the current head `af472b8f13cc` — all eight points still apply, and the two credential-handling ones are the ones I'd like to see closed before this lands: **F4 (authority_host scheme) — highest priority.** `normalizeAuthorityHost()` in `ADLSRuntimeCompatibility.java` only strips trailing slashes and checks non-blank; it never enforces `https`. Since `ADLSHadoopConf` already hard-codes `abfss` to keep credentials off plain HTTP, the OAuth token endpoint should get the same treatment: reject any `authority_host` whose scheme is not `https` (and ideally validate that it parses as a host-only URI, so a stray path/query can't redirect the POST). Otherwise `client_secret` can be sent in cleartext or to an arbitrary URL. Please also add a negative unit test for `http://` and a malformed value. **F1 (trim mismatch).** `ADLSConfigValidator.required()` validates `value.trim()`, but `ADLSHadoopConf.buildWithReadOnlyConfig()` re-reads the raw `config.get(...)` for `account_name` / `container` / `endpoint_suffix` / `account_key` / `tenant_id` / `client_*`. Whitespace from env-var substitution therefore passes validation and then either throws an un-mapped `IllegalArgumentException` from `validateDnsLabel` or silently ends up inside `account_key`. Simplest fix: have `validate()` return the normalized values and build the Hadoop conf from those, or trim at each `config.get` site in `ADLSHadoopConf`. A test with padded `account_name` and `account_key` would lock this in. **F8.** Related to the above: the `hadoop_adls_properties` protection is a prefix denylist, so class-loading and token-provider ABFS keys still get through despite the "non-routing, non-credential" contract on the option. Either extend the denylist to cover those key families or switch to an allowlist — whichever you pick, please document it alongside F3. **Docs (F2, F3, F5, F6, F7)** — these are straightforward but should go in the same PR: - F2: state the HNS (hierarchical namespace) requirement for the `tmp_path -> path` rename to be atomic, since the sink doc claims exactly-once. - F3: document the validation rules (`account_name`/container naming, auth mutual exclusion, blocked `hadoop_adls_properties` keys) and fix the template placeholders so they pass those rules. - F5: bring the option tables in line with what `ADLSFileSinkFactory` / `ADLSFileSourceFactory` actually expose, and reconcile the en/zh sink tables. - F6: mention the `ConfigShadeUtils` change adding `account_key` to the default log-mask list in both docs and the PR description, since it's a global core-starter change. - F7: fix the English source doc defaults — `discovery_mode` is `ONCE` and `start_mode` is `EARLIEST`, matching the code and the Chinese doc. Once a commit addressing F4 and F1 (with tests) is up, ping me and I'll re-review promptly; the doc items can ride along in the same push. <!-- streview-comment:1297 --> -- 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]
