Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,4 @@ disable =
no-value-for-parameter,
unexpected-keyword-arg,
inconsistent-return-statements,
duplicate-code,
attribute-defined-outside-init,
duplicate-code
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,7 @@ def __init__(self, block, api, token, serverless):
self.token = token # Should be instance of the Token class!
self.routes = Routes(200)
self.hostnames = Hostnames(200)
self.conf = ServiceConfig(
endpoints=[],
last_updated_at=-1, # Has not been updated yet
blocked_uids=[],
bypassed_ips=[],
received_any_stats=True,
)
self.conf = 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.

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.firewall_lists = FirewallLists()
self.rate_limiter = RateLimiter(
max_items=5000, time_to_live_in_ms=120 * 60 * 1000 # 120 minutes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@ def update_service_config(connection_manager, res):
logger.debug("Updating blocking, setting blocking to : %s", res["block"])
connection_manager.block = bool(res["block"])

connection_manager.conf.update(
endpoints=res.get("endpoints", []),
last_updated_at=res.get("configUpdatedAt", get_unixtime_ms()),
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", []))
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.

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_last_updated_at(
res.get("configUpdatedAt", get_unixtime_ms())
)
connection_manager.conf.set_blocked_user_ids(res.get("blockedUserIds", []))
connection_manager.conf.set_bypassed_ips(res.get("allowedIPAddresses", []))
if res.get("receivedAnyStats", True):
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.

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.

connection_manager.conf.enable_received_stats()
47 changes: 16 additions & 31 deletions aikido_zen/background_process/service_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,16 @@
from aikido_zen.helpers.match_endpoints import match_endpoints


# noinspection PyAttributeOutsideInit
class ServiceConfig:
"""Class holding the config of the connection_manager"""

def __init__(
self,
endpoints,
last_updated_at: int,
blocked_uids,
bypassed_ips,
received_any_stats: bool,
):
# Init the class using update function :
self.update(
endpoints, last_updated_at, blocked_uids, bypassed_ips, received_any_stats
)

def update(
self,
endpoints,
last_updated_at: int,
blocked_uids,
bypassed_ips,
received_any_stats: bool,
):
self.last_updated_at = last_updated_at
self.received_any_stats = bool(received_any_stats)
self.blocked_uids = set(blocked_uids)
self.set_endpoints(endpoints)
self.set_bypassed_ips(bypassed_ips)
def __init__(self):
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.

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.

self.endpoints = []
self.bypassed_ips = IPMatcher()
self.blocked_uids = set()
self.last_updated_at = -1
self.received_any_stats = False

def set_endpoints(self, endpoints):
"""Sets non-graphql endpoints"""

self.endpoints = [
endpoint for endpoint in endpoints if not endpoint.get("graphql")
]
Expand All @@ -66,11 +42,20 @@ def get_endpoints(self, route_metadata):
return match_endpoints(route_metadata, self.endpoints)

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

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.

self.bypassed_ips = IPMatcher()
for ip in bypassed_ips:
self.bypassed_ips.add(ip)

def is_bypassed_ip(self, ip):
"""Checks if the IP is on the bypass list"""
return self.bypassed_ips.has(ip)

def set_blocked_user_ids(self, blocked_user_ids):
self.blocked_uids = set(blocked_user_ids)

def enable_received_any_stats(self):
self.received_any_stats = True

def set_last_updated_at(self, last_updated_at: int):
self.last_updated_at = last_updated_at
123 changes: 50 additions & 73 deletions aikido_zen/background_process/service_config_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@


def test_service_config_initialization():
service_config = ServiceConfig()
endpoints = [
{
"graphql": False,
Expand Down Expand Up @@ -51,26 +52,33 @@ def test_service_config_initialization():
"force_protection_off": False,
},
]
last_updated_at = "2023-10-01"
service_config = ServiceConfig(
endpoints,
last_updated_at,
["0", "0", "1", "5"],
["127.0.0.1", "123.1.2.0/24", "132.1.0.0/16"],
True,
)

# Check that non-GraphQL endpoints are correctly filtered
assert len(service_config.endpoints) == 3
assert len(service_config.endpoints) == 0
service_config.set_endpoints(endpoints)
assert (
len(service_config.endpoints) == 3
) # Check that non-GraphQL endpoints are correctly filtered
assert service_config.endpoints[0]["route"] == "/v1"
assert service_config.endpoints[1]["route"] == "/v3"
assert service_config.endpoints[2]["route"] == "/admin"
assert service_config.last_updated_at == last_updated_at

service_config.set_last_updated_at(37982562953)
assert service_config.last_updated_at == 37982562953

assert isinstance(service_config.bypassed_ips, IPMatcher)
service_config.set_bypassed_ips(["127.0.0.1", "123.1.2.0/24", "132.1.0.0/16"])
assert isinstance(service_config.bypassed_ips, IPMatcher)
assert service_config.bypassed_ips.has("127.0.0.1")
assert service_config.bypassed_ips.has("123.1.2.2")
assert not service_config.bypassed_ips.has("1.1.1.1")
assert service_config.blocked_uids == set(["1", "0", "5"])

assert len(service_config.blocked_uids) == 0
service_config.set_blocked_user_ids({"0", "0", "1", "5"})
assert service_config.blocked_uids == {"1", "0", "5"}

assert not service_config.received_any_stats
service_config.enable_received_any_stats()
assert service_config.received_any_stats == True

v1_endpoint = service_config.get_endpoints(
{
Expand All @@ -96,41 +104,9 @@ def test_service_config_initialization():
assert not admin_endpoint["allowedIPAddresses"].has("192.168.0.1")


# Sample data for testing
sample_endpoints = [
{"url": "http://example.com/api/v1", "graphql": False, "context": "user"},
{"url": "http://example.com/api/v2", "graphql": True, "context": "admin"},
{"url": "http://example.com/api/v3", "graphql": False, "context": "guest"},
]


@pytest.fixture
def service_config():
return ServiceConfig(
endpoints=sample_endpoints,
last_updated_at="2023-10-01T00:00:00Z",
blocked_uids=["user1", "user2"],
bypassed_ips=["192.168.1.1", "10.0.0.1"],
received_any_stats=True,
)


def test_initialization(service_config):
assert len(service_config.endpoints) == 2 # Only non-graphql endpoints
assert service_config.last_updated_at == "2023-10-01T00:00:00Z"
assert isinstance(service_config.bypassed_ips, IPMatcher)
assert service_config.blocked_uids == {"user1", "user2"}


def test_ip_blocking():
config = ServiceConfig(
endpoints=sample_endpoints,
last_updated_at="2023-10-01T00:00:00Z",
blocked_uids=["user1", "user2"],
bypassed_ips=["192.168.1.1", "10.0.0.0/16", "::1/128"],
received_any_stats=True,
)

config = ServiceConfig()
config.set_bypassed_ips(["192.168.1.1", "10.0.0.0/16", "::1/128"])
assert config.is_bypassed_ip("192.168.1.1")
assert config.is_bypassed_ip("10.0.0.1")
assert config.is_bypassed_ip("10.0.1.2")
Expand All @@ -142,38 +118,39 @@ def test_ip_blocking():


def test_service_config_with_empty_allowlist():
endpoints = [
{
"graphql": False,
"method": "GET",
"route": "/admin",
"rate_limiting": {
"enabled": False,
"max_requests": 10,
"window_size_in_ms": 1000,
},
"allowedIPAddresses": [],
"force_protection_off": False,
},
]
last_updated_at = "2023-10-01"
service_config = ServiceConfig(
endpoints,
last_updated_at,
["0", "0", "1", "5"],
["127.0.0.1", "123.1.2.0/24", "132.1.0.0/16"],
True,
)
service_config = ServiceConfig()

# Check that non-GraphQL endpoints are correctly filtered
service_config.set_endpoints(
[
{
"graphql": False,
"method": "GET",
"route": "/admin",
"rate_limiting": {
"enabled": False,
"max_requests": 10,
"window_size_in_ms": 1000,
},
"allowedIPAddresses": [],
"force_protection_off": False,
},
]
)
assert len(service_config.endpoints) == 1
assert service_config.endpoints[0]["route"] == "/admin"
assert service_config.last_updated_at == last_updated_at

service_config.set_last_updated_at(29839537)
assert service_config.last_updated_at == 29839537

service_config.set_blocked_user_ids({"0", "0", "1", "5"})
assert service_config.blocked_uids == {"1", "0", "5"}

service_config.set_bypassed_ips(["127.0.0.1", "123.1.2.0/24", "132.1.0.0/16"])
assert isinstance(service_config.bypassed_ips, IPMatcher)
assert service_config.bypassed_ips.has("127.0.0.1")
assert service_config.bypassed_ips.has("123.1.2.2")
assert not service_config.bypassed_ips.has("1.1.1.1")
assert service_config.blocked_uids == set(["1", "0", "5"])
assert service_config.is_bypassed_ip("127.0.0.1")
assert service_config.is_bypassed_ip("123.1.2.2")
assert not service_config.is_bypassed_ip("1.1.1.1")

admin_endpoint = service_config.get_endpoints(
{
Expand Down
40 changes: 21 additions & 19 deletions aikido_zen/context/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def __init__(self, context_obj=None, body=None, req=None, source=None):
self.parsed_userinput = {}
self.xml = {}
self.outgoing_req_redirects = []
self.set_body(body)
self.body = Context.parse_body_object(body)
self.headers: Headers = Headers()
self.cookies = {}
self.query = {}
Expand Down Expand Up @@ -107,26 +107,28 @@ def set_cookies(self, cookies):
self.cookies = cookies

def set_body(self, body):
try:
self.set_body_internal(body)
except Exception as e:
logger.debug("Exception occurred whilst setting body: %s", e)
self.body = Context.parse_body_object(body)

def set_body_internal(self, body):
@staticmethod
def parse_body_object(body):
"""Sets the body and checks if it's possibly JSON"""
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.

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.

self.body = body
if isinstance(self.body, (str, bytes)) and len(body) == 0:
# Make sure that empty bodies like b"" don't get sent.
self.body = None
if isinstance(self.body, bytes):
self.body = self.body.decode("utf-8") # Decode byte input to string.
if not isinstance(self.body, str):
return
if self.body.strip()[0] in ["{", "[", '"']:
# Might be JSON, but might not have been parsed correctly by server because of wrong headers
parsed_body = json.loads(self.body)
if parsed_body:
self.body = parsed_body
try:
if isinstance(body, (str, bytes)) and len(body) == 0:
# 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.

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.

if not isinstance(body, str):
return body
if body.strip()[0] in ["{", "[", '"']:
# Might be JSON, but might not have been parsed correctly by server because of wrong headers
parsed_body = json.loads(body)
if parsed_body:
return parsed_body
return body
except Exception as e:
logger.debug("Exception occurred whilst parsing body: %s", e)
return body

def get_route_metadata(self):
"""Returns a route_metadata object"""
Expand Down
11 changes: 4 additions & 7 deletions aikido_zen/ratelimiting/init_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,10 @@ def user():

def create_connection_manager(endpoints=[], bypassed_ips=[]):
cm = MagicMock()
cm.conf = ServiceConfig(
endpoints=endpoints,
last_updated_at=1,
blocked_uids=[],
bypassed_ips=bypassed_ips,
received_any_stats=True,
)
cm.conf = ServiceConfig()
cm.conf.set_endpoints(endpoints)
cm.conf.enable_received_any_stats()
cm.conf.set_bypassed_ips(bypassed_ips)
cm.rate_limiter = RateLimiter(
max_items=5000, time_to_live_in_ms=120 * 60 * 1000 # 120 minutes
)
Expand Down
11 changes: 4 additions & 7 deletions aikido_zen/sources/functions/request_handler_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,19 +146,16 @@ def set_context(remote_address, user_agent=""):


def create_service_config():
config = ServiceConfig(
endpoints=[
config = ServiceConfig()
config.set_endpoints(
[
{
"method": "POST",
"route": "/posts/:number",
"graphql": False,
"allowedIPAddresses": ["1.1.1.1", "2.2.2.2", "3.3.3.3"],
}
],
last_updated_at=None,
blocked_uids=set(),
bypassed_ips=[],
received_any_stats=False,
]
)
get_cache().config = config
return config
Expand Down
Loading
Loading