[#43] Add CORS Configuration #46
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bburda
force-pushed
the
feat/cors-setup
branch
2 times, most recently
from
7a63652 to
0d3def2
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds CORS (Cross-Origin Resource Sharing) configuration to the ROS 2 Medkit Gateway REST server to enable browser-based clients to interact with the API. The implementation introduces a new CorsConfig struct with YAML-based configuration and implements CORS handling via a pre-routing handler that sets appropriate headers and handles OPTIONS preflight requests.
Key Changes:
- New
CorsConfigstruct with fields for allowed origins, methods, headers, credentials, and max age - Pre-routing handler in REST server that sets CORS headers and handles OPTIONS preflight requests
- YAML configuration with documented defaults for CORS settings
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/config.hpp | New file defining the CorsConfig struct with CORS configuration fields |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp | Added CorsConfig member and include for config.hpp |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/rest_server.hpp | Updated constructor to accept CorsConfig and added CORS helper methods |
| src/ros2_medkit_gateway/src/gateway_node.cpp | Added CORS parameter declarations, configuration parsing, enabled logic, and logging |
| src/ros2_medkit_gateway/src/rest_server.cpp | Implemented CORS pre-routing handler, set_cors_headers(), and is_origin_allowed() methods |
| src/ros2_medkit_gateway/config/gateway_params.yaml | Added documented CORS configuration section with examples and defaults |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add CORS configuration via ROS 2 parameters to allow browser-based clients (web UIs) to call the Gateway REST API without code changes. Configuration options (in gateway_params.yaml): - cors.allowed_origins: list of allowed origins (empty = allow all) - cors.allowed_methods: HTTP methods to allow - cors.allowed_headers: headers to allow in requests - cors.allow_credentials: enable credentials support - cors.max_age_seconds: preflight cache duration CORS is disabled when no configuration is provided. Handles preflight OPTIONS requests with 204 No Content response.
- 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
bburda
force-pushed
the
feat/cors-setup
branch
from
0d3def2 to
b0367c8
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/rest_server.hpp
Outdated
Show resolved
Hide resolved
- 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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mfaferek93
reviewed
Nov 30, 2025
- Change OPTIONS preflight to return 403 Forbidden for disallowed origins instead of 204 No Content without CORS headers - Prevents endpoint discovery by unauthorized origins - Clarify README security note about startup failure for invalid config - Add explicit wildcard check in CorsConfigBuilder validation
mfaferek93
approved these changes
Nov 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Add CORS configuration to REST Server with YAML setup
Issue
Type
Testing
Checklist