-
Notifications
You must be signed in to change notification settings - Fork 11
add outbound blocking #550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d416bf5 to
69e8750
Compare
This reverts commit 138f4fe.
| return self.bypassed_ips.has(ip) | ||
|
|
||
| def update_domains(self, domains): | ||
| self.domains = {domain["hostname"]: domain["mode"] for domain in domains} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_domains mutates self.domains (a shared dict) without synchronization; concurrent readers may observe partial updates or race conditions.
Details
✨ AI Reasoning
ServiceConfig gained mutable fields domains and block_new_outgoing_requests that are written by update_service_config and read by sinks. The methods update_domains and set_block_new_outgoing_requests mutate shared ServiceConfig state without any synchronization or atomic swap, introducing a data race between writer and readers.
🔧 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/background_process/cloud_connection_manager/update_service_config.py
Show resolved
Hide resolved
| if not cache: | ||
| return | ||
|
|
||
| cache.hostnames.add(hostname, port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modifies shared collection cache.hostnames (cache.hostnames.add) without synchronization; this mutates shared state and can cause races or iteration errors.
Details
✨ AI Reasoning
The new function report_and_check_hostname retrieves a shared thread-local/global cache via get_cache() and calls cache.hostnames.add(hostname, port) (line 18). This is a mutation of shared mutable state. There's no locking or atomic API shown, and other threads may concurrently access or iterate the same hostnames collection, risking race conditions or runtime errors (e.g., mutation during iteration). The change moved hostname registration earlier (it used to be added in vulnerabilities.init but that code was removed), so this PR introduced the new mutation point.
🔧 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.
This reverts commit 69e8750.
Summary by Aikido
⚡ Enhancements
🔧 Refactors