[OIiveirra]

What problem does this PR solve?

Issue Number: N/A

Related PR: #69526

Problem Summary:

LOCAL file discovery could escape the configured secure directory through expanded paths and symlinks. The opt-in setting also needs to cover the LOCAL write entry.

• Disable LOCAL TVF and FILE TVF with local storage by default using the startup-only enable_local_tvf setting. Apply shared admission before INSERT/EXPLAIN property validation and final sink binding, including remote-named sinks selecting the built-in LocalFileJniWriter class, including dot/slash spellings and JNI C-string identity.
• Apply the same gate to native HDFS sinks selecting local storage through the NameNode/default filesystem or the selected Hadoop LocalFileSystem/RawLocalFileSystem provider. Recheck normalized backend properties and the selected filesystem before FE cleanup. Handle trimmed keys and canonical/legacy default-FS aliases without changing the actual Hadoop configuration. Indirect filesystem selectors require the existing startup opt-in; explicit remote configurations and unselected local providers remain available.
• Check existing ADMIN/OPERATOR read privileges before property parsing, backend selection and file discovery, and during authorization.
• Canonicalize the nonempty secure root and each expanded target before containment checks. Treat the canonical root as a literal glob prefix, including when a root symlink resolves to a name containing glob characters. Reject invalid/out-of-root resolution with generic errors.
• Validate LOCAL Lance paths before schema discovery and scanning, resolve relative paths under the secure root, and preserve fixed-version catalog routing. Recognize file URI schemes case-insensitively after SDK-compatible whitespace/control preprocessing, use filesystem-rooted paths for rootless file URLs, normalize URL dot segments and decode once before canonical containment. Normalize bare LOCAL Lance dot segments against the canonical secure root before following symlinks, retaining literal whitespace, percent characters and backslashes. Expand SDK-supported bare ~/~/ home aliases before anchoring and containment. Preserve file URL drive components and raw-drive parent semantics according to the pinned Unix SDK, including the distinct file:/C| and file:///C| identities. Ordinary filesystem/glob behavior is unchanged.
• Preserve LOCAL/S3 scan-range identity and validate the final remote Lance URI for every storage-property type, including Broker passthrough. Reject local aliases, signed ports and malformed IPv6 before SDK access. Preserve supported providers, underscore buckets, hostless GooseFS and S3/OSS HTTP normalization.
• Explicitly enable LOCAL TVF in existing regression deployments to retain their coverage.

Release note

LOCAL TVF and FILE TVF with local storage are disabled by default. Set enable_local_tvf=true in fe.conf on every query-serving FE and restart those FEs to enable them; the setting cannot be changed at runtime. LOCAL INSERT/EXPLAIN and remote-named sinks using the built-in local JNI writer honor the setting. Enabled reads still require ADMIN/OPERATOR and remain inside BE user_files_secure_path, including directories containing glob metacharacters. Resolution errors do not disclose target names.

LOCAL Lance preserves dataset identity for file URI preprocessing, rootless URLs and bare dot segments, while retaining literal bare-path whitespace and secure-root containment. S3 Lance rejects local aliases and malformed remote dataset authorities before opening data.

Hadoop sinks and FE cleanup honor LOCAL admission for their effective local storage. When enable_local_tvf is false, indirect Hadoop provider/default-FS selectors (including runtime references and the native default selector) require enabling the setting and restarting every FE; literal remote configurations and unused local provider settings retain their behavior.

Check List (For Author)

• Test
  • Regression test (assertions added; full local execution incomplete)
  • Unit Test
  • Manual test (add detailed scripts or steps below)
  • No need to test or manual test. Explain why:
    • This is a refactor/code format and no logic has been changed.
    • Previous test can cover this change.
    • No code files have been changed.
    • Other reason

Local validation for the latest revision:

• 45 selected FE tests passed through the standard JDK17 wrapper with fresh XML, zero failures/errors/skips and unchanged sources. Native HDFS cases cover file:/// defaults, selected LocalFileSystem/RawLocalFileSystem, trimmed keys, canonical/legacy aliases, indirect selectors, normalized configuration resources and startup opt-in; explicit remote and unselected local-provider controls pass.

• INSERT/EXPLAIN, direct binding and a global-LOAD command entry are covered. Cleanup tests assert rejection before FileSystemFactory or executor interactions and perform no real filesystem cleanup. Existing class-loader alias checks remain covered.

• Java Checkstyle and git diff --check passed. Independent Standards and Spec reviews found no remaining issue in this supplement.

