From e62a710f461adc448118bcdeb42cdc3cc5287ad6 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 1 Feb 2026 18:16:49 +0000 Subject: [PATCH 1/4] fix(gateway): Validate entity type matches route path for dual-path endpoints - Add expected_type parameter to HandlerContext::get_entity_info() - Update data, fault, config, and operation handlers to validate entity type - Return 400 Bad Request when App ID is used on Component route (and vice versa) - Add unit tests for extract_entity_type_from_path() utility This fixes the issue where /components/{id}/data incorrectly accepted App IDs in runtime-only discovery mode, where entity IDs can collide between entity types. --- .../http/handlers/handler_context.hpp | 23 ++- .../ros2_medkit_gateway/http/http_utils.hpp | 33 ++++ .../src/http/handlers/config_handlers.cpp | 61 ++++++ .../src/http/handlers/data_handlers.cpp | 74 +++++-- .../src/http/handlers/fault_handlers.cpp | 86 ++++++-- .../src/http/handlers/handler_context.cpp | 77 +++++++- .../src/http/handlers/operation_handlers.cpp | 37 ++++ .../src/http/rest_server.cpp | 5 - .../test/test_gateway_node.cpp | 50 +++++ .../test/test_integration.test.py | 186 +++++++++++++++++- 10 files changed, 584 insertions(+), 48 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp index f6eee12f..ed68651b 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp @@ -29,6 +29,7 @@ #include "ros2_medkit_gateway/auth/auth_manager.hpp" #include "ros2_medkit_gateway/config.hpp" #include "ros2_medkit_gateway/http/error_codes.hpp" +#include "ros2_medkit_gateway/http/http_utils.hpp" #include "ros2_medkit_gateway/models/entity_capabilities.hpp" #include "ros2_medkit_gateway/models/entity_types.hpp" @@ -151,13 +152,16 @@ class HandlerContext { /** * @brief Get information about any entity (Component, App, Area, Function) * - * Searches through all entity types in order: Component, App, Area, Function. - * Returns the first match found. + * If expected_type is specified, searches ONLY in that collection. + * If expected_type is UNKNOWN (default), searches all types in order: + * Component, App, Area, Function - returns the first match found. * * @param entity_id Entity ID to look up + * @param expected_type Optional: restrict search to specific entity type * @return EntityInfo with resolved details, or EntityInfo with UNKNOWN type if not found */ - EntityInfo get_entity_info(const std::string & entity_id) const; + EntityInfo get_entity_info(const std::string & entity_id, + SovdEntityType expected_type = SovdEntityType::UNKNOWN) const; /** * @brief Validate that entity supports a resource collection @@ -172,6 +176,19 @@ class HandlerContext { static std::optional validate_collection_access(const EntityInfo & entity, ResourceCollection collection); + /** + * @brief Validate entity type matches the route path + * + * Ensures semantic correctness - /components/{id} should only accept + * component IDs, not app IDs. This prevents the dual-path routing bug + * where /components/{app_id} would incorrectly succeed. + * + * @param entity Entity information (from get_entity_info) + * @param expected_type Expected type based on route (from extract_entity_type_from_path) + * @return std::nullopt if valid, error message if type mismatch + */ + static std::optional validate_entity_type(const EntityInfo & entity, SovdEntityType expected_type); + /** * @brief Set CORS headers on response if origin is allowed * @param res HTTP response diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp index c17c379c..558ade91 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp @@ -18,6 +18,8 @@ #include +#include "ros2_medkit_gateway/models/entity_types.hpp" + namespace ros2_medkit_gateway { /// API version prefix for all endpoints @@ -32,6 +34,37 @@ inline std::string api_path(const std::string & endpoint) { return std::string(API_BASE_PATH) + endpoint; } +/** + * @brief Extract expected entity type from request path + * + * Parses the URL path to determine which entity type the route expects. + * Used for semantic validation - ensuring /components/{id} only accepts + * component IDs, not app IDs. + * + * @param path Request path (e.g., "/api/v1/components/my_component/data") + * @return Expected entity type, or UNKNOWN if path doesn't match entity routes + */ +inline SovdEntityType extract_entity_type_from_path(const std::string & path) { + // Path format: /api/v1/{entity_type}/{id}/... + // Check for each entity type prefix after API base path + const std::string base = std::string(API_BASE_PATH) + "/"; + + if (path.find(base + "components/") == 0 || path.find(base + "components") == 0) { + return SovdEntityType::COMPONENT; + } + if (path.find(base + "apps/") == 0 || path.find(base + "apps") == 0) { + return SovdEntityType::APP; + } + if (path.find(base + "areas/") == 0 || path.find(base + "areas") == 0) { + return SovdEntityType::AREA; + } + if (path.find(base + "functions/") == 0 || path.find(base + "functions") == 0) { + return SovdEntityType::FUNCTION; + } + + return SovdEntityType::UNKNOWN; +} + /** * @brief Fault status filter flags for fault listing endpoints */ diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index 08c71e38..c8fcd9a9 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -19,6 +19,7 @@ #include "ros2_medkit_gateway/gateway_node.hpp" #include "ros2_medkit_gateway/http/error_codes.hpp" +#include "ros2_medkit_gateway/http/http_utils.hpp" #include "ros2_medkit_gateway/http/x_medkit.hpp" using json = nlohmann::json; @@ -236,6 +237,18 @@ void ConfigHandlers::handle_list_configurations(const httplib::Request & req, ht return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + // Get aggregated configurations info for this entity auto agg_configs = cache.get_entity_configurations(entity_id); @@ -394,6 +407,18 @@ void ConfigHandlers::handle_get_configuration(const httplib::Request & req, http return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + // Get aggregated configurations info auto agg_configs = cache.get_entity_configurations(entity_id); @@ -552,6 +577,18 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + // Get aggregated configurations info auto agg_configs = cache.get_entity_configurations(entity_id); @@ -657,6 +694,18 @@ void ConfigHandlers::handle_delete_configuration(const httplib::Request & req, h return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + // Get aggregated configurations info auto agg_configs = cache.get_entity_configurations(entity_id); @@ -746,6 +795,18 @@ void ConfigHandlers::handle_delete_all_configurations(const httplib::Request & r return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + // Get aggregated configurations info auto agg_configs = cache.get_entity_configurations(entity_id); diff --git a/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp index 9f960f14..17c48d78 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp @@ -47,10 +47,26 @@ void DataHandlers::handle_list_data(const httplib::Request & req, httplib::Respo // First, verify that the entity actually exists in the cache const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + + // Get expected type from route and look up entity in that specific collection + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); + if (entity_info.type == EntityType::UNKNOWN) { + // Check if entity exists in ANY collection (for better error message) + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + // Entity exists but wrong type -> 400 + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + // Entity doesn't exist at all -> 404 + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } @@ -136,12 +152,25 @@ void DataHandlers::handle_get_data_item(const httplib::Request & req, httplib::R return; } - // Verify entity exists - const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + // Verify entity exists in the correct collection for this route + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); + if (entity_info.type == EntityType::UNKNOWN) { + // Check if entity exists in ANY collection (for better error message) + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + // Entity exists but wrong type -> 400 + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + // Entity doesn't exist at all -> 404 + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } @@ -268,12 +297,25 @@ void DataHandlers::handle_put_data_item(const httplib::Request & req, httplib::R return; } - // Verify entity exists - const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + // Verify entity exists in the correct collection for this route + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); + if (entity_info.type == EntityType::UNKNOWN) { + // Check if entity exists in ANY collection (for better error message) + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + // Entity exists but wrong type -> 400 + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + // Entity doesn't exist at all -> 404 + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } diff --git a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp index b1f92fdf..00d6861a 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp @@ -194,10 +194,21 @@ void FaultHandlers::handle_list_faults(const httplib::Request & req, httplib::Re return; } - auto entity_info = ctx_.get_entity_info(entity_id); + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); if (entity_info.type == EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } @@ -367,12 +378,24 @@ void FaultHandlers::handle_get_fault(const httplib::Request & req, httplib::Resp return; } - auto entity_info = ctx_.get_entity_info(entity_id); + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); if (entity_info.type == EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } + std::string namespace_path = entity_info.namespace_path; auto fault_mgr = ctx_.node()->get_fault_manager(); @@ -436,10 +459,21 @@ void FaultHandlers::handle_clear_fault(const httplib::Request & req, httplib::Re } // Verify entity exists - auto entity_info = ctx_.get_entity_info(entity_id); + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); if (entity_info.type == EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } @@ -488,10 +522,21 @@ void FaultHandlers::handle_clear_all_faults(const httplib::Request & req, httpli } // Verify entity exists - auto entity_info = ctx_.get_entity_info(entity_id); + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); if (entity_info.type == EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } @@ -602,10 +647,21 @@ void FaultHandlers::handle_get_component_snapshots(const httplib::Request & req, return; } - auto entity_info = ctx_.get_entity_info(entity_id); + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = ctx_.get_entity_info(entity_id, expected_type); if (entity_info.type == EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); + auto any_entity = ctx_.get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", + {{"entity_id", entity_id}}); + } return; } diff --git a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp index afe882cc..90fedcbf 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp @@ -72,11 +72,70 @@ HandlerContext::get_component_namespace_path(const std::string & component_id) c return tl::unexpected("Component not found"); } -EntityInfo HandlerContext::get_entity_info(const std::string & entity_id) const { +EntityInfo HandlerContext::get_entity_info(const std::string & entity_id, SovdEntityType expected_type) const { const auto & cache = node_->get_thread_safe_cache(); EntityInfo info; info.id = entity_id; + // If expected_type is specified, search ONLY in that collection + // This prevents ID collisions (e.g., Area "powertrain" vs Component "powertrain") + if (expected_type != SovdEntityType::UNKNOWN) { + switch (expected_type) { + case SovdEntityType::COMPONENT: + if (auto component = cache.get_component(entity_id)) { + info.type = EntityType::COMPONENT; + info.namespace_path = component->namespace_path; + info.fqn = component->fqn; + info.id_field = "component_id"; + info.error_name = "Component"; + return info; + } + break; + + case SovdEntityType::APP: + if (auto app = cache.get_app(entity_id)) { + info.type = EntityType::APP; + info.namespace_path = app->bound_fqn.value_or(""); + info.fqn = app->bound_fqn.value_or(""); + info.id_field = "app_id"; + info.error_name = "App"; + return info; + } + break; + + case SovdEntityType::AREA: + if (auto area = cache.get_area(entity_id)) { + info.type = EntityType::AREA; + info.namespace_path = area->namespace_path; + info.fqn = area->namespace_path; + info.id_field = "area_id"; + info.error_name = "Area"; + return info; + } + break; + + case SovdEntityType::FUNCTION: + if (auto func = cache.get_function(entity_id)) { + info.type = EntityType::FUNCTION; + info.namespace_path = ""; + info.fqn = ""; + info.id_field = "function_id"; + info.error_name = "Function"; + return info; + } + break; + + case SovdEntityType::SERVER: + default: + break; + } + // Not found in expected collection + info.type = EntityType::UNKNOWN; + info.error_name = "Entity"; + return info; + } + + // No expected_type specified - search all collections in order (legacy behavior) // Search components first (O(1) lookup) if (auto component = cache.get_component(entity_id)) { info.type = EntityType::COMPONENT; @@ -135,6 +194,22 @@ std::optional HandlerContext::validate_collection_access(const Enti return std::nullopt; } +std::optional HandlerContext::validate_entity_type(const EntityInfo & entity, + SovdEntityType expected_type) { + // If expected_type is UNKNOWN, skip validation (path doesn't specify an entity type) + if (expected_type == SovdEntityType::UNKNOWN) { + return std::nullopt; + } + + // Check if entity type matches expected type from route + if (entity.sovd_type() != expected_type) { + return "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity.sovd_type()); + } + + return std::nullopt; +} + void HandlerContext::set_cors_headers(httplib::Response & res, const std::string & origin) const { res.set_header("Access-Control-Allow-Origin", origin); diff --git a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp index c43e9d3c..544f9c1c 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -18,6 +18,7 @@ #include "ros2_medkit_gateway/gateway_node.hpp" #include "ros2_medkit_gateway/http/error_codes.hpp" +#include "ros2_medkit_gateway/http/http_utils.hpp" #include "ros2_medkit_gateway/http/x_medkit.hpp" #include "ros2_medkit_gateway/operation_manager.hpp" @@ -58,6 +59,18 @@ void OperationHandlers::handle_list_operations(const httplib::Request & req, htt return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + switch (entity_ref->type) { case SovdEntityType::COMPONENT: ops = cache.get_component_operations(entity_id); @@ -197,6 +210,18 @@ void OperationHandlers::handle_get_operation(const httplib::Request & req, httpl return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + // Get aggregated operations based on entity type AggregatedOperations ops; std::string entity_type; @@ -384,6 +409,18 @@ void OperationHandlers::handle_create_execution(const httplib::Request & req, ht return; } + // Validate entity type matches the route path + auto expected_type = extract_entity_type_from_path(req.path); + if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(entity_ref->type), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(entity_ref->type)}}); + return; + } + // Get aggregated operations based on entity type AggregatedOperations ops; std::string entity_type; diff --git a/src/ros2_medkit_gateway/src/http/rest_server.cpp b/src/ros2_medkit_gateway/src/http/rest_server.cpp index 3460d35c..5e9c5226 100644 --- a/src/ros2_medkit_gateway/src/http/rest_server.cpp +++ b/src/ros2_medkit_gateway/src/http/rest_server.cpp @@ -578,11 +578,6 @@ void RESTServer::setup_routes() { discovery_handlers_->handle_get_area(req, res); }); - // TODO(#158): Dual-path design issue - in runtime-only mode, these routes accept both - // component IDs and app IDs. Semantically, /components should only accept - // synthetic component IDs, not individual ROS 2 node IDs. Consider enforcing - // proper entity type validation in a future refactor. - // // Component topic data (specific topic) - register before general route // Use (.+) for topic_name to accept slashes from percent-encoded URLs (%2F -> /) srv->Get((api_path("/components") + R"(/([^/]+)/data/(.+)$)"), diff --git a/src/ros2_medkit_gateway/test/test_gateway_node.cpp b/src/ros2_medkit_gateway/test/test_gateway_node.cpp index 27ec6ca0..c3ce7565 100644 --- a/src/ros2_medkit_gateway/test/test_gateway_node.cpp +++ b/src/ros2_medkit_gateway/test/test_gateway_node.cpp @@ -22,6 +22,7 @@ #include #include "ros2_medkit_gateway/gateway_node.hpp" +#include "ros2_medkit_gateway/http/http_utils.hpp" using namespace std::chrono_literals; using httplib::StatusCode; @@ -697,6 +698,55 @@ TEST_F(TestGatewayNode, test_invalid_function_id_bad_request) { EXPECT_EQ(res->status, StatusCode::BadRequest_400); } +// ============================================================================= +// Entity type path extraction tests +// ============================================================================= + +TEST(ExtractEntityTypePath, components_route) { + using ros2_medkit_gateway::extract_entity_type_from_path; + using ros2_medkit_gateway::SovdEntityType; + + EXPECT_EQ(extract_entity_type_from_path("/api/v1/components"), SovdEntityType::COMPONENT); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/components/my_comp"), SovdEntityType::COMPONENT); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/components/my_comp/data"), SovdEntityType::COMPONENT); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/components/test/operations"), SovdEntityType::COMPONENT); +} + +TEST(ExtractEntityTypePath, apps_route) { + using ros2_medkit_gateway::extract_entity_type_from_path; + using ros2_medkit_gateway::SovdEntityType; + + EXPECT_EQ(extract_entity_type_from_path("/api/v1/apps"), SovdEntityType::APP); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/apps/my_app"), SovdEntityType::APP); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/apps/my_app/data"), SovdEntityType::APP); +} + +TEST(ExtractEntityTypePath, areas_route) { + using ros2_medkit_gateway::extract_entity_type_from_path; + using ros2_medkit_gateway::SovdEntityType; + + EXPECT_EQ(extract_entity_type_from_path("/api/v1/areas"), SovdEntityType::AREA); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/areas/root"), SovdEntityType::AREA); +} + +TEST(ExtractEntityTypePath, functions_route) { + using ros2_medkit_gateway::extract_entity_type_from_path; + using ros2_medkit_gateway::SovdEntityType; + + EXPECT_EQ(extract_entity_type_from_path("/api/v1/functions"), SovdEntityType::FUNCTION); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/functions/my_func"), SovdEntityType::FUNCTION); +} + +TEST(ExtractEntityTypePath, unknown_routes) { + using ros2_medkit_gateway::extract_entity_type_from_path; + using ros2_medkit_gateway::SovdEntityType; + + EXPECT_EQ(extract_entity_type_from_path("/api/v1/health"), SovdEntityType::UNKNOWN); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/faults"), SovdEntityType::UNKNOWN); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/version-info"), SovdEntityType::UNKNOWN); + EXPECT_EQ(extract_entity_type_from_path("/other/path"), SovdEntityType::UNKNOWN); +} + int main(int argc, char ** argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/src/ros2_medkit_gateway/test/test_integration.test.py b/src/ros2_medkit_gateway/test/test_integration.test.py index ef19dca0..d93176a4 100644 --- a/src/ros2_medkit_gateway/test/test_integration.test.py +++ b/src/ros2_medkit_gateway/test/test_integration.test.py @@ -1698,10 +1698,15 @@ def test_40_action_status_endpoint(self): self.assertEqual(response.status_code, 202) execution_id = response.json()['id'] - # Check status immediately (allow extra time for action server response) + # Poll for execution to be registered (may take a moment) exec_url = (f'{self.BASE_URL}/apps/long_calibration/operations/' f'long_calibration/executions/{execution_id}') - status_response = requests.get(exec_url, timeout=10) + status_response = None + for _ in range(10): + status_response = requests.get(exec_url, timeout=10) + if status_response.status_code == 200: + break + time.sleep(0.2) self.assertEqual(status_response.status_code, 200) data = status_response.json() @@ -3651,12 +3656,17 @@ def test_100_get_execution_status(self): execution_id = create_response.json()['id'] - # Now get the execution status - response = requests.get( - f'{self.BASE_URL}/apps/{app_id}/operations/{operation_id}' - f'/executions/{execution_id}', - timeout=10 - ) + # Poll for execution to be registered (may take a moment) + response = None + for _ in range(10): + response = requests.get( + f'{self.BASE_URL}/apps/{app_id}/operations/{operation_id}' + f'/executions/{execution_id}', + timeout=10 + ) + if response.status_code == 200: + break + time.sleep(0.2) self.assertEqual( response.status_code, 200, @@ -4248,3 +4258,163 @@ def test_115_list_function_data_invalid_id(self): self.assertIn('invalid', data['message'].lower()) print('✓ Function data invalid ID test passed') + + # ========== Entity Type Validation Tests (test_116-120) ========== + + def test_116_component_route_rejects_app_id(self): + """ + Test that /components/{id}/data rejects app IDs. + + In runtime-only discovery mode, /components/{id} should only accept + synthetic component IDs, not individual ROS 2 node (app) IDs. + + @verifies REQ_INTEROP_003 + """ + # First, get list of apps to find a valid app ID + apps_response = requests.get(f'{self.BASE_URL}/apps', timeout=10) + self.assertEqual(apps_response.status_code, 200) + + apps = apps_response.json().get('items', []) + if len(apps) == 0: + self.skipTest('No apps available for testing') + + # Use the first available app + app_id = apps[0]['id'] + + # Verify this ID is recognized as an app via /apps/{id} + app_response = requests.get(f'{self.BASE_URL}/apps/{app_id}', timeout=10) + self.assertEqual(app_response.status_code, 200) + + # Now try to use this app ID with /components/{id}/data - should fail + response = requests.get( + f'{self.BASE_URL}/components/{app_id}/data', + timeout=10 + ) + self.assertEqual( + response.status_code, + 400, + f'Expected 400 when using app ID "{app_id}" with /components route' + ) + + data = response.json() + self.assertIn('error_code', data) + self.assertEqual(data['error_code'], 'invalid-parameter') + self.assertIn('Invalid entity type for route', data['message']) + self.assertIn('expected_type', data.get('parameters', {})) + self.assertIn('actual_type', data.get('parameters', {})) + self.assertEqual(data['parameters']['expected_type'], 'Component') + self.assertEqual(data['parameters']['actual_type'], 'App') + + print(f'✓ Component route correctly rejects app ID: {app_id}') + + def test_117_component_route_rejects_app_id_operations(self): + """ + Test that /components/{id}/operations rejects app IDs. + + @verifies REQ_INTEROP_003 + """ + # First, get list of apps to find a valid app ID + apps_response = requests.get(f'{self.BASE_URL}/apps', timeout=10) + self.assertEqual(apps_response.status_code, 200) + + apps = apps_response.json().get('items', []) + if len(apps) == 0: + self.skipTest('No apps available for testing') + + app_id = apps[0]['id'] + + # Try to use app ID with /components/{id}/operations - should fail + response = requests.get( + f'{self.BASE_URL}/components/{app_id}/operations', + timeout=10 + ) + self.assertEqual(response.status_code, 400) + + data = response.json() + self.assertEqual(data['error_code'], 'invalid-parameter') + self.assertIn('Invalid entity type for route', data['message']) + + print(f'✓ Component operations route correctly rejects app ID: {app_id}') + + def test_118_component_route_rejects_app_id_configurations(self): + """ + Test that /components/{id}/configurations rejects app IDs. + + @verifies REQ_INTEROP_003 + """ + apps_response = requests.get(f'{self.BASE_URL}/apps', timeout=10) + self.assertEqual(apps_response.status_code, 200) + + apps = apps_response.json().get('items', []) + if len(apps) == 0: + self.skipTest('No apps available for testing') + + app_id = apps[0]['id'] + + response = requests.get( + f'{self.BASE_URL}/components/{app_id}/configurations', + timeout=10 + ) + self.assertEqual(response.status_code, 400) + + data = response.json() + self.assertEqual(data['error_code'], 'invalid-parameter') + self.assertIn('Invalid entity type for route', data['message']) + + print(f'✓ Component configurations route correctly rejects app ID: {app_id}') + + def test_119_component_route_rejects_app_id_faults(self): + """ + Test that /components/{id}/faults rejects app IDs. + + @verifies REQ_INTEROP_003 + """ + apps_response = requests.get(f'{self.BASE_URL}/apps', timeout=10) + self.assertEqual(apps_response.status_code, 200) + + apps = apps_response.json().get('items', []) + if len(apps) == 0: + self.skipTest('No apps available for testing') + + app_id = apps[0]['id'] + + response = requests.get( + f'{self.BASE_URL}/components/{app_id}/faults', + timeout=10 + ) + self.assertEqual(response.status_code, 400) + + data = response.json() + self.assertEqual(data['error_code'], 'invalid-parameter') + self.assertIn('Invalid entity type for route', data['message']) + + print(f'✓ Component faults route correctly rejects app ID: {app_id}') + + def test_120_app_routes_work_with_app_id(self): + """ + Test that /apps/{id}/data works correctly with app IDs. + + Verify that while /components rejects app IDs, /apps accepts them. + + @verifies REQ_INTEROP_003 + """ + apps_response = requests.get(f'{self.BASE_URL}/apps', timeout=10) + self.assertEqual(apps_response.status_code, 200) + + apps = apps_response.json().get('items', []) + if len(apps) == 0: + self.skipTest('No apps available for testing') + + app_id = apps[0]['id'] + + # /apps/{id}/data should work + response = requests.get( + f'{self.BASE_URL}/apps/{app_id}/data', + timeout=10 + ) + self.assertEqual(response.status_code, 200) + + data = response.json() + self.assertIn('items', data) + + print(f'✓ App routes correctly accept app ID: {app_id}') From 175377237efeb3d34f2e48a325c07732d27cb269 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 1 Feb 2026 19:30:23 +0000 Subject: [PATCH 2/4] fix(gateway): address PR #160 review comments - Fix collection matching in extract_entity_type_from_path to require segment boundaries (prevents /componentship matching as COMPONENT) - Remove unused validate_entity_type helper from HandlerContext - Fix integration tests 116-119 to avoid app/component ID collisions by selecting app IDs that don't exist in the components list - Add unit tests for segment-boundary matching edge cases --- .../http/handlers/handler_context.hpp | 13 +--- .../ros2_medkit_gateway/http/http_utils.hpp | 23 +++++- .../src/http/handlers/handler_context.cpp | 16 ---- .../test/test_gateway_node.cpp | 7 ++ .../test/test_integration.test.py | 77 +++++++++++++++++-- 5 files changed, 99 insertions(+), 37 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp index ed68651b..31880719 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp @@ -176,18 +176,7 @@ class HandlerContext { static std::optional validate_collection_access(const EntityInfo & entity, ResourceCollection collection); - /** - * @brief Validate entity type matches the route path - * - * Ensures semantic correctness - /components/{id} should only accept - * component IDs, not app IDs. This prevents the dual-path routing bug - * where /components/{app_id} would incorrectly succeed. - * - * @param entity Entity information (from get_entity_info) - * @param expected_type Expected type based on route (from extract_entity_type_from_path) - * @return std::nullopt if valid, error message if type mismatch - */ - static std::optional validate_entity_type(const EntityInfo & entity, SovdEntityType expected_type); + /** * @brief Set CORS headers on response if origin is allowed diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp index 558ade91..a556e938 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp @@ -49,16 +49,31 @@ inline SovdEntityType extract_entity_type_from_path(const std::string & path) { // Check for each entity type prefix after API base path const std::string base = std::string(API_BASE_PATH) + "/"; - if (path.find(base + "components/") == 0 || path.find(base + "components") == 0) { + // Require a segment boundary after the collection name: + // matches "/api/v1/components" or "/api/v1/components/...", but not "/api/v1/componentship". + auto matches_collection = [&path, &base](const std::string & collection) -> bool { + const std::string prefix = base + collection; + if (path.compare(0, prefix.size(), prefix) != 0) { + return false; + } + if (path.size() == prefix.size()) { + // Exact match: "/api/v1/{collection}" + return true; + } + // Require a '/' segment separator after the collection name + return path[prefix.size()] == '/'; + }; + + if (matches_collection("components")) { return SovdEntityType::COMPONENT; } - if (path.find(base + "apps/") == 0 || path.find(base + "apps") == 0) { + if (matches_collection("apps")) { return SovdEntityType::APP; } - if (path.find(base + "areas/") == 0 || path.find(base + "areas") == 0) { + if (matches_collection("areas")) { return SovdEntityType::AREA; } - if (path.find(base + "functions/") == 0 || path.find(base + "functions") == 0) { + if (matches_collection("functions")) { return SovdEntityType::FUNCTION; } diff --git a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp index 90fedcbf..6f8c86fc 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp @@ -194,22 +194,6 @@ std::optional HandlerContext::validate_collection_access(const Enti return std::nullopt; } -std::optional HandlerContext::validate_entity_type(const EntityInfo & entity, - SovdEntityType expected_type) { - // If expected_type is UNKNOWN, skip validation (path doesn't specify an entity type) - if (expected_type == SovdEntityType::UNKNOWN) { - return std::nullopt; - } - - // Check if entity type matches expected type from route - if (entity.sovd_type() != expected_type) { - return "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity.sovd_type()); - } - - return std::nullopt; -} - void HandlerContext::set_cors_headers(httplib::Response & res, const std::string & origin) const { res.set_header("Access-Control-Allow-Origin", origin); diff --git a/src/ros2_medkit_gateway/test/test_gateway_node.cpp b/src/ros2_medkit_gateway/test/test_gateway_node.cpp index c3ce7565..cbd28095 100644 --- a/src/ros2_medkit_gateway/test/test_gateway_node.cpp +++ b/src/ros2_medkit_gateway/test/test_gateway_node.cpp @@ -745,6 +745,13 @@ TEST(ExtractEntityTypePath, unknown_routes) { EXPECT_EQ(extract_entity_type_from_path("/api/v1/faults"), SovdEntityType::UNKNOWN); EXPECT_EQ(extract_entity_type_from_path("/api/v1/version-info"), SovdEntityType::UNKNOWN); EXPECT_EQ(extract_entity_type_from_path("/other/path"), SovdEntityType::UNKNOWN); + + // Verify paths that look similar to entity collections but aren't + // (segment boundary validation) + EXPECT_EQ(extract_entity_type_from_path("/api/v1/componentship"), SovdEntityType::UNKNOWN); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/applications"), SovdEntityType::UNKNOWN); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/areascan"), SovdEntityType::UNKNOWN); + EXPECT_EQ(extract_entity_type_from_path("/api/v1/functional"), SovdEntityType::UNKNOWN); } int main(int argc, char ** argv) { diff --git a/src/ros2_medkit_gateway/test/test_integration.test.py b/src/ros2_medkit_gateway/test/test_integration.test.py index d93176a4..239594d0 100644 --- a/src/ros2_medkit_gateway/test/test_integration.test.py +++ b/src/ros2_medkit_gateway/test/test_integration.test.py @@ -4278,8 +4278,24 @@ def test_116_component_route_rejects_app_id(self): if len(apps) == 0: self.skipTest('No apps available for testing') - # Use the first available app - app_id = apps[0]['id'] + # Get list of components to avoid selecting an app ID that is also a component ID + components_response = requests.get(f'{self.BASE_URL}/components', timeout=10) + self.assertEqual(components_response.status_code, 200) + + components = components_response.json().get('items', []) + component_ids = {c.get('id') for c in components if isinstance(c, dict) and 'id' in c} + + # Find an app ID that does not collide with any component ID + app_id = None + for app in apps: + if not isinstance(app, dict) or 'id' not in app: + continue + if app['id'] not in component_ids: + app_id = app['id'] + break + + if app_id is None: + self.skipTest('No app ID available that is distinct from component IDs') # Verify this ID is recognized as an app via /apps/{id} app_response = requests.get(f'{self.BASE_URL}/apps/{app_id}', timeout=10) @@ -4321,7 +4337,24 @@ def test_117_component_route_rejects_app_id_operations(self): if len(apps) == 0: self.skipTest('No apps available for testing') - app_id = apps[0]['id'] + # Get list of components to avoid selecting an app ID that is also a component ID + components_response = requests.get(f'{self.BASE_URL}/components', timeout=10) + self.assertEqual(components_response.status_code, 200) + + components = components_response.json().get('items', []) + component_ids = {c.get('id') for c in components if isinstance(c, dict) and 'id' in c} + + # Find an app ID that does not collide with any component ID + app_id = None + for app in apps: + if not isinstance(app, dict) or 'id' not in app: + continue + if app['id'] not in component_ids: + app_id = app['id'] + break + + if app_id is None: + self.skipTest('No app ID available that is distinct from component IDs') # Try to use app ID with /components/{id}/operations - should fail response = requests.get( @@ -4349,7 +4382,24 @@ def test_118_component_route_rejects_app_id_configurations(self): if len(apps) == 0: self.skipTest('No apps available for testing') - app_id = apps[0]['id'] + # Get list of components to avoid selecting an app ID that is also a component ID + components_response = requests.get(f'{self.BASE_URL}/components', timeout=10) + self.assertEqual(components_response.status_code, 200) + + components = components_response.json().get('items', []) + component_ids = {c.get('id') for c in components if isinstance(c, dict) and 'id' in c} + + # Find an app ID that does not collide with any component ID + app_id = None + for app in apps: + if not isinstance(app, dict) or 'id' not in app: + continue + if app['id'] not in component_ids: + app_id = app['id'] + break + + if app_id is None: + self.skipTest('No app ID available that is distinct from component IDs') response = requests.get( f'{self.BASE_URL}/components/{app_id}/configurations', @@ -4376,7 +4426,24 @@ def test_119_component_route_rejects_app_id_faults(self): if len(apps) == 0: self.skipTest('No apps available for testing') - app_id = apps[0]['id'] + # Get list of components to avoid selecting an app ID that is also a component ID + components_response = requests.get(f'{self.BASE_URL}/components', timeout=10) + self.assertEqual(components_response.status_code, 200) + + components = components_response.json().get('items', []) + component_ids = {c.get('id') for c in components if isinstance(c, dict) and 'id' in c} + + # Find an app ID that does not collide with any component ID + app_id = None + for app in apps: + if not isinstance(app, dict) or 'id' not in app: + continue + if app['id'] not in component_ids: + app_id = app['id'] + break + + if app_id is None: + self.skipTest('No app ID available that is distinct from component IDs') response = requests.get( f'{self.BASE_URL}/components/{app_id}/faults', From fa4279d6b0a8c914bc4a5997bfa65a51f4381c69 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 1 Feb 2026 19:53:57 +0000 Subject: [PATCH 3/4] fix(gateway): unify entity validation across handlers - Add validate_entity_for_route() helper in HandlerContext that combines entity_id validation, path-based type extraction, and entity lookup - Refactor data_handlers.cpp (3 handlers) to use unified validation - Refactor config_handlers.cpp (7 handlers) to use unified validation - Refactor fault_handlers.cpp (5 handlers) to use unified validation - Refactor operation_handlers.cpp (3 handlers) to use unified validation - Net reduction of ~270 lines of duplicated validation code Addresses review comments from mfaferek93 on PR #160 --- .../http/handlers/handler_context.hpp | 19 ++- .../src/http/handlers/config_handlers.cpp | 155 +++--------------- .../src/http/handlers/data_handlers.cpp | 98 ++--------- .../src/http/handlers/fault_handlers.cpp | 142 +++------------- .../src/http/handlers/handler_context.cpp | 36 ++++ .../src/http/handlers/operation_handlers.cpp | 95 ++--------- 6 files changed, 136 insertions(+), 409 deletions(-) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp index 31880719..25674c6d 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp @@ -176,7 +176,24 @@ class HandlerContext { static std::optional validate_collection_access(const EntityInfo & entity, ResourceCollection collection); - + /** + * @brief Validate entity exists and matches expected route type + * + * Unified validation helper that: + * 1. Validates entity ID format + * 2. Looks up entity in the expected collection (based on route path) + * 3. Sends appropriate error responses: + * - 400 with "invalid-parameter" if ID format is invalid + * - 400 with "invalid-parameter" if entity exists but wrong type for route + * - 404 with "entity-not-found" if entity doesn't exist + * + * @param req HTTP request (used to extract expected type from path) + * @param res HTTP response (error responses sent here) + * @param entity_id Entity ID to validate + * @return EntityInfo if valid, std::nullopt if error response was sent + */ + std::optional validate_entity_for_route(const httplib::Request & req, httplib::Response & res, + const std::string & entity_id) const; /** * @brief Set CORS headers on response if origin is allowed diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index c8fcd9a9..bd91ec4c 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -221,35 +221,14 @@ void ConfigHandlers::handle_list_configurations(const httplib::Request & req, ht entity_id = req.matches[1]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; - } - - // First, verify that the entity actually exists in the cache - const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } // Get aggregated configurations info for this entity + const auto & cache = ctx_.node()->get_thread_safe_cache(); auto agg_configs = cache.get_entity_configurations(entity_id); // If no nodes to query, return empty result @@ -384,11 +363,10 @@ void ConfigHandlers::handle_get_configuration(const httplib::Request & req, http entity_id = req.matches[1]; param_id = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } // Parameter ID may be prefixed with app_id: for aggregated configs @@ -398,28 +376,8 @@ void ConfigHandlers::handle_get_configuration(const httplib::Request & req, http return; } - // Verify entity exists - const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; - } - // Get aggregated configurations info + const auto & cache = ctx_.node()->get_thread_safe_cache(); auto agg_configs = cache.get_entity_configurations(entity_id); if (agg_configs.nodes.empty()) { @@ -533,11 +491,10 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http entity_id = req.matches[1]; param_id = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } if (param_id.empty() || param_id.length() > MAX_AGGREGATED_PARAM_ID_LENGTH) { @@ -568,28 +525,8 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http return; } - // Verify entity exists - const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; - } - // Get aggregated configurations info + const auto & cache = ctx_.node()->get_thread_safe_cache(); auto agg_configs = cache.get_entity_configurations(entity_id); if (agg_configs.nodes.empty()) { @@ -678,35 +615,14 @@ void ConfigHandlers::handle_delete_configuration(const httplib::Request & req, h entity_id = req.matches[1]; param_id = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; - } - - // Verify entity exists - const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } // Get aggregated configurations info + const auto & cache = ctx_.node()->get_thread_safe_cache(); auto agg_configs = cache.get_entity_configurations(entity_id); if (agg_configs.nodes.empty()) { @@ -779,35 +695,14 @@ void ConfigHandlers::handle_delete_all_configurations(const httplib::Request & r entity_id = req.matches[1]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; - } - - // Verify entity exists - const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } // Get aggregated configurations info + const auto & cache = ctx_.node()->get_thread_safe_cache(); auto agg_configs = cache.get_entity_configurations(entity_id); if (agg_configs.nodes.empty()) { diff --git a/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp index 17c48d78..9c96d34d 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp @@ -38,39 +38,15 @@ void DataHandlers::handle_list_data(const httplib::Request & req, httplib::Respo entity_id = req.matches[1]; - auto validation_result = ctx_.validate_entity_id(entity_id); - if (!validation_result) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", validation_result.error()}, {"entity_id", entity_id}}); - return; - } - - // First, verify that the entity actually exists in the cache - const auto & cache = ctx_.node()->get_thread_safe_cache(); - - // Get expected type from route and look up entity in that specific collection - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - // Check if entity exists in ANY collection (for better error message) - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - // Entity exists but wrong type -> 400 - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - // Entity doesn't exist at all -> 404 - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Use unified cache method to get aggregated data + const auto & cache = ctx_.node()->get_thread_safe_cache(); auto aggregated = cache.get_entity_data(entity_id); // Get data access manager for type introspection @@ -145,33 +121,10 @@ void DataHandlers::handle_get_data_item(const httplib::Request & req, httplib::R // cpp-httplib automatically decodes percent-encoded characters in URL path topic_name = req.matches[2]; - auto validation_result = ctx_.validate_entity_id(entity_id); - if (!validation_result) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", validation_result.error()}, {"entity_id", entity_id}}); - return; - } - - // Verify entity exists in the correct collection for this route - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - // Check if entity exists in ANY collection (for better error message) - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - // Entity exists but wrong type -> 400 - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - // Entity doesn't exist at all -> 404 - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } // Determine the full ROS topic path @@ -250,11 +203,10 @@ void DataHandlers::handle_put_data_item(const httplib::Request & req, httplib::R entity_id = req.matches[1]; topic_name = req.matches[2]; - auto validation_result = ctx_.validate_entity_id(entity_id); - if (!validation_result) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", validation_result.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } // Parse request body @@ -297,28 +249,6 @@ void DataHandlers::handle_put_data_item(const httplib::Request & req, httplib::R return; } - // Verify entity exists in the correct collection for this route - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - // Check if entity exists in ANY collection (for better error message) - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - // Entity exists but wrong type -> 400 - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - // Entity doesn't exist at all -> 404 - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; - } - // Build full topic path (mirror GET logic: only prefix '/' when needed) std::string full_topic_path = topic_name; if (!full_topic_path.empty() && full_topic_path.front() != '/') { diff --git a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp index 00d6861a..6884fde4 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp @@ -187,30 +187,12 @@ void FaultHandlers::handle_list_faults(const httplib::Request & req, httplib::Re entity_id = req.matches[1]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; - } - - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; auto filter = parse_fault_status_param(req); if (!filter.is_valid) { @@ -364,12 +346,12 @@ void FaultHandlers::handle_get_fault(const httplib::Request & req, httplib::Resp entity_id = req.matches[1]; fault_code = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Fault codes may contain dots and underscores, validate basic constraints if (fault_code.empty() || fault_code.length() > 256) { @@ -378,24 +360,6 @@ void FaultHandlers::handle_get_fault(const httplib::Request & req, httplib::Resp return; } - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; - } - std::string namespace_path = entity_info.namespace_path; auto fault_mgr = ctx_.node()->get_fault_manager(); @@ -444,12 +408,12 @@ void FaultHandlers::handle_clear_fault(const httplib::Request & req, httplib::Re entity_id = req.matches[1]; fault_code = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Validate fault code if (fault_code.empty() || fault_code.length() > 256) { @@ -458,25 +422,6 @@ void FaultHandlers::handle_clear_fault(const httplib::Request & req, httplib::Re return; } - // Verify entity exists - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; - } - auto fault_mgr = ctx_.node()->get_fault_manager(); auto result = fault_mgr->clear_fault(fault_code); @@ -514,31 +459,12 @@ void FaultHandlers::handle_clear_all_faults(const httplib::Request & req, httpli entity_id = req.matches[1]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; - } - - // Verify entity exists - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Get all faults for this entity auto fault_mgr = ctx_.node()->get_fault_manager(); @@ -633,12 +559,12 @@ void FaultHandlers::handle_get_component_snapshots(const httplib::Request & req, entity_id = req.matches[1]; fault_code = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Validate fault code if (fault_code.empty() || fault_code.length() > 256) { @@ -647,24 +573,6 @@ void FaultHandlers::handle_get_component_snapshots(const httplib::Request & req, return; } - auto expected_type = extract_entity_type_from_path(req.path); - auto entity_info = ctx_.get_entity_info(entity_id, expected_type); - if (entity_info.type == EntityType::UNKNOWN) { - auto any_entity = ctx_.get_entity_info(entity_id); - if (any_entity.type != EntityType::UNKNOWN) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(any_entity.sovd_type()), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(any_entity.sovd_type())}}); - } else { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - } - return; - } - // Optional topic filter from query parameter std::string topic_filter = req.get_param_value("topic"); diff --git a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp index 6f8c86fc..3267a817 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp @@ -194,6 +194,42 @@ std::optional HandlerContext::validate_collection_access(const Enti return std::nullopt; } +std::optional HandlerContext::validate_entity_for_route(const httplib::Request & req, + httplib::Response & res, + const std::string & entity_id) const { + // Step 1: Validate entity ID format + auto validation_result = validate_entity_id(entity_id); + if (!validation_result) { + send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", + {{"details", validation_result.error()}, {"entity_id", entity_id}}); + return std::nullopt; + } + + // Step 2: Get expected type from route path and look up entity + auto expected_type = extract_entity_type_from_path(req.path); + auto entity_info = get_entity_info(entity_id, expected_type); + + if (entity_info.type == EntityType::UNKNOWN) { + // Step 3: Check if entity exists in ANY collection (for better error message) + auto any_entity = get_entity_info(entity_id); + if (any_entity.type != EntityType::UNKNOWN) { + // Entity exists but wrong type for this route -> 400 + send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, + "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + + to_string(any_entity.sovd_type()), + {{"entity_id", entity_id}, + {"expected_type", to_string(expected_type)}, + {"actual_type", to_string(any_entity.sovd_type())}}); + } else { + // Entity doesn't exist at all -> 404 + send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", {{"entity_id", entity_id}}); + } + return std::nullopt; + } + + return entity_info; +} + void HandlerContext::set_cors_headers(httplib::Response & res, const std::string & origin) const { res.set_header("Access-Control-Allow-Origin", origin); diff --git a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp index 544f9c1c..02952c21 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp @@ -38,12 +38,12 @@ void OperationHandlers::handle_list_operations(const httplib::Request & req, htt entity_id = req.matches[1]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Use ThreadSafeEntityCache for O(1) entity lookup and aggregated operations const auto & cache = ctx_.node()->get_thread_safe_cache(); @@ -52,26 +52,7 @@ void OperationHandlers::handle_list_operations(const httplib::Request & req, htt AggregatedOperations ops; std::string entity_type; - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; - } - - switch (entity_ref->type) { + switch (entity_info.sovd_type()) { case SovdEntityType::COMPONENT: ops = cache.get_component_operations(entity_id); entity_type = "component"; @@ -192,40 +173,20 @@ void OperationHandlers::handle_get_operation(const httplib::Request & req, httpl entity_id = req.matches[1]; operation_id = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Use ThreadSafeEntityCache for O(1) entity lookup const auto & cache = ctx_.node()->get_thread_safe_cache(); - // Find entity - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; - } - // Get aggregated operations based on entity type AggregatedOperations ops; std::string entity_type; - switch (entity_ref->type) { + switch (entity_info.sovd_type()) { case SovdEntityType::COMPONENT: ops = cache.get_component_operations(entity_id); entity_type = "component"; @@ -379,12 +340,12 @@ void OperationHandlers::handle_create_execution(const httplib::Request & req, ht entity_id = req.matches[1]; operation_id = req.matches[2]; - auto entity_validation = ctx_.validate_entity_id(entity_id); - if (!entity_validation) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", - {{"details", entity_validation.error()}, {"entity_id", entity_id}}); - return; + // Validate entity ID and type for this route + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent } + auto entity_info = *entity_opt; // Parse request body json body = json::object(); @@ -401,30 +362,10 @@ void OperationHandlers::handle_create_execution(const httplib::Request & req, ht // Use ThreadSafeEntityCache for O(1) entity lookup const auto & cache = ctx_.node()->get_thread_safe_cache(); - // Find entity and get its operations - auto entity_ref = cache.find_entity(entity_id); - if (!entity_ref) { - HandlerContext::send_error(res, StatusCode::NotFound_404, ERR_ENTITY_NOT_FOUND, "Entity not found", - {{"entity_id", entity_id}}); - return; - } - - // Validate entity type matches the route path - auto expected_type = extract_entity_type_from_path(req.path); - if (expected_type != SovdEntityType::UNKNOWN && entity_ref->type != expected_type) { - HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, - "Invalid entity type for route: expected " + to_string(expected_type) + ", got " + - to_string(entity_ref->type), - {{"entity_id", entity_id}, - {"expected_type", to_string(expected_type)}, - {"actual_type", to_string(entity_ref->type)}}); - return; - } - // Get aggregated operations based on entity type AggregatedOperations ops; std::string entity_type; - switch (entity_ref->type) { + switch (entity_info.sovd_type()) { case SovdEntityType::COMPONENT: ops = cache.get_component_operations(entity_id); entity_type = "component"; From 0a2c1367b30148367e475306c999262d3c074063 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 1 Feb 2026 20:19:35 +0000 Subject: [PATCH 4/4] fix(gateway): preserve JSON validation before entity existence check In handle_set_configuration, validate JSON body format before checking entity existence. This ensures BadRequest (400) is returned for invalid JSON instead of NotFound (404), matching the expected error priority. Fixes test_set_configuration_invalid_json and test_set_configuration_missing_value_field tests. --- .../src/http/handlers/config_handlers.cpp | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index bd91ec4c..5c9bbc5d 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -491,10 +491,12 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http entity_id = req.matches[1]; param_id = req.matches[2]; - // Validate entity ID and type for this route - auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); - if (!entity_opt) { - return; // Error response already sent + // Validate entity_id format first + auto entity_validation = ctx_.validate_entity_id(entity_id); + if (!entity_validation) { + HandlerContext::send_error(res, StatusCode::BadRequest_400, ERR_INVALID_PARAMETER, "Invalid entity ID", + {{"details", entity_validation.error()}, {"entity_id", entity_id}}); + return; } if (param_id.empty() || param_id.length() > MAX_AGGREGATED_PARAM_ID_LENGTH) { @@ -503,7 +505,7 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http return; } - // Parse request body + // Parse request body before checking entity existence json body; try { body = json::parse(req.body); @@ -525,6 +527,12 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http return; } + // Now validate entity exists and matches route type + auto entity_opt = ctx_.validate_entity_for_route(req, res, entity_id); + if (!entity_opt) { + return; // Error response already sent + } + // Get aggregated configurations info const auto & cache = ctx_.node()->get_thread_safe_cache(); auto agg_configs = cache.get_entity_configurations(entity_id);