Skip to content

fix(catalog): validate identifier names used to build catalog paths - #264

Open
lucasfang wants to merge 4 commits into
apache:mainfrom
lucasfang:dev9
Open

fix(catalog): validate identifier names used to build catalog paths#264
lucasfang wants to merge 4 commits into
apache:mainfrom
lucasfang:dev9

Conversation

@lucasfang

@lucasfang lucasfang commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Purpose

FileSystemCatalog builds 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 CatalogUtils and is applied in FileSystemCatalog::NewDatabasePath and FileSystemCatalog::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 for local, oss://, hdfs:// and any other FileSystem, 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 internal CatalogUtils rather than in a public header. RestCatalog is intentionally left unchanged, because ResourcePaths url-encodes every URL segment. One C++-specific difference: NewDataTablePath uses the parsed table name rather than the raw object string, so CatalogUtils::CheckValidTableName validates each parsed component (data table name, branch name, system table name). FileSystemCatalog::ListSnapshots gets the same check on its branch argument, since that value flows into BranchManager::BranchPath.

Behavior change to be aware of: such names used to be accepted silently and now return Status::Invalid before 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 both CreateDatabase and CreateTable against the expected error message. The same test asserts that my.db, a..b and 数据 remain creatable as databases and that orders and 订单 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, that GetDatabaseLocation returns an empty string, and that a legitimate table in the same warehouse is untouched.
  • Full local run: paimon-core-test --gtest_filter='FileSystemCatalogTest.*' passes 26/26, and the whole unittest target 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.h only gets a doc comment update: GetDatabaseLocation returns std::string and has no error channel, so it now returns an empty string for a name that does not form a valid location, which matches the existing RestCatalog::GetDatabaseLocation convention for unknown databases. The private static FileSystemCatalog::NewDatabasePath changes from std::string to Result<std::string>, which is internal only. The new CatalogUtils::CheckValidDatabaseName, CheckValidTableName and CheckValidBranchName live in src/paimon/core/catalog/catalog_utils.h and 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 GetDatabaseLocation doc comment described above.

Generative AI tooling

Generated-by: Qoder

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));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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("");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why not return Result?

@zjw1111 zjw1111 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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()));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

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