Redis resilience, fail-open pipeline, configurable country-check logging, non-blocking cloud-IP refresh#39
Merged
Merged
Conversation
A Redis-backed check (notably the IP-ban lookup) raises GuardRedisError when Redis is unreachable. The pipeline caught it generically and, under the default fail_secure=True, returned HTTP 500 - so a Redis outage took down every request that reached the IP-security check, even though the rate-limit check already degrades to in-memory on the same error. Treat a GuardRedisError as a dependency outage, not a security verdict: add redis_fail_open (default True) and, when set, skip the failing check and continue instead of honoring fail_secure. Set redis_fail_open=False to keep the previous strict behavior. Sync mirror updated identically.
…body Penetration detection always called request.body(), which buffers the entire payload in memory - and under a BaseHTTPMiddleware-based proxy that also defeats request streaming, since the body is cached before it reaches the downstream handler. There was no way to keep the cheap URL/query/header scanning while opting out of body scanning. Add SecurityConfig.scan_request_body (default True, preserving current behavior). When False, detect_penetration_attempt returns before reading the body, so request.body() is never called. The URL, query params and headers are still scanned. Sync mirror updated identically.
RedisManager called Redis.from_url with no socket timeouts, so a partitioned or black-holed Redis blocked every request that touched it indefinitely; there was also no retry on transient connection failures. Add redis_socket_connect_timeout, redis_socket_timeout, redis_health_check_interval, redis_max_connections and redis_retries to SecurityConfig and pass them through from_url via a _connection_kwargs helper. Values encoded in redis_url query params still take precedence (redis-py applies them last). Timeouts default to non-None so the failure mode is bounded out of the box. Sync mirror updated identically.
_log_country_check_result logged whitelisted / not-affected verdicts at
INFO straight to the root logger via bare logging.info, so the per-request
country chatter was unreachable by named-logger levels and by
log_suspicious_level / log_request_level.
Route it through logging.getLogger("guard_core") and add
SecurityConfig.log_country_check_level (default "INFO", preserving prior
behaviour; set None to silence). Continues the v3.1.0 noise reduction:
blocked-country hits stay WARNING, no-rules / no-geolocation stay DEBUG.
Sync mirror updated identically.
…are root logging
The per-component 'Potential attack detected from …' line in
_check_request_component logged at a hardcoded WARNING on the root logger,
bypassing config and duplicating the authoritative SuspiciousActivityCheck
log. Thread log_suspicious_level down from detect_penetration_attempt and
route it through the guard_core logger; log_suspicious_level=None silences it.
Also move the remaining bare root-logger calls in utils.py / cloud_handler.py
(client-IP errors, enhanced-detection fallback, spoof log_fn, cloud IP-range
fetch errors) onto named guard_core loggers, so getLogger('guard_core').
setLevel(...) governs the whole library. No behavioural change at default
levels. Sync mirror updated identically.
Tests updated to assert against the named loggers (patch logging.Logger.*,
caplog logger='guard_core').
CloudIpRefreshCheck awaited the multi-provider IP-range fetch inline on every request once the refresh interval elapsed. A slow or unreachable fetch (e.g. no egress to the Azure range endpoint) froze the caller for ~10s, inflating the latency of every in-flight request on the worker; the post-fetch timestamp update also let concurrent requests stampede duplicate fetches. CloudManager.schedule_refresh() now runs the refresh as a lock-guarded, single-flight background task. The check bumps the debounce timestamp up front and fires-and-forgets, so the request path never awaits a network fetch. The lock keeps the single-flight gate correct under multi-threaded (sync) request handling, the in-flight flag is reset if the task fails to start, and failures are logged with tracebacks.
feat(redis): expose connection timeouts, pool cap, and retry/backoff
…fresh fix(cloud): non-blocking cloud-provider IP range refresh
…eck-log-level Configurable + named-logger guard_core logging (country, detection, noise)
feat(detection): add scan_request_body to skip buffering the request body
Sync pipeline mirrors async's GuardRedisError fail-open handling but lacked matching test coverage, letting the two drift silently. Claude-Session: https://claude.ai/code/session_01XQXLGmGHR3Z25G5UP4VtxU
fix(pipeline): fail open on Redis outage instead of 500-ing the request
Resolves conflicts across pyproject.toml, CHANGELOG.md, cloud_handler.py (async+sync), and utils.py (async+sync). Drops scan_request_body (and its dedicated tests) in favor of upstream's detection_scan_body, which is a strict superset (adds per-route override, decorator, and nested/form/ multipart exclusion). Threads the fork's log_suspicious_level feature through upstream's new body-scan helper split (_scan_blob_body, _scan_json_value, _scan_form_body, _scan_multipart_body) to keep both features working together; the raw merge had silently dropped log_level out of scope in _scan_blob_body (NameError on any large/blob body scan). Also fixes two long-standing async->sync generator gaps surfaced while regenerating the (already-drifted) sync mirror: `from guard_core import utils` wasn't being rewritten to the sync module, and an `await task` inside `contextlib.suppress` left a bare no-op statement after await-stripping.
github-actions
Bot
added
documentation
…try-check-level fields security-config.md bills itself as the complete field reference but was missing redis_socket_connect_timeout, redis_socket_timeout, redis_health_check_interval, redis_max_connections, redis_retries, redis_fail_open, and log_country_check_level. Claude-Session: https://claude.ai/code/session_01XQXLGmGHR3Z25G5UP4VtxU
rennf93
self-requested a review
July 6, 2026 13:02
…strumentation SecurityEvent/SecurityMetric validate per request and EventBatch re-validates every buffered event on flush; a host app running logfire.instrument_pydantic() therefore exports a span per security event — hundreds of thousands a day under real traffic. Set the logfire plugin to record="off" on those models (settings are read at validator build, so force a rebuild) so instrumented apps keep spans for their own models only. Claude-Session: https://claude.ai/code/session_01K2KjMxyKNaCxrfoxMSAcXx
…nc refresh tests The sync schedule_refresh set _refresh_in_flight = True after starting the background thread. A fast thread's finally-block could set it False before that assignment ran, which then clobbered it back to True, permanently wedging cloud IP refresh off. Set the flag before start(), reverting on start failure. The sync nonblocking-refresh test file had zero test functions (the generator strips async-only tests); add hand-maintained sync tests including a regression for the ordering, and register the file as HAND_MAINTAINED. Also fixes the sync fixture's threading.Thread.done() -> is_alive().
…itative redis_fail_open defaulted to True, which silently overrode an explicit fail_secure=True for Redis errors specifically -- a user opting into the strict blocking posture got carved out for Redis outages without knowing. Default to False so fail_secure is the single source of truth for every check failure; opt into Redis-specific fail-open explicitly.
…d agent-instrumentation opt-out Closes coverage gaps in David's new code and our race fix: schedule_refresh's create-task/thread-start failure path (flag reset), _run_refresh's swallow-and-log path, the muted fail-open branch in the pipeline, and the no-agent-installed no-op in _mute_pydantic_plugin_instrumentation.
…plexity rank B rennf93#39 pushed both over the xenon max-absolute-B gate (C, cc 11): the fail-open branch in execute and the log-level ternary in detect_penetration_attempt. Extract SecurityCheckPipeline._handle_check_error / _log_extra and a _resolve_log_level helper; behavior unchanged, both back to rank B.
…ddleware protocol InMemoryCloudIpStore dropped its ttl argument, so non-Redis deployments fetched provider ranges exactly once per process and every later periodic refresh silently no-oped on the eternal cache. The store now records a monotonic expiry per provider and refetches after it passes. The periodic check also called the cloud_handler singleton directly, bypassing adapter overrides of refresh_cloud_ip_ranges; schedule_refresh now takes the refresh callable so the middleware protocol method stays on the periodic path, and the debounce timestamp is restored when scheduling fails so the next request retries instead of waiting a full interval.
… cannot rebuild The pydantic plugin opt-out only guarded the guard_agent import; a model_config mutation or forced rebuild raising anything else propagated out of 'import guard_core'. The mutation loop is now guarded and logs a warning, leaving instrumentation on rather than taking the app down.
…il_open doc default A socket timeout of 0 puts the socket in non-blocking mode (every Redis call fails instantly) rather than disabling the timeout, so both fields now validate gt=0. The security-config reference still documented redis_fail_open as defaulting to True with inverted guidance; the row now matches the False default.
…evel defaults; share level dispatch redis.exceptions ConnectionError/TimeoutError shadowed the builtins module-wide; they are now imported as RedisConnectionError and RedisTimeoutError. The nine scan helpers carried dead log_level defaults that could drift from the config-resolved value — the parameter is now required, as every production call site already threads it. _log_country_check_result now uses the shared _log_at_level dispatch and requires its config instead of a defensive getattr fallback.
…esilience settings cloud-providers.md still showed the pre-rennf93#39 inline refresh on the request path; it now documents schedule_refresh's single-flight background task, the debounce restore, the protocol-routed adapter override, and the TTL-honoring in-memory store. redis.md gains the six resilience fields, the connection kwargs they drive, the incr() retry caveat, and the fail_secure/redis_fail_open pipeline interaction.
Owner
|
Merged — thank you for this contribution, David! 🎉 A few updates on what landed alongside your commits (we pushed 13 maintainer commits to your branch before merging; your authorship is intact on all of your work): Fixes on top of your PR
Your logfire span-storm: your The payload retry-storm ( Release: this ships in guard-core 3.5.0 (imminent) — the changelog consolidates your Redis resilience fields, |
rennf93
added a commit
that referenced
this pull request
Jul 15, 2026
…rumentation work into the 3.5.0 section The six Redis resilience fields, redis_fail_open semantics, log_country_check_level and the log-level routing, the non-blocking single-flight cloud refresh (with the TTL store and protocol routing), and the pydantic-instrumentation opt-out now live in the single 3.5.0 release section, mirrored in docs/release-notes.md, with contributor credit. Supersedes the back-dated v3.1.2 block on the PR branch, removed at rebase.
rennf93
added a commit
that referenced
this pull request
Jul 15, 2026
This was referenced Jul 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four independent fixes/features developed against an earlier base, now rebased on top of
master(3.4.0) with conflicts resolved:SecurityConfigexposes connection timeouts, pool cap, and retry/backoff for the Redis handler._log_country_check_resultnow routes through the namedguard_corelogger (was bare rootlogging.info) and honors a newlog_country_check_levelconfig; penetration-detection hits now honor the existinglog_suspicious_levelinstead of a hardcoded root-loggerWARNING.CloudManager.schedule_refresh()runs the multi-provider IP-range fetch as a lock-guarded, single-flight background task instead of blocking the request path for the ~10s fetch.A
scan_request_bodytoggle from this branch's earlier history is dropped — it's fully superseded bydetection_scan_body(already onmaster), which covers the same case plus per-route override, decorator, and nested/form/multipart exclusion.Also fixes two async→sync generator gaps hit while re-running
make syncagainstmaster's currentunasync.py:from guard_core import utilswasn't rewritten to the sync module in the generated mirror, andawait taskinsidecontextlib.suppressleft a bare no-op statement after await-stripping.Test plan
pytest -m "not integration"— 3870 passed, 4 skippedruff check— cleanmypy guard_core— cleanpython scripts/unasync.py --check— sync mirror in parityhttps://claude.ai/code/session_01XQXLGmGHR3Z25G5UP4VtxU