fix: address CORS code review feedback

- Introduce CorsConfigBuilder with fluent interface for configuration
- Move CORS validation (credentials + wildcard) into builder
- Extract config.cpp for builder implementation
- Remove dead code (unreachable empty origins check)
- Fix OPTIONS preflight: no Max-Age for disallowed origins
- Use StatusCode::NoContent_204 instead of magic number
- Fix YAML comment: wildcard headers not supported
- Consolidate CORS tests into single file with 10 test cases
- Fix pep257 docstring length warnings

Author: bburda

src/ros2_medkit_gateway/CMakeLists.txt

@@ -30,6 +30,7 @@ include_directories(include)
 # Gateway node executable
 add_executable(gateway_node
   src/main.cpp
+  src/config.cpp
   src/gateway_node.cpp
   src/rest_server.cpp
   src/discovery_manager.cpp

src/ros2_medkit_gateway/config/gateway_params.yaml

@@ -32,15 +32,15 @@ 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 = allow all origins (not recommended for production)
+      # Empty list = CORS disabled
       allowed_origins: []
 
       # Allowed HTTP methods for CORS requests
       # Default: ["GET", "PUT", "OPTIONS"]
       allowed_methods: ["GET", "PUT", "OPTIONS"]
 
       # Allowed headers in CORS requests
-      # Use ["*"] to allow all headers
+      # List the allowed headers explicitly (wildcard "*" is not supported)
       allowed_headers: ["Content-Type", "Accept"]
 
       # Whether to allow credentials (cookies, authorization headers)

src/ros2_medkit_gateway/include/ros2_medkit_gateway/config.hpp

@@ -29,6 +29,35 @@ struct CorsConfig {
   std::vector<std::string> allowed_headers;
   bool allow_credentials{false};
   int max_age_seconds{86400};
+
+  // Pre-built header values for performance
+  std::string methods_header;
+  std::string headers_header;
+};
+
+/**
+ * @brief Builder for CorsConfig with fluent interface
+ *
+ * Usage:
+ *   auto config = CorsConfigBuilder()
+ *       .with_origins({"http://localhost:5173"})
+ *       .with_methods({"GET", "PUT", "OPTIONS"})
+ *       .with_headers({"Content-Type", "Accept"})
+ *       .with_credentials(true)
+ *       .with_max_age(3600)
+ *       .build();
+ */
+class CorsConfigBuilder {
+ public:
+  CorsConfigBuilder & with_origins(std::vector<std::string> origins);
+  CorsConfigBuilder & with_methods(std::vector<std::string> methods);
+  CorsConfigBuilder & with_headers(std::vector<std::string> headers);
+  CorsConfigBuilder & with_credentials(bool credentials);
+  CorsConfigBuilder & with_max_age(int seconds);
+  CorsConfig && build();
+
+ private:
+  CorsConfig config_;
 };
 
 }  // namespace ros2_medkit_gateway

src/ros2_medkit_gateway/include/ros2_medkit_gateway/rest_server.hpp

