He-Pin commented on code in PR #1807:
URL: https://github.com/apache/pekko-connectors/pull/1807#discussion_r3789329507
##########
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:
Heads up: `startsWith` here is susceptible to a sibling-prefix bypass. If
`normPath` is `/base` and someone manages to produce a result of
`/base-evil/file`, then `"/base-evil/file".startsWith("/base")` is `true` and
the check passes silently.
I realize the earlier `validatePath(name, ...)` already rejects `..`
segments and the absolute-path check prevents `/`-prefixed names, so this is
currently unreachable. But as defense-in-depth it should be tight:
```scala
require(
normalizedFwd == normalizedBaseFwd ||
normalizedFwd.startsWith(normalizedBaseFwd + "/"),
s"concatPath result '$result' escapes base path '$normPath' after
normalization")
```
This way the secondary check actually holds up if the primary checks are
ever relaxed or bypassed.
##########
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:
Minor: `Paths.get().normalize()` is platform-dependent — on Windows it
resolves against the current working drive, which makes the behaviour of this
check vary by host OS. Since `..` segments are already rejected by
`validatePath` above and absolute names are rejected below, the concatenation
is structurally guaranteed to stay under the base. This whole
normalize+startsWith block might be redundant complexity that is hard to reason
about across platforms.
If you want to keep it as belt-and-suspenders, a pure string-based segment
check (split on `/`, walk the segments, ensure depth never goes negative
relative to base) would be deterministic regardless of OS and avoid the
`Paths.get` quirks. Up to you whether the extra safety net is worth the
platform surface area.
--
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]
