Re: [PR] Validate REST table identifiers [iceberg-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

zeroshade commented on code in PR #1354:
URL: https://github.com/apache/iceberg-go/pull/1354#discussion_r3521447982


##########
catalog/catalog.go:
##########
@@ -214,6 +214,16 @@ func NamespaceFromIdent(ident table.Identifier) table.Identifier {
 	return ident[:len(ident)-1]
 }
 
+// ValidateTableIdentifier checks that an identifier contains at least one namespace level and a table name.
+func ValidateTableIdentifier(ident table.Identifier) error {
+	if len(ident) < 2 {

Review Comment:
   This only rejects identifiers with fewer than two elements, so path-altering components still pass validation: `table.Identifier{"ns", ""}`, `{"ns", "."}`, `{"ns", ".."}`, and `{"ns", "a/b"}`. In the REST client these flow through `splitIdentForPath` into `url.URL.JoinPath`, where `/`, `.`, and `..` are interpreted as path structure/cleaning, so the REST path-injection issue is only partially fixed. Please reject empty components, `.`, `..`, `/`, and control characters in every component; preferably also path-encode the final table/view name like Java's `RESTUtil.encodeString` / pyiceberg's `safe=""`, and add tests showing no malformed REST path is generated.



##########
catalog/rest/rest.go:
##########
@@ -1156,8 +1155,8 @@ func (r *Catalog) CommitTransaction(ctx context.Context, commits []table.TableCo
 
 	changes := make([]tableChange, len(commits))
 	for i, c := range commits {
-		if len(c.Identifier) == 0 {
-			return catalog.ErrMissingIdentifier
+		if err := catalog.ValidateTableIdentifier(c.Identifier); err != nil {
+			return fmt.Errorf("%w: %w", catalog.ErrMissingIdentifier, err)

Review Comment:
   Wrapping every malformed identifier as `ErrMissingIdentifier` conflates an absent identifier with a present-but-invalid one. Please keep `ErrMissingIdentifier` only for `len(c.Identifier) == 0`, and otherwise return the original `ValidateTableIdentifier` error.



##########
catalog/rest/rest_test.go:
##########
@@ -2688,13 +2688,41 @@ func (r *RestCatalogSuite) TestCatalogWithCustomTransport() {
 	r.Equal("/v1/config", transport.calls[0].path)
 
 	// Not expected to succeed
-	tbl, err := cat.LoadTable(context.Background(), table.Identifier{"unknown"})
+	tbl, err := cat.LoadTable(context.Background(), table.Identifier{"namespace", "unknown"})
 	r.Error(err)
 	r.Nil(tbl)
 
 	r.Len(transport.calls, 2)
 	r.Equal("GET", transport.calls[1].method)
-	r.Equal("/v1/namespaces/tables/unknown", transport.calls[1].path)
+	r.Equal("/v1/namespaces/namespace/tables/unknown", transport.calls[1].path)
+}
+
+func (r *RestCatalogSuite) TestTableAndViewIdentifiersRequireNamespace() {
+	var transport mockTransport
+
+	cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL, rest.WithCustomTransport(&transport))
+	r.Require().NoError(err)
+	r.Require().Len(transport.calls, 1)
+
+	_, err = cat.LoadTable(context.Background(), table.Identifier{"table"})
+	r.ErrorIs(err, catalog.ErrNoSuchTable)
+
+	_, err = cat.LoadView(context.Background(), table.Identifier{"view"})
+	r.ErrorIs(err, catalog.ErrNoSuchTable)

Review Comment:
   Question: invalid view identifiers now surface `ErrNoSuchTable` rather than `ErrNoSuchView` via the shared table-named helper. That may be acceptable, but it is a little surprising for view APIs.



-- 
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@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org