• Only four FE files changed per branch. BE/Lance source is unchanged; its earlier scoped results remain historical evidence for the unchanged code. Full Groovy regression and actual remote-service data reads remain unverified locally.

• Behavior changed:

  • No.
  • Yes. Enforce startup opt-in and secure local paths; preserve valid local path identity.

• Does this need documentation?

  • No.
  • Yes. Document enable_local_tvf and the LOCAL/FILE-local upgrade behavior. Documentation PR pending.

Check List (For Reviewer who merge this PR)

• Confirm the release note
• Confirm test cases
• Confirm document
• Add branch pick label

[hello-stephen]

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[OIiveirra]

/review

[github-actions]

Request changes: two verified findings are attached inline: a P1 remote-URI validation bypass that reaches local Lance storage, and a P2 regression for valid percent-encoded LOCAL Lance file URIs. There were no existing inline threads or additional user focus points.

Reviewed all 20 changed files at a7a0f19, with independent BE, FE/configuration, and URI/path risk reviews. The policy goal is only partly achieved: direct LOCAL/FILE-local authorization and top-level glob/dataset containment improve, but the S3 URI check still permits local fallback.

Critical checkpoints:

1. Goal and proof: traced constructor authorization, file discovery, schema RPC, range construction, SDK metadata open, dataset/cache key, scan and close. The two inline cases remain incorrect; added tests omit both.
2. Scope and clarity: changes are focused on startup policy, path/URI validation, range identity and supporting tests/configuration.
3. Concurrency and locks: no new thread, lock, lock order or shared production mutation; existing query/scanner and RPC contexts remain in use.
4. Lifecycle/static initialization: local URI values, existing dataset RAII and session ownership remain intact; no new cross-TU initializer dependency or cleanup defect found.
5. Configuration: enable_local_tvf is false by default and nonmutable; ConfigBase rejects runtime updates. Enabling requires configuration and restart on every query-serving FE. Seven regression deployments explicitly opt in.
6. Compatibility: default-off and ADMIN/OPERATOR requirements are intentional behavior changes. Existing fixed-version catalogs retain their warehouse paths; the encoded file-URI regression needs correction. Mixed deployments require upgrading the nodes that enforce the policy.
7. Protocol definitions: no Thrift/protobuf IDs, required fields, function symbols or persistent format definitions changed.
8. Parallel paths: checked LOCAL, FILE delegation, non-Lance glob, Lance schema/latest scan/cache reuse, remote fixed snapshots and catalog/search/row-ID paths. HDFS/HTTP TVFs reject Lance in common parsing.
9. Conditional logic: explicit per-range LOCAL/S3 identity and scan-level fallback agree with their producers; positive catalog versions intentionally bypass LOCAL-TVF containment. The URI predicate has the documented P1 exception.
10. Test coverage: inspected BE containment/symlink/root/schema/scan tests, FE gate/privilege/revocation/URI/range tests, and LOCAL/FILE SELECT/EXPLAIN regression checks. Mocked loader acceptance does not prove Rust provider dispatch; real remote reads and full Groovy execution were not verified here.
11. Expected results: no golden result files changed; added row IDs and file sizes agree with the committed fixture and literals.
12. Observability/errors: new Status failures propagate through existing schema/RPC/scan boundaries; rejected expanded targets use generic errors. Existing open/version/URI profile counters remain. No new execution stage requiring new metrics was introduced.
13. Transactions/persistence/failover: not affected; no EditLog, publish, delete-bitmap or visible-version change.
14. Data writes/atomicity/crashes: production changes are read-only; no new transaction, write rollback or crash-recovery protocol.
15. FE/BE propagation: schema already tags file_type; execution now preserves the TVF identity in the existing optional range field. Both LOCAL schema and execution use the resolver before opening/cache access.
16. Performance/memory: added validation runs at construction or dataset/split setup, outside row loops; no significant new allocation, unbounded cache or read amplification identified.
17. Other checks: decoding, NULLs, row alignment, predicates and EOF behavior are unchanged. Top-level directory containment does not provide an SDK-wide sandbox for preexisting dataset-internal symlinks; no introduced defect was established for that separate limitation.

Static source review only: no builds or tests were run and no product files were changed. No unresolved review candidate or uncovered changed file remains. Operator documentation is still pending according to the PR description; the release note describes the intended startup migration.

Review converged after 3 round(s).

[hello-stephen]

BE UT Coverage Report

Increment line coverage 100.00% (40/40) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	64.28% (29908/46527)
Line Coverage	49.13% (315490/642201)
Region Coverage	44.98% (254167/565092)
Branch Coverage	46.62% (119950/257319)

[hello-stephen]

BE Regression && UT Coverage Report

Increment line coverage 100.00% (40/40) 🎉

Increment coverage report
Complete coverage report

Category	Coverage
Function Coverage	76.38% (34510/45184)
Line Coverage	61.19% (389644/636822)
Region Coverage	57.98% (328173/566003)
Branch Coverage	58.98% (151590/257027)

[OIiveirra]

/review

[OIiveirra]

/review

[github-actions]

Static review of 90a3a93 found one new P2: LOCAL Lance file URIs with parent segments can select a different in-root dataset because filesystem symlinks are resolved before URL dot-segment normalization. The inline comment contains the concrete case and fix direction.

Existing threads were independently revalidated: P1 4361485535 is fixed by original-port digit validation and IPv6 validation; P2 4361485545 is fixed for its escaped-versus-literal path cases by one-pass file-URI decoding. Neither is reposted or carried as a current blocker. The new finding concerns URL dot-segment ordering in the newly added filesystem helper, independently of the earlier percent-decoding issue. No additional user focus was supplied.

Critical checkpoint conclusions:

1. Goal and proof: startup opt-in, early read authorization and pre-open containment are implemented; the new P2 is the remaining identified correctness defect. Focused tests exist but omit its dot-segment/symlink interaction.
2. Scope: changes are focused on LOCAL admission, secure paths, Lance URI identity and regression opt-ins.
3. Concurrency and locks: validation uses existing planning/RPC/scanner contexts and request-local state; no new threads, locks, lock order or lock-held heavy work.
4. Lifecycle and static initialization: dataset ownership/cleanup is unchanged; no new resource cycle or cross-translation-unit initializer dependency.
5. Configuration: enable_local_tvf defaults false and is immutable at runtime. Enabling it requires configuration and restart on every query-serving FE, as described in the PR.
6. Compatibility: the default-disable behavior is intentional; positive-version catalog routing is preserved. File-URI compatibility needs the inline correction. No wire/storage format or function-signature change.
7. Shared definitions: no Thrift/protobuf definitions changed; cross-release field-ID checks are not applicable.
8. Parallel paths: LOCAL/FILE reads, INSERT/EXPLAIN, schema/execution, shared-storage/single-BE, cloud/shared-nothing selection, S3/Broker normalization and fixed-version catalogs were traced.
9. Special conditions: range-type precedence, version-zero LOCAL confinement, catalog exceptions, literal glob roots, provider allowlist, signed ports and IPv6 checks match their producers and pinned SDK behavior, apart from the inline URI-normalization defect.
10. Tests: inspected all changed FE/BE tests and the Groovy suite. Positive/negative admission, privilege ordering, aliases/authorities, range identity, symlink/glob roots and escaped URI cases are represented; dot segments with symlinks are missing. Opted-in pipeline configurations normally exercise the enabled Groovy branch.
11. Results: no expected-result files changed; new expectations agree with inspected inputs. Author-reported test runs were not independently executed.
12. Observability: admission errors are explicit; generic containment errors avoid exposing expanded target names. Existing reader profiles and failure propagation remain in place.
13. Persistence and transactions: no EditLog, transaction publication, visible-version, delete-bitmap or persisted metadata changes.
14. Data writes and crashes: only sink admission changes; external-file write atomicity and crash/cleanup behavior are otherwise unchanged.
15. FE-to-BE transport: existing optional file_type is now explicit on Lance TVF execution ranges; schema requests were already tagged, and catalog/remote snapshot versions remain fixed.
16. Performance and memory: bounded planning/per-open path validation introduces no per-row allocation, retained buffer or cache amplification; per-match canonicalization is required for glob confinement.
17. Further review: rechecked parser/provider dispatch, cached dataset opens, bare percent/query/fragment identity, legacy tags, authorization initialization and symlink resolution. No additional substantiated defect or unresolved code-analysis question remains.

Coverage: all 22 changed paths were reviewed, with two complete-scope reviewers and a separate remote-URI risk reviewer. Static inspection only: no Doris builds, test suites or live SDK/data scans were run; isolated filesystem path demonstrations supported the inline finding. Source files and submodules were unchanged. Runtime interoperability and full regression execution remain validation limits, not claimed results.

Review converged after 2 round(s).

