nvharikrishna commented on code in PR #367:
URL: https://github.com/apache/cassandra-sidecar/pull/367#discussion_r3587828381
##########
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:
This class is a utility class and all its methods are static, so Guice
cannot be used. Compared to all other work done during downloading, I think the
save by this cache is very minimal. For simplicity, I can remove the cache
entirely.
##########
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:
Thanks for pointing it out. There is some ambiguity w.r.t usage of
IllegalArgumentException. Changed it to BAD_REQUEST as `resolveLexically`
throws it for malformed URL. This usage is overlapping with file/dir that don’t
exist. Introduced new exception to bring clear separation between malformed
URLs and well constructed URLs, but file/directory doesn't exist so that
existing behavior is preserved.
##########
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:
> @throws IOException – if the file does not exist or an I/O error occurs
Documentation says IOException, and implementation may throw just
IOException and not NoSuchFileException. So, checking for file existence and
throwing `NoSuchFileException` explicitly.
--
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]
