disk_path.starts_with(user_files_path) is a string-prefix check, so /var/lib/clickhouse/user_files_evil/... is accepted even though it is outside user_files_path. This keeps a path-escape bypass in the server-side guard. Please compare normalized filesystem paths with a boundary-aware helper (for example pathStartsWith on canonicalized paths) instead of starts_with.

Seems valid

This validation is lexical only (lexically_normal / lexically_relative) and does not resolve symlinks. A path like <user_files>/allowed_link/file where allowed_link is a symlink to /tmp still passes the check but accesses data outside user_files. Please validate against weakly_canonical (or use openat-style resolution with RESOLVE_BENEATH) before allowing the operation.

I think this is ok. Our mental model - allow to access everything which is inside user_files_path

Ok, decided to verify symlinks as well

DiskLocal::getObjectStorage is now used as a security gate, but this method is also called from generic disk registration checks (for example DiskSelector::recordDisk compares getObjectStorage for plain disks). In server mode this can reject valid non-user_files local disks and break startup/config validation even when LocalObjectStorage is never used. Please move this restriction to IcebergLocal/StorageLocalConfiguration construction paths (where local object storage is actually requested), or gate it behind an explicit local-object-storage usage path only.

It is what it it, I didn't see failed tests in CI because of this, so I don't see any reasons to change this

Asked @alexey-milovidov for comment

I decided to revert this logic myself. I think that we need to give an opportunity to create disks over arbitrary folder (and leave enforcement only on an Iceberg configuration level)

⚠️ resolvePathRelativelyToBase canonicalizes combined, but compares it against non-canonical norm_base. If user_files_path (base) is configured through a symlink, valid paths under that directory resolve to a different canonical prefix and will be rejected with PATH_ACCESS_DENIED. Please canonicalize the base too (e.g. auto canonical_base = std::filesystem::weakly_canonical(norm_base);) and use canonical_base for both lexically_relative and pathStartsWith checks.

It is very minor, but ok

resolvePathRelativelyToBase dereferences rel.begin() without checking rel.empty(). For inputs where combined_canonical == norm_base (or equivalent), lexically_relative can be empty, and rel.begin()->string() is undefined behavior. Please guard emptiness before dereference (e.g. if (!rel.empty() && rel.begin()->string() == "..")), and keep the prefix check as the primary security check.

Done

dir is not updated here. This expression creates a temporary fs::path and immediately discards it, so the loop keeps using the same directory.

After the first successful rmdir, the next iteration calls rmdir on the same path again and usually throws CANNOT_RMDIR (ENOENT) during removeObject, so deletes can fail even on valid paths.

Please assign the parent path, e.g. dir = dir.parent_path();.

⚠️ The PATH_ACCESS_DENIED message is misleading here: this helper validates all local object paths (reads/writes/deletes), but the text says Explicit metadata file path and table path directory.

Please make this error text generic (for example, "Path {...} is outside the allowed base path {...}") so users can understand failures in non-metadata flows.

combined_canonical validation here is still vulnerable to a TOCTOU race: path resolution and file open happen in separate steps, so a symlink inside user_files can be swapped after this check and before readObject / writeObject open the path.

Because this PR is a security hardening change, the comment "avoid TOCTOU attack" is not correct with the current implementation and may provide a false safety signal. Please either:

1. enforce beneath/no-symlink resolution at open time (e.g. openat2 with RESOLVE_BENEATH|RESOLVE_NO_SYMLINKS on Linux, or equivalent fd-based walk), or
2. at minimum, remove the TOCTOU claim and document that this only blocks lexical/canonical traversal but not concurrent path replacement.

Using tempfile.mktemp here introduces a TOCTOU race: the filename is generated first and opened later, so another process can create/replace that path in between. This can make the test flaky under parallel execution and is generally unsafe.

Please switch to creating the file atomically (tempfile.mkstemp/NamedTemporaryFile(delete=False)) for manifest_list_local, manifest_local, and patched_manifest_local.

nit: is this include needed or injected by IDE?

Why are we allowing any path to be accessed for ch-local? Should we perhaps totally restrict it down to user-files only for any scenario, like we did for Iceberg? If we can't do it because of the wicked amount of tests/supporting tools relying on such behaviour, I'd explicitly call this out in a comment somewhere here

I'm not much familiar with std::filesystem, so please ignore me if I'm confused.

I.
As std::fs::relative already performs weakly_canonical normalization inside and the base path is meant to be absolute, perhaps something like this would be a bit cleaner?

if (!fileOrSymlinkPathStartsWith(path, base_path) || !pathStartsWith(path, base_path))
          throw ....

      return fs::weakly_canonical(path).string();

II.
Certainly out of scope for this change, but since I dug into std::fs::relative I was surprised to learn that it throws on malformed paths. I don't think we need to do anything with it, just sharing.

nit: we're calling the same resolvePathRelativelyToKeyPrefix at !exists(..) one line below. I am not a big fan of that, but happy to leave it as is

Why do we need resolvePathRelativelyToKeyPrefix again here? Isn't resolved_path already resolved?

I might be missing some under-the-hood nuances, but couldn't we just call removeObject(..) without any checks? It seems doing all of them inside as well, from L153:

void LocalObjectStorage::removeObject(const StoredObject & object) const
{
    throwIfReadonly();
    auto resolved_path = resolvePathRelativelyToKeyPrefix(object.remote_path

    if (!exists(object))
        return;
   ....