[github-actions]

Static review of all 22 changed paths at 90a3a93aae68c021024842b1f19e32965e730508, with independent BE, FE, and URI-risk coverage.

One new P2 finding is inline: valid uppercase/mixed-case file: schemes now fail LOCAL Lance schema discovery. The existing dot-segment/symlink finding still applies and was not reposted. The earlier signed-port/IPv6 bypass and percent-decoding finding are fixed for their reported cases. No existing P0/P1 was independently confirmed to remain applicable, so existing_blocking_comment_ids is empty. Review opinion: URI compatibility still needs correction.

Individual critical-checkpoint conclusions:

Checkpoint	Conclusion
Goal and functional proof	Default-off LOCAL/FILE-local reads and LOCAL writes, early read authorization, canonical containment, and remote/local Lance routing are implemented on traced paths; new and existing URI compatibility defects remain as identified above. Tests are present, but were not executed here.
Scope and clarity	Production changes focus on admission, path resolution, and range identity, with matching tests/configuration opt-ins.
Concurrency and locking	New state is request-local or immutable after startup. Existing heavy-pool/scanner tasks remain; no new threads, locks, lock order, or lock-held I/O was introduced.
Lifecycle and static initialization	Schema dataset RAII and scan/session ownership remain intact; no new ownership cycle, cache handle, or cross-TU static dependency found.
Configuration	enable_local_tvf=false is startup-only because ConfField defaults to nonmutable. Every serving FE must opt in and restart; seven regression deployment configs opt in.
Compatibility and upgrades	Default-disable is intentional and documented. Existing optional range tags are reused; legacy scan-level fallback, positive-version catalog warehouses, and remote options remain. File-scheme compatibility has the new inline regression. No storage-format/symbol change.
Thrift/protobuf definitions	None changed; cross-release ID/enum comparison is not applicable.
Parallel paths	Checked LOCAL, FILE delegation, SELECT/EXPLAIN, INSERT/EXPLAIN, schema/scan/cache opens, non-Lance glob, remote range override, catalog routing, and the existing LOCAL-backed metadata-glob caller.
Special conditions	Comments explain version-zero LOCAL routing, per-range type precedence, Broker passthrough, GooseFS authority exception, and port/IPv6 validation. The reported URI defects identify remaining incorrect conditions.
Test coverage	FE tests cover gates/auth/delegation/sinks/range tags/remote URI rejection; BE tests cover glob classes, symlinks, literal roots, NUL, relative/escaped datasets, schema/scan identity, and catalog/remote routing. Case-insensitive scheme and known dot-segment identity cases are missing. The Groovy disabled branch requires a separately disabled deployment; unit tests cover disabled admission.
Changed expected results	No .out files or data fixtures changed. Added row/error assertions agree with inspected control flow; execution was not verified.
Observability	Errors propagate through exceptions/Status; glob RPC publishes entries only on overall success. Existing schema warnings and dataset URI/version profiles remain; no new metrics-critical operation introduced.
Transactions and persistence	No EditLog, internal visible-version, delete-bitmap, transaction, or failover protocol change.
Writes and crash behavior	LOCAL sink gate runs before writer creation; existing external-write/cleanup and nontransactional semantics are unchanged. No new crash-cleanup path found.
FE/BE propagation	Lance TVF ranges explicitly carry existing FILE_LOCAL/FILE_S3 tags; schema ranges already did. URI/version/fragments/options and consumer precedence were inspected. No new transmission field.
Performance and memory	Validation is linear in URI/path or match count at planning/discovery/open, outside row/batch loops. No new significant untracked buffer, unbounded cache, or repeated per-row work found.
Other concerns and user focus	No additional user focus was supplied. Every candidate/risk was accepted, deduplicated, or dismissed with evidence; no concrete unresolved static-review coverage gap remains.

Validation limits: static inspection only, including pinned SDK/parser primary sources. No builds, tests, runtime reproduction, deployment restart, real remote-store reads, or exhaustive parser fuzzing were performed. Author-reported test results were treated as context, not independent validation.

Review converged after 2 round(s).

[OIiveirra]

/review

[github-actions]

Static review of #69527 at 33cbacc reports two new P2 dataset-identity regressions in LOCAL Lance file-URI resolution. Both can silently select another existing dataset inside the allowed root. The inline comments provide independent triggering inputs and fix directions.

