He-Pin commented on code in PR #1807:
URL: https://github.com/apache/pekko-connectors/pull/1807#discussion_r3789332599
##########
ftp/src/main/scala/org/apache/pekko/stream/connectors/ftp/impl/CommonFtpOperations.scala:
##########
@@ -108,10 +113,52 @@ private[ftp] trait CommonFtpOperations {
}
private[ftp] object CommonFtpOperations {
- def concatPath(path: String, name: String): String =
- if (path.endsWith("/")) {
- path ++ name
+
+ /**
+ * Normalize a path to use `/` separators. FTP uses `/` by protocol;
+ * normalizing early ensures all downstream checks only need to handle `/`.
+ */
+ private def normalizeSeparators(path: String): String = path.replace('\\',
'/')
+
+ /**
+ * Validate that a path does not contain traversal sequences (`..`).
+ * Rejects null values and paths containing `..` as a path segment.
+ * Accepts both `/` and `\` separators; backslashes are normalized to `/`
before checking.
+ *
+ * @param path the path to validate
+ * @param fieldName the name of the field for error messages
+ * @throws IllegalArgumentException if the path contains traversal sequences
+ */
+ def validatePath(path: String, fieldName: String): Unit = {
+ require(path != null, s"$fieldName must not be null")
+ val normalized = normalizeSeparators(path)
+ val segments = normalized.split('/')
+ require(!segments.contains(".."), s"$fieldName must not contain path
traversal sequences: '$path'")
+ }
+
+ def concatPath(path: String, name: String): String = {
+ validatePath(name, "name")
+ val normName = normalizeSeparators(name)
+ require(!normName.startsWith("/"), s"name must not be an absolute path:
'$normName'")
+
+ require(path != null, "path must not be null")
+ val normPath = normalizeSeparators(path)
+ val result = if (normPath.endsWith("/")) {
+ normPath ++ normName
} else {
- s"$path/$name"
+ s"$normPath/$normName"
}
+
+ // Validate the normalized result doesn't escape the base path
+ val normalized = java.nio.file.Paths.get(result).normalize().toString
Review Comment:
`Paths.get().normalize()` is platform-dependent (Windows resolves against
current drive). Since `..` and absolute names are already rejected, this block
is redundant — consider a pure string segment walk instead, or drop it.
##########
ftp/src/main/scala/org/apache/pekko/stream/connectors/ftp/impl/CommonFtpOperations.scala:
##########
@@ -108,10 +113,52 @@ private[ftp] trait CommonFtpOperations {
}
private[ftp] object CommonFtpOperations {
- def concatPath(path: String, name: String): String =
- if (path.endsWith("/")) {
- path ++ name
+
+ /**
+ * Normalize a path to use `/` separators. FTP uses `/` by protocol;
+ * normalizing early ensures all downstream checks only need to handle `/`.
+ */
+ private def normalizeSeparators(path: String): String = path.replace('\\',
'/')
+
+ /**
+ * Validate that a path does not contain traversal sequences (`..`).
+ * Rejects null values and paths containing `..` as a path segment.
+ * Accepts both `/` and `\` separators; backslashes are normalized to `/`
before checking.
+ *
+ * @param path the path to validate
+ * @param fieldName the name of the field for error messages
+ * @throws IllegalArgumentException if the path contains traversal sequences
+ */
+ def validatePath(path: String, fieldName: String): Unit = {
+ require(path != null, s"$fieldName must not be null")
+ val normalized = normalizeSeparators(path)
+ val segments = normalized.split('/')
+ require(!segments.contains(".."), s"$fieldName must not contain path
traversal sequences: '$path'")
+ }
+
+ def concatPath(path: String, name: String): String = {
+ validatePath(name, "name")
+ val normName = normalizeSeparators(name)
+ require(!normName.startsWith("/"), s"name must not be an absolute path:
'$normName'")
+
+ require(path != null, "path must not be null")
+ val normPath = normalizeSeparators(path)
+ val result = if (normPath.endsWith("/")) {
+ normPath ++ normName
} else {
- s"$path/$name"
+ s"$normPath/$normName"
}
+
+ // Validate the normalized result doesn't escape the base path
+ val normalized = java.nio.file.Paths.get(result).normalize().toString
+ val normalizedBase = java.nio.file.Paths.get(normPath).normalize().toString
+ // On Windows, Paths.get normalizes to backslash; compare with
forward-slash versions
+ val normalizedFwd = normalized.replace('\\', '/')
+ val normalizedBaseFwd = normalizedBase.replace('\\', '/')
+ require(
+ normalizedFwd.startsWith(normalizedBaseFwd),
Review Comment:
`startsWith` has a sibling-prefix bypass:
`"/base-evil/file".startsWith("/base")` is true. Currently unreachable since
`..` is rejected upstream, but the check should be `normalizedFwd ==
normalizedBaseFwd || normalizedFwd.startsWith(normalizedBaseFwd + "/")` to
actually hold as defense-in-depth.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
