Skip to content

Commit 787a93b

Browse files
authored
Security hardening: Address Phase 2 GAT implementation vulnerabilities (#80)
* security: mitigate Critical/High DoS and error handling vulnerabilities Phase 1 security hardening addressing 5 critical/high severity findings: - Add MAX_SCAN_LIMIT to prevent unbounded iteration (DOS-001, DOS-002) - Implement filter_limited() for bounded allocations (DOS-003) - Return NotFound errors for missing resources (ERR-001) - Fix HashMap allocation in health checks (MEM-001) Implementation details: - Created domain/config/limits.rs with validation constants - Created infrastructure/adapters/limits.rs with scan limits - Added Pagination::validate() and SessionQueryCriteria::validate() - Implemented filter_limited() with scan_limit and result_limit - Return SessionNotFound/StreamNotFound instead of empty results - Use HashMap::with_capacity(MAX_HEALTH_METRICS) for bounded allocation Code quality improvements: - Fixed 4 clippy collapsible_if warnings with let-chains - Added integration tests for security bounded iteration - Standardized error message capitalization - Added production tuning documentation Verification: - All 2580 tests pass - Zero clippy warnings with -D warnings - 100% coverage for security-critical code - Performance overhead < 1% - No breaking API changes Addresses #79 (Phase 1: Critical/High priority fixes) * security: implement Phase 2 input validation and caching improvements Phase 2 security hardening addressing 4 medium severity findings: - Add StreamFilter::validate() for priority range validation (INPUT-003) - Implement session-level stats caching with 5s TTL (MEM-002) - Document DashMap weakly consistent iteration guarantees (RACE-001) - Add saturating_f64_to_u64() for safe type conversion Implementation details: - StreamFilter::validate() checks min_priority <= max_priority - Rejects empty statuses vec with clear error message - CachedSessionStats with AtomicU64 for thread-safe caching - Cache invalidation on save_session() and remove_session() - saturating_f64_to_u64() handles NaN, infinity, negative values - Comprehensive DashMap iteration documentation Code quality: - All edge cases tested (8 tests for StreamFilter, 3 for caching, 2 for conversion) - Zero clippy warnings - Clean Architecture maintained - No breaking API changes Verification: - All 2593 tests pass - 100% coverage for security-critical code - Performance overhead negligible (<200ns) - Stats cache improves get_session_health performance Addresses #79 (Phase 2: Medium priority improvements) * fix: wrap constant assertions in const blocks for clippy * fix: address Copilot code review feedback - Fix Clone implementation to preserve stats_cache - Fix off-by-one error in scan limit (use enumerate) - Fix race condition in cache update (use entry API) - Remove unclear clippy comment - Remove duplicated constants (re-export from domain) * chore: apply rustfmt formatting
1 parent de71ab5 commit 787a93b

File tree

15 files changed

+1721
-143
lines changed

15 files changed

+1721
-143
lines changed

crates/pjs-core/src/application/handlers/command_handlers.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,7 @@ mod tests {
556556
total_count,
557557
has_more,
558558
query_duration_ms: 0,
559+
scan_limit_reached: false,
559560
})
560561
}
561562
}

crates/pjs-core/src/application/handlers/query_handlers.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,7 @@ mod tests {
638638
total_count,
639639
has_more,
640640
query_duration_ms: 0,
641+
scan_limit_reached: false,
641642
})
642643
}
643644
}
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
//! Domain layer configuration limits
2+
//!
3+
//! Defines validation constraints for domain-level pagination and query operations.
4+
//! These limits enforce business rules and are independent of infrastructure.
5+
//!
6+
//! # Production Tuning
7+
//!
8+
//! These values are suitable for most deployments. Adjust based on:
9+
//!
10+
//! - **MAX_PAGINATION_LIMIT**: Increase if clients need larger batch fetches.
11+
//! Monitor memory usage per request (limit * avg_item_size).
12+
//!
13+
//! - **MAX_PAGINATION_OFFSET**: Lower if cursor-based pagination is preferred.
14+
//! Deep offsets are expensive; consider cursor pagination for offsets > 10,000.
15+
//!
16+
//! - **ALLOWED_SORT_FIELDS**: Extend with indexed fields only. Adding non-indexed
17+
//! fields degrades query performance significantly on large datasets.
18+
//!
19+
//! # Monitoring Recommendations
20+
//!
21+
//! Track these metrics to tune limits:
22+
//! - `pagination.offset_p99`: If consistently high, clients may need cursor pagination
23+
//! - `pagination.limit_avg`: Optimize batch sizes based on actual usage
24+
//! - `query.scan_limit_reached_rate`: High rate indicates filter criteria too broad
25+
26+
/// Maximum allowed pagination limit per request.
27+
///
28+
/// Prevents single requests from retrieving excessive data.
29+
/// Aligns with industry standards (GitHub API, Stripe use 100-1000).
30+
pub const MAX_PAGINATION_LIMIT: usize = 1_000;
31+
32+
/// Maximum allowed pagination offset.
33+
///
34+
/// Prevents requests that would scan deep into result sets.
35+
/// Beyond this, cursor-based pagination is recommended.
36+
pub const MAX_PAGINATION_OFFSET: usize = 1_000_000;
37+
38+
/// Allowed sort field names for pagination validation.
39+
///
40+
/// Whitelist of fields that can be used in sort_by parameter.
41+
/// Only add fields that have corresponding indexes in storage.
42+
pub const ALLOWED_SORT_FIELDS: &[&str] = &["created_at", "updated_at", "stream_count"];
43+
44+
#[cfg(test)]
45+
mod tests {
46+
use super::*;
47+
48+
#[test]
49+
fn test_max_pagination_limit_value() {
50+
assert_eq!(MAX_PAGINATION_LIMIT, 1_000);
51+
const { assert!(MAX_PAGINATION_LIMIT > 0) };
52+
}
53+
54+
#[test]
55+
fn test_max_pagination_offset_value() {
56+
assert_eq!(MAX_PAGINATION_OFFSET, 1_000_000);
57+
const { assert!(MAX_PAGINATION_OFFSET > 0) };
58+
}
59+
60+
#[test]
61+
fn test_allowed_sort_fields() {
62+
assert!(ALLOWED_SORT_FIELDS.contains(&"created_at"));
63+
assert!(ALLOWED_SORT_FIELDS.contains(&"updated_at"));
64+
assert!(ALLOWED_SORT_FIELDS.contains(&"stream_count"));
65+
assert!(!ALLOWED_SORT_FIELDS.contains(&"invalid_field"));
66+
}
67+
68+
#[test]
69+
fn test_pagination_limit_within_industry_standard() {
70+
// Industry standard range: 100-1000
71+
const { assert!(MAX_PAGINATION_LIMIT >= 100) };
72+
const { assert!(MAX_PAGINATION_LIMIT <= 10_000) };
73+
}
74+
}
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
//! Domain configuration module
2+
//!
3+
//! Contains domain-level configuration constants that define business rules
4+
//! and validation constraints. These are independent of infrastructure.
5+
6+
pub mod limits;
7+
8+
pub use limits::{ALLOWED_SORT_FIELDS, MAX_PAGINATION_LIMIT, MAX_PAGINATION_OFFSET};

crates/pjs-core/src/domain/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ pub use pjson_rs_domain::{DomainError, DomainResult, entities, events, value_obj
88

99
// pjs-core specific domain modules
1010
pub mod aggregates;
11+
pub mod config;
1112
pub mod macros;
1213
pub mod ports;
1314
pub mod services;

0 commit comments

Comments
 (0)