Linter: enable attribute-defined-outside-init rule #528
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| blocked_uids=res.get("blockedUserIds", []), | ||
| bypassed_ips=res.get("allowedIPAddresses", []), | ||
| received_any_stats=res.get("receivedAnyStats", True), | ||
| connection_manager.conf.set_endpoints(res.get("endpoints", [])) |
There was a problem hiding this comment.
Replacing a single atomic update with multiple unsynchronized set_* calls can cause other threads to observe partially-updated connection_manager.conf (race condition).
Details
✨ AI Reasoning
1) The function applies configuration changes from server response onto a shared connection_manager.conf object by calling multiple setter methods in sequence.
2) This change replaced a single update(...) call with several separate set_* calls, which can leave the shared config in a partially-updated state visible to other threads, introducing a race condition where other threads may observe inconsistent intermediate state.
3) Because these setter calls mutate shared state without any synchronization, the change harms thread-safety and was introduced by this diff (the atomic update was removed).
🔧 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.
| ) | ||
| connection_manager.conf.set_blocked_user_ids(res.get("blockedUserIds", [])) | ||
| connection_manager.conf.set_bypassed_ips(res.get("allowedIPAddresses", [])) | ||
| if res.get("receivedAnyStats", True): |
There was a problem hiding this comment.
Handling of 'receivedAnyStats' changed from explicitly setting to only enabling when true, which alters prior semantics and can leave the flag unchanged when false.
Details
🔧 How do I fix it?
Support both old and new parameter names during transition periods. Add new optional parameters with defaults. Keep existing response fields while adding new ones. Focus on backwards compatibility.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| # Make sure that empty bodies like b"" don't get sent. | ||
| return None | ||
| if isinstance(body, bytes): | ||
| body = body.decode("utf-8") # Decode byte input to string. |
There was a problem hiding this comment.
Function parameter 'body' is reassigned (body = body.decode(...)), which obscures the original argument value.
Details
✨ AI Reasoning
1) What the code does: The new static method parse_body_object(body) transforms and possibly decodes the provided body; it reassigns the parameter variable 'body' during decoding (body = body.decode("utf-8")).
2) Whether it harms maintainability: Reassigning a function parameter can confuse readers about the original input and makes debugging/original-value access harder; it also departs from the prior instance-field-based transformation approach.
3) Why violation is true/false: This reassignment was introduced in this diff (previous implementation mutated self.body instead); it's a clear instance of argument reassignment introduced by the change and worth avoiding.
🔧 How do I fix it?
Create new local variables instead of reassigning parameters. Use different variable names to clearly distinguish between input and modified values.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| def set_body_internal(self, body): | ||
| @staticmethod | ||
| def parse_body_object(body): | ||
| """Sets the body and checks if it's possibly JSON""" |
There was a problem hiding this comment.
Docstring for parse_body_object describes what the function does rather than why (redundant "what" comment).
Details
🔧 How do I fix it?
Write comments that explain the purpose, reasoning, or business logic behind the code using words like 'because', 'so that', or 'in order to'.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| bypassed_ips=[], | ||
| received_any_stats=True, | ||
| ) | ||
| self.conf = ServiceConfig() |
There was a problem hiding this comment.
Replacing explicit ServiceConfig(...) with ServiceConfig() changes initial received_any_stats default and can invert report_initial_stats behavior.
Details
🔧 How do I fix it?
Trace execution paths carefully. Ensure precondition checks happen before using values, validate ranges before checking impossible conditions, and don't check for states that the code has already ruled out.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| self.blocked_uids = set(blocked_uids) | ||
| self.set_endpoints(endpoints) | ||
| self.set_bypassed_ips(bypassed_ips) | ||
| def __init__(self): |
There was a problem hiding this comment.
Removing the atomic update(...) and replacing it with multiple setters allows partially-applied config changes and introduces thread-safety/race condition risk.
Details
✨ AI Reasoning
1) The ServiceConfig class originally provided an update(...) method that atomically set multiple related attributes (endpoints, last_updated_at, blocked_uids, bypassed_ips, received_any_stats).
2) The diff removes that update(...) method and instead adds separate setters (set_blocked_user_ids, set_bypassed_ips, enable_received_any_stats, set_last_updated_at) and an init that initializes fields.
3) This change allows callers to update related configuration fields in multiple separate calls, creating a window for other threads to observe partially-updated state and increasing risk of race conditions.
4) This harms thread-safety compared to the previous atomic update approach and is a meaningful regression in concurrent scenarios.
5) The issue was introduced by this PR (update removed and separate setters added), so it is relevant to flag here.
🔧 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.
|
|
||
| def set_bypassed_ips(self, bypassed_ips): | ||
| """Creates an IPMatcher from the given bypassed ip set""" | ||
| """Creates a new IPMatcher from the given bypassed ip set""" |
There was a problem hiding this comment.
Comment documents what the method does instead of why (explain rationale for creating a new IPMatcher rather than describing mechanics).
Details
🔧 How do I fix it?
Write comments that explain the purpose, reasoning, or business logic behind the code using words like 'because', 'so that', or 'in order to'.
More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
| """ | ||
|
|
||
| def __init__(self): | ||
| self.config = ServiceConfig() |
There was a problem hiding this comment.
Initialization logic for config/routes/middleware_installed is duplicated between init and reset
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.
No description provided.