Re: [PR] fix(writer): reject directory-traversal partition paths across all writers [hudi]

["nsivabalan (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org