fix(core): add resource limits and harden production WASM safety#26
fix(core): add resource limits and harden production WASM safety#26
Conversation
- Add src/limits.rs with MAX_CONFIG_BYTES (100MB), MAX_CONTEXT_BYTES (1MB), MAX_REF_DEPTH (64), MAX_JSON_DEPTH (128) and check_json_depth() validator - Enforce config and context size limits at the WASM FFI boundary in update_state_internal and evaluate_internal before any memory reads - Add JSON nesting depth pre-parse scan in FlagEvaluator::update_state to prevent stack overflows; applied to both JSON and YAML (post-conversion) paths - Add $ref hop-depth counter to resolve_refs (depth counts only $ref hops, not structural JSON traversal) - Fix mutex.lock().unwrap() -> unwrap_or_else(|e| e.into_inner()) in the native non-WASM evaluator module to survive mutex poisoning gracefully - Eliminate double get_state() call and production .unwrap() in evaluate_internal by binding state once - Add adversarial tests: oversized config/context, deeply nested JSON, deep $ref chains, and circular $ref regression - Document all limits and enforcement points in ARCHITECTURE.md Closes #23 Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and security of the Rust core and WASM runtime by introducing comprehensive resource limits. These limits are designed to protect the evaluator from adversarial or accidentally oversized inputs, preventing potential memory exhaustion, stack overflows, and other stability issues. The changes ensure that the system fails gracefully and deterministically when limits are exceeded, providing a more resilient and predictable evaluation environment. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces important hardening measures by adding resource limits for configuration and context sizes, as well as nesting depth for JSON and $ref resolution. The changes are well-implemented and include comprehensive tests for the new limits. The handling of mutex poisoning and the removal of an .unwrap() call further improve the robustness of the evaluator. I've identified one area for improvement regarding code duplication.
src/lib.rs
Outdated
| if context_len as usize > MAX_CONTEXT_BYTES { | ||
| return EvaluationResult::error( | ||
| ErrorCode::ParseError, | ||
| format!( | ||
| "Context size ({} bytes) exceeds the maximum allowed size of {} bytes (1 MB)", | ||
| context_len, MAX_CONTEXT_BYTES | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The logic for parsing the context from memory, including this new size check, is duplicated in evaluate_internal (lines 628-636). This duplication extends to the surrounding memory reading and JSON parsing logic.
To improve maintainability and reduce the risk of future inconsistencies, consider extracting this shared logic into a private helper function. For example, a function with a signature like fn parse_context_from_memory(context_ptr: *const u8, context_len: u32) -> Result<Value, EvaluationResult> could encapsulate this behavior. Both evaluate_by_index_internal and evaluate_internal could then call this helper.
…plication Resolve the review comment on PR #26: the context-parsing logic (size check → UTF-8 read → JSON parse) was duplicated verbatim in both evaluate_by_index_internal and evaluate_internal. Extract it into a single private helper fn parse_context_from_memory(ptr, len) -> Result<Value, EvaluationResult> Both callers now delegate to this helper. evaluate_internal retains its extra short-circuit (flag has no targeting rules → skip parsing) as a plain if/else around the helper call. Signed-off-by: Simon Schrottner <simon.schrottner@aepfli.at> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…plication Resolve the review comment on PR #26: the context-parsing logic (size check → UTF-8 read → JSON parse) was duplicated verbatim in both evaluate_by_index_internal and evaluate_internal. Extract it into a single private helper fn parse_context_from_memory(ptr, len) -> Result<Value, EvaluationResult> Both callers now delegate to this helper. evaluate_internal retains its extra short-circuit (flag has no targeting rules → skip parsing) as a plain if/else around the helper call. Signed-off-by: Simon Schrottner <simon.schrottner@aepfli.at> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
fdb515b to
4b6412a
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
EvaluationResult's size (136 bytes) is inherent to the type; boxing it would complicate all call-sites for no real benefit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com>
Summary
Implements production hardening for the Rust core and WASM runtime as described in #23.
Changes
New:
src/limits.rsDefines resource limit constants with real-world quantification:
MAX_CONFIG_BYTESMAX_CONTEXT_BYTESMAX_REF_DEPTHMAX_JSON_DEPTHsrc/lib.rsupdate_state_internal: fires beforestring_from_memory— oversizedconfig_lenrejected without touching memoryevaluate_internal/evaluate_by_index_internalmutex.lock().unwrap()→unwrap_or_else(|e| e.into_inner()).unwrap(): doubleget_state()inevaluate_internalreplaced with single bindingsrc/evaluator.rsFlagEvaluator::update_state:check_json_depth()beforeserde_jsonrecursion; covers JSON and YAML (post-conversion) inputssrc/model/feature_flag.rsresolve_refs: counts $ref hops only; rejects chains longer than 64 hopsTests
tests/adversarial_tests.rs: deeply nested JSON, exact-limit JSON, deep/exact $ref chains, circular $ref regressionadversarial_wasm_testsmodule insrc/lib.rs: oversized config/contextARCHITECTURE.mdNew Resource Limits section documenting all constants, enforcement points, and error behavior.
Closes #23