laskoviymishka commented on code in PR #1228:
URL: https://github.com/apache/iceberg-go/pull/1228#discussion_r3448961945
##########
catalog/hadoop/hadoop_test.go:
##########
@@ -999,9 +1024,121 @@ func (s *HadoopCatalogTestSuite)
TestCreateTableShortIdentifier() {
_, err := s.cat.CreateTable(ctx, []string{"tbl"}, s.testSchema())
s.Require().Error(err)
+ s.ErrorIs(err, catalog.ErrNoSuchTable)
s.Contains(err.Error(), "at least a namespace and table name")
}
+func (s *HadoopCatalogTestSuite) tablePathOperations(ctx context.Context)
[]struct {
+ name string
+ run func(table.Identifier) error
+} {
+ return []struct {
+ name string
+ run func(table.Identifier) error
+ }{
+ {
+ name: "CreateTable",
+ run: func(ident table.Identifier) error {
+ _, err := s.cat.CreateTable(ctx, ident,
s.testSchema())
+
+ return err
+ },
+ },
+ {
+ name: "LoadTable",
+ run: func(ident table.Identifier) error {
+ _, err := s.cat.LoadTable(ctx, ident)
+
+ return err
+ },
+ },
+ {
+ name: "CheckTableExists",
+ run: func(ident table.Identifier) error {
+ _, err := s.cat.CheckTableExists(ctx, ident)
+
+ return err
+ },
+ },
+ {
+ name: "CommitTable",
+ run: func(ident table.Identifier) error {
+ _, _, err := s.cat.CommitTable(ctx, ident, nil,
nil)
+
+ return err
+ },
+ },
+ {
+ name: "DropTable",
+ run: func(ident table.Identifier) error {
+ return s.cat.DropTable(ctx, ident)
+ },
+ },
+ {
+ name: "PurgeTable",
+ run: func(ident table.Identifier) error {
+ return s.cat.PurgeTable(ctx, ident)
+ },
+ },
+ }
+}
+
+func (s *HadoopCatalogTestSuite) TestTableOperationsRejectInvalidIdentifiers()
{
+ ctx := context.Background()
+ tests := []struct {
+ name string
+ ident table.Identifier
+ }{
+ {"empty namespace component", table.Identifier{"", "tbl"}},
+ {"empty table name", table.Identifier{"ns", ""}},
+ {"dot namespace component", table.Identifier{".", "tbl"}},
+ {"dot table name", table.Identifier{"ns", "."}},
+ {"dot dot namespace component", table.Identifier{"..", "tbl"}},
+ {"dot dot table name", table.Identifier{"ns", ".."}},
+ {"slash namespace component", table.Identifier{"a/b", "tbl"}},
+ {"slash table name", table.Identifier{"ns", "a/b"}},
+ {"backslash namespace component", table.Identifier{`a\b`,
"tbl"}},
+ {"backslash table name", table.Identifier{"ns", `a\b`}},
+ }
+
+ for _, tt := range tests {
+ s.Run(tt.name, func() {
+ for _, op := range s.tablePathOperations(ctx) {
+ s.Run(op.name, func() {
+ err := op.run(tt.ident)
+ s.Require().Error(err)
+ s.ErrorIs(err, catalog.ErrNoSuchTable)
+ })
+ }
+ })
Review Comment:
This test doesn't actually exercise the warehouse-boundary check it's named
for.
The `".."` component is rejected by `validateTableIdentifier` before
`validateWarehousePath` is ever reached — if you deleted
`validateWarehousePath` entirely, every assertion here still passes. There's
currently no input that reaches the warehouse check having passed identifier
validation, which is itself the evidence that the check is redundant (see the
note on `validateWarehousePath`).
If `validateWarehousePath` survives as a real symlink guard, I'd add a case
that actually escapes through a symlink. If it doesn't, I'd rename this to
reflect that it's documenting identifier-validation behavior. While we're here,
the inner loop doesn't wrap `op.run` in `s.Run(op.name, ...)` like
`TestTableOperationsRejectInvalidIdentifiers` does, so a failure won't name
which op broke.
##########
catalog/hadoop/hadoop.go:
##########
@@ -397,16 +443,16 @@ func (c *Catalog) LoadTable(ctx context.Context, ident
table.Identifier) (*table
}
func (c *Catalog) CheckTableExists(_ context.Context, ident table.Identifier)
(bool, error) {
- if len(ident) < 2 {
- return false, nil
+ if err := c.validateTablePath(ident); err != nil {
+ return false, err
Review Comment:
This flips the contract for invalid identifiers from `(false, nil)` to
`(false, err)`.
`(false, nil)` is the canonical "definitively does not exist" answer, and
the REST catalog in this repo deliberately returns `false, nil` on
`ErrNoSuchTable` (the `CheckTableExists` path in rest.go). Making Hadoop return
an error for a trivially-invalid input means callers guarding `if err != nil`
now hit the error branch on what used to be a benign "no". I'd keep
`CheckTableExists` returning `false, nil` for identifier-validation failures —
a pure existence check shouldn't error on garbage input — and leave the
erroring behavior to the write/load ops.
##########
catalog/hadoop/hadoop.go:
##########
@@ -164,6 +176,40 @@ func (c *Catalog) tableToPath(ident table.Identifier)
string {
return filepath.Join(append([]string{c.warehouse}, ident...)...)
}
+func (c *Catalog) validateWarehousePath(path string, errType error, objectType
string) error {
+ absPath, err := filepath.Abs(path)
Review Comment:
I don't think this guard buys what the PR description says it does.
`filepath.Abs` and `filepath.Rel` are both purely lexical — neither
dereferences symlinks. By the time we reach here, `validateTableIdentifier` has
already rejected every component that could escape (`.`, `..`, `/`, `\`), so
for any input that gets this far the relative path is guaranteed to stay under
the warehouse. The one case it can't catch — a symlink inside the warehouse
pointing out — is exactly the case lexical resolution misses.
So as written it's effectively dead code with a security rationale, which is
the risky kind: someone later drops the component checks assuming the path
guard covers traversal. I'd either make it real with `filepath.EvalSymlinks`
(falling back to the lexical checks when the target doesn't exist yet, since
the table dir won't exist on create), or drop it and add a comment that symlink
escapes are out of scope. wdyt?
##########
catalog/hadoop/hadoop.go:
##########
@@ -164,6 +176,40 @@ func (c *Catalog) tableToPath(ident table.Identifier)
string {
return filepath.Join(append([]string{c.warehouse}, ident...)...)
}
+func (c *Catalog) validateWarehousePath(path string, errType error, objectType
string) error {
+ absPath, err := filepath.Abs(path)
+ if err != nil {
+ return fmt.Errorf("%w: failed to resolve %s path: %v", errType,
objectType, err)
+ }
Review Comment:
`errType` is wrapped with `%w` but the underlying OS error is dropped with
`%v` here and on the `filepath.Rel` branch below — so `errors.Is(err,
fs.ErrPermission)` and friends are always false. Go 1.20+ allows multiple `%w`
in one `Errorf`, so I'd switch both to `%w`. This is also the classic errorlint
pattern that `.golangci.yml` flags, so CI will fail on it as-is.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
