Feat/websearch - #253
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional live web search: provider interface, Staan provider, content fetcher, WebSearchService, pipeline and router integration to include web results in context and sources, Hydra/env configuration, docs, tests, and lxml dependency. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Pipeline as RagPipeline
participant WebSvc as WebSearchService
participant Provider as StaanProvider
participant Fetcher as ContentFetcher
participant LLM as LLM
Client->>Pipeline: chat_completion(partition=None, payload{websearch:true})
alt concurrent retrieval
par docs
Pipeline->>Pipeline: retrieve documents (optional)
and web
Pipeline->>WebSvc: search(query)
WebSvc->>Provider: search(query)
Provider-->>WebSvc: [WebResult...]
WebSvc->>Fetcher: enrich(results)
Fetcher-->>WebSvc: [WebResult with content...]
WebSvc-->>Pipeline: web_results
end
end
Pipeline->>Pipeline: format_web_context(web_results)
Pipeline->>LLM: prompt(with docs + web context)
LLM-->>Pipeline: llm_response
Pipeline->>Client: llm_output, docs, web_results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
paultranvan
force-pushed
the
feat/websearch
branch
6 times, most recently
from
February 24, 2026 18:18
9be79c6 to
b3919fe
Compare
paultranvan
marked this pull request as ready for review
February 25, 2026 10:32
paultranvan
force-pushed
the
feat/websearch
branch
from
February 25, 2026 10:42
57e3919 to
a04becc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
.env.example (1)
57-62: Document all websearch runtime knobs in.env.example.Lines 57-62 expose only the core variables; adding the
WEBSEARCH_FETCH_*options here would reduce configuration surprises in ops/debug sessions.Suggested doc-only addition
# Web Search # WEBSEARCH_API_TOKEN= # Web search provider API token. If unset, web search is silently disabled. # WEBSEARCH_BASE_URL=https://api.staan.ai/search/web # Web search provider endpoint # WEBSEARCH_TOP_K=5 # Number of web results to include (default: 5) # WEBSEARCH_LANG=fr-FR # Search language/market (default: fr-FR) +# WEBSEARCH_FETCH_CONTENT=true # Fetch full page content for top results +# WEBSEARCH_FETCH_MAX_RESULTS=3 # Max number of URLs to fetch +# WEBSEARCH_FETCH_TIMEOUT=1.0 # Per-URL fetch timeout in seconds +# WEBSEARCH_FETCH_MAX_TOKENS=500 # Max extracted tokens per page🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 57 - 62, Add the missing websearch runtime knobs to the .env.example by documenting the fetch-related variables alongside the existing WEBSEARCH_* entries: include commented lines for WEBSEARCH_FETCH_TIMEOUT (default in ms, e.g. 5000) , WEBSEARCH_FETCH_RETRIES (default attempts, e.g. 2), and WEBSEARCH_FETCH_CONCURRENCY (parallel requests, e.g. 3) with short descriptions and example defaults, keeping the same comment style as WEBSEARCH_API_TOKEN/WEBSEARCH_BASE_URL/WEBSEARCH_TOP_K/WEBSEARCH_LANG so ops can discover and tune these options at runtime.openrag/components/websearch/test_content_fetcher.py (1)
18-94: Good test coverage across fetch paths.The tests effectively cover HTML extraction, timeout handling, HTTP errors, boilerplate stripping, and non-HTML content filtering using
httpx.MockTransport.Note: the mock handlers (e.g., lines 23–24) don't set an explicit
content-typeheader, relying on httpx's default behavior wheretext=sets"text/plain; charset=utf-8". This works because_fetch_singleaccepts bothtext/htmlandtext/plain, but it means these tests don't exercise thetext/htmlcontent-type branch. Consider settingheaders={"content-type": "text/html"}on at least the HTML-based responses for more precise coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/test_content_fetcher.py` around lines 18 - 94, Update the MockTransport responses in the HTML-based tests (e.g., TestFetchSingleURL.test_extracts_text_from_html, test_strips_boilerplate_html) to include an explicit "content-type": "text/html" header so fetcher._fetch_single exercises the text/html branch; modify the mock_handler returns to pass headers={"content-type": "text/html"} (and likewise for any other HTML fixtures) while leaving the PDF test using "application/pdf" unchanged.openrag/components/websearch/__init__.py (1)
1-4: Consider using absolute imports for consistency with the rest of the package.Other files in this PR (e.g.,
service.py,staan.py) use absolute imports likefrom components.websearch.base import .... Using relative imports here creates an inconsistency. As per coding guidelines: "Use absolute imports from theopenrag/directory as the Python path root, avoiding relative imports across packages."While relative imports in
__init__.pyfor same-package re-exports are common Python practice, switching to absolute imports would align with the project convention.Suggested change
-from .base import BaseWebSearchProvider as BaseWebSearchProvider -from .base import WebResult as WebResult -from .content_fetcher import ContentFetcher as ContentFetcher -from .service import WebSearchService as WebSearchService +from components.websearch.base import BaseWebSearchProvider as BaseWebSearchProvider +from components.websearch.base import WebResult as WebResult +from components.websearch.content_fetcher import ContentFetcher as ContentFetcher +from components.websearch.service import WebSearchService as WebSearchService🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/__init__.py` around lines 1 - 4, The __init__.py currently uses relative imports for re-exporting BaseWebSearchProvider, WebResult, ContentFetcher, and WebSearchService; change these to absolute imports using the project root package path (e.g., import from components.websearch.base, components.websearch.content_fetcher, components.websearch.service) so the re-exports for BaseWebSearchProvider, WebResult, ContentFetcher, and WebSearchService match the absolute-import style used elsewhere (like in service.py and staan.py) and maintain consistency across the package.openrag/components/websearch/providers/staan.py (1)
3-5:loggeris imported but never used.
get_logger()is called andloggeris assigned but never referenced in this module. Either remove it or add debug logging (e.g., log the number of results returned).Option A: Remove unused logger
import httpx from components.websearch.base import BaseWebSearchProvider, WebResult -from utils.logger import get_logger - -logger = get_logger()Option B: Add debug logging
results = data if isinstance(data, list) else data.get("web", {}).get("results", []) + logger.debug("Staan search completed", query=query, n_results=len(results)) return [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/providers/staan.py` around lines 3 - 5, The module imports get_logger and assigns logger but never uses it; either remove the unused import/assignment (delete get_logger import and the logger = get_logger() line) or add meaningful debug logging where search results are processed (e.g., inside the function that returns results — reference the function handling returned results in this file) using logger.debug to log useful info such as the number of results returned and any notable metadata; ensure get_logger() is only kept if logger.debug calls are added, otherwise remove both the import and the logger variable.openrag/components/websearch/service.py (1)
17-33: Solid graceful degradation pattern.The search method handles all failure modes (no provider, empty results, exceptions) without propagating errors to the caller. This is appropriate for a non-critical augmentation feature.
One minor improvement: the
except Exceptionblock only logsstr(e), which loses the traceback. Consider using Loguru's exception capture for easier debugging:Optional improvement
except Exception as e: - logger.warning("Web search failed, continuing without web context", error=str(e)) + logger.opt(exception=True).warning("Web search failed, continuing without web context") return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/service.py` around lines 17 - 33, The except block in async def search(self, query: str) catches exceptions but logs only str(e), losing traceback; update the handler in search (referencing self.provider, self.content_fetcher) to log the full exception/traceback using Loguru’s exception capture (e.g., use logger.exception(...) or logger.opt(exception=True).exception(...)) instead of logger.warning, and keep the same return [] behavior so callers still get graceful degradation.openrag/components/utils.py (2)
123-154: No aggregate token budget unlikeformat_context.
format_context(line 96) caps total tokens viamax_context_tokens, butformat_web_contexthas no such limit. WithContentFetchertruncating individual pages to ~500 tokens andtop_k=5, the practical risk is moderate (~2500 tokens). However, if these defaults change or more results are returned, the context could silently exceed the model's budget.Consider adding a
max_context_tokensparameter (mirroringformat_context) or at least documenting why it's intentionally omitted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/utils.py` around lines 123 - 154, format_web_context currently lacks an overall token budget and can exceed model limits when many or large web results are passed; update format_web_context to accept a max_context_tokens parameter (mirroring format_context) and enforce an aggregate token cap when building parts: track cumulative tokens (using the same tokenizer/utility used by format_context), skip or truncate subsequent web_result bodies when adding would exceed max_context_tokens, and still return the formatted string and source_numbers; keep start_index behavior and mirror truncation logic used by ContentFetcher/top_k to maintain consistent budgeting.
123-126: Use a concrete type hint instead of barelist.
web_results: listloses theWebResultcontract. Typing it explicitly improves IDE support and catches misuse early.Proposed fix
+from components.websearch.base import WebResult + def format_web_context( - web_results: list, + web_results: list[WebResult], start_index: int = 1, ) -> tuple[str, list[int]]:If you want to avoid a circular/heavy import at module level, the existing lazy import pattern is fine — but consider using
TYPE_CHECKING:from __future__ import annotations from typing import TYPE_CHECKING if TYPE_CHECKING: from components.websearch.base import WebResult🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/utils.py` around lines 123 - 126, Change the bare list annotation on format_web_context to a concrete list[WebResult] so callers and IDEs know the expected type: add from __future__ import annotations at top if not present, use typing.TYPE_CHECKING and inside that block import WebResult from components.websearch.base (i.e., reference the WebResult symbol), then update the signature def format_web_context(web_results: list[WebResult], start_index: int = 1) -> tuple[str, list[int]] to use the concrete type while avoiding runtime imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/pipeline.py`:
- Around line 257-258: payload.get("metadata", {}) can return None when the
payload explicitly contains null, causing metadata.get(...) to raise; change the
metadata handling to coerce to a dict and guard its type before calling .get.
Replace the current two lines with logic like: metadata =
payload.get("metadata") or {}; if not isinstance(metadata, dict): metadata = {};
then read use_websearch = metadata.get("websearch", False) (or
bool(metadata.get("websearch", False"))) so metadata and use_websearch accesses
won't raise. Ensure you update the code paths that reference the metadata
variable in the same scope (e.g., surrounding function in pipeline.py) to use
this guarded metadata.
In `@openrag/components/websearch/content_fetcher.py`:
- Around line 45-50: The _fetch_single function currently allows fetching
arbitrary URLs and uses requests with TLS verification disabled; update it to
validate and normalize the input URL (ensure scheme is http or https), resolve
and check the final IP/hostname against private/loopback/multicast ranges and
deny requests to internal addresses (perform DNS resolution and optional
follow-redirect safety checks inside _fetch_single before calling client.get),
remove any verify=False usage so TLS certificate verification is enabled for the
httpx client, and replace relative imports with absolute ones (e.g., change from
components... / from utils... to from openrag.components... and from
openrag.utils...) so imports conform to the absolute import policy.
In `@openrag/components/websearch/providers/__init__.py`:
- Line 1: Replace the relative re-export of StaanProvider with an absolute
import from the package root: update the import in __init__.py to import
StaanProvider from the full module path (the module that defines the class, e.g.
openrag.components.websearch.providers.staan) and re-export it as StaanProvider
so codebase conventions are followed and the symbol StaanProvider is still
available.
In `@openrag/routers/openai.py`:
- Around line 122-127: The dict merge currently uses {"source_type": "document",
"file_url": encoded_url, "chunk_url": str(request.url_for("get_extract",
extract_id=doc_metadata["_id"])), **doc_metadata} which allows doc_metadata to
override source_type; change the merge order or remove source_type from
doc_metadata so "document" is authoritative—for example, place **doc_metadata
first and then set "source_type": "document" after, or explicitly delete/pop the
"source_type" key from doc_metadata before merging; update the code around the
dict construction that references doc_metadata, encoded_url, and
request.url_for("get_extract", extract_id=doc_metadata["_id"]).
- Around line 129-139: The loop that appends provider-controlled web results (in
the web_results handling in openrag/routers/openai.py) currently forwards
result.url and display fields directly; validate and sanitize before returning:
parse result.url (e.g., with urllib.parse.urlparse) and only accept schemes
"http" or "https" (skip or omit entries with other schemes or missing netloc),
normalize/percent-decode the URL to remove control characters, and use sanitized
forms for display_url/title/snippet by passing them through the existing
sanitize_text function (or an escape routine) to prevent XSS/link-injection;
update the links append logic to only include entries that pass URL
scheme/netloc validation and use the sanitized fields.
---
Nitpick comments:
In @.env.example:
- Around line 57-62: Add the missing websearch runtime knobs to the .env.example
by documenting the fetch-related variables alongside the existing WEBSEARCH_*
entries: include commented lines for WEBSEARCH_FETCH_TIMEOUT (default in ms,
e.g. 5000) , WEBSEARCH_FETCH_RETRIES (default attempts, e.g. 2), and
WEBSEARCH_FETCH_CONCURRENCY (parallel requests, e.g. 3) with short descriptions
and example defaults, keeping the same comment style as
WEBSEARCH_API_TOKEN/WEBSEARCH_BASE_URL/WEBSEARCH_TOP_K/WEBSEARCH_LANG so ops can
discover and tune these options at runtime.
In `@openrag/components/utils.py`:
- Around line 123-154: format_web_context currently lacks an overall token
budget and can exceed model limits when many or large web results are passed;
update format_web_context to accept a max_context_tokens parameter (mirroring
format_context) and enforce an aggregate token cap when building parts: track
cumulative tokens (using the same tokenizer/utility used by format_context),
skip or truncate subsequent web_result bodies when adding would exceed
max_context_tokens, and still return the formatted string and source_numbers;
keep start_index behavior and mirror truncation logic used by
ContentFetcher/top_k to maintain consistent budgeting.
- Around line 123-126: Change the bare list annotation on format_web_context to
a concrete list[WebResult] so callers and IDEs know the expected type: add from
__future__ import annotations at top if not present, use typing.TYPE_CHECKING
and inside that block import WebResult from components.websearch.base (i.e.,
reference the WebResult symbol), then update the signature def
format_web_context(web_results: list[WebResult], start_index: int = 1) ->
tuple[str, list[int]] to use the concrete type while avoiding runtime imports.
In `@openrag/components/websearch/__init__.py`:
- Around line 1-4: The __init__.py currently uses relative imports for
re-exporting BaseWebSearchProvider, WebResult, ContentFetcher, and
WebSearchService; change these to absolute imports using the project root
package path (e.g., import from components.websearch.base,
components.websearch.content_fetcher, components.websearch.service) so the
re-exports for BaseWebSearchProvider, WebResult, ContentFetcher, and
WebSearchService match the absolute-import style used elsewhere (like in
service.py and staan.py) and maintain consistency across the package.
In `@openrag/components/websearch/providers/staan.py`:
- Around line 3-5: The module imports get_logger and assigns logger but never
uses it; either remove the unused import/assignment (delete get_logger import
and the logger = get_logger() line) or add meaningful debug logging where search
results are processed (e.g., inside the function that returns results —
reference the function handling returned results in this file) using
logger.debug to log useful info such as the number of results returned and any
notable metadata; ensure get_logger() is only kept if logger.debug calls are
added, otherwise remove both the import and the logger variable.
In `@openrag/components/websearch/service.py`:
- Around line 17-33: The except block in async def search(self, query: str)
catches exceptions but logs only str(e), losing traceback; update the handler in
search (referencing self.provider, self.content_fetcher) to log the full
exception/traceback using Loguru’s exception capture (e.g., use
logger.exception(...) or logger.opt(exception=True).exception(...)) instead of
logger.warning, and keep the same return [] behavior so callers still get
graceful degradation.
In `@openrag/components/websearch/test_content_fetcher.py`:
- Around line 18-94: Update the MockTransport responses in the HTML-based tests
(e.g., TestFetchSingleURL.test_extracts_text_from_html,
test_strips_boilerplate_html) to include an explicit "content-type": "text/html"
header so fetcher._fetch_single exercises the text/html branch; modify the
mock_handler returns to pass headers={"content-type": "text/html"} (and likewise
for any other HTML fixtures) while leaving the PDF test using "application/pdf"
unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.env.example.gitignore.hydra_config/config.yamlCLAUDE.mddocs/content/docs/documentation/API.mdxdocs/content/docs/documentation/env_vars.mddocs/content/docs/documentation/features_in_details.mdopenrag/components/pipeline.pyopenrag/components/utils.pyopenrag/components/websearch/__init__.pyopenrag/components/websearch/base.pyopenrag/components/websearch/content_fetcher.pyopenrag/components/websearch/providers/__init__.pyopenrag/components/websearch/providers/staan.pyopenrag/components/websearch/service.pyopenrag/components/websearch/test_content_fetcher.pyopenrag/routers/openai.pypyproject.tomltests/api_tests/test_openai_compat.py
Comment on lines
+45
to
+50
| async def _fetch_single(self, client: httpx.AsyncClient, url: str) -> str | None: | ||
| """Fetch a single URL and extract text. Returns None on any failure.""" | ||
| try: | ||
| response = await asyncio.wait_for( | ||
| client.get(url, follow_redirects=True), | ||
| timeout=self.timeout, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its line count
wc -l openrag/components/websearch/content_fetcher.pyRepository: linagora/openrag
Length of output: 111
🏁 Script executed:
# Read the relevant sections: lines 40-55 and around line 106
sed -n '40,55p' openrag/components/websearch/content_fetcher.pyRepository: linagora/openrag
Length of output: 763
🏁 Script executed:
# Check around line 106 for the AsyncClient initialization
sed -n '100,115p' openrag/components/websearch/content_fetcher.pyRepository: linagora/openrag
Length of output: 792
🏁 Script executed:
# Check imports and exception handling at the beginning of the file
head -30 openrag/components/websearch/content_fetcher.pyRepository: linagora/openrag
Length of output: 885
🏁 Script executed:
# Search for all occurrences of verify= in the file
rg 'verify\s*=' openrag/components/websearch/content_fetcher.py -nRepository: linagora/openrag
Length of output: 183
🏁 Script executed:
# Check if OpenRAGError is used for custom exceptions
rg 'class.*Error|raise' openrag/components/websearch/content_fetcher.py -nRepository: linagora/openrag
Length of output: 102
🏁 Script executed:
# Verify logger usage
rg 'logger|get_logger' openrag/components/websearch/content_fetcher.py -nRepository: linagora/openrag
Length of output: 352
🏁 Script executed:
# Check if other files in the codebase use .bind() vs keyword args pattern
rg 'logger\.(debug|info|warning|error).*\.bind\(' openrag/ -A 2 | head -20Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Also check how logger calls are typically made in other files
rg 'logger\.(debug|info).*=' openrag/ -A 1 | head -30Repository: linagora/openrag
Length of output: 1776
🏁 Script executed:
# Verify that imports ARE indeed relative and should be absolute per guidelines
cat -n openrag/components/websearch/content_fetcher.py | head -10Repository: linagora/openrag
Length of output: 320
🏁 Script executed:
# Check if other Python files in openrag/ use absolute imports
rg '^from (openrag\.|components\.|utils\.)' openrag/ -n | head -15Repository: linagora/openrag
Length of output: 1416
🏁 Script executed:
# Verify no URL validation exists before the fetch
sed -n '30,60p' openrag/components/websearch/content_fetcher.pyRepository: linagora/openrag
Length of output: 1327
Fix SSRF vulnerability and TLS verification; use absolute imports.
This component fetches arbitrary URLs without validation and disables TLS verification, creating critical SSRF and MITM risks. Additionally, imports violate the absolute import requirement.
Issues:
- Line 45-50:
_fetch_single()fetches any provided URL without checking if it targets internal/private endpoints. - Line 106:
verify=Falsedisables certificate validation on all requests. - Lines 5-7: Relative imports (
from components...,from utils...) should be absolute (from openrag.components...,from openrag.utils...).
Proposed hardening
+import ipaddress
+from urllib.parse import urlparse
@@
class ContentFetcher:
@@
+ def _is_safe_public_url(self, url: str) -> bool:
+ parsed = urlparse(url)
+ if parsed.scheme not in {"http", "https"} or not parsed.hostname:
+ return False
+ host = parsed.hostname
+ try:
+ ip = ipaddress.ip_address(host)
+ return ip.is_global
+ except ValueError:
+ # Hostname case: keep a minimal denylist; can be tightened with DNS/IP policy.
+ return host not in {"localhost"}
@@
async def _fetch_single(self, client: httpx.AsyncClient, url: str) -> str | None:
"""Fetch a single URL and extract text. Returns None on any failure."""
try:
+ if not self._is_safe_public_url(url):
+ logger.debug("Skipping unsafe URL", url=url)
+ return None
response = await asyncio.wait_for(
client.get(url, follow_redirects=True),
timeout=self.timeout,
)
@@
- async with httpx.AsyncClient(timeout=timeout, verify=False, headers={"User-Agent": _USER_AGENT}) as client:
+ async with httpx.AsyncClient(timeout=timeout, verify=True, headers={"User-Agent": _USER_AGENT}) as client:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/websearch/content_fetcher.py` around lines 45 - 50, The
_fetch_single function currently allows fetching arbitrary URLs and uses
requests with TLS verification disabled; update it to validate and normalize the
input URL (ensure scheme is http or https), resolve and check the final
IP/hostname against private/loopback/multicast ranges and deny requests to
internal addresses (perform DNS resolution and optional follow-redirect safety
checks inside _fetch_single before calling client.get), remove any verify=False
usage so TLS certificate verification is enabled for the httpx client, and
replace relative imports with absolute ones (e.g., change from components... /
from utils... to from openrag.components... and from openrag.utils...) so
imports conform to the absolute import policy.
Collaborator
Author
There was a problem hiding this comment.
SSRF mitigated with local check in 446405c
verify=False not relevant, we only fetch URL, no data is sent
relevant import not relevant
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the commit that supposedly fixed SSRF
git show 446405c0a4607d1c0587d9dc388c95a1db15df7d --stat
echo "---"
# Show the actual changes in content_fetcher.py from that commit
git show 446405c0a4607d1c0587d9dc388c95a1db15df7d -- openrag/components/websearch/content_fetcher.py
echo "---"
# Check the current state of the file to see imports and verify=False
cat openrag/components/websearch/content_fetcher.py
Length of output: 6802
@paultranvan Thank you for addressing the loopback check. However, there are still important security gaps:
1. SSRF mitigation is incomplete
The loopback check blocks localhost and 127.*, but SSRF attacks can target other internal ranges:
- Private IPv4:
10.0.0.0/8,172.16.0.0/12,192.168.0.0/16 - Link-local:
169.254.0.0/16 - IPv6 loopback:
::1 - Internal hostnames (e.g.,
internal-api,metadata.google.internal)
A compromised or malicious search provider could return URLs targeting cloud metadata services (http://169.254.169.254/), internal APIs, or other private endpoints.
2. verify=False is a security risk even for read-only fetches
When verify=False, an attacker performing a MITM attack can:
- Inject malicious or misleading content into the HTML response
- This poisoned content enters your RAG context and influences LLM answers to users
- In a RAG system, compromised fetched content directly affects response quality and trustworthiness
The fact that you're "only fetching" doesn't reduce the risk—the fetched content becomes authoritative input to your system.
3. Relative imports
Lines 6 and 8 use relative imports (from components..., from utils...) instead of absolute ones (from openrag.components..., from openrag.utils...). This can cause import resolution issues depending on how the module is executed.
I understand if you'd like to defer some of these, but wanted to clarify the security implications so you can make an informed decision.
🧠 Learnings used
Learnt from: Ahmath-Gadji
Repo: linagora/openrag PR: 245
File: openrag/routers/users.py:59-95
Timestamp: 2026-02-13T10:12:27.454Z
Learning: In Python code, when using a constant like DEFAULT_FILE_QUOTA to control global quota enforcement, the condition should disable quotas only when the value is strictly negative (DEFAULT_FILE_QUOTA < 0). A value of 0 should mean quotas are enabled with a limit of 0 files. This pattern should be reviewed in all modules that implement quota checks, e.g., any function that reads DEFAULT_FILE_QUOTA and makes a 'disable' decision. Specifically verify similar comparisons and ensure behavior is consistent across files (not assuming <= 0 disables quotas).
Collaborator
Author
There was a problem hiding this comment.
- SSRF : this commit add mitigation for most of the cases: caf17a6 Only Only hostname resolving to private IP is not handled, but DNS check is most complex and seems overkill here
- verify=False: the MITM attack require to position between a running openRAG and a website returned by the search provider to change it content dynamically. Possible but not highly plausible. Either way, let's make it configurable: 81e5db6
- not relevant
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the SSRF mitigation commit
git show caf17a6ff9760c2bf96c8db99ca5f38771fc22e7 --stat
echo "=== SSRF MITIGATION CHANGES ==="
git show caf17a6ff9760c2bf96c8db99ca5f38771fc22e7 -- openrag/components/websearch/content_fetcher.py
echo ""
echo "=== SSL VERIFICATION CONFIG COMMIT ==="
git show 81e5db608aeb0a738c9138d7e78d4e15114eeb7a --stat
echo ""
echo "=== SSL VERIFICATION CHANGES ==="
git show 81e5db608aeb0a738c9138d7e78d4e15114eeb7a -- openrag/components/websearch/content_fetcher.py
echo ""
echo "=== CURRENT STATE OF FILE ==="
cat openrag/components/websearch/content_fetcher.py
Length of output: 9661
@paultranvan Thank you for the updates! I've reviewed both commits and appreciate your pragmatic approach.
SSRF mitigation (commit caf17a6):
Your implementation is actually more comprehensive than you described. The ipaddress.ip_address(host).is_global check filters out:
- Loopback addresses (127.0.0.0/8, ::1) ✓
- Private IPv4 ranges (10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) ✓
- Link-local addresses (169.254.0.0/16) ✓
- Other non-global addresses ✓
This covers the private IP ranges I was concerned about. The only remaining gap is hostnames that resolve to private IPs (as you noted), but I respect your assessment that DNS-based validation would be excessive for this use case.
SSL verification (commit 81e5db6):
Making it configurable is a reasonable compromise. Security-conscious deployments can enable it via WEBSEARCH_FETCH_VERIFY_SSL=true, while the default maintains your preferred pragmatic stance on MITM risk.
Imports:
Noted.
The implementation looks solid for your threat model. Thanks for working through these concerns!
🧠 Learnings used
Learnt from: Ahmath-Gadji
Repo: linagora/openrag PR: 245
File: openrag/routers/users.py:59-95
Timestamp: 2026-02-13T10:12:27.454Z
Learning: In Python code, when using a constant like DEFAULT_FILE_QUOTA to control global quota enforcement, the condition should disable quotas only when the value is strictly negative (DEFAULT_FILE_QUOTA < 0). A value of 0 should mean quotas are enabled with a limit of 0 files. This pattern should be reviewed in all modules that implement quota checks, e.g., any function that reads DEFAULT_FILE_QUOTA and makes a 'disable' decision. Specifically verify similar comparisons and ensure behavior is consistent across files (not assuming <= 0 disables quotas).
Collaborator
Author
There was a problem hiding this comment.
It's a pleasure working with you @coderabbitai
There was a problem hiding this comment.
@paultranvan Thank you! It's been a pleasure collaborating with you as well. I appreciate your thoughtful approach to the security considerations and the pragmatic solutions you implemented. The web search feature looks solid, and the configurable SSL verification strikes a good balance. Wishing you success with the PR! 🚀
/)_/)
(^.^) Happy to help!
(> <)
🧠 Learnings used
Learnt from: Ahmath-Gadji
Repo: linagora/openrag PR: 245
File: openrag/routers/users.py:59-95
Timestamp: 2026-02-13T10:12:27.454Z
Learning: In Python code, when using a constant like DEFAULT_FILE_QUOTA to control global quota enforcement, the condition should disable quotas only when the value is strictly negative (DEFAULT_FILE_QUOTA < 0). A value of 0 should mean quotas are enabled with a limit of 0 files. This pattern should be reviewed in all modules that implement quota checks, e.g., any function that reads DEFAULT_FILE_QUOTA and makes a 'disable' decision. Specifically verify similar comparisons and ensure behavior is consistent across files (not assuming <= 0 disables quotas).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
openrag/components/websearch/content_fetcher.py (4)
6-8:⚠️ Potential issue | 🔴 CriticalRelative imports still violate the absolute-import coding guideline.
from components.websearch.baseandfrom utils.loggerare relative imports; they must be absolute per the project's Python path root convention.-from components.websearch.base import WebResult -from utils.logger import get_logger +from openrag.components.websearch.base import WebResult +from openrag.utils.logger import get_loggerAs per coding guidelines: "Use absolute imports from the
openrag/directory as the Python path root, avoiding relative imports across packages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 6 - 8, Update the imports in content_fetcher.py to use absolute imports rooted at the project package instead of relative module paths: replace references to components.websearch.base and utils.logger with the absolute package paths that expose WebResult and get_logger (e.g., import WebResult from openrag.components.websearch.base and import get_logger from openrag.utils.logger), leaving the external html_to_markdown.convert import as-is; ensure the symbols WebResult and get_logger are the ones imported so existing references in the file continue to work.
117-117:⚠️ Potential issue | 🟠 Major
verify=Falsedisables TLS certificate validation on all outbound fetch requests.This allows MITM attacks on fetched content. Flagged in a prior review with a one-line fix.
-async with httpx.AsyncClient(timeout=timeout, verify=False, headers={"User-Agent": _USER_AGENT}) as client: +async with httpx.AsyncClient(timeout=timeout, verify=True, headers={"User-Agent": _USER_AGENT}) as client:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` at line 117, The code disables TLS verification by passing verify=False to httpx.AsyncClient; change this to enable certificate validation (remove verify=False or set verify=True or provide a proper SSL context/cafile via ssl or certifi) so outbound fetches via httpx.AsyncClient (the instantiation using timeout, headers={"User-Agent": _USER_AGENT}) perform normal TLS certificate checks; ensure any special-case bypass is guarded behind an explicit, documented opt-in configuration rather than a default.
117-117:⚠️ Potential issue | 🟠 Major
verify=Falsedisables TLS certificate validation on all outbound fetch requests.Allows MITM attacks on fetched content. This was flagged in a prior review and proposed a one-line fix.
-async with httpx.AsyncClient(timeout=timeout, verify=False, headers={"User-Agent": _USER_AGENT}) as client: +async with httpx.AsyncClient(timeout=timeout, verify=True, headers={"User-Agent": _USER_AGENT}) as client:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` at line 117, The code disables TLS verification by creating httpx.AsyncClient(..., verify=False) in the fetch routine (the AsyncClient instantiation in content_fetcher.py), which must be fixed: remove verify=False so httpx uses default certificate validation or explicitly set verify=certifi.where() and make it configurable via the ContentFetcher/fetch method (e.g., add a verify_ssl or ssl_context parameter to the ContentFetcher constructor or fetch call) so callers can opt-out in controlled environments; update httpx.AsyncClient usage accordingly and ensure tests/configs pass the new parameter when needed.
6-8:⚠️ Potential issue | 🔴 CriticalRelative imports still violate the absolute-import coding guideline.
from components.websearch.baseandfrom utils.loggerare relative imports; they must be absolute per the project's Python path root convention.-from components.websearch.base import WebResult -from utils.logger import get_logger +from openrag.components.websearch.base import WebResult +from openrag.utils.logger import get_loggerAs per coding guidelines: "Use absolute imports from the
openrag/directory as the Python path root, avoiding relative imports across packages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 6 - 8, The file uses package-relative imports that violate the absolute-import rule; replace "from components.websearch.base import WebResult" and "from utils.logger import get_logger" with absolute imports rooted at the project package (e.g., import WebResult from openrag.components.websearch.base and get_logger from openrag.utils.logger), and update any other imports in this module if they follow the same pattern so all references use the openrag.* absolute path.
🧹 Nitpick comments (6)
openrag/components/websearch/content_fetcher.py (6)
104-121: Duplicated fetch/gather/zip pattern — consider extracting a helper.The three-line fetch/gather/assign block is identical in both the
_client_overridebranch and the production branch. A private helper removes the duplication and prevents the two paths from silently diverging in future edits.♻️ Proposed refactor
+ async def _fetch_all( + self, client: httpx.AsyncClient, to_fetch: list[WebResult] + ) -> list[str | None]: + tasks = [self._fetch_single(client, r.url) for r in to_fetch] + contents: list[str | None] = list(await asyncio.gather(*tasks)) + for result, content in zip(to_fetch, contents): + result.content = content + return contents async def enrich(self, results: list[WebResult]) -> list[WebResult]: ... client = self._client_override if client is not None: - tasks = [self._fetch_single(client, r.url) for r in to_fetch] - contents = await asyncio.gather(*tasks) - for result, content in zip(to_fetch, contents): - result.content = content + contents = await self._fetch_all(client, to_fetch) else: ... async with httpx.AsyncClient(...) as client: - tasks = [self._fetch_single(client, r.url) for r in to_fetch] - contents = await asyncio.gather(*tasks) - for result, content in zip(to_fetch, contents): - result.content = content + contents = await self._fetch_all(client, to_fetch)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 104 - 121, Extract the duplicated fetch/gather/zip logic into a private async helper (e.g. _populate_contents_from_client(client, to_fetch)) that builds tasks via self._fetch_single(client, r.url), awaits asyncio.gather, and assigns content to each result.result.content = content; then replace both branches that check self._client_override and the async with httpx.AsyncClient(...) block to simply call await self._populate_contents_from_client(client, to_fetch) (passing either the override client or the newly created client) so the fetch flow lives in one place and cannot diverge.
80-81: Silentpasson lxml parse failure discards diagnostic signal.- except Exception: - pass # If lxml parsing fails, convert the raw HTML + except Exception as e: + logger.debug("lxml parsing failed, using raw HTML", url=url, error=str(e))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 80 - 81, The except block that currently swallows all exceptions during the lxml parsing step should surface the error and then proceed with the fallback conversion; update the except in the lxml parsing region of content_fetcher.py (the try that attempts lxml parsing and the comment "If lxml parsing fails, convert the raw HTML") to catch Exception as e, log or record the exception (e.g., via logger.error or similar) including the exception message and context, and then continue with the existing raw-HTML conversion fallback so you don’t silently lose diagnostic signals.
104-121: Duplicated fetch/gather/zip pattern — extract a helper.The fetch-gather-assign block appears identically in both the
_client_overridebranch (lines 106–109) and the production branch (lines 118–121). Extracting it eliminates the duplication and reduces the risk of the two paths diverging silently.♻️ Proposed refactor
+ async def _fetch_all(self, client: httpx.AsyncClient, to_fetch: list[WebResult]) -> list[str | None]: + tasks = [self._fetch_single(client, r.url) for r in to_fetch] + contents = await asyncio.gather(*tasks) + for result, content in zip(to_fetch, contents): + result.content = content + return list(contents) async def enrich(self, results: list[WebResult]) -> list[WebResult]: if not results: return results to_fetch = results[: self.max_results] client = self._client_override if client is not None: - tasks = [self._fetch_single(client, r.url) for r in to_fetch] - contents = await asyncio.gather(*tasks) - for result, content in zip(to_fetch, contents): - result.content = content + contents = await self._fetch_all(client, to_fetch) else: timeout = httpx.Timeout( connect=self.timeout, read=self.timeout, write=self.timeout, pool=self.timeout, ) async with httpx.AsyncClient(timeout=timeout, verify=True, headers={"User-Agent": _USER_AGENT}) as client: - tasks = [self._fetch_single(client, r.url) for r in to_fetch] - contents = await asyncio.gather(*tasks) - for result, content in zip(to_fetch, contents): - result.content = content + contents = await self._fetch_all(client, to_fetch) n_enriched = sum(1 for c in contents if c is not None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 104 - 121, The duplicated fetch/gather/zip logic in the client override and production branches should be extracted into a single async helper (e.g., async def _fetch_and_assign(client, to_fetch):) that takes an httpx client and the to_fetch list, creates tasks using self._fetch_single(client, r.url), awaits asyncio.gather, and assigns each returned content to result.content; then call that helper from both the _client_override branch and the AsyncClient context block so both paths share the same implementation and behavior.
90-92:except TimeoutErrormisses httpx's own timeout exception.
httpx.TimeoutExceptionis the base class for httpx timeout errors and is not a subclass of Python's built-inTimeoutError.asyncio.TimeoutErrorwas made an alias of the built-inTimeoutErroronly in Python 3.11, soexcept TimeoutErroronly fires whenasyncio.wait_for's deadline is reached. If the httpx client's own timeout fires first (which is likely given both are set toself.timeout), the exception ishttpx.TimeoutException, falls through toexcept Exception, and logs "Content fetch failed" with anerror=field instead of the dedicated "Content fetch timed out" message — silently misidentifying timeouts as generic failures.♻️ Proposed fix
+ except httpx.TimeoutException: + logger.debug("Content fetch timed out", url=url) + return None except TimeoutError: logger.debug("Content fetch timed out", url=url) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 90 - 92, The except clause currently catches only the built-in TimeoutError, missing httpx's own timeout class; update the exception handling in the content fetch routine (the block using except TimeoutError and logger.debug("Content fetch timed out", url=url)) to catch both asyncio.TimeoutError (or built-in TimeoutError for compatibility) and httpx.TimeoutException instead — e.g. except (asyncio.TimeoutError, httpx.TimeoutException): — ensure httpx (and asyncio if referenced) is imported, keep the same logger.debug message and return None so timeouts are logged with the dedicated "Content fetch timed out" path instead of falling through to the generic Exception handler.
80-81: Silentpasson lxml parse failure discards diagnostic signal.Swallowing all
Exceptions with no log makes it impossible to detect systematic parse failures (e.g., encoding issues, malformed HTML from a specific source). A debug-level log costs nothing.- except Exception: - pass # If lxml parsing fails, convert the raw HTML + except Exception as e: + logger.debug("lxml parsing failed, using raw HTML", url=url, error=str(e))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 80 - 81, The except Exception: block that swallows lxml parse errors should capture the exception and log it at debug level instead of silently passing; change the handler to except Exception as e: and call the module/class logger (e.g., logger.debug or self.logger.debug) to include the exception message and minimal context (like the HTML snippet or URL variable used when parsing) before falling back to converting raw HTML. This targets the lxml parsing block (the except Exception: following the lxml parse attempt) so you can quickly locate and update that catch site.
90-92: Add explicit catch forhttpx.TimeoutExceptionto ensure consistent timeout logging.
asyncio.wait_forraisesasyncio.TimeoutError(aliased toTimeoutErrorin Python ≥ 3.11), so that branch correctly handles asyncio timeouts. However, if httpx's internal timeout fires first, it raiseshttpx.TimeoutException, which does not inherit fromTimeoutErrorand falls through to the genericexcept Exceptionhandler. Both paths correctly log and return None, but the log messages differ: asyncio timeouts produce "Content fetch timed out" while httpx timeouts produce "Content fetch failed" with an error field. For consistent timeout logging, add an explicit handler forhttpx.TimeoutExceptionbefore theTimeoutErrorhandler.Proposed fix
+ except httpx.TimeoutException: + logger.debug("Content fetch timed out", url=url) + return None except TimeoutError: logger.debug("Content fetch timed out", url=url) return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 90 - 92, The timeout handling in content_fetcher.py should explicitly catch httpx.TimeoutException so httpx-level timeouts are logged the same as asyncio timeouts; add an except httpx.TimeoutException: block before the existing except TimeoutError: that calls logger.debug("Content fetch timed out", url=url) and returns None, ensure httpx is imported if not already, and keep the existing generic except Exception: handler for other failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/websearch/content_fetcher.py`:
- Around line 46-49: The current _is_loopback_url only checks "localhost" and
127.*; replace it with a robust IP-based check: extract and strip bracketed
hostnames in _is_loopback_url, try to parse the host with ipaddress.ip_address
and return True if not ip.is_global (which covers loopback, RFC1918, link-local,
metadata IPs, IPv6 ::1, 0.0.0.0, etc.); if parsing fails (hostname),
synchronously resolve the hostname via socket.getaddrinfo and apply the same
ip.is_global check to all returned addresses, returning True if any resolved
address is non-global; ensure you catch and treat resolution errors
conservatively (treat unresolved as internal) and keep the method name
_is_loopback_url to locate the change.
- Around line 46-49: The current _is_loopback_url only checks "localhost" and
"127.*" and misses IPv6 loopback, RFC1918, link-local, unspecified and cloud
metadata addresses; update _is_loopback_url(url: str) to parse the URL host,
resolve hostnames to all IPs (use socket.getaddrinfo, offload to an executor if
necessary for async code) and for each resolved address use
ipaddress.ip_address(...) to reject addresses that are ip.is_loopback,
ip.is_private, ip.is_link_local, ip.is_multicast, ip.is_reserved/unspecified, or
exactly 169.254.169.254; also handle literal IPv6 forms like [::1] by stripping
brackets before parsing. Ensure the function returns True for any disallowed
internal address and False only for safe public addresses.
---
Duplicate comments:
In `@openrag/components/websearch/content_fetcher.py`:
- Around line 6-8: Update the imports in content_fetcher.py to use absolute
imports rooted at the project package instead of relative module paths: replace
references to components.websearch.base and utils.logger with the absolute
package paths that expose WebResult and get_logger (e.g., import WebResult from
openrag.components.websearch.base and import get_logger from
openrag.utils.logger), leaving the external html_to_markdown.convert import
as-is; ensure the symbols WebResult and get_logger are the ones imported so
existing references in the file continue to work.
- Line 117: The code disables TLS verification by passing verify=False to
httpx.AsyncClient; change this to enable certificate validation (remove
verify=False or set verify=True or provide a proper SSL context/cafile via ssl
or certifi) so outbound fetches via httpx.AsyncClient (the instantiation using
timeout, headers={"User-Agent": _USER_AGENT}) perform normal TLS certificate
checks; ensure any special-case bypass is guarded behind an explicit, documented
opt-in configuration rather than a default.
- Line 117: The code disables TLS verification by creating
httpx.AsyncClient(..., verify=False) in the fetch routine (the AsyncClient
instantiation in content_fetcher.py), which must be fixed: remove verify=False
so httpx uses default certificate validation or explicitly set
verify=certifi.where() and make it configurable via the ContentFetcher/fetch
method (e.g., add a verify_ssl or ssl_context parameter to the ContentFetcher
constructor or fetch call) so callers can opt-out in controlled environments;
update httpx.AsyncClient usage accordingly and ensure tests/configs pass the new
parameter when needed.
- Around line 6-8: The file uses package-relative imports that violate the
absolute-import rule; replace "from components.websearch.base import WebResult"
and "from utils.logger import get_logger" with absolute imports rooted at the
project package (e.g., import WebResult from openrag.components.websearch.base
and get_logger from openrag.utils.logger), and update any other imports in this
module if they follow the same pattern so all references use the openrag.*
absolute path.
---
Nitpick comments:
In `@openrag/components/websearch/content_fetcher.py`:
- Around line 104-121: Extract the duplicated fetch/gather/zip logic into a
private async helper (e.g. _populate_contents_from_client(client, to_fetch))
that builds tasks via self._fetch_single(client, r.url), awaits asyncio.gather,
and assigns content to each result.result.content = content; then replace both
branches that check self._client_override and the async with
httpx.AsyncClient(...) block to simply call await
self._populate_contents_from_client(client, to_fetch) (passing either the
override client or the newly created client) so the fetch flow lives in one
place and cannot diverge.
- Around line 80-81: The except block that currently swallows all exceptions
during the lxml parsing step should surface the error and then proceed with the
fallback conversion; update the except in the lxml parsing region of
content_fetcher.py (the try that attempts lxml parsing and the comment "If lxml
parsing fails, convert the raw HTML") to catch Exception as e, log or record the
exception (e.g., via logger.error or similar) including the exception message
and context, and then continue with the existing raw-HTML conversion fallback so
you don’t silently lose diagnostic signals.
- Around line 104-121: The duplicated fetch/gather/zip logic in the client
override and production branches should be extracted into a single async helper
(e.g., async def _fetch_and_assign(client, to_fetch):) that takes an httpx
client and the to_fetch list, creates tasks using self._fetch_single(client,
r.url), awaits asyncio.gather, and assigns each returned content to
result.content; then call that helper from both the _client_override branch and
the AsyncClient context block so both paths share the same implementation and
behavior.
- Around line 90-92: The except clause currently catches only the built-in
TimeoutError, missing httpx's own timeout class; update the exception handling
in the content fetch routine (the block using except TimeoutError and
logger.debug("Content fetch timed out", url=url)) to catch both
asyncio.TimeoutError (or built-in TimeoutError for compatibility) and
httpx.TimeoutException instead — e.g. except (asyncio.TimeoutError,
httpx.TimeoutException): — ensure httpx (and asyncio if referenced) is imported,
keep the same logger.debug message and return None so timeouts are logged with
the dedicated "Content fetch timed out" path instead of falling through to the
generic Exception handler.
- Around line 80-81: The except Exception: block that swallows lxml parse errors
should capture the exception and log it at debug level instead of silently
passing; change the handler to except Exception as e: and call the module/class
logger (e.g., logger.debug or self.logger.debug) to include the exception
message and minimal context (like the HTML snippet or URL variable used when
parsing) before falling back to converting raw HTML. This targets the lxml
parsing block (the except Exception: following the lxml parse attempt) so you
can quickly locate and update that catch site.
- Around line 90-92: The timeout handling in content_fetcher.py should
explicitly catch httpx.TimeoutException so httpx-level timeouts are logged the
same as asyncio timeouts; add an except httpx.TimeoutException: block before the
existing except TimeoutError: that calls logger.debug("Content fetch timed out",
url=url) and returns None, ensure httpx is imported if not already, and keep the
existing generic except Exception: handler for other failures.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openrag/components/websearch/content_fetcher.pyopenrag/components/websearch/test_content_fetcher.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/components/websearch/test_content_fetcher.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/websearch/providers/staan.py`:
- Around line 2-3: Update the imports in staan.py to use absolute package-root
paths: replace the relative module imports for BaseWebSearchProvider and
WebResult (currently referenced as components.websearch.base) and get_logger
(currently utils.logger) with absolute imports starting from the openrag package
(e.g., import BaseWebSearchProvider, WebResult from
openrag.components.websearch.base and get_logger from openrag.utils.logger) so
module resolution works consistently across runtime environments.
- Around line 9-13: Replace the two relative imports with absolute ones: change
any import of components.websearch.base to openrag.components.websearch.base and
utils.logger.get_logger to openrag.utils.logger.get_logger so the module imports
from the openrag package; then in the __init__ method for the provider (def
__init__(self, api_token: str, base_url: str, top_k: int = 5, lang: str =
"fr-FR")), validate top_k and raise OpenRAGError if top_k is negative (e.g., if
top_k < 0: raise OpenRAGError("top_k must be non-negative")) to prevent negative
slicing behavior and follow project error types.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openrag/components/websearch/base.pyopenrag/components/websearch/providers/staan.pyopenrag/routers/openai.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openrag/routers/openai.py
Comment on lines
+9
to
+13
| def __init__(self, api_token: str, base_url: str, top_k: int = 5, lang: str = "fr-FR"): | ||
| self.api_token = api_token | ||
| self.base_url = base_url | ||
| self.top_k = top_k | ||
| self.lang = lang |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the StaanProvider implementation and top_k usage
cat -n openrag/components/websearch/providers/staan.py | head -50Repository: linagora/openrag
Length of output: 1568
🏁 Script executed:
# Check if OpenRAGError exists and how exceptions are handled
find . -name "exceptions.py" -o -name "exception.py" | head -5
fd exceptions --type fRepository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Search for call sites of StaanProvider
rg -n "StaanProvider\s*\(" --type pyRepository: linagora/openrag
Length of output: 225
🏁 Script executed:
# Check how top_k is actually used in the code
rg -n "self\.top_k" openrag/components/websearch/providers/staan.py -A 2 -B 2Repository: linagora/openrag
Length of output: 327
🏁 Script executed:
# Find OpenRAGError
rg -n "class OpenRAGError" --type py
fd exceptions --type f | head -10Repository: linagora/openrag
Length of output: 125
🏁 Script executed:
# Check the pipeline.py call site to see how StaanProvider is instantiated
sed -n '95,105p' openrag/pipeline.pyRepository: linagora/openrag
Length of output: 122
🏁 Script executed:
# Verify if ValueError is used elsewhere or if OpenRAGError is the pattern
rg -n "raise ValueError\|raise OpenRAGError" --type py | head -20Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check import patterns in other providers
ls -la openrag/components/websearch/providers/
rg -n "^from\|^import" openrag/components/websearch/providers/*.py | head -15Repository: linagora/openrag
Length of output: 289
🏁 Script executed:
# Find pipeline.py
fd pipeline --type fRepository: linagora/openrag
Length of output: 90
🏁 Script executed:
# Check current import patterns in staan.py against guidelines
cat -n openrag/components/websearch/providers/staan.py | head -15Repository: linagora/openrag
Length of output: 625
🏁 Script executed:
# Check how other files in openrag import things
rg -n "^from openrag\|^import openrag" --type py | head -10Repository: linagora/openrag
Length of output: 42
🏁 Script executed:
# Check BaseWebSearchProvider to understand the base class
find . -name "base.py" -path "*/websearch/*" | head -5Repository: linagora/openrag
Length of output: 98
🏁 Script executed:
# Check the call site in pipeline.py
sed -n '90,110p' openrag/components/pipeline.pyRepository: linagora/openrag
Length of output: 1010
🏁 Script executed:
# Check BaseWebSearchProvider to understand the contract
cat -n openrag/components/websearch/base.pyRepository: linagora/openrag
Length of output: 593
🏁 Script executed:
# Check if there are other providers and how they handle validation
ls -la openrag/components/websearch/providers/ && echo "---" && find openrag/components/websearch/providers -name "*.py" -type fRepository: linagora/openrag
Length of output: 392
🏁 Script executed:
# Check how OpenRAGError is used in similar contexts
rg -B2 -A2 "raise OpenRAGError\|if.*<\s*0" --type py | head -40Repository: linagora/openrag
Length of output: 42
Fix imports to use absolute paths and validate top_k parameter with proper exception type.
The file has two issues:
-
Imports must use absolute paths (lines 2–3): Replace relative imports with absolute imports from
openrag/:from components.websearch.base import ...→from openrag.components.websearch.base import ...from utils.logger import get_logger→from openrag.utils.logger import get_logger
-
Validate
top_kto prevent negative-limit behavior (line 9): A negativetop_kproduces unintended slicing behavior (results[:-5]returns all but last 5 results instead of limiting to top 5). RaiseOpenRAGError(notValueError) as required by guidelines:
Proposed fix
import httpx
-from components.websearch.base import BaseWebSearchProvider, WebResult
-from utils.logger import get_logger
+from openrag.components.websearch.base import BaseWebSearchProvider, WebResult
+from openrag.utils.logger import get_logger
+from openrag.utils.exceptions.base import OpenRAGError
logger = get_logger()
class StaanProvider(BaseWebSearchProvider):
def __init__(self, api_token: str, base_url: str, top_k: int = 5, lang: str = "fr-FR"):
+ if top_k < 0:
+ raise OpenRAGError("top_k must be >= 0")
self.api_token = api_token
self.base_url = base_url
self.top_k = top_k
self.lang = lang🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/websearch/providers/staan.py` around lines 9 - 13, Replace
the two relative imports with absolute ones: change any import of
components.websearch.base to openrag.components.websearch.base and
utils.logger.get_logger to openrag.utils.logger.get_logger so the module imports
from the openrag package; then in the __init__ method for the provider (def
__init__(self, api_token: str, base_url: str, top_k: int = 5, lang: str =
"fr-FR")), validate top_k and raise OpenRAGError if top_k is negative (e.g., if
top_k < 0: raise OpenRAGError("top_k must be non-negative")) to prevent negative
slicing behavior and follow project error types.
paultranvan
force-pushed
the
feat/websearch
branch
from
February 25, 2026 17:46
718b5d5 to
29b33d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
openrag/components/websearch/providers/staan.py (2)
2-3: Use absolute imports from theopenrag/directory.As per coding guidelines and the previous review: replace the relative imports with absolute ones rooted at
openrag/:-from components.websearch.base import BaseWebSearchProvider, WebResult -from utils.logger import get_logger +from openrag.components.websearch.base import BaseWebSearchProvider, WebResult +from openrag.utils.logger import get_logger🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/providers/staan.py` around lines 2 - 3, Replace the relative module imports in this file with absolute imports rooted at the project package: change the imports that bring in BaseWebSearchProvider and WebResult (currently from components.websearch.base) and get_logger (currently from utils.logger) to use absolute package paths starting at openrag so the module imports reference openrag.components.websearch.base and openrag.utils.logger respectively, keeping the same symbol names (BaseWebSearchProvider, WebResult, get_logger).
9-13: Validatetop_kin__init__usingOpenRAGError.As per coding guidelines and the previous review: a negative
top_ksilently produces wrong slicing behaviour (results[:-n]), and any custom exception must inherit fromOpenRAGError:+from openrag.utils.exceptions.base import OpenRAGError class StaanProvider(BaseWebSearchProvider): def __init__(self, api_token: str, base_url: str, top_k: int = 5, lang: str = "fr-FR"): + if top_k < 0: + raise OpenRAGError("top_k must be non-negative") self.api_token = api_token🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/providers/staan.py` around lines 9 - 13, Validate the top_k parameter in the __init__ of the provider: check that top_k is an int and non-negative (e.g., top_k >= 0) and if not raise an OpenRAGError with a clear message; update the __init__ (the constructor shown) to perform this guard before assigning self.top_k so negative values cannot cause wrong slicing behavior, and ensure the raised exception is OpenRAGError (or a subclass) rather than a generic Exception.openrag/routers/openai.py (2)
133-141:⚠️ Potential issue | 🟠 Major
result.urlis forwarded to the client without sanitization or scheme validation.
display_urlis now scheme-validated, but the primary"url"field at Line 136 is passed directly from the provider without any checks, leaving link-injection/XSS risk for downstream UI renderers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/routers/openai.py` around lines 133 - 141, The code currently forwards result.url into the links dict without sanitization or scheme checks; update the links construction (the block that calls links.append and references result.url and display_url) to validate the URL scheme and sanitize the URL before including it as the "url" field — reuse the same scheme-validation logic used to produce display_url and the sanitize_text function (or existing URL-sanitizer helper) so the "url" entry only contains a safe, canonical HTTP/HTTPS URL (or is omitted/nullified if invalid), and ensure any dangerous characters are escaped via sanitize_text.
122-127:⚠️ Potential issue | 🟡 Minor
source_type: "document"can be silently overridden by**doc_metadata.If
doc_metadatacontains a"source_type"key, the spread wins and the authoritative"document"value is lost.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/routers/openai.py` around lines 122 - 127, The dict literal currently spreads doc_metadata after setting "source_type": "document", which allows doc_metadata["source_type"] to override it; change the merge order or sanitize doc_metadata so "source_type" remains authoritative: either spread doc_metadata first and then set "source_type": "document" (so it wins), or remove any "source_type" key from doc_metadata (e.g., pop it) before merging; update the dict creation that references encoded_url, request.url_for("get_extract", extract_id=doc_metadata["_id"]), and doc_metadata accordingly.openrag/components/websearch/content_fetcher.py (3)
123-123:⚠️ Potential issue | 🟠 Major
verify=Falsedisables TLS certificate validation, enabling MITM content poisoning.Fetched HTML is injected directly into the RAG context; a MITM attacker can replace it with adversarial content that influences LLM answers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` at line 123, The AsyncClient is created with verify=False which disables TLS verification (see the httpx.AsyncClient call in content_fetcher.py); change it to use secure defaults by removing verify=False or setting verify=True, and if you need configurable behavior expose a vetted parameter (e.g., fetch_verify or ssl_context) on the fetching function/class so callers can opt in with an explicit rationale; additionally consider allowing a custom CA bundle or ssl.SSLContext to be passed into the function that calls httpx.AsyncClient so certificate validation can be enforced while still supporting special trust stores.
47-55:⚠️ Potential issue | 🟠 MajorSSRF protection is still incomplete for non-IP hostnames.
For hostname-based URLs (e.g.,
internal.corp,metadata.google.internal) theexcept ValueErrorbranch returnsFalse, letting them through without DNS resolution checks. RFC-1918 literals (10.*,172.16–31.*,192.168.*) and link-local (169.254.*) are now covered for IP literals, but hostnames that resolve to private ranges are not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 47 - 55, The _is_loopback_url function currently skips DNS resolution for non-IP hostnames (the except ValueError branch), allowing hostnames that resolve to private or link-local addresses to pass; change that branch to resolve the hostname (e.g., via socket.getaddrinfo or similar) and iterate over all returned address tuples, converting each to an ipaddress.ip_address and returning True if any resolved address is non-global (private/loopback/link-local), otherwise False; also catch and handle resolution errors/timeouts (treat as False or log) to avoid crashing and keep the existing behavior for literal IP handling in the try branch.
7-9: 🛠️ Refactor suggestion | 🟠 MajorRelative imports violate the absolute-import coding guideline.
from components.websearch.baseandfrom utils.loggerare relative-style imports. All imports must use theopenrag/directory as the path root.♻️ Proposed fix
-from components.websearch.base import WebResult -from utils.logger import get_logger +from openrag.components.websearch.base import WebResult +from openrag.utils.logger import get_loggerAs per coding guidelines: "Use absolute imports from the
openrag/directory as the Python path root, avoiding relative imports across packages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 7 - 9, Update the import statements in content_fetcher.py to use absolute imports rooted at the openrag package: replace the current lines importing WebResult, convert, and get_logger (from components.websearch.base, html_to_markdown, utils.logger) with absolute imports that reference openrag (e.g., import WebResult from openrag.components.websearch.base and get_logger from openrag.utils.logger) while leaving the usage of WebResult, convert, and get_logger unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/websearch/content_fetcher.py`:
- Around line 80-87: The lxml tree is being mutated while iterating which can
skip siblings and a bare except hides AttributeError when el.getparent() is
None; update the block that builds "tree" (using lxml.html.fromstring) to first
collect elements into a list before removing: for each tag in _BOILERPLATE_TAGS
do elems = list(tree.iter(tag)) and then for el in elems get parent =
el.getparent() and only call parent.remove(el) if parent is not None; also
replace the bare "except Exception" with a narrow exception handler (e.g., catch
lxml.etree.ParserError or capture Exception as e) and log or handle the error
instead of silently passing so the fallback conversion of raw HTML remains
intentional.
In `@openrag/components/websearch/providers/staan.py`:
- Line 25: The code in the search() function silently returns [] when the API
response shape is unexpected; instead, detect when data is neither a list nor
has data["web"]["results"], log the full unexpected payload (or a concise repr)
via the module-level logger (e.g., logger.warning or logger.error) including
context like the function name and any request identifiers, then fall back to
returning [] so callers remain stable; update the assignment around the results
variable and add a guarded logger call that prints the problematic data before
returning the empty list.
In `@openrag/routers/openai.py`:
- Line 7: The import line using a relative-style path should be changed to an
absolute import from the project root; replace the statement that imports
sanitize_text (currently written as "from
components.indexer.utils.text_sanitizer import sanitize_text") with an absolute
import anchored at the openrag package root (importing sanitize_text from the
correct module path under openrag), updating the import in
openrag/routers/openai.py so it follows the project's absolute-import guideline
and references the sanitize_text symbol directly.
---
Duplicate comments:
In `@openrag/components/websearch/content_fetcher.py`:
- Line 123: The AsyncClient is created with verify=False which disables TLS
verification (see the httpx.AsyncClient call in content_fetcher.py); change it
to use secure defaults by removing verify=False or setting verify=True, and if
you need configurable behavior expose a vetted parameter (e.g., fetch_verify or
ssl_context) on the fetching function/class so callers can opt in with an
explicit rationale; additionally consider allowing a custom CA bundle or
ssl.SSLContext to be passed into the function that calls httpx.AsyncClient so
certificate validation can be enforced while still supporting special trust
stores.
- Around line 47-55: The _is_loopback_url function currently skips DNS
resolution for non-IP hostnames (the except ValueError branch), allowing
hostnames that resolve to private or link-local addresses to pass; change that
branch to resolve the hostname (e.g., via socket.getaddrinfo or similar) and
iterate over all returned address tuples, converting each to an
ipaddress.ip_address and returning True if any resolved address is non-global
(private/loopback/link-local), otherwise False; also catch and handle resolution
errors/timeouts (treat as False or log) to avoid crashing and keep the existing
behavior for literal IP handling in the try branch.
- Around line 7-9: Update the import statements in content_fetcher.py to use
absolute imports rooted at the openrag package: replace the current lines
importing WebResult, convert, and get_logger (from components.websearch.base,
html_to_markdown, utils.logger) with absolute imports that reference openrag
(e.g., import WebResult from openrag.components.websearch.base and get_logger
from openrag.utils.logger) while leaving the usage of WebResult, convert, and
get_logger unchanged.
In `@openrag/components/websearch/providers/staan.py`:
- Around line 2-3: Replace the relative module imports in this file with
absolute imports rooted at the project package: change the imports that bring in
BaseWebSearchProvider and WebResult (currently from components.websearch.base)
and get_logger (currently from utils.logger) to use absolute package paths
starting at openrag so the module imports reference
openrag.components.websearch.base and openrag.utils.logger respectively, keeping
the same symbol names (BaseWebSearchProvider, WebResult, get_logger).
- Around line 9-13: Validate the top_k parameter in the __init__ of the
provider: check that top_k is an int and non-negative (e.g., top_k >= 0) and if
not raise an OpenRAGError with a clear message; update the __init__ (the
constructor shown) to perform this guard before assigning self.top_k so negative
values cannot cause wrong slicing behavior, and ensure the raised exception is
OpenRAGError (or a subclass) rather than a generic Exception.
In `@openrag/routers/openai.py`:
- Around line 133-141: The code currently forwards result.url into the links
dict without sanitization or scheme checks; update the links construction (the
block that calls links.append and references result.url and display_url) to
validate the URL scheme and sanitize the URL before including it as the "url"
field — reuse the same scheme-validation logic used to produce display_url and
the sanitize_text function (or existing URL-sanitizer helper) so the "url" entry
only contains a safe, canonical HTTP/HTTPS URL (or is omitted/nullified if
invalid), and ensure any dangerous characters are escaped via sanitize_text.
- Around line 122-127: The dict literal currently spreads doc_metadata after
setting "source_type": "document", which allows doc_metadata["source_type"] to
override it; change the merge order or sanitize doc_metadata so "source_type"
remains authoritative: either spread doc_metadata first and then set
"source_type": "document" (so it wins), or remove any "source_type" key from
doc_metadata (e.g., pop it) before merging; update the dict creation that
references encoded_url, request.url_for("get_extract",
extract_id=doc_metadata["_id"]), and doc_metadata accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openrag/components/websearch/base.pyopenrag/components/websearch/content_fetcher.pyopenrag/components/websearch/providers/staan.pyopenrag/components/websearch/test_content_fetcher.pyopenrag/routers/openai.py
🚧 Files skipped from review as they are similar to previous changes (2)
- openrag/components/websearch/base.py
- openrag/components/websearch/test_content_fetcher.py
Comment on lines
+80
to
+87
| try: | ||
| tree = lxml.html.fromstring(html) | ||
| for tag in _BOILERPLATE_TAGS: | ||
| for el in tree.iter(tag): | ||
| el.getparent().remove(el) | ||
| html = lxml.html.tostring(tree, encoding="unicode") | ||
| except Exception: | ||
| pass # If lxml parsing fails, convert the raw HTML |
There was a problem hiding this comment.
Mutating the lxml tree during a lazy iter() call can silently skip elements.
tree.iter(tag) produces a lazy generator; calling el.getparent().remove(el) while iterating can cause sibling elements of the same tag to be skipped. Additionally, el.getparent() returns None when el is the root, triggering an AttributeError that is silently swallowed by the bare except, leaving the raw un-stripped HTML to be converted instead.
Collect elements before mutating the tree:
♻️ Proposed fix
- for tag in _BOILERPLATE_TAGS:
- for el in tree.iter(tag):
- el.getparent().remove(el)
+ for tag in _BOILERPLATE_TAGS:
+ for el in tree.iter(tag):
+ parent = el.getparent()
+ if parent is not None:
+ parent.remove(el)Or, to avoid mutation-during-iteration entirely:
- for tag in _BOILERPLATE_TAGS:
- for el in tree.iter(tag):
- el.getparent().remove(el)
+ elements_to_remove = [
+ el for tag in _BOILERPLATE_TAGS for el in tree.iter(tag)
+ if el.getparent() is not None
+ ]
+ for el in elements_to_remove:
+ el.getparent().remove(el)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/websearch/content_fetcher.py` around lines 80 - 87, The
lxml tree is being mutated while iterating which can skip siblings and a bare
except hides AttributeError when el.getparent() is None; update the block that
builds "tree" (using lxml.html.fromstring) to first collect elements into a list
before removing: for each tag in _BOILERPLATE_TAGS do elems =
list(tree.iter(tag)) and then for el in elems get parent = el.getparent() and
only call parent.remove(el) if parent is not None; also replace the bare "except
Exception" with a narrow exception handler (e.g., catch lxml.etree.ParserError
or capture Exception as e) and log or handle the error instead of silently
passing so the fallback conversion of raw HTML remains intentional.
| response.raise_for_status() | ||
| data = response.json() | ||
|
|
||
| results = data if isinstance(data, list) else data.get("web", {}).get("results", []) |
There was a problem hiding this comment.
Log unexpected API response shapes instead of silently returning an empty list.
When data is neither a plain list nor contains a web.results key, the method silently returns []. Production API format changes or misconfiguration become invisible. The module-level logger is initialized but never called inside search().
📋 Proposed fix
-results = data if isinstance(data, list) else data.get("web", {}).get("results", [])
+if isinstance(data, list):
+ results = data
+else:
+ results = data.get("web", {}).get("results", [])
+ if not results and "web" not in data:
+ logger.warning("Unexpected Staan API response structure", keys=list(data.keys()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openrag/components/websearch/providers/staan.py` at line 25, The code in the
search() function silently returns [] when the API response shape is unexpected;
instead, detect when data is neither a list nor has data["web"]["results"], log
the full unexpected payload (or a concise repr) via the module-level logger
(e.g., logger.warning or logger.error) including context like the function name
and any request identifiers, then fall back to returning [] so callers remain
stable; update the assignment around the results variable and add a guarded
logger call that prints the problematic data before returning the empty list.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
openrag/components/websearch/content_fetcher.py (2)
50-57:⚠️ Potential issue | 🟠 MajorSSRF filter is still incomplete for hostname-based internal targets.
Current filtering blocks non-global IP literals but allows hostnames without resolution checks, so internal DNS names/metadata endpoints can still pass. Also, redirects are followed without validating the final target.
🛡️ Proposed hardening
+import socket @@ `@staticmethod` def _is_loopback_url(url: str) -> bool: host = urlparse(url).hostname or "" - if host == "localhost": - return True - try: - return not ipaddress.ip_address(host).is_global - except ValueError: - return False # Regular hostname, let it through + if not host: + return True + if host == "localhost": + return True + try: + return not ipaddress.ip_address(host).is_global + except ValueError: + try: + infos = socket.getaddrinfo(host, None) + return any(not ipaddress.ip_address(info[4][0]).is_global for info in infos) + except OSError: + return True # Fail closed on DNS errors @@ response = await asyncio.wait_for( client.get(url, follow_redirects=True), timeout=self.timeout, ) + if self._is_loopback_url(str(response.url)): + logger.warning("Blocked redirected URL in web search results", url=str(response.url)) + return NoneAlso applies to: 63-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 50 - 57, The _is_loopback_url function currently allows hostnames without resolving them and you also follow redirects without re-checking targets; update _is_loopback_url to resolve the hostname (e.g., via socket.getaddrinfo or a DNS resolver) and verify every resolved IP (IPv4/IPv6) is global (reject if any are non-global), handle empty/None host safely, and ensure any HTTP redirect-following code re-evaluates the redirected URL with the same _is_loopback_url checks before following (reject requests to internal/metadata addresses).
82-89:⚠️ Potential issue | 🟡 MinorAvoid mutating the lxml tree during lazy iteration and avoid silent exception swallowing.
Removing nodes while iterating can skip siblings;
except Exception: passhides parse/removal issues.♻️ Proposed fix
try: tree = lxml.html.fromstring(html) for tag in _BOILERPLATE_TAGS: - for el in tree.iter(tag): - el.getparent().remove(el) + for el in list(tree.iter(tag)): + parent = el.getparent() + if parent is not None: + parent.remove(el) html = lxml.html.tostring(tree, encoding="unicode") - except Exception: - pass # If lxml parsing fails, convert the raw HTML + except Exception as e: + logger.debug("lxml cleanup failed; falling back to raw HTML", error=str(e))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/websearch/content_fetcher.py` around lines 82 - 89, The current code mutates the lxml tree while iterating (tree.iter(tag)) which can skip siblings and also swallows all exceptions; change the removal loop to iterate over a static list of nodes (e.g., for el in list(tree.iter(tag)) or using tree.xpath to collect elements first) and remove each via el.getparent().remove(el), and replace the broad "except Exception: pass" with catching specific parse errors (e.g., lxml.etree.ParserError) or "except Exception as e" and surface the error (log with logger.exception or re-raise) so parse/removal failures are not silently ignored; refer to _BOILERPLATE_TAGS, lxml.html.fromstring, tree.iter and lxml.html.tostring to locate the code to change.openrag/components/pipeline.py (2)
112-112:⚠️ Potential issue | 🟠 MajorDefaulting
fetch_verify_ssltoFalsekeeps insecure TLS as the baseline.This leaves cert validation off unless explicitly enabled. Prefer secure-by-default and require explicit opt-out.
🔒 Proposed fix
- verify_ssl=config.websearch.get("fetch_verify_ssl", False), + verify_ssl=config.websearch.get("fetch_verify_ssl", True),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/pipeline.py` at line 112, The code sets verify_ssl via config.websearch.get("fetch_verify_ssl", False) which defaults to False and makes TLS validation insecure by default; change the default to True so verify_ssl=config.websearch.get("fetch_verify_ssl", True) (or invert to an explicit opt-out like fetch_verify_ssl_disabled) so certificate validation is enabled unless the user explicitly disables it; update any usage or docs referencing fetch_verify_ssl to reflect the secure-by-default behavior.
258-259:⚠️ Potential issue | 🟠 MajorGuard
metadatawhen it is explicitlynull.
payload.get("metadata", {})still returnsNonewhen the key exists withnull, so.get("websearch", ...)can raise.🐛 Proposed fix
- metadata = payload.get("metadata", {}) + metadata = payload.get("metadata") or {} + if not isinstance(metadata, dict): + metadata = {} use_websearch = metadata.get("websearch", False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/pipeline.py` around lines 258 - 259, The code assumes metadata is a dict but payload may contain "metadata": null; change the guard so metadata is a dict before calling .get — e.g. replace metadata = payload.get("metadata", {}) with metadata = payload.get("metadata") or {} (or validate with isinstance(metadata, dict) and fallback to {}), then compute use_websearch = metadata.get("websearch", False); this ensures metadata is never None before calling metadata.get.
🧹 Nitpick comments (1)
openrag/components/pipeline.py (1)
10-11: Use absoluteopenrag.*imports for new websearch/context additions.These added imports use non-absolute paths and a relative intra-package import; align them to
openrag.*.♻️ Proposed fix
-from components.websearch import WebSearchService -from components.websearch.providers import StaanProvider +from openrag.components.websearch import WebSearchService +from openrag.components.websearch.providers import StaanProvider @@ -from .utils import format_context, format_web_context +from openrag.components.utils import format_context, format_web_context @@ - from components.websearch.content_fetcher import ContentFetcher + from openrag.components.websearch.content_fetcher import ContentFetcherAs per coding guidelines: "Use absolute imports from the
openrag/directory as the Python path root, avoiding relative imports across packages."Also applies to: 21-21, 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openrag/components/pipeline.py` around lines 10 - 11, The imports for WebSearchService and StaanProvider use intra-package paths; change them to absolute imports rooted at the package (e.g. import WebSearchService from openrag.components.websearch and StaanProvider from openrag.components.websearch.providers) and update any other similar import occurrences in this file (references to WebSearchService, StaanProvider, or other websearch/context additions around the same sections) so all new websearch/context imports follow the openrag.* absolute import convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openrag/components/websearch/content_fetcher.py`:
- Line 125: The failing Ruff formatting centers on the httpx.AsyncClient(...)
call in content_fetcher.py; run ruff format --fix (or manually reformat) to
adjust the AsyncClient call spacing/line breaks so it matches project style
(e.g., break long argument list across lines and normalize whitespace) for the
line containing httpx.AsyncClient(timeout=timeout, verify=self.verify_ssl,
headers={"User-Agent": _USER_AGENT}); ensure the call remains inside the async
with block and preserves the variables httpx.AsyncClient, timeout,
self.verify_ssl, and _USER_AGENT.
---
Duplicate comments:
In `@openrag/components/pipeline.py`:
- Line 112: The code sets verify_ssl via
config.websearch.get("fetch_verify_ssl", False) which defaults to False and
makes TLS validation insecure by default; change the default to True so
verify_ssl=config.websearch.get("fetch_verify_ssl", True) (or invert to an
explicit opt-out like fetch_verify_ssl_disabled) so certificate validation is
enabled unless the user explicitly disables it; update any usage or docs
referencing fetch_verify_ssl to reflect the secure-by-default behavior.
- Around line 258-259: The code assumes metadata is a dict but payload may
contain "metadata": null; change the guard so metadata is a dict before calling
.get — e.g. replace metadata = payload.get("metadata", {}) with metadata =
payload.get("metadata") or {} (or validate with isinstance(metadata, dict) and
fallback to {}), then compute use_websearch = metadata.get("websearch", False);
this ensures metadata is never None before calling metadata.get.
In `@openrag/components/websearch/content_fetcher.py`:
- Around line 50-57: The _is_loopback_url function currently allows hostnames
without resolving them and you also follow redirects without re-checking
targets; update _is_loopback_url to resolve the hostname (e.g., via
socket.getaddrinfo or a DNS resolver) and verify every resolved IP (IPv4/IPv6)
is global (reject if any are non-global), handle empty/None host safely, and
ensure any HTTP redirect-following code re-evaluates the redirected URL with the
same _is_loopback_url checks before following (reject requests to
internal/metadata addresses).
- Around line 82-89: The current code mutates the lxml tree while iterating
(tree.iter(tag)) which can skip siblings and also swallows all exceptions;
change the removal loop to iterate over a static list of nodes (e.g., for el in
list(tree.iter(tag)) or using tree.xpath to collect elements first) and remove
each via el.getparent().remove(el), and replace the broad "except Exception:
pass" with catching specific parse errors (e.g., lxml.etree.ParserError) or
"except Exception as e" and surface the error (log with logger.exception or
re-raise) so parse/removal failures are not silently ignored; refer to
_BOILERPLATE_TAGS, lxml.html.fromstring, tree.iter and lxml.html.tostring to
locate the code to change.
---
Nitpick comments:
In `@openrag/components/pipeline.py`:
- Around line 10-11: The imports for WebSearchService and StaanProvider use
intra-package paths; change them to absolute imports rooted at the package (e.g.
import WebSearchService from openrag.components.websearch and StaanProvider from
openrag.components.websearch.providers) and update any other similar import
occurrences in this file (references to WebSearchService, StaanProvider, or
other websearch/context additions around the same sections) so all new
websearch/context imports follow the openrag.* absolute import convention.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.hydra_config/config.yamlopenrag/components/pipeline.pyopenrag/components/websearch/content_fetcher.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .hydra_config/config.yaml
paultranvan
force-pushed
the
feat/websearch
branch
2 times, most recently
from
March 2, 2026 13:04
0f3cdec to
6e99244
Compare
Ahmath-Gadji
reviewed
Mar 9, 2026
| Any field not provided falls back to the default OpenRAG LLM configuration. | ||
| | Option | Type | Default | Description | | ||
| |--------|------|---------|-------------| | ||
| | `websearch` | `bool` | `false` | Augments the RAG context with live web search results. When used with a partition (`openrag-{partition}`), document and web results are combined. When used without a partition (direct LLM mode), web results are the sole context. Requires `WEBSEARCH_API_TOKEN` to be configured. See [web search configuration](/openrag/documentation/env_vars/#web-search-configuration). | |
Collaborator
There was a problem hiding this comment.
It would nice to add the websearch metadata here like use_map_reduce
openrag/openrag/models/openai.py
Lines 18 to 34 in 6e99244
Collaborator
| title = sanitize_text(result.title) | ||
| url = result.url | ||
| body = sanitize_text(result.content) if result.content else sanitize_text(result.snippet) | ||
| parts.append(f"[Source {n}]\n{title}\n{url}\n{body}") |
Collaborator
There was a problem hiding this comment.
As suggested here #253 (comment)
do mind the websearch_max_tokens value while adding web sources.
Collaborator
There was a problem hiding this comment.
I wonder if it's necessary to add the url here: "[Source {n}]\n{title}\n{url}\n{body}"
Long URLs can be token expensive
Collaborator
Author
There was a problem hiding this comment.
Fair point, URL is not necessary, fixed in e6759c7
Ahmath-Gadji
reviewed
Mar 9, 2026
Comment on lines
+70
to
+79
| websearch: | ||
| api_token: ${oc.env:WEBSEARCH_API_TOKEN, ""} | ||
| base_url: ${oc.env:WEBSEARCH_BASE_URL, "https://api.staan.ai/search/web"} | ||
| top_k: ${oc.decode:${oc.env:WEBSEARCH_TOP_K, 5}} | ||
| lang: ${oc.env:WEBSEARCH_LANG, fr-FR} | ||
| fetch_content: ${oc.decode:${oc.env:WEBSEARCH_FETCH_CONTENT, true}} | ||
| fetch_max_results: ${oc.decode:${oc.env:WEBSEARCH_FETCH_MAX_RESULTS, 3}} | ||
| fetch_timeout: ${oc.decode:${oc.env:WEBSEARCH_FETCH_TIMEOUT, 1.0}} | ||
| fetch_max_tokens: ${oc.decode:${oc.env:WEBSEARCH_FETCH_MAX_TOKENS, 500}} | ||
| fetch_verify_ssl: ${oc.decode:${oc.env:WEBSEARCH_FETCH_VERIFY_SSL, false}} |
Collaborator
There was a problem hiding this comment.
To keep the architecture flexible and future-proof, I suggest preparing the config structure following the same pattern as the retriever config:
- Create a
websearchsubfolder in.hydra_config. - Add a
base.yamlwith shared web search parameters (base_url,api_token,lang). - Add a
staan.yamlthat inherits frombase.yamland defines Staan-specific fields (including aproviderfield, similar totypein the retriever config). - Load the web search provider in
config.yamlvia an environment variable:
- websearch: ${oc.env:WEBSEARCH_PROVIDER, staan}This way, we can slot in alternative providers with minimal friction whenever needed.
Ahmath-Gadji
reviewed
Mar 9, 2026
Comment on lines
+1
to
+4
| from .base import BaseWebSearchProvider as BaseWebSearchProvider | ||
| from .base import WebResult as WebResult | ||
| from .content_fetcher import ContentFetcher as ContentFetcher | ||
| from .service import WebSearchService as WebSearchService |
Collaborator
There was a problem hiding this comment.
With the suggested config refacto, the init of the websearch could follow the logic implemented in embedder and the retriver:
openrag/openrag/components/indexer/embeddings/__init__.py
Lines 1 to 21 in 5de9a99
openrag/openrag/components/retriever.py
Lines 302 to 320 in 5de9a99
Ahmath-Gadji
reviewed
Mar 9, 2026
Comment on lines
+95
to
+118
| # Web search | ||
| ws_token = config.websearch.get("api_token", "") | ||
| if ws_token: | ||
| provider = StaanProvider( | ||
| api_token=ws_token, | ||
| base_url=config.websearch.get("base_url", "https://api.staan.ai/search/web"), | ||
| top_k=config.websearch.get("top_k", 5), | ||
| lang=config.websearch.get("lang", "fr-FR"), | ||
| ) | ||
| content_fetcher = None | ||
| if config.websearch.get("fetch_content", True): | ||
| from components.websearch.content_fetcher import ContentFetcher | ||
|
|
||
| content_fetcher = ContentFetcher( | ||
| max_results=config.websearch.get("fetch_max_results", 3), | ||
| timeout=config.websearch.get("fetch_timeout", 1.0), | ||
| max_tokens_per_page=config.websearch.get("fetch_max_tokens", 500), | ||
| verify_ssl=config.websearch.get("fetch_verify_ssl", False), | ||
| ) | ||
| self.web_search_service = WebSearchService(provider=provider, content_fetcher=content_fetcher) | ||
| logger.info("Web search enabled", fetch_content=content_fetcher is not None) | ||
| else: | ||
| self.web_search_service = WebSearchService(provider=None) | ||
| logger.info("Web search disabled (WEBSEARCH_API_TOKEN not set)") |
Collaborator
There was a problem hiding this comment.
I think the websearch init logic could be inside the WebSearchService class and during init we only have to provide the config object.
Ahmath-Gadji
reviewed
Mar 9, 2026
Comment on lines
151
to
153
|
|
||
| async def _prepare_for_chat_completion(self, partition: list[str], payload: dict): | ||
| async def _prepare_for_chat_completion(self, partition: list[str] | None, payload: dict): | ||
| messages = payload["messages"] |
Collaborator
There was a problem hiding this comment.
And inside format_web_context we fill up the web content while respecting this limit websearch_max_tokens
Comment on lines
+134
to
+138
| { | ||
| "source_type": "web", | ||
| "url": url, | ||
| "title": sanitize_text(result.title), | ||
| "snippet": sanitize_text(result.snippet), |
Collaborator
There was a problem hiding this comment.
what's missing on chainlit frontend is source handling of this type.
Lines 161 to 169 in 5de9a99
The implementation could go like this:
for i, s in enumerate(metadata_sources):
if s.get("source_type") == "web":
title = s.get("title") or s.get("url", f"Web source {i + 1}")
url = s.get("url", "")
snippet = s.get("snippet", "")
content = f"**[{title}]({url})**\n\n{snippet}"
source_name = title
# Deduplicate names
if source_name in d:
source_name = f"{title} ({i})"
d[source_name] = cl.Text(content=content, name=source_name, display="side")
continue
Ahmath-Gadji
reviewed
Mar 9, 2026
Comment on lines
+196
to
+208
| context, included_indices = format_context(docs, max_context_tokens=self.max_context_tokens) | ||
| docs = [docs[i] for i in included_indices] | ||
|
|
||
| # Avoid misleading "No document found" when web results will provide context | ||
| if not docs and web_results: | ||
| context = "" | ||
|
|
||
| # Append web results as additional sources with continuous numbering | ||
| if web_results: | ||
| n_rag_sources = len(docs) | ||
| web_formatted, _ = format_web_context(web_results, start_index=n_rag_sources + 1) | ||
| sep = "-" * 10 + "\n\n" | ||
| context = f"{context}{sep}{web_formatted}" if context else web_formatted |
Collaborator
There was a problem hiding this comment.
With this implementation, max_context_tokens applies only to RAG documents and not to web search results. For better token control, it could instead be treated as a global constraint over all retrieved content.
In this approach, the total context budget would be shared between RAG results and web search results. When web results are present, we would reserve part of the token budget for them and trim the RAG context accordingly.
max_context_tokens = (
self.max_context_tokens - websearch_max_tokens
if web_results
else self.max_context_tokens
)
context, included_indices = format_context(
docs,
max_context_tokens=max_context_tokens
)- websearch_max_tokens is the length (in terms of tokens) of search results combined.
Collaborator
There was a problem hiding this comment.
And inside format_web_context we fill up the web content while respecting this limit websearch_max_tokens
Collaborator
Author
There was a problem hiding this comment.
That's a very good point, fixed here: 3bdfe51
Add optional web search augmentation with a pluggable provider architecture, allowing the LLM to combine RAG document context with live web results. Clients enable it via metadata.websearch=true in chat completion requests. - Add websearch module (WebSearchService, BaseWebSearchProvider, StaanProvider) - Generic config: WEBSEARCH_API_TOKEN, WEBSEARCH_BASE_URL, WEBSEARCH_LANG - Concurrent RAG + web search via asyncio.gather() in combined mode - Web-only mode (no partition) with graceful fallback to plain LLM when no web results are available - Web results treated as regular document sources with continuous [Source N] numbering and source_type field in API response - Top-level sanitize_text import in utils.py - Fix mock_vllm path in CLAUDE.md, add web search documentation
- Add web search section to Key Features page - Add web search env vars (WEBSEARCH_*) to env vars reference - Add websearch metadata option and curl examples to API docs
Restructure the extra arguments section as a table with metadata field prominently mentioned in the intro sentence.
Block localhost and 127.* URLs before fetching to prevent requests to local services.
payload.get("metadata", {}) returns None when the key exists with a
null value, causing AttributeError on the subsequent .get() call.
paultranvan
force-pushed
the
feat/websearch
branch
2 times, most recently
from
March 9, 2026 16:19
a71afa6 to
834d3e8
Compare
paultranvan
force-pushed
the
feat/websearch
branch
from
March 9, 2026 16:25
834d3e8 to
3bdfe51
Compare
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.
Add web search capacity, based on staan API
Summary by CodeRabbit
New Features
Documentation
Tests
Chores