Harden integrations outbound egress policy#2604
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the outbound HTTP policy for audit integrations by routing workflow research adapters and tokenizer resolver requests through the central HTTP client helpers (fetch and afetch) instead of raw clients. This ensures compliance with egress policies, such as denying private or loopback targets. Unit and regression tests have been updated accordingly. The reviewer feedback recommends enhancing the _managed_afetch helper to optionally accept an existing client, which would allow reusing a single client across sequential requests (e.g., in PubMed search) to preserve connection pooling and reduce connection overhead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| async def _managed_afetch(**kwargs: Any) -> Any: | ||
| timeout = kwargs.get("timeout") | ||
| async with create_async_client(timeout=timeout) as client: | ||
| return await afetch(client=client, **kwargs) |
There was a problem hiding this comment.
To support reusing a single httpx.AsyncClient across multiple requests (which is highly recommended for connection pooling and performance), let's update the _managed_afetch helper to accept an optional pre-existing client parameter.
| async def _managed_afetch(**kwargs: Any) -> Any: | |
| timeout = kwargs.get("timeout") | |
| async with create_async_client(timeout=timeout) as client: | |
| return await afetch(client=client, **kwargs) | |
| async def _managed_afetch(client: Any = None, **kwargs: Any) -> Any: | |
| if client is not None: | |
| return await afetch(client=client, **kwargs) | |
| timeout = kwargs.get("timeout") | |
| async with create_async_client(timeout=timeout) as client: | |
| return await afetch(client=client, **kwargs) |
There was a problem hiding this comment.
Addressed in 13010ef. _managed_afetch now accepts an optional pre-existing async client and delegates to central afetch with that client instead of always creating its own context. Added adapter-level coverage proving PubMed passes the same client into both central fetch calls.
| # Search for IDs | ||
| search_url = f"{base_url}/esearch.fcgi" | ||
| search_params = { | ||
| "db": "pubmed", | ||
| "term": query, | ||
| "retmax": max_results, | ||
| "retmode": "json", | ||
| } | ||
| search_response = await _managed_afetch(method="GET", url=search_url, params=search_params, timeout=30) | ||
| search_data = search_response.json() | ||
|
|
||
| id_list = search_data.get("esearchresult", {}).get("idlist", []) | ||
| if not id_list: | ||
| return {"papers": [], "total_results": 0, "query": query} | ||
|
|
||
| # Fetch details | ||
| fetch_url = f"{base_url}/esummary.fcgi" | ||
| fetch_params = { | ||
| "db": "pubmed", | ||
| "id": ",".join(id_list), | ||
| "retmode": "json", | ||
| } | ||
| fetch_response = await _managed_afetch(method="GET", url=fetch_url, params=fetch_params, timeout=30) | ||
| fetch_data = fetch_response.json() |
There was a problem hiding this comment.
In the previous implementation, a single httpx.AsyncClient was created and reused for both the search and fetch requests. Reusing the client is highly recommended as it leverages connection pooling, avoiding the overhead of establishing a new TCP and SSL connection for each individual request.
Using _managed_afetch twice here creates and closes two separate clients sequentially, which introduces unnecessary latency and performance overhead.
We can restore the connection pooling behavior while still adhering to the central outbound HTTP policy by wrapping both requests in a single async with create_async_client(timeout=30) as client: block and passing the client directly to _managed_afetch.
async with create_async_client(timeout=30) as client:
# Search for IDs
search_url = f"{base_url}/esearch.fcgi"
search_params = {
"db": "pubmed",
"term": query,
"retmax": max_results,
"retmode": "json",
}
search_response = await _managed_afetch(client=client, method="GET", url=search_url, params=search_params, timeout=30)
search_data = search_response.json()
id_list = search_data.get("esearchresult", {}).get("idlist", [])
if not id_list:
return {"papers": [], "total_results": 0, "query": query}
# Fetch details
fetch_url = f"{base_url}/esummary.fcgi"
fetch_params = {
"db": "pubmed",
"id": ",".join(id_list),
"retmode": "json",
}
fetch_response = await _managed_afetch(client=client, method="GET", url=fetch_url, params=fetch_params, timeout=30)
fetch_data = fetch_response.json()There was a problem hiding this comment.
Addressed in 13010ef. PubMed search now wraps the ESearch and ESummary calls in one create_async_client(timeout=30) context while still routing both requests through _managed_afetch/central afetch. The new test_pubmed_search_adapter_reuses_central_http_client regression verifies one client context and two fetch calls sharing the same client.
Remediates TASK-12138 and audit findings AUDIT-2026-06-27-INTEGRATIONS-001/002 by routing workflow research and tokenizer HTTP requests through central HTTP policy helpers with regression coverage.
rmusser01
force-pushed
the
codex/audit-integrations-egress-2026-07-04
branch
from
883b6c4 to
13010ef
Compare
PR Summary by QodoHarden integrations outbound egress by routing adapter/provider HTTP via central helpers
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
74 rules 1. Blocking Path.write_bytes() in async
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| response.raise_for_status() | ||
| filename = pdf_url.split("/")[-1] or "paper.pdf" | ||
| output_path = str(art_dir / filename) | ||
| Path(output_path).write_bytes(response.content) |
There was a problem hiding this comment.
1. Blocking path.write_bytes() in async 📘 Rule violation ➹ Performance
run_arxiv_download_adapter() performs a synchronous file write (Path(...).write_bytes(...)) inside an async def, which can block the event loop and degrade concurrency/latency under load.
Agent Prompt
## Issue description
`Path(output_path).write_bytes(response.content)` is synchronous I/O executed inside an async adapter (`run_arxiv_download_adapter`). This can block the event loop.
## Issue Context
The compliance rule requires avoiding blocking synchronous I/O in async-capable code paths. Use an async file API or offload the blocking write to a thread.
## Fix Focus Areas
- tldw_Server_API/app/core/Workflows/adapters/research/search.py[203-207]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| with suppress(Exception): | ||
| setattr(encoding, "timeout_seconds", original_timeout) |
There was a problem hiding this comment.
2. Silent exception suppression in finally 📘 Rule violation ☼ Reliability
with suppress(Exception) silently ignores failures when restoring encoding.timeout_seconds, which can hide real errors and leave runtime state inconsistent without any contextual message.
Agent Prompt
## Issue description
The `finally` block uses `with suppress(Exception):` when restoring `encoding.timeout_seconds`, swallowing any exception without emitting a contextual error message.
## Issue Context
The compliance rule flags swallowing exceptions with empty handlers/suppression as a red flag. If restoration fails, either raise a contextual exception or record a contextual message (e.g., warning/log) so failures are observable.
## Fix Focus Areas
- tldw_Server_API/app/core/LLM_Calls/tokenizer_resolver.py[892-895]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def _http_post(*, url: str, payload: dict[str, Any], headers: dict[str, str], timeout: float) -> Any: | ||
| try: | ||
| import requests # type: ignore | ||
| from tldw_Server_API.app.core.http_client import fetch | ||
| except Exception as exc: | ||
| raise TokenizerUnavailable("Provider tokenizer HTTP client unavailable") from exc | ||
| return requests.post(url, json=payload, headers=headers, timeout=timeout) | ||
| return fetch( | ||
| method="POST", | ||
| url=url, | ||
| json=payload, | ||
| headers=headers, | ||
| timeout=timeout, | ||
| allow_redirects=True, | ||
| ) |
There was a problem hiding this comment.
3. Redirect leaks auth headers 🐞 Bug ⛨ Security
tokenizer_resolver._http_post sends provider Authorization/x-api-key headers through http_client.fetch with allow_redirects=True, and the central redirect loop follows Location hops without stripping those headers. If a provider responds with a cross-host redirect, the request will resend secrets to the redirect target.
Agent Prompt
### Issue description
`tokenizer_resolver._http_post` now uses the central `http_client.fetch(..., allow_redirects=True)` while passing headers that often contain secrets (e.g., `Authorization: Bearer ...`, `x-api-key`). The central sync redirect implementation follows redirects by updating `cur_url` and looping, but it does **not** clear/partition sensitive headers when the redirect target changes origin, so secrets can be resent to the redirected host.
### Issue Context
- `_http_post` is used by multiple tokenizer/count adapters and is invoked with credential-bearing headers.
- `http_client.fetch` (response-based API) implements manual redirect handling and reuses the original `headers` across hops.
### Fix Focus Areas
- Option A (local fix): disable redirects for tokenizer provider POSTs (set `allow_redirects=False`) unless explicitly required.
- Option B (central fix, preferred): in `http_client` redirect handling for response-based `fetch`/`afetch`, detect cross-origin redirects and either (1) strip sensitive headers (`Authorization`, cookies, API keys, etc.) and clear cookies, or (2) fail closed unless the caller explicitly opts in.
- tldw_Server_API/app/core/LLM_Calls/tokenizer_resolver.py[652-664]
- tldw_Server_API/app/core/http_client.py[2814-2843]
- tldw_Server_API/app/core/http_client.py[2941-2964]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| ## Stage 4: Verification And Task Record | ||
| **Goal**: Prove the remediation and record it for repeatable audit follow-up. | ||
| **Success Criteria**: Focused pytest passes, `git diff --check` passes, Bandit runs on touched production files, and `TASK-12138` records final verification. |
There was a problem hiding this comment.
4. Wrong task id in plan 🐞 Bug ⚙ Maintainability
The remediation plan’s Stage 4 success criteria still references TASK-12138, but this PR adds TASK-12146; this breaks audit traceability for the recorded verification step.
Agent Prompt
### Issue description
The plan document references the old task id `TASK-12138`, but the actual remediation task added in this PR is `TASK-12146`. This inconsistency makes it harder to trace verification artifacts back to the correct task.
### Issue Context
A new backlog task file exists with id `TASK-12146`.
### Fix Focus Areas
- Docs/superpowers/plans/2026-07-04-audit-integrations-egress-remediation-plan.md[19-22]
- backlog/tasks/task-12146 - Harden-audit-Integrations-outbound-HTTP-policy.md[1-4]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s; sync docs - Add tests/Chat_Macros/property/test_macro_parser_properties.py: 10 hypothesis properties for parse_macro_args/_coerce_bool/ load_macro_definition, importorskip-guarded so they skip on dev today and activate when codex/chat-macros-v1 (PR #2618) merges. Validated against that branch: 10/10 pass. - ci.yml: add tests/RPG (11 files, silently unsharded on dev — shard guard was red repo-wide; 177/177 pass locally) and tests/Chat_Macros (covers the prep file now and the macro branch's tests on merge) to the gap-verified-7 shard. - Audit/plan: egress (#2604) and ACP (#2619) fixes merged to dev today — update live-unfixed wording; chat-macros re-added as prep-done pending PR #2618. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Follow-up PR #2634 addresses the remaining review comments from this PR: blocking write_bytes in run_arxiv_download_adapter, allow_redirects=False for credential-bearing tokenizer POSTs (redirect header-leak), logged (not suppressed) timeout restore, and the TASK-12138→TASK-12146 plan traceability fix. |
…s; sync docs - Add tests/Chat_Macros/property/test_macro_parser_properties.py: 10 hypothesis properties for parse_macro_args/_coerce_bool/ load_macro_definition, importorskip-guarded so they skip on dev today and activate when codex/chat-macros-v1 (PR #2618) merges. Validated against that branch: 10/10 pass. - ci.yml: add tests/RPG (11 files, silently unsharded on dev — shard guard was red repo-wide; 177/177 pass locally) and tests/Chat_Macros (covers the prep file now and the macro branch's tests on merge) to the gap-verified-7 shard. - Audit/plan: egress (#2604) and ACP (#2619) fixes merged to dev today — update live-unfixed wording; chat-macros re-added as prep-done pending PR #2618. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Task 4 nightly shuffle: use -p pytest_randomly (full module name), not the entry-point alias, per review — more reliable under PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 (gemini review) - Add tracked follow-up: central http_client cross-origin redirect header stripping (qodo Option B from PR #2604), superseding the per-caller allow_redirects=False mitigation from PR #2634 - fake-indexeddb package.json comment was already addressed in a prior revision Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Addresses AUDIT-2026-06-27-INTEGRATIONS-001 and AUDIT-2026-06-27-INTEGRATIONS-002 / TASK-12146.
Change summary:
TASK-12138toTASK-12146to avoid collisions with current dev tasks.Latest-dev validation:
.venv/bin/python -m pytest tldw_Server_API/tests/Workflows/adapters/test_research_adapters.py::test_pubmed_search_adapter_reuses_central_http_client -q(1 passed)..venv/bin/python -m pytest tldw_Server_API/tests/Workflows/adapters/test_research_adapters.py tldw_Server_API/tests/Writing/test_tokenizer_resolver_unit.py -q(117 passed)..venv/bin/python -m bandit -r tldw_Server_API/app/core/Workflows/adapters/research/search.py tldw_Server_API/app/core/Workflows/adapters/research/bibliography.py tldw_Server_API/app/core/LLM_Calls/tokenizer_resolver.py -f json -o /tmp/bandit_integrations_egress_latest_dev.jsonwith 0 findings.git diff --check.httpx.AsyncClient,httpx.Client,requests.,urllib.request, andurlopen(returned no matches.Review notes:
_managed_afetch, wrapping PubMed requests in onecreate_async_client(timeout=30)context, and adding an adapter-level regression proving bothafetchcalls receive the same client.Draft until the required human-authored Change summary is added.