fix(security): thread safety, SSRF, and provider reachability (ADR-012 Wave 1) by d-oit · Pull Request #364 · d-oit/do-web-doc-resolver

d-oit · 2026-05-13T06:06:57Z

Summary

Wave 1 implementation of ADR-012 (Correctness & Safety Fixes) covering thread safety, SSRF hardening, and provider reachability.

Changes

Thread Safety (T1-T6)

CircuitBreakerRegistry: Add threading.RLock around breakers dict; fix falsy-threshold bug (threshold=0 no longer silently replaced)
RoutingMemory: Add threading.RLock around domain_stats; extract magic numbers to SCORE_BASE, RECENCY_DECAY_DAYS, SCORE_SCALE constants
providers_impl: Add _rate_limits_lock around rate limit tracking
utils: Add _session_lock and _cache_lock (RLock) around shared session and cache
semantic_cache: Add double-checked locking for singleton creation; atomic transaction for eviction
resolve: Monkey-patching preserved (needed until ADR-014 state.py); TODO marks the transition

SSRF Hardening (S1-S3)

Mistral browser: Validate URL with is_safe_url() before API call
is_url(): Reject ftp:// and ftps:// schemes (only http/https)
safeFetch(): Validate initial URL before fetch (not just redirects)

Provider Reachability (P1-P2)

Add LLMS_TXT, SERPER, DOCLING, OCR to resolve_direct() dispatch
Fix Profile.max_hops() returning None for unknown profiles (now returns 4)

Package.json Fixes (ADR-013 I6-I8)

typescript: ^6.0.3 → ^5.7.0
next: ^16.2.6 → ^15.3.0
Remove duplicate overrides key

Testing

162 Python unit tests pass
52 Rust CLI tests pass
Web typecheck clean, lint clean (0 errors)
ruff, black, cargo fmt, cargo clippy all pass

Quality Gate

RLock (not Lock) used for reentrant-safe cache access — _get_from_cache() calls _get_cache() which both hold the same lock; threading.Lock would deadlock
Conftest updated to use lock-safe .clear() methods
Tests updated for ftp/ftps rejection behavior

Follow-up

See plans/GOAP_FOLLOWUP.md for remaining Waves 2-6.

vercel · 2026-05-13T06:07:02Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
do-web-doc-resolover	Ready	Preview, Comment	May 13, 2026 7:21am

deepsource-io · 2026-05-13T06:07:35Z

DeepSource Code Review

We reviewed changes in 6c2f4ce...dd9e92f on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Updated (UTC)	Details
JavaScript	May 13, 2026 7:21a.m.	Review ↗
Python	May 13, 2026 7:21a.m.	Review ↗
Rust	May 13, 2026 7:21a.m.	Review ↗
Shell	May 13, 2026 7:21a.m.	Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

codacy-production · 2026-05-13T06:08:19Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 4 complexity · 7 duplication

Metric Results

Complexity 4

Duplication 7

View in Codacy

AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.

_{TIP This summary will be updated as you push new changes.}

deepsource-io · 2026-05-13T06:08:46Z

@@ -257,6 +267,9 @@ def resolve_with_firecrawl(url: str, max_chars: int = MAX_CHARS) -> ResolvedResu


 def resolve_with_mistral_browser(url: str, max_chars: int = MAX_CHARS) -> ResolvedResult | None:


`resolve_with_mistral_browser` has a cyclomatic complexity of 17 with "high" risk

A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.

deepsource-io · 2026-05-13T06:08:46Z


 def resolve_with_mistral_browser(url: str, max_chars: int = MAX_CHARS) -> ResolvedResult | None:
+    if not is_safe_url(url):
+        logger.warning(f"SSRF: blocked URL {url}")


Use lazy % formatting in logging functions

Formatting the message manually before passing it to a logging call does unnecessary work if logging is disabled. Consider using the logging module's built-in formatting features to avoid that.

codacy-production

Pull Request Overview

While the PR successfully introduces core security and reachability features in line with ADR-012, there are several implementation flaws that should prevent merging in its current state. Most critically, the shared SQLite connection in the Semantic Cache is not thread-safe and will likely cause runtime errors under the concurrent load of the resolver. Additionally, the SSRF validation implemented for Mistral was not applied to Jina or Firecrawl providers, leaving those vectors unprotected.\n\nThere is a consistent design mismatch where the code uses threading.Lock instead of the threading.RLock specified in the design documents (AUDIT.md and GOAP_FOLLOWUP.md) for the Circuit Breaker and Routing Memory components. This could lead to deadlocks if recursive calls occur. Finally, while Codacy reports the PR is up to standards, the complexity of routing_memory.py combined with zero test coverage and logic duplication makes it a high-risk area for regression.

About this PR

Inconsistent security posture: SSRF validation via is_safe_url was added to the Mistral provider but is missing from other providers (Jina, Firecrawl) that also fetch external URLs. These should be standardized using the _safe_request wrapper.
Systemic mismatch with ADR-012 requirements: The implementation uses threading.Lock across several registries (CircuitBreaker, RoutingMemory) whereas the design specifically requires threading.RLock. This inconsistency increases the risk of deadlocks during complex resolution cycles.