The four existing threads were independently revalidated: signed ports/malformed IPv6 (4361485535), percent decoding (4361485545), dot-segment/symlink order (4365942546), and case-insensitive schemes (4365978318). Their reported triggers are fixed at this head; no existing P0/P1 remains confirmed. These new findings require neither percent escapes, dot segments nor mixed-case schemes and are distinct from those threads. No additional user focus was supplied.

Critical checkpoints:

1. Goal and evidence: default-deny LOCAL/FILE-local admission and canonical containment are implemented; SDK-compatible file-URI identity still fails in the two inline cases. Added tests exercise the intended gates and prior fixes but omit these collisions.
2. Scope: changes are focused on admission, range identity, filesystem validation and matching tests/configuration.
3. Concurrency: no new threads or locks. Glob/schema RPCs use the existing heavy-work pool; readers own per-scan state, and shared session initialization retains call_once. No new lock-order or heavy-under-lock issue found.
4. Lifecycle/static initialization: validation precedes schema and cached dataset opens; scanner/dataset release order is preserved. No new persistent owner, reference cycle or cross-TU static dependency.
5. Configuration: enable_local_tvf defaults false, is immutable, and explicitly requires every FE restart. Pipeline opt-ins are deliberate.
6. Compatibility: existing file-type fields and values are reused, including scan-level fallback; positive-version catalog and remote ranges preserve their providers. No new storage-format or symbol incompatibility found.
7. Thrift/proto: no definitions changed; cross-release ID comparison is inapplicable.
8. Parallel paths: LOCAL and FILE delegation, SELECT/EXPLAIN, legacy construction, independent INSERT/EXPLAIN sink binding, schema RPC, execution ranges, remote TVFs and local catalogs were traced. Existing sink ADMIN/LOAD admission remains.
9. Conditions: comments explain version-zero LOCAL handling, per-range precedence, canonical containment and final remote-URI validation. Both new URI identity findings apply to schema discovery and scanning.
10. Tests: reviewed positive/negative admission, privilege revocation, URI normalization, symlink/glob containment, literal root operators, remote/catalog preservation, range tags and actual scanner assertions. Disabled-sink regression coverage is conditional on configuration; binder unit tests cover it independently. Add distinguishable datasets and bare-path controls for the two new cases.
11. Results: no expected-result files changed. Assertions/error text were checked statically. No builds, tests or benchmarks were run; prior thread reports are not claimed as this invocation's validation.
12. Observability: existing errors and Lance URI/version/open profiling remain. Admission checks do not need new metrics; no added logging/counter defect found.
13. Persistence/transactions: no EditLog, transaction-state or visibility-protocol changes. Fixed snapshot and delete/bitmap semantics are untouched.
14. Writes/crashes: the independent LOCAL sink gains admission checking; existing writer/coordinator cleanup and file-output atomicity are unchanged. No new crash-related leak/deadlock identified.
15. FE/BE variables: execution now explicitly carries LOCAL/S3 range identity; schema requests already carry it. No additional variable or serialization path is introduced.
16. Performance/memory: added parsing/canonicalization occurs during admission or split setup rather than per row; no new large retained allocation, redundant dataset open or hot-path regression was substantiated.
17. Further sweep: checked parser/provider mismatches, rootless URLs, whitespace preprocessing, second parsing of canonical paths, symlink/dot/decode ordering, partial glob-result leakage and metadata-count/catalog exemptions. No unresolved candidates or uncovered changed paths remain.

Coverage includes all 22 changed files and their relevant upstream/downstream paths. Findings are supported by checked-out code and pinned SDK sources; runtime behavior remains unverified under the static-only contract.

Review converged after 2 round(s).

[OIiveirra]

/review

[github-actions]

Three new findings: one P1 admission bypass, one P2 LOCAL Lance home-path regression, and one P3 drive-style file-URL compatibility regression. Request changes for the P1; all verified findings are inline.

Reviewed exact head 0d96b297bd650f18441aa054b9a4cc49cedb6ed5 against the supplied merge-base diff. All 22 changed files were covered by the main review, separate FE/BE complete reviews, and an additional parser/admission risk review. The review converged in two rounds on this unchanged finding set.

All six existing threads were independently revalidated as fixed: signed-port/mapped-IPv6 local fallback, percent decoding, URL dot-segment/symlink ordering, case-insensitive file schemes, rootless file URLs, and URL whitespace/control preprocessing. No existing P0/P1 remains applicable, so no existing blocking IDs are carried forward and none of those issues is reposted. There was no additional user-provided focus.

Critical checkpoint conclusions:

