From c6bcd2810ae7f29bda0444ab637e64e453df4f47 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Mon, 23 Jan 2023 02:38:42 +0100 Subject: [PATCH] Allow UUIDs without hyphens (#854) We used UUIDs without hyphens for a period in the past. Modify the BrokerController to allow those. Migrating would be ideal. But migrating IDs is not simple. We have a bigger DB migration planned in the future and will migrate IDs to a single type then. --- .../controllers/BrokerController.java | 42 +++++++++++------ .../controllers/ArgumentValidationTest.java | 45 +++++++++++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java diff --git a/src/main/java/com/conveyal/analysis/controllers/BrokerController.java b/src/main/java/com/conveyal/analysis/controllers/BrokerController.java index b59c71037..409180fef 100644 --- a/src/main/java/com/conveyal/analysis/controllers/BrokerController.java +++ b/src/main/java/com/conveyal/analysis/controllers/BrokerController.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import com.mongodb.QueryBuilder; +import org.apache.commons.math3.analysis.function.Exp; import org.apache.http.Header; import org.apache.http.HttpEntity; import org.apache.http.HttpResponse; @@ -134,10 +135,10 @@ private Object singlePoint(Request request, Response response) { AnalysisRequest analysisRequest = objectFromRequestBody(request, AnalysisRequest.class); // Some parameters like regionId weren't sent by older frontends. Fail fast on missing parameters. - checkUuidParameter(analysisRequest.regionId, "region ID"); - checkUuidParameter(analysisRequest.projectId, "project ID"); - checkUuidParameter(analysisRequest.bundleId, "bundle ID"); - checkUuidParameters(analysisRequest.modificationIds, "modification IDs"); + checkIdParameter(analysisRequest.regionId, "region ID"); + checkIdParameter(analysisRequest.projectId, "project ID"); + checkIdParameter(analysisRequest.bundleId, "bundle ID"); + checkIdParameters(analysisRequest.modificationIds, "modification IDs"); checkNotNull(analysisRequest.workerVersion, "Worker version must be provided in request."); // Transform the analysis UI/backend task format into a slightly different type for R5 workers. @@ -388,30 +389,45 @@ private static void enforceAdmin (Request request) { } } + /** Factor out exception creation for readability/repetition. */ + private static IllegalArgumentException uuidException (String fieldName) { + return new IllegalArgumentException(String.format("The %s does not appear to be an ObjectId or UUID.", fieldName)); + } + /** * Validate a request parameter that is expected to be a non-null String containing a Mongo ObjectId or a - * UUID converted to a string in the standard way. + * UUID converted to a string, or a UUID converted to a string with the hyphens removed. * @param name a human-readable name for the parameter being validated, for substitution into error messages. * @throws IllegalArgumentException if the parameter fails any of these conditions. */ - public static void checkUuidParameter (String parameter, String name) { + public static void checkIdParameter(String parameter, String name) { if (parameter == null) { throw new IllegalArgumentException(String.format("The %s is not set in the request.", name)); } - // Should be either 24 hex digits (Mongo ID) or 32 hex digits with dashes (UUID) + // Should be either 24 hex digits (Mongo ID), 32 hex digits with dashes (UUID), or 32 hex digits without dashes if (ObjectId.isValid(parameter)) { return; } - try { - UUID.fromString(parameter); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException(String.format("The %s does not appear to be an ObjectId or UUID.", name)); + if (parameter.length() == 36) { + try { + UUID.fromString(parameter); + } catch (IllegalArgumentException e) { + throw uuidException(name); + } + } else if (parameter.length() == 32) { + for (char c : parameter.toCharArray()) { + if (Character.digit(c, 16) == -1) { + throw uuidException(name); + } + } + } else { + throw uuidException(name); } } - public static void checkUuidParameters (Iterable parameters, String name) { + public static void checkIdParameters(Iterable parameters, String name) { for (String parameter: parameters) { - checkUuidParameter(parameter, name); + checkIdParameter(parameter, name); } } diff --git a/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java b/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java new file mode 100644 index 000000000..a56769fba --- /dev/null +++ b/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java @@ -0,0 +1,45 @@ +package com.conveyal.analysis.controllers; + +import com.conveyal.analysis.components.broker.Broker; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.UUID; + +/** + * Tests that check how parameters received over the HTTP API are validated. + * These validators should be fairly strict about what they accept, and should not tolerate the presence of things + * like semicolons or double dashes that indicate attempts to corrupt or gain access to database contents. + * + * Arguably we should have another layer of input sanitization that not only refuses but logs anything that contains + * characters or substrings that could be associated with an attempted attack, and that same validator should be + * applied to every input (perhaps called from every other input validator). + */ +public class ArgumentValidationTest { + + @Test + void testIdValidation () { + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("hello", "param"); + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("Robert'); DROP TABLE Students;--", "param"); + // https://xkcd.com/327/ + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("0123456789012345", "param"); + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("0123456789ABCDEF67890ZZZZ5678901", "param"); + }); + Assertions.assertDoesNotThrow(() -> { + BrokerController.checkIdParameter("0123456789abcDEF6789012345678901", "param"); + }); + Assertions.assertDoesNotThrow(() -> { + String validUuid = UUID.randomUUID().toString(); + BrokerController.checkIdParameter(validUuid, "param"); + }); + } + + +}