Skip to content

[fix](tvf) Restrict local paths and require explicit startup opt-in - #69529

Closed
OIiveirra wants to merge 3 commits into
apache:branch-3.1from
OIiveirra:fix/jira-sdbsec-112-3.1
Closed

OIiveirra wants to merge 3 commits into
apache:branch-3.1from
OIiveirra:fix/jira-sdbsec-112-3.1

Conversation

@OIiveirra

@OIiveirra OIiveirra commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: N/A

Related PR: #69526

Problem Summary: Glob results were not anchored to the canonical secure directory. Require explicit startup opt-in and enforce canonical path containment even when local TVF use is enabled.

Summary

LOCAL TVF and FILE TVF with local storage → startup opt-in → ADMIN/OPERATOR → canonical secure-root validation → reader
  • Keep the startup-only enable_local_tvf=false gate before parameter parsing, backend selection and file access.

  • Resolve both the secure root and every glob result to canonical paths, then check component containment. Reject an empty secure root and local-path requests with remote schemes.

  • Enforce ADMIN/OPERATOR before local file access and again during privilege checking.

  • Keep target-validation errors free of wildcard-expanded or resolved target names, including dangling symlinks.

Dedicated regression deployments explicitly opt in to retain LOCAL coverage.

Release note

LOCAL TVF and FILE TVF with local storage is disabled by default. To enable it, set enable_local_tvf=true in fe.conf on every query-serving FE and restart those FEs. The setting is not runtime-mutable. Enabled local reads require ADMIN/OPERATOR and remain restricted to user_files_secure_path.

Target-validation errors no longer include expanded or resolved target filenames.

Evidence

  • Before: Local reads could proceed without startup opt-in, and canonical glob results were not checked against a nonempty secure root.

  • After: Disabled calls stop before file-service interaction; enabled calls apply authorization and canonical containment.

  • 10 focused FE tests passed with no failures or skips.

  • 4 focused BE tests passed with no failures or skips. The BE run used the supported Clang16 ASAN profile.

  • Changed C++ lines pass clang-format 16; relevant Java Checkstyle passes.

  • SQL regression coverage is included; the full Groovy regression runner and a live remote S3 Lance read were not run locally.

  • Supplemental redaction assertions cover outside wildcard matches and dangling symlinks. The 4 focused BE tests were rerun and passed with no failures or skips.

Check List (For Author)

  • Test: focused unit tests and static checks as reported above.
  • Behavior changed: Yes. Explicit startup opt-in, local authorization and secure-directory containment.
  • Documentation: the release note above describes the configuration and behavior change.

Merge Danger

Door: Two-way through configuration and a binary rollback; opt-in retains the new path restrictions.

Blast Radius: LOCAL TVF and FILE TVF with local storage consumers. Existing deployments must explicitly opt in, and paths outside the configured secure directory will be rejected.

Check List (For Reviewer who merge this PR)

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

### What problem does this PR solve?

Issue Number: N/A

Problem Summary: Add the startup-only enable_local_tvf setting, defaulting to false. Reject LOCAL TVF and FILE TVF with local storage before property parsing or backend access. Explicit opt-in restores each branch's existing behavior. Dedicated regression deployments opt in to retain LOCAL 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 restore the existing behavior.

### Check List (For Author)

- Test: 6 focused FE unit tests passed with zero failures or skips; focused FE Checkstyle passed. SQL regression coverage added; runtime validation recorded separately.
- Behavior changed: Yes. Local TVF access requires explicit startup opt-in.
- Does this need documentation: Yes. Release note documents the default and opt-in.
@OIiveirra
OIiveirra requested a review from morrySnow as a code owner October 23, 2026 09:45
Copilot AI balanced review requested due to automatic review settings October 23, 2026 09:45
@hello-stephen

Copy link
Copy Markdown
Contributor

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#69526

Problem Summary: Canonical glob results were not checked against the secure root. Validate canonical local paths against a nonempty canonical secure root, reject remote schemes in the shared local resolver, and enforce ADMIN/OPERATOR before local file access and during privilege checking. Keep the existing default-disabled startup gate. Give the existing glob fixture an explicit secure root so profile-specific test directories do not change its assumptions.

### Release note

Enabled LOCAL reads remain restricted to user_files_secure_path and require ADMIN/OPERATOR privileges.

### Check List (For Author)

- Test: 10 focused FE tests and 4 focused BE tests passed with zero failures/skips; Java Checkstyle and changed-line clang-format 16 passed. BE tests used the supported Clang16 ASAN profile.
- Behavior changed: Yes, canonical secure-root containment and local authorization after explicit opt-in.
- Does this need documentation: Covered by the PR release note.
@OIiveirra OIiveirra changed the title [fix](fe) Disable LOCAL TVF by default (branch-3.1) [fix](tvf) Restrict local paths and require explicit startup opt-in Oct 23, 2026
### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#69526

Problem Summary: A rejected glob result could disclose its expanded filename through a canonical-path error or a failed resolution of a dangling symlink. Return one generic target-validation error for both cases before the status reaches SQL clients. Extend temporary-directory tests to assert that neither real target names nor paths appear in errors.

### Release note

LOCAL path validation errors no longer reveal expanded or resolved target filenames.

### Check List (For Author)

- Test: 4 focused BE tests passed with zero failures/skips, including external wildcard, dangling symlink and error redaction assertions. Modified lines pass clang-format 16; independent Standards and Spec review found no remaining issue in this supplement.
- Behavior changed: Yes, target-validation errors use a generic message.
- Does this need documentation: Covered by the PR release note.
@OIiveirra OIiveirra closed this Oct 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants