You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "lucasfang (via GitHub)" <gi...@apache.org> on 2026/08/31 07:34:55 UTC
[PR] fix(catalog): validate identifier names used to build catalog paths [paimon-cpp]
lucasfang opened a new pull request, #264:
URL: https://github.com/apache/paimon-cpp/pull/264
### Purpose
Linked issue: close #xxx
`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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(catalog): validate identifier names used to build catalog paths [paimon-cpp]
Posted by "zjw1111 (via GitHub)" <gi...@apache.org>.
zjw1111 commented on code in PR #264:
URL: https://github.com/apache/paimon-cpp/pull/264#discussion_r3902118562
##########
src/paimon/core/core_options.cpp:
##########
@@ -799,8 +799,14 @@ struct CoreOptions::Impl {
parser.Parse<bool>(Options::PREFETCH_IO_METRICS_ENABLED, &prefetch_io_metrics_enabled));
// Parse scan.fallback-branch - fallback branch when partition not found
PAIMON_RETURN_NOT_OK(parser.Parse(Options::SCAN_FALLBACK_BRANCH, &scan_fallback_branch));
+ if (scan_fallback_branch) {
+ PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(scan_fallback_branch.value()));
+ }
// Parse branch - branch name, default "main"
PAIMON_RETURN_NOT_OK(parser.Parse(Options::BRANCH, &branch));
+ // Both branches name a directory under the table root, so they must stay a single path
+ // component.
+ PAIMON_RETURN_NOT_OK(BranchManager::CheckValidBranch(branch));
Review Comment:
[P2] A whitespace-only branch passes this validation but remains unnormalized in `CoreOptions`. `OrphanFilesCleanerImpl` later passes the raw value from `GetBranch()` to `RealtimeCommitProperties::OffsetsDirectory()`, which resolves it to `branch/branch- /metadata` instead of the main branch's `metadata` directory. Consequently, orphan cleanup for `branch = " "` skips the main branch's real-time offset files even though `CheckValidBranch` treats that value as main.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(catalog): validate identifier names used to build catalog paths [paimon-cpp]
Posted by "zjw1111 (via GitHub)" <gi...@apache.org>.
zjw1111 commented on code in PR #264:
URL: https://github.com/apache/paimon-cpp/pull/264#discussion_r3893803910
##########
src/paimon/core/catalog/file_system_catalog.cpp:
##########
@@ -123,7 +127,8 @@ Result<bool> FileSystemCatalog::TableExists(const Identifier& identifier) const
}
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("");
Review Comment:
why not return `Result`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(catalog): validate identifier names used to build catalog paths [paimon-cpp]
Posted by "zjw1111 (via GitHub)" <gi...@apache.org>.
zjw1111 commented on code in PR #264:
URL: https://github.com/apache/paimon-cpp/pull/264#discussion_r3901527492
##########
src/paimon/rest/rest_catalog.cpp:
##########
@@ -159,16 +159,16 @@ Status RestCatalog::DropDatabase(const std::string& name, bool ignore_if_not_exi
return status;
}
-std::string RestCatalog::GetDatabaseLocation(const std::string& db_name) const {
+Result<std::string> RestCatalog::GetDatabaseLocation(const std::string& db_name) const {
// The virtual "sys" database has no location and is unknown to the server.
if (CatalogUtils::IsSystemDatabase(db_name)) {
- return "";
+ return std::string();
}
Result<GetDatabaseResponse> response = api_->GetDatabase(db_name);
if (!response.ok()) {
PAIMON_LOG_WARN(logger_, "failed to get location of database %s: %s", db_name.c_str(),
response.status().ToString().c_str());
- return "";
+ return std::string();
Review Comment:
already has result, still need empty string here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(catalog): validate identifier names used to build catalog paths [paimon-cpp]
Posted by "lxy-9602 (via GitHub)" <gi...@apache.org>.
lxy-9602 merged PR #264:
URL: https://github.com/apache/paimon-cpp/pull/264
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(catalog): validate identifier names used to build catalog paths [paimon-cpp]
Posted by "zjw1111 (via GitHub)" <gi...@apache.org>.
zjw1111 commented on code in PR #264:
URL: https://github.com/apache/paimon-cpp/pull/264#discussion_r3893948053
##########
src/paimon/core/catalog/catalog_utils.cpp:
##########
@@ -33,6 +36,27 @@ Status SystemTableError(const Identifier& identifier, const std::string& action)
action, identifier.ToString()));
}
+/// Rejects names that cannot be used as a single path component: such a name would make the
+/// path built from it escape the directory it is joined to.
+Status CheckValidIdentifierName(const std::string& kind, const std::string& name) {
+ const char* reason = nullptr;
+ if (StringUtils::IsNullOrWhitespaceOnly(name)) {
+ reason = "cannot be empty or whitespace";
+ } else if (name == "." || name == "..") {
+ reason = "cannot be '.' or '..'";
+ } 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;
Review 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.
##########
src/paimon/core/catalog/catalog_utils.cpp:
##########
@@ -33,6 +36,27 @@ Status SystemTableError(const Identifier& identifier, const std::string& action)
action, identifier.ToString()));
}
+/// Rejects names that cannot be used as a single path component: such a name would make the
+/// path built from it escape the directory it is joined to.
+Status CheckValidIdentifierName(const std::string& kind, const std::string& name) {
+ const char* reason = nullptr;
+ if (StringUtils::IsNullOrWhitespaceOnly(name)) {
+ reason = "cannot be empty or whitespace";
+ } else if (name == "." || name == "..") {
+ reason = "cannot be '.' or '..'";
+ } 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;
+ })) {
+ reason = "cannot contain control characters";
+ }
+ if (reason != nullptr) {
+ return Status::Invalid(fmt::format("{} name {}: '{}'", kind, reason, name));
Review 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.
##########
src/paimon/core/catalog/catalog_utils.cpp:
##########
@@ -70,4 +94,31 @@ Status CatalogUtils::CheckNotBranch(const Identifier& identifier, const std::str
return Status::OK();
}
+Status CatalogUtils::CheckValidDatabaseName(const std::string& db_name) {
+ return CheckValidIdentifierName("database", db_name);
+}
+
+Status CatalogUtils::CheckValidTableName(const Identifier& identifier) {
+ PAIMON_ASSIGN_OR_RAISE(std::string data_table_name, identifier.GetDataTableName());
+ 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()));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(catalog): validate identifier names used to build catalog paths [paimon-cpp]
Posted by "lxy-9602 (via GitHub)" <gi...@apache.org>.
lxy-9602 commented on code in PR #264:
URL: https://github.com/apache/paimon-cpp/pull/264#discussion_r3892968497
##########
src/paimon/core/catalog/file_system_catalog.cpp:
##########
@@ -511,6 +522,7 @@ Status FileSystemCatalog::RenameTable(const Identifier& from_table, const Identi
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));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org