Checkpoint	Conclusion
Goal and functional proof	Default LOCAL admission and canonical containment improve, but the JNI route bypasses the new switch and two SDK path identities regress. New tests cover the principal named routes; they omit these three cases.
Scope and clarity	Changes remain focused on admission, path handling, scan identity and regression opt-in.
Concurrency and locks	No new workers, shared query state, lock ordering or heavy work inside locks; existing schema/scanner and glob execution contexts remain.
Lifecycle and initialization	Early auth sees initialized superclass state; later auth rechecks resource/global privileges. Schema RAII, reader dataset ownership, session initialization and C-string lifetimes remain valid. No new cross-TU dependency or reference cycle.
Configuration	enable_local_tvf=false is intentionally startup-only; ConfField defaults to immutable and ConfigBase rejects runtime changes. Every serving FE must be configured/restarted.
Compatibility	No wire schema, function signature or storage-format change. Existing range file-type tags are reused. Positive-version catalog warehouses retain routing; complete enforcement requires upgraded BEs.
Thrift/protobuf definitions	None changed; no cross-branch field-ID/enum comparison is applicable.
Parallel paths	LOCAL/FILE reads, INSERT/EXPLAIN, schema/scan, native/JNI writers, S3/Broker normalization and fixed-version catalogs traced. The JNI omission is inline.
Special conditions	URI-only preprocessing/decode/dot ordering and version-zero LOCAL dispatch are justified; tilde and drive exceptions need the inline fixes.
Test coverage	BE tests include real schema/rows, containment, glob classes, symlinks and URI collisions. FE tests cover admission/tagging; S3 positives mock metadata loading. JNI bypass, tilde and drive cases are missing.
Expected results	Changed assertions and expected rows/errors match their fixtures; no generated result files changed.
Observability and errors	New failures are explicit and expanded denied paths are sanitized. Existing dataset profiles/errors remain; no new per-row metrics/logging requirement.
Transactions and persistence	No EditLog, native visibility, delete bitmap or transaction protocol changes; no failover persistence added.
Writes and crash behavior	Named LOCAL writes are gated before property validation. Existing external-file write atomicity/lifetimes are unchanged; JNI can still truncate a local target through a remote name.
FE-to-BE propagation	Schema and execution retain LOCAL/S3 identity through the existing file_type field, including FILE delegation. No new transmitted variable.
Performance and memory	Path checks occur before dataset open/key reuse, with no new per-row work or large allocation. No further substantiated performance or memory defect.
Other risks and disposition	All R1-R3 risks and candidates are accepted, deduplicated or dismissed with evidence in the ledger; no unresolved review-scope candidate remains.

Static review only: no builds or tests were run. SDK behavior was checked against pinned primary sources. Full Groovy execution, disabled-mode SQL/JNI reproduction and real remote data reads remain unexecuted; inspected test code and the PR author's reported runs are not independent runtime validation. These execution limits do not imply uncovered static scope.

Review converged after 2 round(s).

[OIiveirra]

/review

[github-actions]

Request changes: one new P1 remains in effective local-storage admission. A native HDFS sink can select fs.defaultFS=file:/// while LOCAL TVF is disabled, creating BE-local files; its optional cleanup can recursively delete an FE-local directory. The inline finding contains the concrete LOAD-user input and complete call-chain evidence. This is a distinct native path from the already-reported JNI writer issue.

Reviewed exact head d1ec854a5d1a1b6751583f0f900b805b9a702fee against the supplied authoritative merge-base diff: all 23 changed paths, all FE/BE tests and regression configurations, and relevant upstream/downstream paths. No additional user focus was supplied. Static inspection only: no builds, tests, runtime reproductions, source modifications or submodule initialization were performed.

The two existing P1 comments were independently revalidated and their reported instances are fixed: 4366199143 now gates the built-in LocalFileJniWriter across remote sink names, including slash/dot/NUL class identity; 4361485535 now rejects signed ports and malformed IPv6 before remote Lance metadata access. Therefore existing_blocking_comment_ids is empty. Existing BE percent-decoding, dot/symlink, case, whitespace, rootless-URL, drive and home-alias scenarios are also addressed; none is reposted.

Critical checkpoints:

