fix(catalog): validate identifier names used to build catalog paths - #264
fix(catalog): validate identifier names used to build catalog paths#264lucasfang wants to merge 4 commits into
Conversation
| Result<std::vector<SnapshotInfo>> FileSystemCatalog::ListSnapshots( | ||
| const Identifier& identifier, const std::string& branch) const { | ||
| PAIMON_RETURN_NOT_OK(CatalogUtils::CheckValidBranchName(branch)); | ||
| PAIMON_ASSIGN_OR_RAISE(bool exists, TableExists(identifier)); |
There was a problem hiding this comment.
Branch validation does not cover all filesystem entry points
This validation only protects FileSystemCatalog::ListSnapshots and branch names encoded in catalog identifiers. Other public branch inputs, including ReadContextBuilder::WithBranch, WriteContextBuilder::WithBranch, Options::BRANCH, and SCAN_FALLBACK_BRANCH, are still passed to BranchManager::BranchPath without path-safety validation.
For example, if a valid rt branch already exists, a branch value such as rt/../../../../../outside produces an invalid path.
| std::string FileSystemCatalog::GetDatabaseLocation(const std::string& db_name) const { | ||
| return NewDatabasePath(warehouse_, db_name); | ||
| // An invalid name has no valid location, keep the same convention as RestCatalog. | ||
| return NewDatabasePath(warehouse_, db_name).value_or(""); |
zjw1111
left a comment
There was a problem hiding this comment.
Three inline findings from the identifier validation review.
| } else if (name.find('/') != std::string::npos || name.find('\\') != std::string::npos) { | ||
| reason = "cannot contain path separators"; | ||
| } else if (std::any_of(name.begin(), name.end(), [](char c) { | ||
| return std::iscntrl(static_cast<unsigned char>(c)) != 0; |
There was a problem hiding this comment.
StringUtils::IsNullOrWhitespaceOnly and std::iscntrl classify individual UTF-8 bytes under the current C locale, while the referenced Rust implementation uses Unicode-aware trim and char::is_control. Under the typical C locale, a name consisting of U+2003 EM SPACE or containing U+0085 is accepted here but rejected by Rust. C++ can therefore create catalog objects that Rust cannot operate on, and the result also depends on process locale.
| reason = "cannot contain control characters"; | ||
| } | ||
| if (reason != nullptr) { | ||
| return Status::Invalid(fmt::format("{} name {}: '{}'", kind, reason, name)); |
There was a problem hiding this comment.
The rejected name is formatted verbatim even when it contains control characters. A newline remains in the returned Status and can inject additional log lines when the status is propagated, while an embedded NUL can truncate downstream C-string output. The referenced Rust implementation escapes these characters through debug formatting.
| PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("table", data_table_name)); | ||
| PAIMON_ASSIGN_OR_RAISE(std::optional<std::string> branch, identifier.GetBranchName()); | ||
| if (branch) { | ||
| PAIMON_RETURN_NOT_OK(CheckValidIdentifierName("branch", branch.value())); |
There was a problem hiding this comment.
This path sends an identifier branch through the generic validator, so a whitespace-only branch is rejected. CheckValidBranchName below and the existing BranchManager::NormalizeBranch both treat a whitespace-only branch as the main branch. Consequently, ListSnapshots(id, " ") is accepted while the equivalent t$branch_ identifier is rejected, producing inconsistent branch semantics between entry points.
Purpose
FileSystemCatalogbuilds every path it touches from the database name and from the components parsed out of the table name, and it did so without checking that those names are usable as a single path component. This PR adds that validation.The check lives in
CatalogUtilsand is applied inFileSystemCatalog::NewDatabasePathandFileSystemCatalog::NewDataTablePath. Those two functions are the only place where catalog paths are built, so all entry points (create, drop, rename, get,List*,*Exists) are covered at once and future entry points inherit the check. A name is rejected when it is empty or whitespace-only, is exactly.or.., contains/or\, or contains control characters. Validation is purely lexical, so it behaves identically forlocal,oss://,hdfs://and any otherFileSystem, needs no extra IO and has no symlink TOCTOU semantics. Names that merely contain a dot stay valid (my.db,a..b), as do non-ascii names.The rule set, the error wording and the scope follow the equivalent change in apache/paimon-rust (
validate_identifier_name, PR #334): only the filesystem catalog is affected and the helpers stay in the internalCatalogUtilsrather than in a public header.RestCatalogis intentionally left unchanged, becauseResourcePathsurl-encodes every URL segment. One C++-specific difference:NewDataTablePathuses the parsed table name rather than the raw object string, soCatalogUtils::CheckValidTableNamevalidates each parsed component (data table name, branch name, system table name).FileSystemCatalog::ListSnapshotsgets the same check on itsbranchargument, since that value flows intoBranchManager::BranchPath.Behavior change to be aware of: such names used to be accepted silently and now return
Status::Invalidbefore any file system access happens.Tests
FileSystemCatalogTest.TestIdentifierNameValidationRules: table-driven coverage of the rejected forms (""," ",".","..", a name with a slash, a name with a backslash, a name with\n, a name with\0), checked through bothCreateDatabaseandCreateTableagainst the expected error message. The same test asserts thatmy.db,a..band数据remain creatable as databases and thatordersand订单remain creatable as tables, so the rules are not over-tightened.FileSystemCatalogTest.TestRejectInvalidNames: asserts that a rejected name is refused by every catalog entry point (CreateDatabase,DatabaseExists,ListTables,DropDatabase,CreateTable,TableExists,GetTableLocation,GetTable,DropTable,RenameTable,ListSnapshots, plus the branch component of a table name), that nothing is created or deleted on those paths, thatGetDatabaseLocationreturns an empty string, and that a legitimate table in the same warehouse is untouched.paimon-core-test --gtest_filter='FileSystemCatalogTest.*'passes 26/26, and the wholeunittesttarget passes 28/28 test binaries, since name validation now sits on every catalog code path.API and Format
No public API signature, ABI or storage format change.
include/paimon/catalog/catalog.honly gets a doc comment update:GetDatabaseLocationreturnsstd::stringand has no error channel, so it now returns an empty string for a name that does not form a valid location, which matches the existingRestCatalog::GetDatabaseLocationconvention for unknown databases. The private staticFileSystemCatalog::NewDatabasePathchanges fromstd::stringtoResult<std::string>, which is internal only. The newCatalogUtils::CheckValidDatabaseName,CheckValidTableNameandCheckValidBranchNamelive insrc/paimon/core/catalog/catalog_utils.hand are not exported as public API.Documentation
No documentation change needed: this is a robustness fix rather than a new feature, and the only user-visible contract update is the
GetDatabaseLocationdoc comment described above.Generative AI tooling
Generated-by: Qoder