2 comments outside of the diff

scripts/circuit_breaker.py

_{line 17-20 🔴 HIGH RISK}
Potential race condition: if self.open_until is cleared by another thread after the check but before the assignment, this will raise AttributeError on line 21. Capture the reference once at the start.

scripts/providers_impl.py

_{line 61 🔴 HIGH RISK}
The SSRF validation check should be applied consistently across all providers that handle user-provided URLs. While resolve_with_mistral_browser correctly implements this, other functions like resolve_with_jina and resolve_with_firecrawl are missing it.

Test suggestions

Verify is_url() rejects ftp://, ftps://, and file:// schemes.\n- [ ] Verify CircuitBreakerRegistry.record_failure() correctly handles threshold=0.\n- [x] Verify safeFetch() throws an SSRF error if the initial URL provided is unsafe.\n- [ ] Verify concurrent access to RoutingMemory.record() does not cause state corruption.\n- [ ] Verify resolve_direct() correctly routes to the new OCR and DOCLING providers.\n- [ ] Unit tests for success rate and recency calculation in scripts/routing_memory.py

_{TIP Improve review quality by adding custom instructions}
_{TIP How was this review? Give us feedback}

codacy-production · 2026-05-13T06:09:52Z

    def __init__(self):
        # domain -> provider -> stats
        self.domain_stats = defaultdict(lambda: defaultdict(lambda: dict(DEFAULT_PROVIDER_STATS)))
+        self._lock = threading.Lock()


_{🟡 MEDIUM RISK}

Implementation divergence: PR description specifies threading.RLock for domain_stats thread safety, but threading.Lock was used.

codacy-production · 2026-05-13T06:09:53Z

+            else:
+                stats["failure"] += 1

    def get_domain_stats(self, provider: str, domain: str) -> dict | None:


_{🟡 MEDIUM RISK}

Suggestion: This method duplicates the logic found in _get_domain_stats_unlocked. You should refactor it to call the internal method while holding the lock to ensure consistency.\n\nThis might be a simple fix:\nsuggestion\n def get_domain_stats(self, provider: str, domain: str) -> dict | None:\n with self._lock:\n return self._get_domain_stats_unlocked(provider, domain)\n

See Clone in Codacy
See Complexity in Codacy

- HIGH: SemanticCache SQLite thread safety — check_same_thread=False + _conn_lock - MEDIUM: CircuitBreakerRegistry Lock→RLock (recursive safe) - MEDIUM: RoutingMemory Lock→RLock + deduplicated get_domain_stats - MEDIUM: Add SSRF is_safe_url() checks to Jina and Firecrawl providers - LOW: Broad except handlers in Mistral browser -> logged exceptions

deepsource-io · 2026-05-13T06:43:17Z

-            self._conn = None
+            with self._conn_lock:
+                self._conn.close()
+                self._conn = None


Attribute '_conn' defined outside __init__

Defining an instance attribute outside __init__ affects the readability of code. It is expected to find all the attributes an instance may have by reading its __init__ method. If there is a need to initialize attribute via sub-initialization methods, it is recommended to assign attributes to None in the init then call the sub-initialization methods.

Add threading locks to all 6 shared mutable state locations with threading.RLock for reentrant-safe cache access. SSRF hardening: reject ftp/ftps in is_url(), validate Mistral browser URLs, validate initial URL in web safeFetch(). Fix resolve_direct() dispatch for LLMS_TXT, SERPER, DOCLING, OCR. Fix Profile.max_hops() returning None for unknown profiles. Fix web/package.json non-existent version specifiers. Update conftest to use lock-safe clear methods. Update tests for new is_url() ftp rejection. All quality gate checks verified: 162 Python tests, 52 Rust tests, ruff, black, cargo fmt, clippy, web lint pass.

Updated next@15.3.0 and typescript@5.7.0 version specifiers in package.json need matching lockfile entries.

S3 SSRF fix added validateUrlForFetchAsync at safeFetch() entry, requiring one additional mock in the Jina proxy URL test.

- HIGH: SemanticCache SQLite thread safety — check_same_thread=False + _conn_lock - MEDIUM: CircuitBreakerRegistry Lock→RLock (recursive safe) - MEDIUM: RoutingMemory Lock→RLock + deduplicated get_domain_stats - MEDIUM: Add SSRF is_safe_url() checks to Jina and Firecrawl providers - LOW: Broad except handlers in Mistral browser -> logged exceptions

Capture self.open_until once at function entry to avoid a TOCTOU (time-of-check-time-of-use) race between the None guard on line 17 and the attribute read on line 20.

Use logger.warning('SSRF blocked: %%s', url) instead of f-string to avoid unnecessary formatting when logging disabled.

- HIGH: SemanticCache SQLite thread safety — check_same_thread=False + _conn_lock - MEDIUM: CircuitBreakerRegistry Lock→RLock (recursive safe) - MEDIUM: RoutingMemory Lock→RLock + deduplicated get_domain_stats - MEDIUM: Add SSRF is_safe_url() checks to Jina and Firecrawl providers - LOW: Broad except handlers in Mistral browser -> logged exceptions

