Conversation
aikido_zen/vulnerabilities/attack_wave_detection/attack_wave_detector.py
Outdated
Show resolved
Hide resolved
aikido_zen/background_process/cloud_connection_manager/on_detected_attack_wave.py
Outdated
Show resolved
Hide resolved
aikido_zen/vulnerabilities/attack_wave_detection/is_web_scan_method.py
Outdated
Show resolved
Hide resolved
aikido_zen/vulnerabilities/attack_wave_detection/query_params_contain_dangerous_strings.py
Outdated
Show resolved
Hide resolved
aikido_zen/vulnerabilities/attack_wave_detection/is_web_scan_path.py
Outdated
Show resolved
Hide resolved
aikido_zen/background_process/cloud_connection_manager/__init__.py
Outdated
Show resolved
Hide resolved
aikido_zen/vulnerabilities/attack_wave_detection/query_params_contain_dangerous_strings.py
Show resolved
Hide resolved
aikido_zen/vulnerabilities/attack_wave_detection/attack_wave_detector.py
Show resolved
Hide resolved
aikido_zen/background_process/cloud_connection_manager/__init__.py
Outdated
Show resolved
Hide resolved
| self.attacks_detected = 0 | ||
| self.attacks_blocked = 0 | ||
|
|
||
| self.attack_waves_detected = 0 |
There was a problem hiding this comment.
Statistics duplicates attack counter logic for attack_waves (attack_waves_detected/blocked and related clear/get_record/empty/on_detected_attack_wave).
Details
🔧 How do I fix it?
Delete extra code. Extract repeated code sequences into reusable functions or methods. Use loops or data structures to eliminate repetitive patterns.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| def on_rate_limit(self): | ||
| self.rate_limited_hits += 1 | ||
|
|
||
| def on_detected_attack_wave(self, blocked: bool): |
There was a problem hiding this comment.
Method on_detected_attack_wave mutates shared counters (attack_waves_detected/attack_waves_blocked) without synchronization
Details
✨ AI Reasoning
1) The Statistics class stores counters in instance attributes and exposes a new method on_detected_attack_wave that increments those counters.
2) If a Statistics instance is shared between threads (likely for global stats), concurrent calls to on_detected_attack_wave can cause race conditions because increments are non-atomic and there is no synchronization.
3) This harms maintainability and can produce incorrect counts under concurrency.
4) The issue is actionable within this PR (add locking or use atomic operations).
5) Other methods already increment counters without locks, but this change introduced new shared mutable state and a new mutating method.
6) Fixing is feasible within PR scope (add a lock or use thread-safe primitives).
🔧 How do I fix it?
Use locks, concurrent collections, or atomic operations when accessing shared mutable state. Avoid modifying collections during iteration. Use proper synchronization primitives like mutex, lock, or thread-safe data structures.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
aikido_zen/vulnerabilities/attack_wave_detection/query_params_contain_dangerous_strings.py
Show resolved
Hide resolved
| return self._detector | ||
|
|
||
|
|
||
| attack_wave_detector_store = AttackWaveDetectorStore() |
There was a problem hiding this comment.
Module-level variable 'attack_wave_detector_store' holds an AttackWaveDetector instance that caches per-IP request state across requests
Details
✨ AI Reasoning
1) The module aikido_zen/storage/attack_wave_detector_store.py creates a module-level instance attack_wave_detector_store that wraps an AttackWaveDetector; this detector holds LRUCache instances which store per-IP suspicious request counts and sent-event timestamps.
2) Persisting that mutable detector at module scope will cache request-specific (per-IP) state across requests and threads, which can leak state between requests and introduce race conditions.
3) The change introduced the global singleton (attack_wave_detector_store) — the issue did not exist before this PR because this file and its module-level instance are new.
🔧 How do I fix it?
Avoid storing request-specific data in module-level variables. Use request-scoped variables or explicitly mark shared caches as intentional.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| """ | ||
| normalized = path.lower() | ||
| segments = normalized.split("/") | ||
| if not segments: |
There was a problem hiding this comment.
The condition if not segments: is unreachable because normalized.split('/') always returns a non-empty list.
Details
✨ AI Reasoning
1) The function is_web_scan_path normalizes the input path and does segments = normalized.split('/').
2) The subsequent check if not segments: return False is unreachable because str.split('/') in Python always returns a list with at least one element (even for an empty string), so segments is never falsy.
3) This harms maintainability by including dead code that confuses readers about possible control flow and intended behavior.
4) The violation is True because the unreachable check was introduced in this change.
5) It's a simple, well-understood Python behavior and fixable within this PR by removing the check or adjusting logic.
🔧 How do I fix it?
Remove code that can never be executed due to control flow. Check for code after return/throw/break statements or in unreachable conditional branches.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
0645e45 to
35d6ffb
Compare
https://aikido.atlassian.net/browse/AIK-6314