fix: address CORS code review feedback

- Fix CORS enabled logic to check only allowed_origins (not methods/headers)
- Add security validation: reject credentials=true with wildcard origin
- Move Access-Control-Max-Age header to preflight responses only
- Add PUT to default allowed_methods for REST API compatibility
- Fix misleading YAML comment about empty origins behavior
- Optimize: pre-build CORS header strings in constructor
- Add 7 CORS integration tests covering all scenarios

Author: bburda

src/ros2_medkit_gateway/CMakeLists.txt

@@ -96,6 +96,13 @@ if(BUILD_TESTING)
     TIMEOUT 120
   )
 
+  # Add CORS integration tests
+  add_launch_test(
+    test/test_cors.test.py
+    TARGET test_cors
+    TIMEOUT 60
+  )
+
   # Demo automotive nodes
   add_executable(demo_engine_temp_sensor
     test/demo_nodes/engine_temp_sensor.cpp

src/ros2_medkit_gateway/config/gateway_params.yaml

@@ -32,12 +32,12 @@ ros2_medkit_gateway:
       # List of allowed origins
       # Examples: ["http://localhost:5173", "https://example.com"]
       # Use ["*"] to allow all origins (not recommended for production)
-      # Empty list with other settings = allow all origins
+      # Empty list = allow all origins (not recommended for production)
       allowed_origins: []
 
       # Allowed HTTP methods for CORS requests
-      # Default: ["GET", "OPTIONS"]
-      allowed_methods: ["GET", "OPTIONS"]
+      # Default: ["GET", "PUT", "OPTIONS"]
+      allowed_methods: ["GET", "PUT", "OPTIONS"]
 
       # Allowed headers in CORS requests
       # Use ["*"] to allow all headers

src/ros2_medkit_gateway/include/ros2_medkit_gateway/rest_server.hpp

@@ -59,6 +59,11 @@ class RESTServer {
   std::string host_;
   int port_;
   CorsConfig cors_config_;
+
+  // Pre-built CORS header values (built once in constructor for performance)
+  std::string cors_methods_header_;
+  std::string cors_headers_header_;
+
   std::unique_ptr<httplib::Server> server_;
 };
 

src/ros2_medkit_gateway/src/gateway_node.cpp

@@ -28,7 +28,7 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") {
   declare_parameter("server.port", 8080);
   declare_parameter("refresh_interval_ms", 2000);
   declare_parameter("cors.allowed_origins", std::vector<std::string>{});
-  declare_parameter("cors.allowed_methods", std::vector<std::string>{"GET", "OPTIONS"});
+  declare_parameter("cors.allowed_methods", std::vector<std::string>{"GET", "PUT", "OPTIONS"});
   declare_parameter("cors.allowed_headers", std::vector<std::string>{"Content-Type", "Accept"});
   declare_parameter("cors.allow_credentials", false);
   declare_parameter("cors.max_age_seconds", 86400);
@@ -45,10 +45,19 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") {
   cors_config_.allow_credentials = get_parameter("cors.allow_credentials").as_bool();
   cors_config_.max_age_seconds = get_parameter("cors.max_age_seconds").as_int();
 
-  // CORS is enabled if any origins are configured or if methods/headers are non-default
-  // Treat missing/empty config as disabled for security
-  cors_config_.enabled = !cors_config_.allowed_origins.empty() || !cors_config_.allowed_methods.empty() ||
-                         !cors_config_.allowed_headers.empty();
+  // CORS is enabled only if allowed_origins is configured
+  // Treat missing/empty origins as disabled for security
+  cors_config_.enabled = !cors_config_.allowed_origins.empty();
+
+  // Validate CORS configuration: credentials cannot be used with wildcard origin
+  if (cors_config_.enabled && cors_config_.allow_credentials) {
+    for (const auto & origin : cors_config_.allowed_origins) {
+      if (origin == "*") {
+        RCLCPP_ERROR(get_logger(), "CORS: allow_credentials cannot be true when allowed_origins contains '*'");
+        throw std::runtime_error("Invalid CORS configuration");
+      }
+    }
+  }
 
   // Validate port range
   if (server_port_ < 1024 || server_port_ > 65535) {

src/ros2_medkit_gateway/src/rest_server.cpp

@@ -31,6 +31,20 @@ RESTServer::RESTServer(GatewayNode * node, const std::string & host, int port, c
   : node_(node), host_(host), port_(port), cors_config_(cors_config) {
   server_ = std::make_unique<httplib::Server>();
 
+  // Pre-build CORS header strings once (performance optimization)
+  for (const auto & method : cors_config_.allowed_methods) {
+    if (!cors_methods_header_.empty()) {
+      cors_methods_header_ += ", ";
+    }
+    cors_methods_header_ += method;
+  }
+  for (const auto & header : cors_config_.allowed_headers) {
+    if (!cors_headers_header_.empty()) {
+      cors_headers_header_ += ", ";
+    }
+    cors_headers_header_ += header;
+  }
+
   // Set up pre-routing handler for CORS (only if enabled)
   if (cors_config_.enabled) {
     server_->set_pre_routing_handler([this](const httplib::Request & req, httplib::Response & res) {
@@ -41,6 +55,8 @@ RESTServer::RESTServer(GatewayNode * node, const std::string & host, int port, c
 
       // Handle preflight OPTIONS requests
       if (req.method == "OPTIONS") {
+        // Set Max-Age only for preflight responses
+        res.set_header("Access-Control-Max-Age", std::to_string(cors_config_.max_age_seconds));
         res.status = 204;  // No Content
         return httplib::Server::HandlerResponse::Handled;
       }
@@ -586,37 +602,18 @@ void RESTServer::handle_component_topic_publish(const httplib::Request & req, ht
 void RESTServer::set_cors_headers(httplib::Response & res, const std::string & origin) const {
   res.set_header("Access-Control-Allow-Origin", origin);
 
-  // Build methods string from config
-  std::string methods_str;
-  for (const auto & method : cors_config_.allowed_methods) {
-    if (!methods_str.empty()) {
-      methods_str += ", ";
-    }
-    methods_str += method;
+  // Use pre-built header strings (built once in constructor)
+  if (!cors_methods_header_.empty()) {
+    res.set_header("Access-Control-Allow-Methods", cors_methods_header_);
   }
-  if (!methods_str.empty()) {
-    res.set_header("Access-Control-Allow-Methods", methods_str);
-  }
-
-  // Build headers string from config
-  std::string headers_str;
-  for (const auto & header : cors_config_.allowed_headers) {
-    if (!headers_str.empty()) {
-      headers_str += ", ";
-    }
-    headers_str += header;
-  }
-  if (!headers_str.empty()) {
-    res.set_header("Access-Control-Allow-Headers", headers_str);
+  if (!cors_headers_header_.empty()) {
+    res.set_header("Access-Control-Allow-Headers", cors_headers_header_);
   }
 
   // Set credentials header if enabled
   if (cors_config_.allow_credentials) {
     res.set_header("Access-Control-Allow-Credentials", "true");
   }
-
-  // Set max age
-  res.set_header("Access-Control-Max-Age", std::to_string(cors_config_.max_age_seconds));
 }
 
 bool RESTServer::is_origin_allowed(const std::string & origin) const {