1. Goal and evidence: default-deny LOCAL/FILE-local reads, early ADMIN/OPERATOR authorization, secure expanded-path containment, and consistent LOCAL Lance schema/scan identity are implemented. The native HDFS admission gap prevents the complete local-storage protection goal. Tests exercise the intended controls but omit this native local-provider case.
2. Scope and clarity: production changes are focused on admission, path resolution and range identity, with corresponding unit/regression coverage and explicit CI opt-ins.
3. Concurrency and locks: no new threads, lock acquisitions, lock-order protocol or shared mutable request state is introduced. Added filesystem checks use per-call state; passwd fallback is reentrant. Existing execution concurrency remains unchanged.
4. Lifecycle and static initialization: no new cross-TU initializer dependency, reference cycle or retained owner is introduced. Dataset ownership, session initialization and scanner close order remain intact.
5. Configuration: enable_local_tvf=false is intentionally startup-only; every serving FE must be restarted after enabling it. The annotation is not mutable, and dynamic configuration rejects immutable settings. Regression deployments explicitly opt in.
6. Compatibility: default-disabled behavior is an intentional operational change requiring the documented FE rollout action. No persisted storage format or protocol definition changes; existing optional range types are reused, and fixed-version catalogs preserve their URI routing. Older FE binaries cannot enforce the new FE admission policy during a mixed-version deployment.
7. Thrift/protobuf definitions: no gensrc files changed, so no field-ID/enum compatibility change is present.
8. Parallel paths: LOCAL and FILE delegation, schema and cached-scan opens, legacy scan-level and modern range tags, native/JNI sinks, remote S3 normalization/broker passthrough, HDFS/HTTP Lance rejection and fixed-version catalogs were traced. The one uncovered effective-provider admission path is reported inline.
9. Special conditions: LOCAL/version-zero validation and positive-version catalog exclusion match their callers. URI-only preprocessing preserves ordinary bare-path spelling; root escaping and component containment cover glob operators and symlinks. Effective native HDFS selection needs the additional admission check.
10. Test coverage: inspected positive/negative FE admission, privilege revocation, JNI aliases, remote URI controls, glob/symlink/root containment, and real Lance schema/key/row identity fixtures. End-to-end Groovy execution, real remote-service reads and the new native-HDFS reproduction were not run under this static-only contract.
11. Expected results: no generated .out files changed. Inspected assertions and Groovy backend-error expectations match their code paths; author-reported passing runs were not treated as validation performed here.
12. Observability: new denials provide actionable enablement/privilege errors and intentionally hide expanded external target names. Existing RPC status and reader profiles remain available; no new hot-path metric requirement was identified.
13. Transactions and persistence: no EditLog, transaction commit, visible-version or delete-bitmap protocol changes are introduced. Positive catalog snapshot routing remains unchanged.
14. Writes and crash behavior: accepted requests retain the existing sink lifecycle and its nontransactional file semantics. New admission normally precedes cleanup/execution, but the reported native HDFS bypass permits local side effects before those protections apply; no separate crash/cleanup regression was verified.
15. FE-BE propagation: no new wire variable is added. Explicit Lance range file_type now matches the schema request; per-range identity takes precedence over legacy scan parameters, and URI/version/options remain in dataset cache keys.
16. Performance and memory: parsing/allocation is bounded by input size and path resolution occurs during schema/dataset setup rather than per output row. No new substantial retained allocation, redundant batch operation or material performance regression was verified.
17. Additional issues and limits: the final missed-issue sweep found no second actionable defect. Pathless file-URL query/fragment handling was explicitly assessed as a low-impact parser edge, rather than a root escape or ordinary valid-dataset regression. No assigned changed-file or risk-mechanism coverage remains open; runtime validation remains excluded.

Converged after 2 review rounds: all assigned reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final finding set, required static coverage is complete, and all candidates and risk focuses have explicit dispositions. Convergence describes review completeness; the new P1 still warrants changes. No documentation file is added in this PR; its description says the documentation PR is pending.

Review converged after 2 round(s).

[OIiveirra]

run buildall

[OIiveirra]

/review

[github-actions]

Changes are still required. The new inline P2 identifies LOCAL Lance reads that can escape user_files_secure_path through dataset child symlinks. Existing P1 4366424083 also remains applicable to effective-local native HDFS storage: its direct file:/// example is fixed, but ordinary ViewFS mount delegation still bypasses the switch. I have retained that comment ID instead of posting a duplicate.

For the remaining P1, with enable_local_tvf=false, a global LOAD user can issue:

INSERT INTO hdfs(
  'file_path'='/local/prefix_', 'format'='csv',
  'fs.defaultFS'='viewfs://cluster',
  'fs.viewfs.mounttable.cluster.link./local'='file:///tmp'
) SELECT 1;