@@ -30,7 +30,7 @@ class GatewayNode;
 
 class RESTServer {
  public:
-  RESTServer(GatewayNode * node, const std::string & host, int port, const CorsConfig & cors_config = {});
+  RESTServer(GatewayNode * node, const std::string & host, int port, const CorsConfig & cors_config);
   ~RESTServer();
 
   void start();
@@ -60,10 +60,6 @@ class RESTServer {
   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/config.cpp

@@ -0,0 +1,80 @@
+// Copyright 2025 bburda
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "ros2_medkit_gateway/config.hpp"
+
+#include <algorithm>
+#include <stdexcept>
+#include <utility>
+
+namespace ros2_medkit_gateway {
+
+CorsConfigBuilder & CorsConfigBuilder::with_origins(std::vector<std::string> origins) {
+  config_.allowed_origins = std::move(origins);
+  return *this;
+}
+
+CorsConfigBuilder & CorsConfigBuilder::with_methods(std::vector<std::string> methods) {
+  config_.allowed_methods = std::move(methods);
+  return *this;
+}
+
+CorsConfigBuilder & CorsConfigBuilder::with_headers(std::vector<std::string> headers) {
+  config_.allowed_headers = std::move(headers);
+  return *this;
+}
+
+CorsConfigBuilder & CorsConfigBuilder::with_credentials(bool credentials) {
+  config_.allow_credentials = credentials;
+  return *this;
+}
+
+CorsConfigBuilder & CorsConfigBuilder::with_max_age(int seconds) {
+  config_.max_age_seconds = seconds;
+  return *this;
+}
+
+CorsConfig && CorsConfigBuilder::build() {
+  // Enable CORS only if origins are configured
+  config_.enabled = !config_.allowed_origins.empty();
+
+  if (config_.enabled) {
+    // Validate: credentials cannot be used with wildcard origin
+    if (config_.allow_credentials) {
+      auto has_wildcard = std::find(config_.allowed_origins.begin(), config_.allowed_origins.end(), "*");
+      if (has_wildcard != config_.allowed_origins.end()) {
+        throw std::invalid_argument("CORS: allow_credentials cannot be true when allowed_origins contains '*'");
+      }
+    }
+
+    // Build cached header strings
+    for (const auto & method : config_.allowed_methods) {
+      if (!config_.methods_header.empty()) {
+        config_.methods_header += ", ";
+      }
+      config_.methods_header += method;
+    }
+
+    for (const auto & header : config_.allowed_headers) {
+      if (!config_.headers_header.empty()) {
+        config_.headers_header += ", ";
+      }
+      config_.headers_header += header;
+    }
+  }
+
+  return std::move(config_);
+}
+
+}  // namespace ros2_medkit_gateway

src/ros2_medkit_gateway/src/gateway_node.cpp

@@ -38,26 +38,15 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") {
   server_port_ = get_parameter("server.port").as_int();
   refresh_interval_ms_ = get_parameter("refresh_interval_ms").as_int();
 
-  // Get CORS configuration
-  cors_config_.allowed_origins = get_parameter("cors.allowed_origins").as_string_array();
-  cors_config_.allowed_methods = get_parameter("cors.allowed_methods").as_string_array();
-  cors_config_.allowed_headers = get_parameter("cors.allowed_headers").as_string_array();
-  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 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");
-      }
-    }
-  }
+  // Build CORS configuration using builder pattern
+  // Throws std::invalid_argument if configuration is invalid
+  cors_config_ = CorsConfigBuilder()
+                     .with_origins(get_parameter("cors.allowed_origins").as_string_array())
+                     .with_methods(get_parameter("cors.allowed_methods").as_string_array())
+                     .with_headers(get_parameter("cors.allowed_headers").as_string_array())
+                     .with_credentials(get_parameter("cors.allow_credentials").as_bool())
+                     .with_max_age(get_parameter("cors.max_age_seconds").as_int())
+                     .build();
 
   // Validate port range
   if (server_port_ < 1024 || server_port_ > 65535) {
@@ -95,9 +84,6 @@ GatewayNode::GatewayNode() : Node("ros2_medkit_gateway") {
       }
       origins_str += origin;
     }
-    if (origins_str.empty()) {
-      origins_str = "* (all)";
-    }
 
     std::string methods_str;
     for (const auto & method : cors_config_.allowed_methods) {

src/ros2_medkit_gateway/src/rest_server.cpp

@@ -31,33 +31,23 @@ 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) {
       std::string origin = req.get_header_value("Origin");
-      if (!origin.empty() && is_origin_allowed(origin)) {
+      bool origin_allowed = !origin.empty() && is_origin_allowed(origin);
+
+      if (origin_allowed) {
         set_cors_headers(res, origin);
       }
 
       // Handle preflight OPTIONS requests
+      // Only set Max-Age and return 204 for allowed origins
       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
+        if (origin_allowed) {
+          res.set_header("Access-Control-Max-Age", std::to_string(cors_config_.max_age_seconds));
+        }
+        res.status = StatusCode::NoContent_204;
         return httplib::Server::HandlerResponse::Handled;
       }
       return httplib::Server::HandlerResponse::Unhandled;
@@ -602,12 +592,12 @@ 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);
 
-  // Use pre-built header strings (built once in constructor)
-  if (!cors_methods_header_.empty()) {
-    res.set_header("Access-Control-Allow-Methods", cors_methods_header_);
+  // Use pre-built header strings from CorsConfig
+  if (!cors_config_.methods_header.empty()) {
+    res.set_header("Access-Control-Allow-Methods", cors_config_.methods_header);
   }
-  if (!cors_headers_header_.empty()) {
-    res.set_header("Access-Control-Allow-Headers", cors_headers_header_);
+  if (!cors_config_.headers_header.empty()) {
+    res.set_header("Access-Control-Allow-Headers", cors_config_.headers_header);
   }
 
   // Set credentials header if enabled
@@ -617,12 +607,10 @@ void RESTServer::set_cors_headers(httplib::Response & res, const std::string & o
 }
 
 bool RESTServer::is_origin_allowed(const std::string & origin) const {
-  // If no origins configured, allow all (for development)
-  if (cors_config_.allowed_origins.empty()) {
-    return true;
-  }
-
   // Check if origin matches any allowed origin
+  // Note: Wildcard "*" is allowed here but credentials+wildcard is blocked at startup
+  // (see gateway_node.cpp validation). When wildcard is used, we echo back the actual
+  // origin for security, as browsers require exact origin match with credentials.
   for (const auto & allowed : cors_config_.allowed_origins) {
     if (allowed == "*" || allowed == origin) {
       return true;