From 19e781b104109c1f119f9ce4d8c8a59997ba03e2 Mon Sep 17 00:00:00 2001 From: Viktor Lofgren Date: Thu, 18 Jan 2024 11:30:17 +0100 Subject: [PATCH] (control) Add basic input validation to node actions Will present a simple error message when required fields aren't populated, instead of a cryptic HTTP status error. --- .../executor/client/ExecutorClient.java | 10 +- .../node/svc/ControlNodeActionsService.java | 93 ++++++++++++++----- .../control/sys/svc/DataSetsService.java | 5 +- 3 files changed, 74 insertions(+), 34 deletions(-) diff --git a/code/api/executor-api/src/main/java/nu/marginalia/executor/client/ExecutorClient.java b/code/api/executor-api/src/main/java/nu/marginalia/executor/client/ExecutorClient.java index d5fee1c4..294a9e86 100644 --- a/code/api/executor-api/src/main/java/nu/marginalia/executor/client/ExecutorClient.java +++ b/code/api/executor-api/src/main/java/nu/marginalia/executor/client/ExecutorClient.java @@ -14,6 +14,7 @@ import nu.marginalia.executor.upload.UploadDirContents; import nu.marginalia.model.gson.GsonFactory; import nu.marginalia.service.descriptor.ServiceDescriptors; import nu.marginalia.service.id.ServiceId; +import nu.marginalia.storage.model.FileStorage; import nu.marginalia.storage.model.FileStorageId; import java.io.OutputStream; @@ -21,7 +22,6 @@ import java.net.URLEncoder; import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.util.ArrayList; -import java.util.List; import java.util.concurrent.TimeUnit; public class ExecutorClient extends AbstractDynamicClient { @@ -97,13 +97,13 @@ public class ExecutorClient extends AbstractDynamicClient { .blockingSubscribe(); } - public void exportAtags(Context ctx, int node, String fid) { + public void exportAtags(Context ctx, int node, FileStorageId fid) { post(ctx, node, "/export/atags?fid="+fid, "").blockingSubscribe(); } - public void exportRssFeeds(Context ctx, int node, String fid) { + public void exportRssFeeds(Context ctx, int node, FileStorageId fid) { post(ctx, node, "/export/feeds?fid="+fid, "").blockingSubscribe(); } - public void exportTermFrequencies(Context ctx, int node, String fid) { + public void exportTermFrequencies(Context ctx, int node, FileStorageId fid) { post(ctx, node, "/export/termfreq?fid="+fid, "").blockingSubscribe(); } @@ -111,7 +111,7 @@ public class ExecutorClient extends AbstractDynamicClient { post(ctx, node, "/export/data", "").blockingSubscribe(); } - public void restoreBackup(Context context, int node, String fid) { + public void restoreBackup(Context context, int node, FileStorageId fid) { post(context, node, "/backup/" + fid + "/restore", "").blockingSubscribe(); } diff --git a/code/services-core/control-service/src/main/java/nu/marginalia/control/node/svc/ControlNodeActionsService.java b/code/services-core/control-service/src/main/java/nu/marginalia/control/node/svc/ControlNodeActionsService.java index 4145f25c..cf94f12e 100644 --- a/code/services-core/control-service/src/main/java/nu/marginalia/control/node/svc/ControlNodeActionsService.java +++ b/code/services-core/control-service/src/main/java/nu/marginalia/control/node/svc/ControlNodeActionsService.java @@ -3,6 +3,7 @@ package nu.marginalia.control.node.svc; import com.google.inject.Inject; import com.google.inject.Singleton; import nu.marginalia.client.Context; +import nu.marginalia.control.ControlValidationError; import nu.marginalia.control.RedirectControl; import nu.marginalia.executor.client.ExecutorClient; import nu.marginalia.executor.model.load.LoadParameters; @@ -87,12 +88,16 @@ public class ControlNodeActionsService { ); } - public Object sideloadEncyclopedia(Request request, Response response) throws Exception { + public Object sideloadEncyclopedia(Request request, Response response) { - Path sourcePath = Path.of(request.queryParams("source")); + String source = request.queryParams("source"); String baseUrl = request.queryParams("baseUrl"); + int nodeId = Integer.parseInt(request.params("node")); - final int nodeId = Integer.parseInt(request.params("node")); + if (baseUrl == null) + throw new ControlValidationError("No baseUrl specified", "A baseUrl must be specified", ".."); + + Path sourcePath = parseSourcePath(source); eventLog.logEvent("USER-ACTION", "SIDELOAD ENCYCLOPEDIA " + nodeId); @@ -101,11 +106,12 @@ public class ControlNodeActionsService { return ""; } - public Object sideloadDirtree(Request request, Response response) throws Exception { + public Object sideloadDirtree(Request request, Response response) { - Path sourcePath = Path.of(request.queryParams("source")); final int nodeId = Integer.parseInt(request.params("node")); + Path sourcePath = parseSourcePath(request.queryParams("source")); + eventLog.logEvent("USER-ACTION", "SIDELOAD DIRTREE " + nodeId); executorClient.sideloadDirtree(Context.fromRequest(request), nodeId, sourcePath); @@ -113,10 +119,10 @@ public class ControlNodeActionsService { return ""; } - public Object sideloadWarc(Request request, Response response) throws Exception { + public Object sideloadWarc(Request request, Response response) { - Path sourcePath = Path.of(request.queryParams("source")); final int nodeId = Integer.parseInt(request.params("node")); + Path sourcePath = parseSourcePath(request.queryParams("source")); eventLog.logEvent("USER-ACTION", "SIDELOAD WARC " + nodeId); @@ -124,11 +130,15 @@ public class ControlNodeActionsService { return ""; } - public Object sideloadStackexchange(Request request, Response response) throws Exception { + public Object sideloadStackexchange(Request request, Response response) { - Path sourcePath = Path.of(request.queryParams("source")); final int nodeId = Integer.parseInt(request.params("node")); + String source = request.queryParams("source"); + if (source == null) + throw new ControlValidationError("No source specified", "A source file/directory must be specified", ".."); + Path sourcePath = Path.of(source); + eventLog.logEvent("USER-ACTION", "SIDELOAD STACKEXCHANGE " + nodeId); executorClient.sideloadStackexchange(Context.fromRequest(request), nodeId, sourcePath); @@ -143,7 +153,8 @@ public class ControlNodeActionsService { private Object triggerAutoRecrawl(Request request, Response response) throws SQLException { int nodeId = Integer.parseInt(request.params("id")); - var toCrawl = FileStorageId.parse(request.queryParams("source")); + + var toCrawl = parseSourceFileStorageId(request.queryParams("source")); changeActiveStorage(nodeId, FileStorageType.CRAWL_DATA, toCrawl); @@ -159,7 +170,7 @@ public class ControlNodeActionsService { private Object triggerNewCrawl(Request request, Response response) throws SQLException { int nodeId = Integer.parseInt(request.params("id")); - var toCrawl = FileStorageId.parse(request.queryParams("source")); + var toCrawl = parseSourceFileStorageId(request.queryParams("source")); changeActiveStorage(nodeId, FileStorageType.CRAWL_SPEC, toCrawl); @@ -174,7 +185,8 @@ public class ControlNodeActionsService { private Object triggerAutoProcess(Request request, Response response) throws SQLException { int nodeId = Integer.parseInt(request.params("id")); - var toProcess = FileStorageId.parse(request.queryParams("source")); + + var toProcess = parseSourceFileStorageId(request.queryParams("source")); changeActiveStorage(nodeId, FileStorageType.PROCESSED_DATA, toProcess); @@ -189,6 +201,10 @@ public class ControlNodeActionsService { int nodeId = Integer.parseInt(request.params("id")); String[] values = request.queryParamsValues("source"); + if (values.length == 0) { + throw new ControlValidationError("No source specified", "At least one source storage must be specified", ".."); + } + List ids = Arrays.stream(values).map(FileStorageId::parse).toList(); changeActiveStorage(nodeId, FileStorageType.PROCESSED_DATA, ids.toArray(new FileStorageId[0])); @@ -204,11 +220,14 @@ public class ControlNodeActionsService { private Object triggerRestoreBackup(Request request, Response response) { int nodeId = Integer.parseInt(request.params("id")); - executorClient.restoreBackup(Context.fromRequest(request), nodeId, request.queryParams("source")); + var toLoad = parseSourceFileStorageId(request.queryParams("source")); + + executorClient.restoreBackup(Context.fromRequest(request), nodeId, toLoad); return ""; } + /** Change the active storage for a node of a particular type. */ private void changeActiveStorage(int nodeId, FileStorageType type, FileStorageId... newActiveStorage) throws SQLException { // It is desirable to have the active storage set to reflect which storage was last used @@ -231,6 +250,10 @@ public class ControlNodeActionsService { final String url = request.queryParams("url"); int nodeId = Integer.parseInt(request.params("id")); + if (url == null || url.isBlank()) { + throw new ControlValidationError("No url specified", "A url must be specified", ".."); + } + executorClient.createCrawlSpecFromDownload(Context.fromRequest(request), nodeId, description, url); return ""; @@ -244,21 +267,41 @@ public class ControlNodeActionsService { private Object exportFromCrawlData(Request req, Response rsp) { String exportType = req.queryParams("exportType"); - String source = req.queryParams("source"); + FileStorageId source = parseSourceFileStorageId(req.queryParams("source")); - if (exportType.equals("atags")) { - executorClient.exportAtags(Context.fromRequest(req), Integer.parseInt(req.params("id")), source); - } - else if (exportType.equals("rss")) { - executorClient.exportRssFeeds(Context.fromRequest(req), Integer.parseInt(req.params("id")), source); - } - else if (exportType.equals("termFreq")) { - executorClient.exportTermFrequencies(Context.fromRequest(req), Integer.parseInt(req.params("id")), source); - } - else { - rsp.status(404); + switch (exportType) { + case "atags" -> executorClient.exportAtags(Context.fromRequest(req), Integer.parseInt(req.params("id")), source); + case "rss" -> executorClient.exportRssFeeds(Context.fromRequest(req), Integer.parseInt(req.params("id")), source); + case "termFreq" -> executorClient.exportTermFrequencies(Context.fromRequest(req), Integer.parseInt(req.params("id")), source); + default -> throw new ControlValidationError("No export type specified", "An export type must be specified", ".."); } return ""; } + + + + private Path parseSourcePath(String source) { + if (source == null) { + throw new ControlValidationError("No source specified", + "A source file/directory must be specified", + ".."); + } + return Path.of(source); + } + + private FileStorageId parseSourceFileStorageId(String source) { + if (source == null) { + throw new ControlValidationError("No source specified", + "A source file storage must be specified", + ".."); + } + + try { + return FileStorageId.parse(source); + } + catch (Exception e) { // Typically NumberFormatException + throw new ControlValidationError("Invalid source specified", "The source file storage is invalid", ".."); + } + } } diff --git a/code/services-core/control-service/src/main/java/nu/marginalia/control/sys/svc/DataSetsService.java b/code/services-core/control-service/src/main/java/nu/marginalia/control/sys/svc/DataSetsService.java index a3d841eb..491f1b62 100644 --- a/code/services-core/control-service/src/main/java/nu/marginalia/control/sys/svc/DataSetsService.java +++ b/code/services-core/control-service/src/main/java/nu/marginalia/control/sys/svc/DataSetsService.java @@ -16,15 +16,12 @@ import java.util.Map; @Singleton public class DataSetsService { - private final HikariDataSource dataSource; private final ControlRendererFactory rendererFactory; private final DomainTypes domainTypes; @Inject - public DataSetsService(HikariDataSource dataSource, - ControlRendererFactory rendererFactory, + public DataSetsService(ControlRendererFactory rendererFactory, DomainTypes domainTypes) { - this.dataSource = dataSource; this.rendererFactory = rendererFactory; this.domainTypes = domainTypes; }