fix(writer): reject directory-traversal partition paths across all writers - #19342
fix(writer): reject directory-traversal partition paths across all writers#19342nsivabalan wants to merge 1 commit into
Conversation
…iters A partition-path field value containing "../" segments could escape the table base path and write Hudi-managed files (partition dirs, partition metadata, base/log files) into arbitrary directories reachable by the writer's credentials. URL-encoding of partition values is opt-in and disabled by default, and nothing else validated the value. Reject whole ".." path segments at partition-path computation time, independent of the urlencode setting, in the three independent partition-path implementations so every writer is covered: - KeyGenUtils.getPartitionPath / getRecordPartitionPath and CustomAvroKeyGenerator -> Avro/record path (Spark Datasource, Spark SQL, structured streaming, Hudi Streamer, Java client, Flink record path). - PartitionPathFormatterBase.combine -> Spark row-writer / bulk_insert path. - RowDataKeyGen.getPartitionPath -> Flink row-writer path. New helpers PartitionPathEncodeUtils.hasPathTraversal / validateNoPathTraversal (throws HoodieKeyException) do the check. Only a whole ".." segment causes an escape (per RFC 3986 URI resolution, verified against java.net.URI.resolve), so legitimate values like 2024-01-01, region=us-west-2, and hive-style rider=../evil (which stays contained) are still accepted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
49f1ad9 to
56a446d
Compare
| } | ||
| } | ||
| return partitionPath.toString(); | ||
| return PartitionPathEncodeUtils.validateNoPathTraversal(partitionPath.toString()); |
There was a problem hiding this comment.
the per-row validation is perf sensitive, can we fix the directory-traversal only when hoodie.datasource.write.partitionpath.urlencode=true since the url encode looks like the right way to normalize the invalid chars. instead of throwing, can we automatically encode it as a valid partition path.
Kind of conservative for introducing heavy per-row validations just for data quilities from a rare case.
There was a problem hiding this comment.
yeah. I was also thinking about the same.
but why specific to hoodie.datasource.write.partitionpath.urlencode=true case.
alternatively, we could move this validation to write handles where we try to create the partition meta file. and throw exception that we may not support this. this is 1 validation per write handle, much lighter than above solution.
There was a problem hiding this comment.
🤖 The write-handle approach (makeNewPath / partition meta creation) does seem like the better fit for a "across all writers" guarantee — it's one check per partition per handle rather than per-row, and it naturally covers writers/keygens that don't route through these keygen classes, so you avoid duplicating the check in each generator. One thing worth confirming: metadata-table and bootstrap paths also flow through makeNewPath, so the validation would need to allow legitimate internal paths (e.g. .hoodie/metadata) to avoid false rejects.
There was a problem hiding this comment.
we could move this validation to write handles where we try to create the partition meta file. and throw exception that we may not support this.
yeah, that's much light-weight, just the throwing is not that user friendly since that means the user must update the upstream data source, since we already have url encode, I'm wondering if we can also encode in such case automatically.
if the encode is not that easy to impl, fine with the one-shot validation per partition.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds a .. path-traversal guard to partition-path computation across the Avro, Spark-Row, and Flink RowData key-generator paths so a malicious record field can't write Hudi files outside the table base path. The segment-scan logic and the coverage across the writer implementations look correct; one edge case in the error-message guidance is worth double-checking (see inline). Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One misplaced Javadoc block worth fixing, otherwise the code is clean and readable.
| * @param partitionPath the relative partition path (or a single partition field value). | ||
| * @return the same {@code partitionPath} if it is safe. | ||
| * @throws HoodieKeyException if the partition path contains a {@code ".."} traversal segment. | ||
| */ |
There was a problem hiding this comment.
🤖 The message suggests enabling partitionpath.urlencode to let a legitimate .. value through, but validation runs after encoding and escapePathName doesn't escape . — so a value that is exactly .. still encodes to .. and this will keep throwing. It only helps for multi-segment values like ../evil (where the / gets escaped). Could you reword so the advice doesn't send users down a dead end for the bare .. case?
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🤖 nit: there are two consecutive Javadoc blocks here — the first one (whose @return says "true if a .. traversal segment is present") clearly belongs on hasPathTraversal, not on validateNoPathTraversal. Could you move it down to sit directly above hasPathTraversal? As-is, hasPathTraversal is left undocumented and validateNoPathTraversal has a confusingly mismatched doc block dangling above its real one.
| } | ||
| } | ||
| return partitionPath.toString(); | ||
| return PartitionPathEncodeUtils.validateNoPathTraversal(partitionPath.toString()); |
There was a problem hiding this comment.
🤖 The write-handle approach (makeNewPath / partition meta creation) does seem like the better fit for a "across all writers" guarantee — it's one check per partition per handle rather than per-row, and it naturally covers writers/keygens that don't route through these keygen classes, so you avoid duplicating the check in each generator. One thing worth confirming: metadata-table and bootstrap paths also flow through makeNewPath, so the validation would need to allow legitimate internal paths (e.g. .hoodie/metadata) to avoid false rejects.
4 participants
Describe the issue this Pull Request addresses
A community user reported that Hudi does not sanitize partition-path values, allowing a directory-traversal write outside a table's declared base path, driven purely by the content of one ingested record field.
Hudi's built-in key generators compute a record's partition path directly from the configured partition field's raw value. Escaping of that value only happens when
hoodie.datasource.write.partitionpath.urlencode=true, which is opt-in and disabled by default, and even that escaping never rejects... The unsanitized value is then joined onto the base path (FSUtils.constructAbsolutePath→StoragePath), which resolves../segments withjava.net.URI.resolvesemantics, so a partition value like../../../../tmp/evilon a single record causesHoodieWriteHandle.makeNewPathto create a partition directory and write base/log files completely outside the table base path.In multi-tenant or shared-storage deployments where one writer's credentials span many tables, a lower-trust data producer (e.g. an upstream Kafka producer feeding a Hudi Streamer job) can influence the partition field and write Hudi-format files into other teams' or tenants' table directories, causing metadata pollution and potential corruption. This was reproduced end-to-end against the released
release-1.2.0artifacts using the real write client (no mocks).Summary and Changelog
Rejects partition paths containing directory-traversal (
..) segments so a record can no longer write Hudi files outside the table base path. The check runs at partition-path computation time, independent of theurlencodesetting (which is opt-in and off by default).New helpers
PartitionPathEncodeUtils.hasPathTraversal/validateNoPathTraversal(the latter throwsHoodieKeyException) reject any whole..path segment. They are wired into the three independent partition-path implementations, so all writers are covered at the source:KeyGenUtils.getPartitionPath/getRecordPartitionPathandCustomAvroKeyGenerator.getPartitionPath— Avro/record path (Spark Datasource, Spark SQL, structured streaming, Hudi Streamer, Java client, and the Flink record path).PartitionPathFormatterBase.combine— Spark row-writer / bulk_insert path (reached fromBuiltinKeyGenerator.getPartitionPathandHoodieDatasetBulkInsertHelper).RowDataKeyGen.getPartitionPath— Flink row-writer path.Only a whole
..segment causes an escape (per RFC 3986 URI resolution, verified againstjava.net.URI.resolve), so legitimate values such as2024-01-01,region=us-west-2, and hive-stylerider=../evil(which resolves tobase/rider=../evil, contained) are still accepted.Tests:
TestPartitionPathEncodeUtils(new) — validator allow/reject cases, including hive-style and backslash variants.TestKeyGenUtils#testGetPartitionPathRejectsPathTraversal— single- and multi-field keygen paths reject a..value with url-encoding off, and a legitimate value is still accepted.Impact
Writers now fail fast with a clear
HoodieKeyExceptionwhen a computed partition path contains a..traversal segment. Any existing pipeline that legitimately produced a partition value that is exactly..(or contains a whole..segment) without url-encoding would now be rejected; such values were never safe as directory names. Read/query, rollback, and sync paths are unchanged.Risk Level
low
The change adds validation only at partition-path computation. It does not alter the resolved path for any legitimate partition value (verified against
java.net.URI.resolvesemantics and covered by unit tests for allowed values). New unit tests cover both the allowed and rejected cases across the Avro, Spark-row, and Flink-row paths.Documentation Update
none
Contributor's checklist