hudi-agent commented on code in PR #19342:
URL: https://github.com/apache/hudi/pull/19342#discussion_r3632980751
##########
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:
🤖 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?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 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.
##########
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:
🤖 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.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
