chore(dash-spv-ffi): test_security.rs enabled #376
Conversation
📝 WalkthroughWalkthroughThe security test suite has been reorganized by moving tests from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🤖 Fix all issues with AI agents
In `@dash-spv-ffi/tests/test_security.rs`:
- Around line 173-184: The test iterates dangerous_paths and only prints results
from dash_spv_ffi_config_set_data_dir without asserting traversal prevention;
update the test (still inside the loop over dangerous_paths) to assert expected
behavior: either assert the return value from dash_spv_ffi_config_set_data_dir
indicates an error for traversal attempts, or after a successful call read back
the stored path (via the corresponding getter) and assert it has been
sanitized/normalized (no ".." or absolute escapes); reference the existing
symbols dangerous_paths, safe_path/CString::new(c_path), config and
dash_spv_ffi_config_set_data_dir to locate the code to modify and add assertions
or rename the test to test_path_traversal_inputs if you intend to keep it
exploratory.
- Around line 43-87: The test_null_pointer_dereferencing uses
Box::into_raw(Box::new(array)) which allocates FFIArray on the heap and is never
freed because dash_spv_ffi_array_destroy only frees the inner data buffer;
replace the heap allocation with a stack-allocated mutable variable (let mut
array = FFIArray { ... }) and call dash_spv_ffi_array_destroy(&mut array as *mut
FFIArray) so the struct is not leaked while preserving the existing null data
pointer behavior.
🧹 Nitpick comments (1)
dash-spv-ffi/tests/test_security.rs (1)
100-111: Consider testing non-zeroelem_sizefor fuller overflow coverage.With
elem_size: 0, size calculations likelen * elem_sizewill always yield 0, which doesn't exercise potential integer overflow in allocation math. To test overflow protection more thoroughly, consider using a small non-zeroelem_sizethat would cause overflow when multiplied byhuge_size.♻️ Optional: Test with non-zero elem_size
let huge_size = usize::MAX / 2; // Avoid actual overflow let huge_array = FFIArray { data: ptr::null_mut(), len: huge_size, capacity: huge_size, - elem_size: 0, + elem_size: 4, // Would overflow: huge_size * 4 > usize::MAX elem_align: 0, };
| #[test] | ||
| #[serial] | ||
| fn test_null_pointer_dereferencing() { | ||
| unsafe { | ||
| // Test all functions with null pointers | ||
|
|
||
| // Config functions | ||
| assert_eq!( | ||
| dash_spv_ffi_config_set_data_dir(ptr::null_mut(), ptr::null()), | ||
| FFIErrorCode::NullPointer as i32 | ||
| ); | ||
| assert_eq!( | ||
| dash_spv_ffi_config_set_validation_mode(ptr::null_mut(), FFIValidationMode::Basic), | ||
| FFIErrorCode::NullPointer as i32 | ||
| ); | ||
| assert_eq!( | ||
| dash_spv_ffi_config_add_peer(ptr::null_mut(), ptr::null()), | ||
| FFIErrorCode::NullPointer as i32 | ||
| ); | ||
|
|
||
| // Client functions | ||
| assert!(dash_spv_ffi_client_new(ptr::null()).is_null()); | ||
| assert_eq!( | ||
| dash_spv_ffi_client_start(ptr::null_mut()), | ||
| FFIErrorCode::NullPointer as i32 | ||
| ); | ||
| assert!(dash_spv_ffi_client_get_sync_progress(ptr::null_mut()).is_null()); | ||
|
|
||
| // Destruction functions should handle null gracefully | ||
| dash_spv_ffi_client_destroy(ptr::null_mut()); | ||
| dash_spv_ffi_config_destroy(ptr::null_mut()); | ||
| dash_spv_ffi_string_destroy(FFIString { | ||
| ptr: ptr::null_mut(), | ||
| length: 0, | ||
| }); | ||
| let array = FFIArray { | ||
| data: ptr::null_mut(), | ||
| len: 0, | ||
| capacity: 0, | ||
| elem_size: 0, | ||
| elem_align: 0, | ||
| }; | ||
| dash_spv_ffi_array_destroy(Box::into_raw(Box::new(array))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the dash_spv_ffi_array_destroy implementation
ast-grep --pattern $'pub unsafe extern "C" fn dash_spv_ffi_array_destroy($$$) {
$$$
}'Repository: dashpay/rust-dashcore
Length of output: 1458
🏁 Script executed:
#!/bin/bash
# Check test file location and structure
ls -la dash-spv-ffi/tests/
# Also check for existing test patterns in unit tests
find dash-spv-ffi/tests -name "*.rs" -type f | head -20Repository: dashpay/rust-dashcore
Length of output: 2070
🏁 Script executed:
#!/bin/bash
# Check the memory management test file for reference patterns
cat -n dash-spv-ffi/tests/unit/test_memory_management.rsRepository: dashpay/rust-dashcore
Length of output: 18509
🏁 Script executed:
#!/bin/bash
# Read the exact test_null_pointer_dereferencing test from test_security.rs
sed -n '43,87p' dash-spv-ffi/tests/test_security.rsRepository: dashpay/rust-dashcore
Length of output: 1701
Fix memory leak in test_null_pointer_dereferencing—use stack allocation for FFIArray.
Lines 78-85 use Box::into_raw(Box::new(array)), which allocates the FFIArray struct on the heap. The dash_spv_ffi_array_destroy function only deallocates the data buffer (when non-null), not the struct itself. This leaves the Box allocation unreleased, causing a memory leak.
Replace with the established pattern used throughout the test suite:
let mut array = FFIArray {
data: ptr::null_mut(),
len: 0,
capacity: 0,
elem_size: 0,
elem_align: 0,
};
dash_spv_ffi_array_destroy(&mut array as *mut FFIArray);This uses stack allocation, where the struct lifetime is managed by the scope and no heap allocation leaks.
🤖 Prompt for AI Agents
In `@dash-spv-ffi/tests/test_security.rs` around lines 43 - 87, The
test_null_pointer_dereferencing uses Box::into_raw(Box::new(array)) which
allocates FFIArray on the heap and is never freed because
dash_spv_ffi_array_destroy only frees the inner data buffer; replace the heap
allocation with a stack-allocated mutable variable (let mut array = FFIArray {
... }) and call dash_spv_ffi_array_destroy(&mut array as *mut FFIArray) so the
struct is not leaked while preserving the existing null data pointer behavior.
| for path in dangerous_paths { | ||
| // Remove null bytes for CString | ||
| let safe_path = path.replace('\0', ""); | ||
| let c_path = CString::new(safe_path).unwrap(); | ||
|
|
||
| // Should accept the path (validation is up to the implementation) | ||
| // but should not allow actual traversal | ||
| let result = dash_spv_ffi_config_set_data_dir(config, c_path.as_ptr()); | ||
|
|
||
| // The implementation should sanitize or validate paths | ||
| println!("Path '{}' result: {}", path, result); | ||
| } |
There was a problem hiding this comment.
Test doesn't verify path traversal prevention.
The test name suggests it validates prevention, but the code only calls set_data_dir and prints the result without asserting that dangerous paths are rejected or sanitized. Consider either:
- Asserting that dangerous paths return an error code, or
- Verifying the actual stored path is sanitized, or
- Renaming to
test_path_traversal_inputsto reflect that this is exploratory.
🔧 Suggested assertion pattern
let result = dash_spv_ffi_config_set_data_dir(config, c_path.as_ptr());
- // The implementation should sanitize or validate paths
- println!("Path '{}' result: {}", path, result);
+ // Verify dangerous paths are rejected or handled safely
+ // Adjust assertion based on actual implementation behavior
+ if path.contains("..") || path.starts_with('/') {
+ assert_ne!(
+ result,
+ FFIErrorCode::Success as i32,
+ "Dangerous path '{}' should be rejected",
+ path
+ );
+ }🤖 Prompt for AI Agents
In `@dash-spv-ffi/tests/test_security.rs` around lines 173 - 184, The test
iterates dangerous_paths and only prints results from
dash_spv_ffi_config_set_data_dir without asserting traversal prevention; update
the test (still inside the loop over dangerous_paths) to assert expected
behavior: either assert the return value from dash_spv_ffi_config_set_data_dir
indicates an error for traversal attempts, or after a successful call read back
the stored path (via the corresponding getter) and assert it has been
sanitized/normalized (no ".." or absolute escapes); reference the existing
symbols dangerous_paths, safe_path/CString::new(c_path), config and
dash_spv_ffi_config_set_data_dir to locate the code to modify and add assertions
or rename the test to test_path_traversal_inputs if you intend to keep it
exploratory.
|
Skills issues here |
There are some test that are not being executed bcs they are not being detected. In general, the dash-spv-ffi test suite has a bad design.
This first step into imprving that moved one of the ignored files into scope to be executed. Bcs it was being ignored nobody detected refactor or changes in API
testesdby this tests so I had to remove those that were no longer valid since I dont know how the refactors intend the calls to be done now if they are possible to be done after all. If this is approved I am planning to apply the same strategy to implement the other tests currently out of scopeSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.