Move validateNamespace call outside of filerClient.WithFilerClient closure
so that validation errors return HTTP 400 (InvalidRequest) instead of 500
(InternalError).
Before: Validation error inside closure → treated as internal error → 500
After: Validation error before closure → handled as bad request → 400
This provides correct error semantics: namespace validation is an input
validation issue, not a server error.
Replace error-swallowing pattern where all errors from getExtendedAttribute
were ignored for bucket policy reads. Now properly distinguish between:
- ErrAttributeNotFound: Policy not found is expected; continue with empty policy
- Other errors: Return internal server error and stop processing
Applied fix to all bucket policy reads in:
- handleDeleteTableBucketPolicy (line 220)
- handleTagResource (line 313)
- handleUntagResource (line 405)
- handleListTagsForResource (line 488)
- And additional occurrences in closures
This prevents silent failures and ensures policy-related errors are surfaced
to callers rather than being silently ignored.
Add *testing.T field to TestCluster struct and initialize it in
startMiniCluster. This allows Stop() to properly log warnings when
cluster shutdown times out. Includes the t field in the test cluster
initialization and restores the logging statement in Stop().
The TestCluster.Stop() method doesn't have access to testing.T object.
Remove the log statement and keep the timeout handling comment for clarity.
The original intent (warning about shutdown timeout) is still captured in
the code comment explaining potential issues.
Replace the custom suffix-only wildcard implementation in matchesActionPattern
and matchesPrincipal with the policy_engine.MatchesWildcard function from
PR #8052. This enables full wildcard support including:
- Middle wildcards: s3tables:Get*Table matches GetTable
- Question mark wildcards: Get? matches any single character
- Combined patterns: s3tables:*Table* matches any action containing 'Table'
Benefits:
- Code reuse: eliminates duplicate wildcard logic
- Complete IAM compatibility: supports all AWS wildcard patterns
- Performance: uses efficient O(n) backtracking algorithm
- Consistency: same wildcard behavior across S3 API and S3 Tables
Add comprehensive unit tests covering exact matches, suffix wildcards,
middle wildcards, question marks, and combined patterns for both action
and principal matching.
The timeout path (2 second wait for graceful shutdown) was silent. Add a
warning log message when it occurs to help diagnose flaky test issues and
indicate when the mini cluster didn't shut down cleanly.
Create extractResourceOwnerAndBucket() helper to consolidate the repeated pattern
of unmarshaling metadata and extracting bucket name from resource path. This
pattern was duplicated in handleTagResource, handleListTagsForResource, and
handleUntagResource. Update all three handlers to use the helper.
Also update remaining uses of getPrincipalFromRequest() (in handler_bucket_create,
handler_bucket_get_list_delete, handler_namespace) to use getAccountID() after
consolidating the two identical methods.
Update handleListTagsForResource to fetch and pass bucket policy to
CheckPermission, matching the behavior of handleTagResource/handleUntagResource.
This enables bucket-policy-based permission grants to be evaluated for
ListTagsForResource, not just ownership-based checks.
Both methods had identical implementations - they return the account ID from
request header or fall back to handler's default. Remove the duplicate
getPrincipalFromRequest and use getAccountID throughout, with updated comment
explaining its dual role as both caller identity and principal for permission
checks.
- Add CanTagResource() to check TagResource permission
- Add CanUntagResource() to check UntagResource permission
- Update CanManageTags() to check both operations (OR logic)
This prevents UntagResource from incorrectly checking 'ManageTags' permission
and ensures each operation validates the correct permission when per-operation
permissions are enforced.
Replace misleading character-only error message with generic 'invalid bucket
name'. The isValidBucketName() function checks multiple constraints beyond
character set (length, reserved prefixes/suffixes, start/end rules), so a
specific character message is inaccurate.
The field name 'Schema' was confusing given it holds a *TableMetadata struct
and serializes as 'metadata' in JSON. Rename to 'Metadata' for clarity and
consistency with the JSON tag and intended meaning.
- Remove dead URL unescape for namespace (regex [a-z0-9_]+ cannot contain
percent-escapes)
- Add URL decoding and validation of extracted table name via
validateTableName() to prevent callers from bypassing request validation
done in other paths
Enforce the same bucket name validation rules (length, characters, reserved
prefixes/suffixes) when extracting from ARN. This prevents accepting ARNs
that the system would never create and ensures consistency with
CreateTableBucket validation.
MaxBuckets is user-controlled and used in uint32(maxBuckets*2) for ListEntries.
Very large values can overflow uint32 or trigger overly expensive scans. Cap
MaxBuckets to 1000 and reject out-of-range values, consistent with MaxTables
handling and S3 MaxKeys validation elsewhere in the codebase.
MaxTables is user-controlled and influences gRPC ListEntries limits via
uint32(maxTables*2). Without an upper bound, very large values can overflow
uint32 or cause excessively large directory scans. Cap MaxTables to 1000 and
return InvalidRequest for out-of-range values, consistent with S3 MaxKeys
handling.
Add request body size limiting (10MB) to readRequestBody method:
- Define maxRequestBodySize constant to prevent unbounded reads
- Use io.LimitReader to enforce size limit
- Add explicit error handling for oversized requests
- Prevents potential DoS attacks via large request bodies
Remove premature HTTP error writes from within WithFilerClient closure
to prevent duplicate status code responses. Error handling is now
consistently performed at the top level using isAuthError.
Relaxed regex validation in utils.go to support hyphens in S3 Tables
namespaces and table names, improving consistency with S3 bucket naming
and allowing derived names from services like S3 Storage Lens.
Updated random string generation to use crypto/rand in s3tables tests.
Increased resilience of IAM distributed tests by adding "connection refused"
to retryable errors.
Refactored resolveResourcePath to return resource type, enabling accurate
NoSuchBucket vs NoSuchTable error codes. Added existence checks before
deleting policies.
Added VersionToken validation to handleDeleteTable. Refactored table
listing to use request context for accurate ARN generation and fixed
cross-namespace pagination issues.
Updated namespace creation to use authenticated account ID for ownership
and unified permission checks across all namespace operations to use the
correct account principal.
Ensured bucket ownership is correctly attributed to the authenticated
account ID and updated ARNs to use the request-derived account ID. Added
standard S3 existence checks for bucket deletion.
Updated getPrincipalFromRequest to prioritize X-Amz-Account-ID header and
added getAccountID helper. Defined ErrVersionTokenMismatch and ErrCodeConflict
for better optimistic concurrency support.
Chris Lu