Repository navigation
Mkulakow/improve kfs request validation - #4640
Open
michalkulakowski wants to merge 3 commits into
Open
michalkulakowski wants to merge 3 commits into
michalkulakowski wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Numeric status compatibility, ambiguous parser overloads, and test configuration cleanup need correction.
Review effort: Balanced
Findings: 4
Open (4)
What changed in this PR
Strengthens OVMS REST request validation by limiting JSON complexity before building the document.
Changes:
- Adds a configurable JSON event limit and HTTP 413 response.
- Parses length-bounded request buffers without copying JSON prefixes.
- Adds configuration and parser tests, plus security documentation.
| File | Description |
|---|---|
| src/utils/rapidjson_utils.hpp | Declares complexity limits and bounded parsing. |
| src/utils/rapidjson_utils.cpp | Enforces complexity during the pre-scan. |
| src/test/ovmsconfig_test.cpp | Tests complexity-limit configuration. |
| src/test/kfs_rest_test.cpp | Tests KServe complexity rejection. |
| src/test/kfs_rest_parser_test.cpp | Tests limits and JSON-prefix parsing. |
| src/test/http_openai_handler_test.cpp | Tests OpenAI endpoint complexity rejection. |
| src/status.hpp | Adds the complexity-exceeded status. |
| src/status.cpp | Defines its error message. |
| src/rest_parser.hpp | Declares length-aware KServe parsing. |
| src/rest_parser.cpp | Applies the configured complexity limit. |
| src/http_server.cpp | Maps complexity rejection to HTTP 413. |
| src/http_rest_api_handler.cpp | Integrates bounded parsing and complexity handling. |
| src/config.hpp | Declares the configuration accessor. |
| src/config.cpp | Validates and supplies the limit. |
| src/cli_parser.cpp | Adds --json_max_complexity. |
| src/capi_frontend/server_settings.hpp | Stores the optional limit. |
| src/BUILD | Adds the parser utility dependency. |
| docs/security_considerations.md | Describes JSON parsing protection. |
| docs/parameters.md | Documents the new option. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| MODEL_NOT_LOADED, | ||
| JSON_INVALID, /*!< The file/content is not valid json */ | ||
| JSON_NESTING_DEPTH_EXCEEDED, /*!< JSON nesting depth exceeds the allowed limit */ | ||
| JSON_COMPLEXITY_EXCEEDED, /*!< JSON structure exceeds the allowed complexity */ |
Comment on lines
+376
to
+379
| ASSERT_EQ(status, ovms::StatusCode::JSON_COMPLEXITY_EXCEEDED); | ||
| ASSERT_EQ(status.string(), "JSON structure exceeds the allowed complexity - JSON body exceeds maximum complexity"); | ||
|
|
||
| config.jsonMaxComplexity = previousMaxComplexity; |
Comment on lines
+1477
to
+1480
| ASSERT_EQ(status.getCode(), ovms::StatusCode::JSON_COMPLEXITY_EXCEEDED); | ||
| ASSERT_EQ(status.string(), "JSON structure exceeds the allowed complexity"); | ||
|
|
||
| config.jsonMaxComplexity = previousMaxComplexity; |
| const char* json, | ||
| std::size_t jsonLength, | ||
| std::size_t maxDepth, | ||
| std::size_t maxComplexity = DEFAULT_MAX_JSON_COMPLEXITY); |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🛠 Summary
CVS-195700
🧪 Checklist
``