Skip to content

Conversation

@bitterpanda63
Copy link
Member

@bitterpanda63 bitterpanda63 commented Nov 13, 2025

Summary by Aikido

Security Issues: 0 πŸ” Quality Issues: 3 Resolved Issues: 0

⚑ Enhancements

  • Initialized ServiceConfig attributes and added explicit setter methods

πŸ”§ Refactors

  • Replaced ServiceConfig.update calls with granular setter invocations throughout codebase
  • Refactored Context body handling to use parse_body_object staticmethod
  • Switched ThreadCache and CloudConnectionManager to construct ServiceConfig directly

More info

# 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.
Copy link

@aikido-pr-checks aikido-pr-checks bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parse_body_object reassigns parameter 'body' (e.g., body = body.decode(...)). Assign decode result to a new local variable to preserve the original argument.

Details

✨ AI Reasoning
​A newly added static method transforms its input parameter by decoding bytes into a string and reassigning the same local parameter name. Reassigning a function parameter can obscure the original value and its type; here the method both decodes and later parses the same parameter, which may confuse readers and complicate debugging. The pattern is normalization (decoding) before use, but reassigning the parameter rather than assigning to a new local variable reduces clarity.

πŸ”§ 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 __init__(self):
self.config = ServiceConfig()
Copy link

@aikido-pr-checks aikido-pr-checks bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initialization logic for config/routes/middleware_installed is duplicated between init and reset

Details

✨ AI Reasoning
​​1) The diff adds explicit initializations in init for config, middleware_installed, and routes (lines 20-23) while reset() also sets config and clears routes (lines 41-42); 2) This duplicates the same initialization logic in two places, increasing maintenance burden because changes must be kept in sync; 3) This harms maintainability by introducing obvious within-file duplication that can lead to inconsistencies; 4) The violation was introduced by this PR (previously init delegated initialization to reset), so it's new; 5) The duplication is straightforward to refactor (extract common init helper or have init call reset); 6) Fixable within this PR scope.

πŸ”§ 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.

@bitterpanda63 bitterpanda63 reopened this Nov 26, 2025
@bitterpanda63 bitterpanda63 marked this pull request as draft November 26, 2025 12:32
@bitterpanda63 bitterpanda63 marked this pull request as ready for review December 30, 2025 10:00
connection_manager.conf.set_blocked_user_ids(res.get("blockedUserIds", []))
connection_manager.conf.set_bypassed_ips(res.get("allowedIPAddresses", []))
if res.get("receivedAnyStats", True):
connection_manager.conf.enable_received_any_stats()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flag doesn't update like other settings but seems fine, will only be false for the first heartbeat (and this flag is deprecated anyway). Needs to start sending heartbeat after 30s, then 2 minutes, then every 10 minutes

self.bypassed_ips = IPMatcher()
self.blocked_uids = set()
self.last_updated_at = -1
self.received_any_stats = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously true fyi

Co-authored-by: Hans Ott <hansott@hotmail.be>
self.body = Context.parse_body_object(body)

@staticmethod
def parse_body_object(body):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method docstring restates what parse_body_object does; explain why these parsing rules exist (e.g., server quirks, expected input shapes) instead of repeating the mechanics.

Details

✨ AI Reasoning
​The new staticmethod parse_body_object has a docstring that repeats the mechanics implemented in the method rather than providing rationale or design decisions. It doesn't explain why empty byte bodies should become None, why JSON is heuristically detected, or compatibility constraints that motivated this behavior.

πŸ”§ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants