Skip to content

🛡️ Sentinel: Fix unbounded retries on client errors #191

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
abhimehro merged 1 commit into from
Feb 13, 2026
Merged

🛡️ Sentinel: Fix unbounded retries on client errors #191

Show file tree
Hide file tree
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
8 changes: 8 additions & 0 deletions .jules/sentinel.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,11 @@
**Prevention:** Explicitly block all known Bidi control characters (U+202A-U+202E, U+2066-U+2069, U+200E-U+200F) in user-visible strings. Also block path separators (/, \) to prevent confusion.

**Implementation:** Pre-compiled character sets at module level for performance, tested comprehensively for all 11 blocked Bidi characters.

## 2026-10-24 - Unbounded Retries on Client Errors (DoS Risk)

**Vulnerability:** The retry logic blindly retried all `httpx.HTTPError` exceptions, including 400 (Bad Request) and 401/403 (Auth failures). This causes API spamming, potential account lockouts, and delays in error reporting.

**Learning:** `httpx.HTTPStatusError` (raised by `raise_for_status()`) inherits from `httpx.HTTPError`. Generic `except httpx.HTTPError:` blocks will catch it and retry client errors unless explicitly handled.

**Prevention:** Inside retry loops, catch `httpx.HTTPStatusError` first. Check `response.status_code`. If `400 <= code < 500` (and not `429`), re-raise immediately.
Comment on lines +13 to +19
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

The heading/description calls this "Unbounded Retries", but _retry_request is bounded by MAX_RETRIES (currently 10). To avoid misleading future readers, consider rewording to "Retries on client errors (4xx)" (or similar) and keep the DoS discussion focused on unnecessary repeated requests rather than "unbounded" behavior.

Suggested change
## 2026-10-24 - Unbounded Retries on Client Errors (DoS Risk)
**Vulnerability:** The retry logic blindly retried all `httpx.HTTPError` exceptions, including 400 (Bad Request) and 401/403 (Auth failures). This causes API spamming, potential account lockouts, and delays in error reporting.
**Learning:** `httpx.HTTPStatusError` (raised by `raise_for_status()`) inherits from `httpx.HTTPError`. Generic `except httpx.HTTPError:` blocks will catch it and retry client errors unless explicitly handled.
**Prevention:** Inside retry loops, catch `httpx.HTTPStatusError` first. Check `response.status_code`. If `400 <= code < 500` (and not `429`), re-raise immediately.
## 2026-10-24 - Retries on Client Errors (4xx) Causing DoS Risk
**Vulnerability:** The retry logic blindly retried all `httpx.HTTPError` exceptions, including 400 (Bad Request) and 401/403 (Auth failures). Even though retries are bounded by `MAX_RETRIES`, this still causes unnecessary repeated requests, API spamming, potential account lockouts, and delays in surfacing real errors.
**Learning:** `httpx.HTTPStatusError` (raised by `raise_for_status()`) inherits from `httpx.HTTPError`. Generic `except httpx.HTTPError:` blocks will catch it and retry client errors unless explicitly handled.
**Prevention:** Inside retry loops, catch `httpx.HTTPStatusError` first. Check `response.status_code`. If `400 <= code < 500` (and not `429`), re-raise immediately so client errors fail fast instead of being retried.

Copilot uses AI. Check for mistakes.
11 changes: 11 additions & 0 deletions main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,17 @@
response.raise_for_status()
return response
except (httpx.HTTPError, httpx.TimeoutException) as e:
# Security Enhancement: Do not retry client errors (4xx) except 429 (Too Many Requests).
# Retrying 4xx errors is inefficient and can trigger security alerts or rate limits.
if isinstance(e, httpx.HTTPStatusError):
code = e.response.status_code
if 400 <= code < 500 and code != 429:
if hasattr(e, "response") and e.response is not None:
Comment on lines +603 to +608
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

This change introduces new behavior (fail-fast on non-429 4xx). There are existing tests for _retry_request logging/redaction, but none that assert we don’t retry on 4xx (e.g., mock_func.call_count == 1 with max_retries>1 for a 401/400) and that we still retry on 429. Adding a focused test would prevent regressions in retry semantics.

Copilot generated this review using guidance from repository custom instructions.
log.debug(
f"Response content: {sanitize_for_log(e.response.text)}"
)
Comment on lines +606 to +611
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

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

In the HTTPStatusError branch, e.response.status_code is accessed before the hasattr(e, "response")/e.response is not None guard. If you want this guard, it needs to wrap the status_code access too; otherwise the guard is misleading. Consider either removing the guard entirely (and rely on HTTPStatusError always having a response) or moving it before reading status_code.

Suggested change
code = e.response.status_code
if 400 <= code < 500 and code != 429:
if hasattr(e, "response") and e.response is not None:
log.debug(
f"Response content: {sanitize_for_log(e.response.text)}"
)
# Ensure the response exists before accessing status_code or text.
# While httpx.HTTPStatusError is expected to always have a response,
# this guard keeps the code robust against library or usage changes.
if not hasattr(e, "response") or e.response is None:
raise
code = e.response.status_code
if 400 <= code < 500 and code != 429:
log.debug(
f"Response content: {sanitize_for_log(e.response.text)}"
)

Copilot uses AI. Check for mistakes.
raise

if attempt == max_retries - 1:
if hasattr(e, "response") and e.response is not None:
log.debug(f"Response content: {sanitize_for_log(e.response.text)}")
Expand Down Expand Up @@ -1005,7 +1016,7 @@
)
return str(pk)

# Check if it returned a list containing our group

Check notice

Code scanning / Pylintpython3 (reported by Codacy)

Use lazy % formatting in logging functions Note

Use lazy % formatting in logging functions
if isinstance(body, dict) and "groups" in body:
for grp in body["groups"]:
if grp.get("group") == name:
Expand Down
Loading