fix(catalog): validate identifier names used to build catalog paths - #264
Open
lucasfang wants to merge 8 commits into
Open
fix(catalog): validate identifier names used to build catalog paths#264lucasfang wants to merge 8 commits into
lucasfang wants to merge 8 commits into
Conversation
lxy-9602
reviewed
Aug 31, 2026
zjw1111
reviewed
Aug 31, 2026
zjw1111
reviewed
Aug 31, 2026
zjw1111
left a comment
Collaborator
There was a problem hiding this comment.
Three inline findings from the identifier validation review.
zjw1111
reviewed
Sep 1, 2026
zjw1111
reviewed
Sep 1, 2026
zjw1111
left a comment
Collaborator
There was a problem hiding this comment.
Found one remaining issue in the whitespace-branch orphan-cleanup path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. 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 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. 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::CheckValidDatabaseNameandCheckValidTableNamedelegate to that helper and are 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.FileSystemCatalog::TableExistsandLoadTableSchemavalidate 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.RestCatalogis 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):NewDataTablePathuses the parsed table name rather than the raw object string, soCheckValidTableNamevalidates 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 thatBranchManager::NormalizeBranchmaps tomain(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 reachesBranchManager::BranchPath: the branch component of a table identifier,FileSystemCatalog::ListSnapshots, thebranchandscan.fallback-branchoptions inCoreOptions, andReadContextBuilder::Finish/WriteContextBuilder::Finishfor the branch set throughWithBranch. Without the last three, a branch such asrt/../../../../../outsidestill escaped the table root even though the catalog path was validated.BranchManager::BranchPathnow normalizes its argument as well, because it decided with the exact string comparison ofIsMainBranchand 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, butRealtimeCommitProperties::OffsetsDirectorydoes not: forbranch = " "a commit wrote the real-time offsets under the main branch, sinceFileStoreCommitImplpasses the normalizedSnapshotManager::Branch(), whileOrphanFilesCleanerImplpassed the rawCoreOptions::GetBranch()and looked for them underbranch/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::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,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.TestCheckSinglePathComponentandTestCheckSinglePathComponentEscapesRejectedName: the rule set itself, and the escaping of a rejected name in the error message.BranchManagerTest.TestCheckValidBranch: a branch normalized tomainis accepted,..,rt/../../../../../outsideand a branch with a newline are rejected.BranchManagerTest.TestBranchPathandRealtimeCommitPropertiesTestoffsets-directory assertions: an empty and a whitespace-only branch resolve to the table root and to the main offsets directory rather than to abranch/branch-directory.CoreOptionsTest.TestRejectBranchLeavingTableRoot,ReadContextTest.TestRejectBranchLeavingTablePathandWriteContextTest.TestRejectBranchLeavingRootPath: the same branch is rejected through thebranchandscan.fallback-branchoptions and throughWithBranchon both context builders.RestCatalogTest.DatabaseOperations: adjusted to the newGetDatabaseLocationsignature, and still asserts that a database the server cannot resolve yields an empty location rather than an error.unittesttarget 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::GetDatabaseLocationnow returnsResult<std::string>instead ofstd::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::GetDatabaseLocationkeeps returning an empty string for a database the server cannot resolve, which is now documented ininclude/paimon/catalog/catalog.h. Everything else is internal:PathUtil::CheckSinglePathComponentandBranchManager::CheckValidBranchare new internal helpers,CatalogUtils::CheckValidDatabaseNameandCheckValidTableNameare internal, and the private staticFileSystemCatalog::NewDatabasePathchanges fromstd::stringtoResult<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
GetDatabaseLocationsignature and doc comment described above.Generative AI tooling
Generated-by: Qoder