vercel Bot deployed to Preview May 13, 2026 06:07 View deployment

deepsource-io Bot reviewed May 13, 2026

View reviewed changes

codacy-production Bot reviewed May 13, 2026

View reviewed changes

vercel Bot deployed to Preview May 13, 2026 06:15 View deployment

vercel Bot deployed to Preview May 13, 2026 06:26 View deployment

vercel Bot deployed to Preview May 13, 2026 06:43 View deployment

deepsource-io Bot reviewed May 13, 2026

View reviewed changes

vercel Bot deployed to Preview May 13, 2026 06:44 View deployment

vercel Bot deployed to Preview May 13, 2026 06:54 View deployment

vercel Bot deployed to Preview May 13, 2026 07:06 View deployment

do-ops885 added 8 commits May 13, 2026 09:20

fix: regenerate package-lock.json after version specifier changes

06e433e

Updated next@15.3.0 and typescript@5.7.0 version specifiers in package.json need matching lockfile entries.

fix: add third validation mock for safeFetch() initial URL check

bdae029

S3 SSRF fix added validateUrlForFetchAsync at safeFetch() entry, requiring one additional mock in the Jina proxy URL test.

fix: eliminate TOCTOU race in CircuitBreakerState.is_open()

dc26d92

Capture self.open_until once at function entry to avoid a TOCTOU (time-of-check-time-of-use) race between the None guard on line 17 and the attribute read on line 20.

docs: update GOAP_FOLLOWUP with TOCTOU race fix

f604ddf

style: black-format semantic_cache.py after SQLite changes

21db866

style: use lazy % formatting in SSRF logging (DeepSource)

b795c70

Use logger.warning('SSRF blocked: %%s', url) instead of f-string to avoid unnecessary formatting when logging disabled.

d-oit force-pushed the fix/adr-012-correctness-and-safety branch from 1664dbc to b795c70 Compare May 13, 2026 07:20

d-oit enabled auto-merge May 13, 2026 07:20

docs: mark ADR-012 Wave 1 as merged

dd9e92f

vercel Bot deployed to Preview May 13, 2026 07:21 View deployment

d-oit disabled auto-merge May 13, 2026 07:28

d-oit merged commit b4dd390 into main May 13, 2026
35 of 37 checks passed

d-oit deleted the fix/adr-012-correctness-and-safety branch May 13, 2026 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): thread safety, SSRF, and provider reachability (ADR-012 Wave 1)#364

fix(security): thread safety, SSRF, and provider reachability (ADR-012 Wave 1)#364
d-oit merged 9 commits into
mainfrom
fix/adr-012-correctness-and-safety

d-oit commented May 13, 2026

Uh oh!

vercel Bot commented May 13, 2026 •

edited

Loading

Uh oh!

deepsource-io Bot commented May 13, 2026 •

edited

Loading

Uh oh!

codacy-production Bot commented May 13, 2026 •

edited

Loading

Uh oh!

deepsource-io Bot May 13, 2026

Uh oh!

deepsource-io Bot May 13, 2026

Uh oh!

codacy-production Bot left a comment

Uh oh!

Uh oh!

codacy-production Bot May 13, 2026

Uh oh!

Uh oh!

codacy-production Bot May 13, 2026

Uh oh!

Uh oh!

Uh oh!

deepsource-io Bot May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -257,6 +267,9 @@ def resolve_with_firecrawl(url: str, max_chars: int = MAX_CHARS) -> ResolvedResu


		def resolve_with_mistral_browser(url: str, max_chars: int = MAX_CHARS) -> ResolvedResult \| None:

Conversation

d-oit commented May 13, 2026

Summary

Changes

Thread Safety (T1-T6)

SSRF Hardening (S1-S3)

Provider Reachability (P1-P2)

Package.json Fixes (ADR-013 I6-I8)

Testing

Quality Gate

Follow-up

Uh oh!

vercel Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepsource-io Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

codacy-production Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Up to standards ✅

Uh oh!

deepsource-io Bot May 13, 2026

Choose a reason for hiding this comment

`resolve_with_mistral_browser` has a cyclomatic complexity of 17 with "high" risk

Uh oh!

deepsource-io Bot May 13, 2026

Choose a reason for hiding this comment

Use lazy % formatting in logging functions

Uh oh!

codacy-production Bot left a comment

Choose a reason for hiding this comment

Pull Request Overview

About this PR

Test suggestions

Uh oh!

Uh oh!

codacy-production Bot May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codacy-production Bot May 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

deepsource-io Bot May 13, 2026

Choose a reason for hiding this comment

Attribute '_conn' defined outside __init__

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vercel Bot commented May 13, 2026 •

edited

Loading

deepsource-io Bot commented May 13, 2026 •

edited

Loading

codacy-production Bot commented May 13, 2026 •

edited

Loading

Attribute '_conn' defined outside init