Skip to content

Improve validation of client ID and env #179

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 1, 2025
Merged

Improve validation of client ID and env #179

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
39 changes: 24 additions & 15 deletions apitally/client/client_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,19 @@ def __init__(self, client_id: str, env: str, request_logging_config: Optional[Re
if hasattr(self, "client_id"):
raise RuntimeError("Apitally client is already initialized") # pragma: no cover

client_id_valid = True
env_valid = True
self.client_id = str(client_id)
self.env = str(env)
self.enabled = True

if not self.validate_client_id(self.client_id):
self.enabled = False
logger.error(f"invalid client_id '{self.client_id}' (expecting string in hexadecimal UUID format)")
if not self.validate_env(self.env):
self.enabled = False
logger.error(
f"invalid env '{self.env}' (expecting string with 1-32 alphanumeric characters and hyphens only)"
)
Comment on lines +47 to +58
Copy link

@coderabbitai coderabbitai bot Nov 1, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Good approach to prevent TypeError, but consider warning about type mismatches.

The string conversion prevents the TypeError that was causing 500 errors, which directly addresses issue #173. However, silently converting non-string types (e.g., None → "None", 1234 → "1234") may hide configuration errors that developers should be aware of.

Consider adding type checks before conversion to log warnings:

+        if not isinstance(client_id, str):
+            logger.warning(f"client_id should be a string, got {type(client_id).__name__}")
         self.client_id = str(client_id)
+        if not isinstance(env, str):
+            logger.warning(f"env should be a string, got {type(env).__name__}")
         self.env = str(env)

Additionally, verify that the enabled flag is checked in the middleware to ensure that when validation fails, the client is actually disabled and doesn't process requests:

🏁 Script executed:

#!/bin/bash
# Description: Verify that the enabled flag is checked before processing requests

# Search for usages of the enabled flag
rg -nP --type=py -C3 'self\.enabled|client\.enabled|instance\.enabled'

Length of output: 9171


---

🏁 Script executed:

```shell
#!/bin/bash
# Retrieve the full implementation of validate_client_id and validate_env methods

rg -nP --type=py 'validate_(client_id|env)' -A 5 'apitally/client/client_base.py'

Length of output: 765

Fix regex pattern or error message inconsistency in validate_env method.

The enabled flag is properly checked throughout all middleware implementations before processing requests, which addresses the concern from the original review. However, there's an inconsistency in the validate_env method:

The regex pattern r"^[\w-]{1,32}$" includes underscores (since \w matches [a-zA-Z0-9_]), but the error message states: "expecting string with 1-32 alphanumeric characters and hyphens only" — which doesn't mention underscores.

Either update the error message to include underscores, or change the regex to r"^[a-zA-Z0-9-]{1,32}$" to match the documented behavior. Lines 114-115 in apitally/client/client_base.py and lines 57-58 must be kept in sync.

🤖 Prompt for AI Agents 
In apitally/client/client_base.py around lines 47-58 (and ensure consistency
with validate_env at lines 114-115), the regex r"^[\w-]{1,32}$" allows
underscores while the error message states "alphanumeric characters and hyphens
only"; update the implementation so both agree — change the regex to only allow
letters, digits and hyphens (e.g. use a pattern that matches [a-zA-Z0-9-]{1,32})
or alternatively update the error message to mention underscores; pick the
former (change the regex to exclude underscores) and ensure the error message
and validate_env pattern on lines 114-115 remain in sync.


try:
UUID(client_id)
except ValueError:
logger.error(f"invalid client_id '{client_id}' (expecting hexadecimal UUID format)")
client_id_valid = False

if re.match(r"^[\w-]{1,32}$", env) is None:
logger.error(f"invalid env '{env}' (expecting 1-32 alphanumeric lowercase characters and hyphens only)")
env_valid = False

self.client_id = client_id
self.env = env
self.enabled = client_id_valid and env_valid
self.instance_uuid = str(uuid4())
self.request_counter = RequestCounter()
self.validation_error_counter = ValidationErrorCounter()
Expand Down Expand Up @@ -104,3 +101,15 @@ def get_sync_data(self) -> Dict[str, Any]:
"consumers": self.consumer_registry.get_and_reset_updated_consumers(),
}
return self.add_uuids_to_data(data)

@staticmethod
def validate_client_id(client_id: str) -> bool:
try:
UUID(client_id)
return True
except ValueError:
return False

@staticmethod
def validate_env(env: str) -> bool:
return re.match(r"^[\w-]{1,32}$", env) is not None
Comment on lines +113 to +115
Copy link

@coderabbitai coderabbitai bot Nov 1, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Regex pattern includes underscores but error message doesn't mention them.

The regex pattern \w matches alphanumeric characters, underscores, and (when combined with the hyphen in the character class) hyphens. However, the error message at line 57 states "alphanumeric characters and hyphens only" without mentioning underscores.

Choose one of the following fixes:

Option 1: Exclude underscores from the pattern (if not intended):

-    def validate_env(env: str) -> bool:
-        return re.match(r"^[\w-]{1,32}$", env) is not None
+    def validate_env(env: str) -> bool:
+        return re.match(r"^[a-zA-Z0-9-]{1,32}$", env) is not None

Option 2: Update the error message to reflect that underscores are allowed:

         logger.error(
-            f"invalid env '{self.env}' (expecting string with 1-32 alphanumeric characters and hyphens only)"
+            f"invalid env '{self.env}' (expecting string with 1-32 alphanumeric characters, hyphens, and underscores)"
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@staticmethod
def validate_env(env: str) -> bool:
return re.match(r"^[\w-]{1,32}$", env) is not None
@staticmethod
def validate_env(env: str) -> bool:
return re.match(r"^[a-zA-Z0-9-]{1,32}$", env) is not None
🤖 Prompt for AI Agents 
In apitally/client/client_base.py around lines 113–115, the validate_env regex
uses \w which allows underscores but the error message (line 57) says
"alphanumeric characters and hyphens only"; to fix, replace \w in the character
class with an explicit alphanumeric set (e.g. [A-Za-z0-9]) so the pattern
becomes /^[A-Za-z0-9-]{1,32}$/ and thus matches the error text; update any
related tests or callers if they relied on underscores.