Both raw and normalized admission inspect only the selected viewfs scheme/provider. Hadoop's ordinary ViewFileSystem delegates creation and deletion to its mount target, so this reaches BE-local /tmp. With file_path=/local/victim/prefix_ and delete_existing_files=true, FE cleanup likewise reaches recursive deletion of /tmp/victim/. Admission must account for delegated local targets before either side effect. These are static source traces, not executed reproductions.

Critical checkpoint conclusions:

1. Goal and proof: startup opt-in, pre-discovery privilege checks and canonical confinement are implemented broadly; the two containment/admission gaps above prevent considering the security goal satisfied. Added tests cover many direct cases but omit these cases.
2. Scope and clarity: the patch stays focused on admission, path identity, range tags and associated test/deployment changes.
3. Concurrency and locks: no new threads, locks or shared per-query mutable state; checks occur during planning or pre-open setup. No new lock-order issue found.
4. Lifecycle and initialization: existing schema/dataset ownership and close flow are retained; no new cross-translation-unit static initialization dependency or ownership cycle found.
5. Configuration: enable_local_tvf defaults false and is startup-only; its message requires restarting every FE. Seven regression environments explicitly opt in.
6. Compatibility: the disabled default is an intentional operational change described in Config and the release note. No storage format or function ABI changes; legacy scan-level type fallback and positive-version catalogs remain. Older binaries retain their former enforcement during upgrades.
7. Thrift/protobuf: no definitions changed; existing file_type is now explicitly supplied on Lance scan ranges.
8. Parallel paths: LOCAL/FILE reads, SELECT/EXPLAIN, INSERT/EXPLAIN, JNI/native writers, final binding, FE cleanup, schema/scan and catalog/TVF/remote routing were traced. ViewFS delegation and Lance child files are the verified gaps.
9. Conditional checks: parser ordering, C-string identity, aliases and indirect Hadoop selectors have explanatory comments; delegated mount targets remain unhandled.
10. Test coverage: FE admission/auth/remote controls, BE glob/path and real Lance identity fixtures, plus the Groovy startup-config checks were inspected. No builds or tests were run under this static-only contract. Add descendant-symlink and delegated-local write/cleanup cases.
11. Expected results: no golden result files changed; new assertions were reviewed statically without claiming a passing runtime result.
12. Observability: rejection messages identify enable/auth failures and redact expanded filesystem targets; no new per-row logging or metric requirement was identified.
13. Transactions, persistence and failover: no EditLog, internal transaction visibility, partition-version or MoW bitmap changes.
14. Writes, atomicity and crashes: existing external writes/cleanup remain nontransactional; this patch adds admission, with the ViewFS gap described above. No new resource cleanup/lifetime defect found.
15. FE/BE propagation: schema and scan preserve LOCAL/S3 type identity; version-zero LOCAL validation and fixed-version catalog routing are consistent. No other transmitted variable was introduced.
16. Performance and memory: new work is per-plan/per-dataset setup, outside row loops; no material additional hot-path or memory-lifetime issue found.
17. Other issues and existing threads: earlier local JNI-writer P1 4366199143 and remote-authority P1 4361485535 are fixed for their reported paths; earlier percent/dot/case/whitespace/rootless/drive/home findings are addressed and are not reposted.

No additional user focus was supplied. All 24 changed paths and assigned risk scopes received static review. The review converged after three rounds with one new verified P2 and the existing P1 above; no unresolved candidate or static coverage gap remains. Runtime behavior, full regression execution and real remote-service access remain unverified.

Review converged after 3 round(s).

Existing P0/P1 findings confirmed for this head: #69527 (comment)

[github-actions]

[P2] Check containment of the files inside a Lance dataset

With enable_local_tvf=true and user_files_secure_path=/srv/user_files, let alias.lance be a real directory under that root whose _versions and data entries link to the corresponding directories of /srv/private/real.lance. An authorized LOCAL Lance read passes this check because canonicalizing alias.lance does not inspect its children. Schema discovery then lists the external manifests, and scanning opens the external fragment files through the pinned SDK without another containment check. Copying the manifests inside the root and linking only data outward also bypasses row-read confinement. The new secure-root validation therefore leaves a symlink escape even though the dataset directory itself is in-root. Validate the actual LOCAL metadata/fragment targets before opening them and add schema/scan negative coverage for outward child links. This concerns descendant files, separately from the existing requested-URI normalization findings.