Improved command data structure & handler

[bitterpanda63]

No description provided.

[aikido-pr-checks]

Function on_start no longer fails fast when connection_manager.token is missing; the prior early-return was removed, reducing clarity of control flow

Details

✨ AI Reasoning
​​1) The on_start function was modified to remove the prior early-return guard that immediately returned when connection_manager.token was falsy; it now always constructs an event and calls connection_manager.report_api_event(event), relying on that function to handle invalid-token cases.
​2) This change worsens clarity by removing a local fail-fast guard and reintroducing an execution path through report_api_event even when no token exists, making the control flow less explicit at the call site and slightly harder to reason about.
​3) Because the PR removed the early-return from on_start (it previously prevented further work when token was missing), this is a regression against the prefer-early-return guideline and therefore a valid violation introduced by this diff.

🔧 How do I fix it?
Place parameter validation and guard clauses at the function start. Use early returns to reduce nesting levels and improve readability.

_{More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}

[aikido-pr-checks]

report_api_event merges the incoming 'event' dict via payload.update(event) without copying, causing unsafe access to shared mutable state across threads

Details

✨ AI Reasoning
​​1) The new CloudConnectionManager.report_api_event method takes an external 'event' object from the background thread and directly calls payload.update(event) to merge it into a new payload; 2) If that 'event' is a mutable dict that is concurrently produced or modified by other threads, merging it without copying can result in race conditions or inconsistent reads; 3) This harms thread-safety because the method reads shared mutable data without synchronization; 4) Fixing this would meaningfully reduce the chance of subtle concurrent-bug reproduction and is feasible within this PR by making a shallow copy of the event before merging; 5) Using immutable or copied data is a common expectation for cross-thread messaging; 6) This is a localized change (in report_api_event) and doesn't require major refactoring.

🔧 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-pr-checks]

Method report_api_event lacks a docstring describing expected event payload, return structure, and error handling behavior.

Details

✨ AI Reasoning
​​1) The new CloudConnectionManager.report_api_event method constructs a payload, merges event fields, calls the API, and handles error logging and exceptions — this is non-trivial control flow affecting reporting behavior.
​2) Lack of a docstring means future readers won't have guidance on intent, expected event shape, return values, or error semantics, harming maintainability.
​3) The method's name alone doesn't fully document the payload merging behavior or the error handling policy.
​4) Adding a brief docstring would be a small, self-contained improvement appropriate for this PR.
​5) This is not a trivial getter; it's a business-logic method for reporting.
​6) Fixable within the PR by adding a short docstring/comment.

🔧 How do I fix it?
Add docstrings or comments explaining what the function does, its parameters, and return values.

_{More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}

[aikido-pr-checks]

Replaced explicit unpacking and on_detected_attack call with direct report_api_event(self.queue.get()) without documenting the new expected queue payload or rationale

Details

✨ AI Reasoning
​​1) The send_to_connection_manager method changed how queue items are processed: previously the code unpacked a 4-tuple and called connection_manager.on_detected_attack; the updated code now forwards queue.get() directly to connection_manager.report_api_event.
​2) This is a meaningful behavioural change that affects the expected queue payload format and routing logic; the method has a docstring but no commentary about the new expected item shape or rationale for the changed dispatch, which reduces clarity for maintainers.
​3) The change occurred in this diff (replaced explicit unpack-and-call with direct pass-through), so documentation was effectively reduced/worsened by this PR.

🔧 How do I fix it?
Add docstrings or comments explaining what the function does, its parameters, and return values.

_{More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}

[aikido-pr-checks]

Accessing context.user and other context attributes without synchronization can race with concurrent mutations of the shared context.

Details

✨ AI Reasoning
​​1) The new helper create_detected_attack_api_event extracts many attributes from the passed context (e.g., getattr(context, 'user', None) and context.get_user_agent()).
​2) Those attribute reads occur without any synchronization and context appears to be mutable/shared; performing multiple non-atomic reads can observe inconsistent state or race with concurrent writers.
​3) This is newly introduced code that increases risk of data races compared to the prior behavior of passing the context object to the background process.

🔧 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-pr-checks]

Multiple unsynchronized reads (including context.get_user_agent()) from shared context may cause data races or inconsistent request data.

Details

✨ AI Reasoning
​​1) The new extract_request_if_possible reads multiple attributes and calls context.get_user_agent() on the context object without any locking (lines accessing method, url, remote_address, source, route, get_user_agent).
​2) These are multiple separate reads from a potentially shared mutable object and may observe inconsistent or partially-updated state under concurrency.
​3) This file/function is newly added in this change and therefore introduces unsynchronized access to shared mutable state.

🔧 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-pr-checks]

Function create_detected_attack_api_event contains non-trivial event-construction and error-handling logic but has no docstring or explanatory comments.

Details

✨ AI Reasoning
​​1) The new function create_detected_attack_api_event(attack, context, blocked, stack) assembles a complex API event dict, performs JSON serialization and length-limiting, extracts request info, and handles exceptions.
​2) Lack of a docstring or explanatory comment makes the purpose, expected shapes of attack and context, and the rationale for truncation/limits unclear, which harms maintainability and makes future changes risky.
​3) This violation is true because the function contains non-trivial business logic and error handling and was introduced in this diff without any explanatory documentation.

🔧 How do I fix it?
Add docstrings or comments explaining what the function does, its parameters, and return values.

_{More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}

[aikido-pr-checks]

Calling context.get_user_agent() without checking the method may raise AttributeError if context lacks get_user_agent.

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.}

[aikido-pr-checks]

Call to comms.send_data_to_bg_process uses three positional arguments (payload.identifier, payload.request, payload.returns_data) instead of named arguments

Details

🔧 How do I fix it?
Use named parameters for functions with 3+ arguments to make the code self-documenting. This prevents parameter order confusion and improves readability.

_{More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}

[aikido-pr-checks]

Call to create_detected_attack_api_event(injection_results, context, blocked, stack) uses four positional arguments instead of named arguments.

Details

🔧 How do I fix it?
Use named parameters for functions with 3+ arguments to make the code self-documenting. This prevents parameter order confusion and improves readability.

_{More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}

[aikido-pr-checks]

Changed outbound heartbeat reporting: switched to connection_manager.report_api_event and removed 'time' and 'agent' from the heartbeat payload which may break the server-side API contract.

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.}

[aikido-pr-checks]

Function process_check_firewall_lists nests IP and user-agent checks inside conditionals rather than using early guard returns, increasing nesting and reducing readability.

Details

✨ AI Reasoning
​​1) process_check_firewall_lists validates ip and user-agent inside nested if blocks after extracting values from data; 2) the nesting (if ip is not None and isinstance... then inner checks; similarly for user_agent) makes the flow less immediately readable and could be flattened with guard clauses/early returns; 3) this harms maintainability by burying the primary decision points inside nested conditionals; 4) the change in this diff updated the function signature and is the location where readability could be improved, so it is appropriate to flag here as introduced/worsened by the PR (the body remains but the function signature change is part of the changed lines);

🔧 How do I fix it?
Place parameter validation and guard clauses at the function start. Use early returns to reduce nesting levels and improve readability.

_{More info - Comment @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}