nsivabalan commented on code in PR #19342:
URL: https://github.com/apache/hudi/pull/19342#discussion_r3716642939
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/keygen/CustomAvroKeyGenerator.java:
##########
@@ -161,7 +162,7 @@ public String getPartitionPath(GenericRecord record) {
partitionPath.append(DEFAULT_PARTITION_PATH_SEPARATOR);
}
}
- return partitionPath.toString();
+ return
PartitionPathEncodeUtils.validateNoPathTraversal(partitionPath.toString());
Review Comment:
Moved the check into the write handles as discussed — pushed in 04bc6c0. It
now runs once per handle (i.e. once per partition being written), right before
the partition directory and its metafile get created, and all four keygen call
sites are reverted to master. Covered:
- `HoodieWriteHandle` constructor — one choke point for every record-path
handle across Java/Spark/Flink (create, merge, append, clustering, binary copy).
- `HoodieRowCreateHandle` (Spark row-writer / bulk_insert) and
`HoodieRowDataCreateHandle` (Flink row-writer) — these don't extend
`HoodieWriteHandle`, so they needed it separately. I verified every bulk-insert
helper, including the bucket and consistent-bucket variants, routes through
these two, so the Spark-row and Flink-row coverage is preserved with no per-row
cost.
On auto-encoding instead of throwing: I looked into it and it doesn't
actually solve the case, for two reasons.
1. `escapePathName` doesn't escape `.`, so a bare `..` encodes to `..`
unchanged. Encoding only neutralizes multi-segment values like `../evil`, where
the `/` gets escaped — the single-segment `..` still escapes the base path.
2. By the time we're in the write handle, the partition path is already
embedded in the record's `HoodieKey`, in the markers, in
`WriteStatus`/`HoodieWriteStat`, and in the MDT files-partition entries.
Silently rewriting it there would desync the physical directory from what the
timeline and index record.
Encoding would therefore have to happen back at keygen time, which is
exactly the per-row cost you wanted to avoid. So I took the
one-shot-validation-per-partition option you were fine with.
Two other notes:
- MDT partition paths are fixed literals (`files`, `column_stats`,
`record_index`, ...), so metadata and bootstrap writes are unaffected. I added
`files`/`column_stats` to the allow-list test to pin that.
- Residual gap either way: this only covers writes that go through a Hudi
write handle. I deliberately did not push validation down into
`FSUtils.constructAbsolutePath`, since that's also called from read paths,
meta-sync, CLI, CDC, and timeline migration — validating there would be a much
broader behavior change than this PR intends.
Tests: replaced the keygen-level test with handle-level coverage in
`TestHoodieWriteHandle` (5 rejected traversals incl. backslash and mid-path
variants, 9 allowed incl. hive-style `rider=../evil`, the MDT literals, and
empty). `TestHoodieWriteHandle` 25/25 and `TestPartitionPathEncodeUtils` 23/23
pass locally, along with `TestKeyGenUtils` / `TestHoodieCreateHandle` /
`TestHoodieMergeHandleFactory` / `TestHoodieBinaryCopyHandleSchemaEvolution`
(34/34). Checkstyle clean.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/PartitionPathEncodeUtils.java:
##########
@@ -141,4 +143,62 @@ public static String escapePartitionValue(String value) {
return escapePathName(value);
}
}
+
+ /**
Review Comment:
Good catch — fixed in 04bc6c0. The two Javadoc blocks were swapped; each now
sits directly above the method it documents.
##########
hudi-common/src/main/java/org/apache/hudi/common/util/PartitionPathEncodeUtils.java:
##########
@@ -141,4 +143,62 @@ public static String escapePartitionValue(String value) {
return escapePathName(value);
}
}
+
+ /**
+ * Returns {@code true} if the given (relative) partition path contains a
directory-traversal
+ * segment (a path segment equal to {@code ".."}). Such a partition path,
once resolved against
+ * the table base path, can escape the base path and write Hudi-managed
files into arbitrary
+ * directories reachable by the writer's credentials.
+ *
+ * <p>The check is intentionally value-content only: it tolerates {@code
'.'} inside a segment
+ * (e.g. date partitions like {@code 2024.01.01}) and only rejects the
standalone {@code ".."}
+ * segment. Both forward slash {@code '/'} and the platform-independent
literal are treated as
+ * separators, since a partition path is always stored using forward slashes.
+ *
+ * @param partitionPath the relative partition path (or a single partition
field value).
+ * @return {@code true} if a {@code ".."} traversal segment is present,
{@code false} otherwise.
+ */
+ /**
+ * Validates that the given (relative) partition path does not contain a
directory-traversal
+ * segment, throwing {@link HoodieKeyException} if it does. This is enforced
regardless of the
+ * {@code hoodie.datasource.write.partitionpath.urlencode} setting, since
url-encoding is opt-in
+ * (disabled by default) and never rejects {@code ".."}.
+ *
+ * @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.
+ */
Review Comment:
Correct, and thanks for catching it — `escapePathName` doesn't escape `.`,
so a bare `..` encodes to `..` and would keep throwing. Dropped the url-encode
advice from the message in 04bc6c0; it now points at sanitizing or remapping
the value in the upstream source or via a transformer instead.
--
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]
