yifan-c commented on code in PR #367:
URL: https://github.com/apache/cassandra-sidecar/pull/367#discussion_r3539598358
##########
server/src/main/java/org/apache/cassandra/sidecar/handlers/livemigration/LiveMigrationFileResolveHandler.java:
##########
@@ -123,65 +124,91 @@ protected void handleInternal(RoutingContext rc,
SocketAddress remoteAddress,
@Nullable Void request)
{
- String reqPath = URLDecoder.decode(rc.request().path(),
StandardCharsets.UTF_8);
+ String reqPath;
+ try
+ {
+ reqPath = URI.create(rc.request().path()).getPath();
+ }
+ catch (IllegalArgumentException e)
+ {
+ rc.fail(wrapHttpException(HttpResponseStatus.BAD_REQUEST,
"Malformed request path", e));
+ return;
+ }
if (reqPath.contains("/../") || reqPath.endsWith("/.."))
{
LOGGER.warn("Tried to access file using relative path({}).
Rejecting the request.", reqPath);
-
rc.response().setStatusCode(HttpResponseStatus.BAD_REQUEST.code()).end();
+ rc.fail(wrapHttpException(HttpResponseStatus.BAD_REQUEST,
+ "Tried to access file using relative
path: " + reqPath));
return;
}
InstanceMetadata instanceMeta = metadataFetcher.instance(host);
String normalizedPath = rc.normalizedPath();
- String localFile;
+ ResolvedPath resolved;
try
{
- localFile =
LiveMigrationInstanceMetadataUtil.localPath(normalizedPath,
instanceMeta).toString();
+ resolved =
LiveMigrationInstanceMetadataUtil.resolveLexically(normalizedPath,
instanceMeta);
}
catch (IllegalArgumentException e)
{
- LOGGER.warn("Invalid path", e);
-
rc.response().setStatusCode(HttpResponseStatus.NOT_FOUND.code()).end();
+ rc.fail(wrapHttpException(HttpResponseStatus.BAD_REQUEST,
e.getMessage(), e));
Review Comment:
Is it expected? 404 -> 400
##########
server/src/main/java/org/apache/cassandra/sidecar/livemigration/LiveMigrationInstanceMetadataUtil.java:
##########
@@ -58,6 +63,14 @@ public class LiveMigrationInstanceMetadataUtil
{
private static final Logger LOGGER =
LoggerFactory.getLogger(LiveMigrationInstanceMetadataUtil.class);
+ /**
+ * Caches the canonical (symlinks resolved) form of each base directory
keyed by its lexical
+ * path. Base directories come from {@link InstanceMetadata}, which is
fixed at startup, so the
+ * canonical form is stable for the process lifetime. The set of distinct
base directories is
+ * bounded by configuration (~5–10 per instance), so no eviction is
required.
+ */
+ private static final ConcurrentMap<Path, Path> BASE_DIR_CANONICAL = new
ConcurrentHashMap<>();
Review Comment:
I know that the `LiveMigrationInstanceMetadataUtil` is a static helper
already. But the static map `BASE_DIR_CANONICAL` catches my attention. I have 2
suggestions.
1. It is common in this project to inject util via guice. Can we do the
same, instead of an unmanaged global object out side of guice?
2. To harden the assumption on the size, can we set a size limit and emit
warning if exceeding certain size (say 10) when inserting into the map.
##########
server/src/main/java/org/apache/cassandra/sidecar/livemigration/LiveMigrationInstanceMetadataUtil.java:
##########
@@ -299,13 +336,68 @@ public static Path localPath(@NotNull String fileUrl,
throw new IllegalArgumentException(errorMessage);
}
- return resolvedPath;
+ return new ResolvedPath(baseDir, resolvedPath);
}
}
+ LOGGER.warn("File url {} does not match any configured live-migration
directory prefix.", fileUrl);
throw new IllegalArgumentException("File url " + fileUrl + " is
unknown.");
}
+ /**
+ * Lexical resolution of a live-migration URL: the configured base
directory paired with the
+ * local path it maps to.
+ */
+ public static final class ResolvedPath
+ {
+ private final Path baseDir;
+ private final Path resolvedPath;
+
+ ResolvedPath(Path baseDir, Path resolvedPath)
+ {
+ this.baseDir = baseDir;
+ this.resolvedPath = resolvedPath;
+ }
+
+ public Path resolvedPath()
+ {
+ return resolvedPath;
+ }
+
+ /**
+ * Verifies that the source-side file represented by this {@code
ResolvedPath} exists and
+ * that its canonical (symlinks resolved) path stays inside the
canonical base directory.
+ * Use this on the source side after
+ * {@link LiveMigrationInstanceMetadataUtil#resolveLexically(String,
InstanceMetadata)} to
+ * enforce that the file the operator is about to serve cannot escape
the configured
+ * directory through a symlink. Callers continue to use {@link
#resolvedPath()} (the
+ * lexical form) for exclusion matching, logging, and serving, since
exclusion patterns
+ * and operator-facing logs are configured against the lexical form.
+ *
+ * <p>Performs blocking filesystem I/O - must be called from a worker
thread, not the event
+ * loop. Throws {@link NoSuchFileException} before any canonical-path
resolution runs, so
+ * callers can distinguish "file missing" from "path escapes via
symlink".
+ *
+ * @throws NoSuchFileException if the resolved file does not exist
+ * @throws IOException if {@link Path#toRealPath} fails
for an I/O reason
+ * other than missing file
+ * @throws IllegalArgumentException if the canonical path escapes the
base directory
+ */
+ public void verifyContainment() throws IOException
+ {
+ if (!Files.exists(resolvedPath))
+ {
+ throw new NoSuchFileException(resolvedPath.toString());
+ }
+ Path canonical = resolvedPath.toRealPath();
Review Comment:
looks like `toRealPath` already throws `NoSuchFileException` when the file
does not exist. The above check is redundant.
> @throws IOException – if the file does not exist or an I/O error occurs
--
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]
