Skip to content

Commit fc9781f

Browse files
fix(rest): reject empty namespace separators (#740)
## Summary - Reject empty REST namespace separators in namespace encode/decode helpers. - Reject empty namespace separators when constructing `ResourcePaths`. - Add regression coverage for direct helper calls and the resource path config boundary. ## Why `DecodeNamespace` advances by `separator.size()` after each match. An empty separator never advances the cursor, so a misconfigured separator can hang namespace parsing. ## Testing - `cmake -S . -B build-rest -G Ninja -DICEBERG_BUILD_BUNDLE=OFF -DICEBERG_BUILD_REST=ON -DICEBERG_BUILD_HIVE=OFF -DICEBERG_BUILD_SQL_CATALOG=OFF` - `cmake --build build-rest --target rest_catalog_test` - `ctest --test-dir build-rest -R rest_catalog_test --output-on-failure`
1 parent 2169e31 commit fc9781f

3 files changed

Lines changed: 35 additions & 0 deletions

File tree

src/iceberg/catalog/rest/resource_paths.cc

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ Result<std::unique_ptr<ResourcePaths>> ResourcePaths::Make(
3434
if (base_uri.empty()) {
3535
return InvalidArgument("Base URI is empty");
3636
}
37+
ICEBERG_PRECHECK(!namespace_separator.empty(),
38+
"REST namespace separator cannot be empty");
3739
return std::unique_ptr<ResourcePaths>(
3840
new ResourcePaths(std::move(base_uri), prefix, namespace_separator));
3941
}

src/iceberg/catalog/rest/rest_util.cc

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,15 @@
3030

3131
namespace iceberg::rest {
3232

33+
namespace {
34+
35+
Status ValidateNamespaceSeparator(std::string_view separator) {
36+
ICEBERG_PRECHECK(!separator.empty(), "REST namespace separator cannot be empty");
37+
return {};
38+
}
39+
40+
} // namespace
41+
3342
std::string_view TrimTrailingSlash(std::string_view str) {
3443
while (!str.empty() && str.back() == '/') {
3544
str.remove_suffix(1);
@@ -67,6 +76,8 @@ Result<std::string> DecodeString(std::string_view str_to_decode) {
6776

6877
Result<std::string> EncodeNamespace(const Namespace& ns_to_encode,
6978
std::string_view separator) {
79+
ICEBERG_RETURN_UNEXPECTED(ValidateNamespaceSeparator(separator));
80+
7081
if (ns_to_encode.levels.empty()) {
7182
return "";
7283
}
@@ -85,6 +96,8 @@ Result<std::string> EncodeNamespace(const Namespace& ns_to_encode,
8596

8697
Result<Namespace> DecodeNamespace(std::string_view str_to_decode,
8798
std::string_view separator) {
99+
ICEBERG_RETURN_UNEXPECTED(ValidateNamespaceSeparator(separator));
100+
88101
if (str_to_decode.empty()) {
89102
return Namespace{.levels = {}};
90103
}

src/iceberg/test/rest_util_test.cc

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525

2626
#include "iceberg/catalog/rest/catalog_properties.h"
2727
#include "iceberg/catalog/rest/endpoint.h"
28+
#include "iceberg/catalog/rest/resource_paths.h"
2829
#include "iceberg/table_identifier.h"
2930
#include "iceberg/test/matchers.h"
3031

@@ -83,6 +84,25 @@ TEST(RestUtilTest, RoundTripNamespaceWithCustomSeparator) {
8384
HasValue(::testing::Eq(ns)));
8485
}
8586

87+
TEST(RestUtilTest, NamespaceRejectsEmptySeparator) {
88+
auto expect_empty_separator_error = [](const auto& result) {
89+
EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
90+
EXPECT_THAT(result, HasErrorMessage("REST namespace separator cannot be empty"));
91+
};
92+
93+
expect_empty_separator_error(EncodeNamespace(Namespace{.levels = {"dogs"}}, ""));
94+
expect_empty_separator_error(EncodeNamespace(Namespace{.levels = {}}, ""));
95+
expect_empty_separator_error(DecodeNamespace("dogs", ""));
96+
expect_empty_separator_error(DecodeNamespace("", ""));
97+
}
98+
99+
TEST(RestUtilTest, ResourcePathsRejectsEmptyNamespaceSeparator) {
100+
auto result = ResourcePaths::Make("https://catalog.example.com", "", "");
101+
102+
EXPECT_THAT(result, IsError(ErrorKind::kInvalidArgument));
103+
EXPECT_THAT(result, HasErrorMessage("REST namespace separator cannot be empty"));
104+
}
105+
86106
TEST(RestUtilTest, EncodeString) {
87107
// RFC 3986 unreserved characters should not be encoded
88108
EXPECT_THAT(EncodeString("abc123XYZ"), HasValue(::testing::Eq("abc123XYZ")));

0 commit comments

Comments
 (0)