feat(rpc): implement duplicate key detection for json-rpc requests #6455
Conversation
Dyslex7c
requested review from
hanabi1224 and
sudo-shashank
and removed request for
a team
|
|
WalkthroughAdds strict duplicate-key JSON validation for RPC params via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPCServer as RPC Server
participant ValidationLayer as JsonValidationLayer
participant JSONValidator as JSON Validator
participant RPCService as Inner RPC Service
Client->>RPCServer: Send JSON-RPC request
RPCServer->>ValidationLayer: Forward request
alt Validation enabled (FOREST_STRICT_JSON)
ValidationLayer->>JSONValidator: validate_json_for_duplicates(params)
JSONValidator->>JSONValidator: Parse & traverse JSON to detect duplicate keys
JSONValidator-->>ValidationLayer: Err(Duplicate key) / Ok
alt Err
ValidationLayer->>RPCServer: Return -32600 ErrorObject
RPCServer-->>Client: Error response
else Ok
ValidationLayer->>RPCService: Forward request
RPCService-->>ValidationLayer: Response
ValidationLayer-->>RPCServer: Forward response
RPCServer-->>Client: Success response
end
else Validation disabled
ValidationLayer->>RPCService: Forward request
RPCService-->>ValidationLayer: Response
ValidationLayer-->>RPCServer: Forward response
RPCServer-->>Client: Success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/rpc/json_validator.rs`:
- Around line 125-135: The helper with_strict_mode mutates the global env var
STRICT_JSON_ENV unsafely and doesn't restore it on panic, causing flaky tests;
fix by serializing access with a static Mutex and using a panic-safe Drop guard
that captures the previous env state and restores or removes STRICT_JSON_ENV in
Drop, run the provided closure while the guard holds the lock, and return the
closure's result (change F: FnOnce() -> R and with_strict_mode -> fn
with_strict_mode<F, R>(enabled: bool, f: F) -> R) so the env is always restored
even if the closure panics; alternatively, if you prefer test-level
serialization instead, add #[serial] to every test that calls with_strict_mode.
- Around line 36-115: Update validate_json_for_duplicates to track whether each
open frame is an object or array (replace object_stack: Vec<HashSet<String>>
with a stack like Vec<Frame> where Frame holds a kind enum {Object,Array} and
for Object a HashSet<String> of keys) so the comma handler only sets
expecting_key = true when the top frame is an Object; fully decode JSON string
escapes (e.g., handle \\, \", \uXXXX, etc.) into normalized keys before
inserting/checking the set to avoid treating equivalent escapes as different;
change the function signature to return anyhow::Result<()> and use .context(...)
on error paths; and add a doc comment describing behavior and the
STRICT_JSON_ENV note, referencing validate_json_for_duplicates, the object/array
stack Frame, and the key normalization step for locating the changes.
In `@src/rpc/validation_layer.rs`:
- Around line 67-79: Batch and notification handlers currently delegate to
self.service.batch and self.service.notification without invoking
json_validator::validate_json_for_duplicates, so duplicate-key validation is
skipped; update ValidationLayer::batch to iterate over each item in the incoming
Batch<'a>, extract each item's params and call
json_validator::validate_json_for_duplicates (respecting the same strict-mode
check used in call()), rejecting or returning an error for any invalid item
before delegating to self.service.batch, and update
ValidationLayer::notification to validate the notification params with
json_validator::validate_json_for_duplicates (again honoring strict mode) before
calling self.service.notification; mirror the same error handling/response
format used in call() for consistency.
In `@tests/rpc_duplicate_keys_test.rs`:
- Around line 8-57: Both tests mutate the global STRICT_JSON_ENV without
synchronization which can cause races; add the attribute #[serial_test::serial]
above test_issue_duplicate_cids and test_correct_format_passes to serialize
them, or refactor each to use the existing helper pattern (with_strict_mode())
used in src/rpc/json_validator.rs so they call with_strict_mode(|| { ...
validate_json_for_duplicates(...) ... }) and avoid unsafe env manipulation;
ensure references to STRICT_JSON_ENV and validate_json_for_duplicates remain
correct and remove the unsafe set/remove_var calls if using the helper.
🧹 Nitpick comments (3)
src/rpc/json_validator.rs (1)
14-34: Add doc comment foris_strict_mode(public API).Public functions should be documented for clarity and API hygiene.
✍️ Suggested doc comment
+/// Returns true when strict JSON duplicate-key validation is enabled via `FOREST_STRICT_JSON`. pub fn is_strict_mode() -> bool {As per coding guidelines, please document public functions.
src/rpc/validation_layer.rs (1)
24-27: Add doc comment forValidation<S>wrapper.This is a public struct within the module; a brief doc helps clarify its role.
✍️ Suggested doc comment
+/// RPC service wrapper that applies duplicate-key validation in strict mode. pub(super) struct Validation<S> { service: S, }As per coding guidelines, please document public structs.
src/lib.rs (1)
67-67: Consider narrower public exposure if onlyjson_validatoris needed.Making the entire
rpcmodule public expands the public API surface substantially. If the goal is just to expose the validator for tests or external users, consider re-exporting only that module (and adjusting tests accordingly) to keep the public surface minimal.🔧 Possible alternative (if acceptable)
- pub mod rpc; + mod rpc; + pub use crate::rpc::json_validator;
hanabi1224
left a comment
hanabi1224
left a comment
There was a problem hiding this comment.
@Dyslex7c thanks for your contribution! Left a few comments.
|
@hanabi1224 I've addressed all the comments and made changes accordingly. Let me know if anything |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/docs/users/reference/env_variables.md`:
- Around line 57-58: The table row for `FOREST_STRICT_JSON` is missing the
trailing pipe and there's no blank line before the next heading
`FOREST_F3_SIDECAR_FFI_BUILD_OPT_OUT`; fix by adding the closing pipe to the
`FOREST_STRICT_JSON` row and insert a blank line after the table so the markdown
heading is separated from the table.
♻️ Duplicate comments (2)
src/rpc/json_validator.rs (1)
63-74: Serialize env-var mutations and restore previous state in tests.
with_strict_modemutates a global env var unsafely without serialization and doesn’t restore any prior value. Parallel tests can race and leak state on panic.🧪 Panic-safe guard example
+ use std::sync::{LazyLock, Mutex}; + + static ENV_LOCK: LazyLock<Mutex<()>> = LazyLock::new(|| Mutex::new(())); + + struct StrictEnvGuard { + _lock: std::sync::MutexGuard<'static, ()>, + prev: Option<std::ffi::OsString>, + } + + impl StrictEnvGuard { + fn new(enabled: bool) -> Self { + let lock = ENV_LOCK.lock().unwrap(); + let prev = std::env::var_os(STRICT_JSON_ENV); + unsafe { + if enabled { + std::env::set_var(STRICT_JSON_ENV, "1"); + } else { + std::env::remove_var(STRICT_JSON_ENV); + } + } + Self { _lock: lock, prev } + } + } + + impl Drop for StrictEnvGuard { + fn drop(&mut self) { + unsafe { + match &self.prev { + Some(v) => std::env::set_var(STRICT_JSON_ENV, v), + None => std::env::remove_var(STRICT_JSON_ENV), + } + } + } + } + fn with_strict_mode<F>(enabled: bool, f: F) where F: FnOnce(), { - if enabled { - unsafe { std::env::set_var(STRICT_JSON_ENV, "1"); } - } else { - unsafe { std::env::remove_var(STRICT_JSON_ENV); } - } - f(); - unsafe { std::env::remove_var(STRICT_JSON_ENV); } + let _guard = StrictEnvGuard::new(enabled); + f(); }Based on learnings,
set_var/remove_varare unsafe in Rust 2024 and should be serialized and panic-safe.src/rpc/validation_layer.rs (1)
82-101: Invalid notifications in batch still execute despite strict mode.In
batch, duplicate-key errors forBatchEntry::Notificationare ignored, so malformed notifications still reach the inner service. This is inconsistent withnotification()(which blocks invalid params) and weakens strict mode for batched notifications.🧩 Possible fix (align with notification path)
match batch_entry { jsonrpsee::core::middleware::BatchEntry::Call(req) => { let err = ErrorObject::owned(-32600, e, None::<()>); let err_entry = jsonrpsee::core::middleware::BatchEntryErr::new( req.id().clone(), err, ); *entry = Err(err_entry); } - jsonrpsee::core::middleware::BatchEntry::Notification(_) => {} + jsonrpsee::core::middleware::BatchEntry::Notification(_) => { + let err = ErrorObject::owned(-32600, e, None::<()>); + let err_entry = jsonrpsee::core::middleware::BatchEntryErr::new( + jsonrpsee::types::Id::Null, + err, + ); + *entry = Err(err_entry); + } }Please verify jsonrpsee’s expected behavior for batch notifications (whether
Id::Nullerrors are acceptable, or if invalid notifications should be dropped instead).jsonrpsee batch notification validation: can BatchEntryErr use Id::Null for notifications?
🧹 Nitpick comments (2)
src/rpc/json_validator.rs (2)
12-17: Add a doc comment foris_strict_mode.Public functions should be documented for API clarity.
📝 Suggested fix
+/// Returns true when strict JSON validation is enabled via `FOREST_STRICT_JSON`. #[inline] pub fn is_strict_mode() -> bool { crate::utils::misc::env::is_env_truthy(STRICT_JSON_ENV) }As per coding guidelines, public functions should have doc comments.
19-56: Align error handling withanyhow::Result+ context.The public API currently returns
Result<(), String>, which diverges from the project’s error-handling guidelines and loses structured context. Consider switching toanyhow::Result<()>and using.context(...).♻️ Proposed refactor
-pub fn validate_json_for_duplicates(json_str: &str) -> Result<(), String> { +pub fn validate_json_for_duplicates(json_str: &str) -> anyhow::Result<()> { if !is_strict_mode() { return Ok(()); } - fn check_value(value: &Value) -> Result<(), String> { + fn check_value(value: &Value) -> anyhow::Result<()> { match value { Value::Object(obj) => { let mut seen = HashSet::default(); for entry in obj.iter() { - let key = entry.key.as_str().ok_or_else(|| { - "Invalid JSON key".to_string() - })?; + let key = entry + .key + .as_str() + .context("Invalid JSON key")?; if !seen.insert(key) { - return Err(format!( + anyhow::bail!( "duplicate key '{}' in JSON object - this likely indicates malformed input. \ Set {}=0 to disable this check", key, STRICT_JSON_ENV - )); + ); } check_value(&entry.value)?; } Ok(()) @@ - let value = Value::from_json(json_str) - .map_err(|e| format!("Invalid JSON: {}", e))?; + let value = Value::from_json(json_str).context("Invalid JSON")?; check_value(&value) }You’ll need to map
anyhow::Errortoerr.to_string()at call sites (e.g., when buildingErrorObject). As per coding guidelines, useanyhow::Resultand.context()for fallible operations.
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
hanabi1224
left a comment
hanabi1224
left a comment
There was a problem hiding this comment.
Please fix CI lint errors in https://github.com/ChainSafe/forest/actions/runs/21254497614/job/61187840642?pr=6455
The doc deployment CI error can be ignored.
|
@Dyslex7c all commits must have verified signatures, also please check if all review comments are addressed and resolved. |
Dyslex7c
force-pushed
the
rpc/strict-map-parsing
branch
from
d78bf4a to
472aedb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/rpc/json_validator.rs`:
- Around line 21-31: Add a rustdoc comment for the public function
is_strict_mode explaining what it returns and its behavior in tests vs runtime
(mention that in tests it checks the environment variable STRICT_JSON_ENV via
crate::utils::misc::env::is_env_truthy and in non-test builds it reads the
static *STRICT_MODE). Place the doc comment immediately above pub fn
is_strict_mode() and keep it concise (one-line summary plus a short paragraph
noting the difference between #[cfg(test)] and #[cfg(not(test)]) behavior and
any side-effects or expected values).
- Around line 33-75: Change validate_json_for_duplicates to return
anyhow::Result<()> and propagate anyhow::Error (use anyhow::Error from any
context), then update all callers in validation_layer.rs: change the
validate_params wrapper signature to return anyhow::Result<()>, and where errors
are passed into ErrorObject::owned(...) convert them to strings via
e.to_string(); finally update tests to compare error text by calling
.unwrap_err().to_string().contains(...) instead of .contains(...). Reference
symbols: validate_json_for_duplicates, validate_params, ErrorObject::owned and
any test assertions that call .unwrap_err().contains(...).
In `@src/rpc/validation_layer.rs`:
- Around line 84-103: When validate_params fails for a notification, do not
convert the notification into an error response—drop it silently (optionally
log) instead of creating a BatchEntryErr; specifically in the batch loop
handling BatchEntry::Notification, skip setting *entry = Err(...) and just
continue so the notification produces no response; similarly, in the standalone
notification path stop returning
MethodResponse::error(jsonrpsee::types::Id::Null, ...) and return no response
(or early-return/None) so notifications never emit an error per JSON-RPC 2.0;
keep validate_params, BatchEntry, BatchEntryErr, BatchEntry::Notification and
MethodResponse::error as the reference points to change.
Dyslex7c
force-pushed
the
rpc/strict-map-parsing
branch
from
472aedb to
c2d8ac7
Compare
All review comments have been addressed and resolved, let me know if anything |
|
@Dyslex7c All comments must be resolved. |
okay done |
4 participants
Summary of changes
Changes introduced in this pull request:
Due to lack of available disk space in my device, I'm unable to run the node (with snapshots downloaded) and perform an E2E test to validate its working. But I have added integration tests and made sure that the core feature is working. Let me know if anything.
Performance constraints are preserved since it's an opt-in feature. There is zero performance overhead when it is disabled and a single-pass O(n) validation when enabled.
Reference issue to close (if applicable)
Closes #6424
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.