Skip to content

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

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

fix(catalog): validate identifier names used to build catalog paths#264
lucasfang wants to merge 8 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. The same holds for the branch name, which selects a directory under the table root. This PR adds that validation.

The rule itself lives in PathUtil::CheckSinglePathComponent(kind, name), next to the other path helpers, so that both the catalog and the branch layer share one implementation. 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. A rejected name is escaped in the error message (\n, \r, \t, \\, \x{xx} for any other control byte), so it can neither add a line to the log the error is written to nor truncate the C string it is copied into.

CatalogUtils::CheckValidDatabaseName and CheckValidTableName delegate to that helper and are 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. FileSystemCatalog::TableExists and LoadTableSchema validate the identifier as a whole in addition, because for a system table name they resolve a rebuilt data table identifier that no longer carries the branch component. RestCatalog is out of scope: it resolves a location on the server and builds url-encoded request paths rather than file system paths, and its own name handling is tracked separately. One C++-specific difference from apache/paimon-rust (validate_identifier_name, PR #334): NewDataTablePath uses the parsed table name rather than the raw object string, so CheckValidTableName validates each parsed component (data table name, branch name, system table name).

A branch name is validated through BranchManager::CheckValidBranch, the single place that owns the branch semantics: a branch that BranchManager::NormalizeBranch maps to main (empty or whitespace-only) names no directory of its own and is accepted, anything else must stay a single path component. That check is applied everywhere a caller-supplied branch reaches BranchManager::BranchPath: the branch component of a table identifier, FileSystemCatalog::ListSnapshots, the branch and scan.fallback-branch options in CoreOptions, and ReadContextBuilder::Finish / WriteContextBuilder::Finish for the branch set through WithBranch. Without the last three, a branch such as rt/../../../../../outside still escaped the table root even though the catalog path was validated.

BranchManager::BranchPath now normalizes its argument as well, because it decided with the exact string comparison of IsMainBranch and therefore only behaved correctly for an already normalized branch. Every manager that builds a branch path (SchemaManager, SnapshotManager, TagManager, ConsumerManager) satisfied that implicit contract by normalizing in its own constructor, but RealtimeCommitProperties::OffsetsDirectory does not: for branch = " " a commit wrote the real-time offsets under the main branch, since FileStoreCommitImpl passes the normalized SnapshotManager::Branch(), while OrphanFilesCleanerImpl passed the raw CoreOptions::GetBranch() and looked for them under branch/branch- /metadata. Normalizing in the one function that turns a branch into a path keeps every producer and consumer on the same directory instead of adding a fifth copy of the same normalization.

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, GetDatabaseLocation, ListSnapshots, plus the branch component of a table name and a system table name that carries such a branch), that nothing is created or deleted on those paths, and that a legitimate table in the same warehouse is untouched.
  • PathUtilsTest.TestCheckSinglePathComponent and TestCheckSinglePathComponentEscapesRejectedName: the rule set itself, and the escaping of a rejected name in the error message.
  • BranchManagerTest.TestCheckValidBranch: a branch normalized to main is accepted, .., rt/../../../../../outside and a branch with a newline are rejected.
  • BranchManagerTest.TestBranchPath and RealtimeCommitPropertiesTest offsets-directory assertions: an empty and a whitespace-only branch resolve to the table root and to the main offsets directory rather than to a branch/branch- directory.
  • CoreOptionsTest.TestRejectBranchLeavingTableRoot, ReadContextTest.TestRejectBranchLeavingTablePath and WriteContextTest.TestRejectBranchLeavingRootPath: the same branch is rejected through the branch and scan.fallback-branch options and through WithBranch on both context builders.
  • RestCatalogTest.DatabaseOperations: adjusted to the new GetDatabaseLocation signature, and still asserts that a database the server cannot resolve yields an empty location rather than an error.
  • Local run: the whole unittest target passes, 28/28 test binaries, since the check now sits on the catalog, options and read/write context paths.

API and Format

No storage format change. One public API signature change: Catalog::GetDatabaseLocation now returns Result<std::string> instead of std::string, so that a name that cannot form a location is reported as an error rather than as an empty string. This is source-incompatible for a caller that uses the returned string directly; the repository has no such caller outside the two implementations and their tests. RestCatalog::GetDatabaseLocation keeps returning an empty string for a database the server cannot resolve, which is now documented in include/paimon/catalog/catalog.h. Everything else is internal: PathUtil::CheckSinglePathComponent and BranchManager::CheckValidBranch are new internal helpers, CatalogUtils::CheckValidDatabaseName and CheckValidTableName are internal, and the private static FileSystemCatalog::NewDatabasePath changes from std::string to Result<std::string>.

Documentation

No documentation change needed: this is a robustness fix rather than a new feature, and the only user-visible contract updates are the GetDatabaseLocation signature and doc comment described above.

Generative AI tooling

Generated-by: Qoder

Comment thread src/paimon/core/catalog/file_system_catalog.cpp
Comment thread src/paimon/core/catalog/file_system_catalog.cpp Outdated

@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.

Comment thread src/paimon/core/catalog/catalog_utils.cpp Outdated
Comment thread src/paimon/core/catalog/catalog_utils.cpp Outdated
Comment thread src/paimon/core/catalog/catalog_utils.cpp Outdated
Comment thread src/paimon/rest/rest_catalog.cpp Outdated

@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.

Found one remaining issue in the whitespace-branch orphan-cleanup path.

Comment thread src/paimon/core/core_options.cpp
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