fix(catalog/hadoop): validate table identifiers

[fallintoplace]

Summary

• validate Hadoop table identifiers before converting them to filesystem paths
• return table-shaped errors for invalid table identifiers in write/load/drop/purge paths
• keep CheckTableExists as a pure existence check that returns false, nil for invalid identifiers
• return namespace errors for invalid ListTables namespaces
• add regression coverage for invalid path components and parent-traversal-style identifiers

Why

Hadoop namespace operations already rejected empty components, dot components, parent traversal, and path separators. Table operations only checked identifier length before building paths with filepath.Join, so path-like components could collapse outside the intended table location.

This keeps table operations on the same component-validation invariant as namespace operations while preserving the existing CheckTableExists behavior.

Testing

• git diff --check
• go test ./catalog/hadoop -count=1
• go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.11.4 run --timeout=10m ./catalog/hadoop/...
• go test ./catalog/... -count=1
• go test ./... -count=1

[tanmayrauth]

LGTM, please check the lint issue.

[zeroshade]

Thanks for tightening this up — nice to see table operations brought onto the same path-safety invariant the namespace operations already enforced. LGTM; approving. A few non-blocking observations below.

What works well

• validateWarehousePath is robust. c.warehouse is normalized to an absolute path in NewCatalog (filepath.Abs + trailing-slash trim), so filepath.Rel(warehouse, abs) compares two absolute paths. The .. / ..<sep> / filepath.IsAbs(rel) trio also covers the Windows different-volume / drive-relative escape, so it's meaningful cross-platform defense-in-depth rather than dead code.
• Validation precedes path-building in every method, and the coverage is complete: the six table methods (CreateTable, LoadTable, CheckTableExists, CommitTable, DropTable, PurgeTable) plus ListTables are the full set of path-building entry points. RenameTable and UpdateNamespaceProperties are unimplemented (return errors, no path-building), so nothing is left unguarded.
• The CheckTableExists change is a consistency improvement. Returning (false, ErrNoSuchTable) for an invalid identifier (instead of (false, nil)) matches what the REST, Glue, and Hive catalogs already do for malformed identifiers, and aligns with this catalog's own CheckNamespaceExists. Callers that only read the bool still observe false.
• Strong test matrix. The tablePathOperations helper exercising all six ops uniformly is clean, and TestTableOperationsRejectTraversalOutsideWarehouse is a good regression: it plants a real table + sentinel outside the warehouse and asserts the destructive ops can't reach them.

Verification

• go build ./... and go vet ./catalog/hadoop/ clean.
• go test ./catalog/hadoop/ -count=1 all pass, including the new invalid-identifier matrix (10 identifiers × 6 ops), the traversal test, the ListTables invalid-identifier cases, and the ErrNoSuchTable-tightened short-identifier tests.

Non-blocking observations

1. validateWarehousePath is effectively unreachable after identifier validation on POSIX. Since validateTableIdentifier already rejects /, \, ., .., and empty components, the warehouse-containment branch can realistically only fire on the Windows drive-relative case. That's fine as belt-and-suspenders, but a one-line comment noting it's intentional defense-in-depth would help future readers (and the escape branch currently has no direct test, since the identifier check fires first — an optional small unit test on validateWarehousePath would cover it).
2. Asymmetry across namespace ops. validateNamespacePath (identifier + warehouse check) is wired only into ListTables; CreateNamespace, DropNamespace, CheckNamespaceExists, ListNamespaces, and LoadNamespaceProperties still call bare validateIdentifier. Not a gap (identifier validation already blocks traversal), but using validateNamespacePath there too would make the defense-in-depth symmetric.
3. Minor semantic note. Returning ErrNoSuchTable for a malformed (vs genuinely-absent) identifier slightly overloads that sentinel, but it matches the existing local convention (malformed namespaces were already wrapped in ErrNoSuchNamespace) and the other catalogs, so consistency wins.

None of these block merge.

[laskoviymishka]

This already has two approvals, so I don't want to hold it up lightly, but a few things give me pause, and one I'd like to settle before it lands. Sorry for that.

The one that concerns me most is the CheckTableExists change. Flipping it from (false, nil) to (false, err) for an invalid identifier changes the contract: (false, nil) is the canonical "definitively does not exist" answer, and the REST catalog in this same repo deliberately returns false, nil on ErrNoSuchTable. A pure existence check erroring on trivially-invalid input means any caller guarding if err != nil now takes the error path on what used to be a benign "no", and Hadoop ends up inconsistent with the other in-tree implementation. I'd keep it returning false, nil for identifier-validation failures and leave the erroring to the write/load ops.

The other thing worth a real look is validateWarehousePath: filepath.Abs/filepath.Rel are purely lexical, so once the component check has rejected ., .., /, and , the boundary check is tautologically true for everything that reaches it. The only escape it could guard against is a symlink, which lexical resolution doesn't dereference — so as written it's dead code carrying a security rationale. I'd either make it real with filepath.EvalSymlinks or drop it and document symlinks as out of scope. The traversal test has the same gap: the .. is rejected by identifier validation before the boundary check runs, so the test would still pass if the guard were deleted. And the %v in that function's error wrapping breaks errors.Is on the underlying OS error and will trip errorlint in CI.

Nice cleanup otherwise — collapsing the scattered len(ident) < 2 guards and adding the per-component checks to ListTables is a real improvement. Details inline.

[laskoviymishka]

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.

[laskoviymishka]

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?

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

good to go now!