Skip to content

Harden integrations outbound egress policy #2604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rmusser01 merged 2 commits into from
Jul 4, 2026
Merged

Harden integrations outbound egress policy #2604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3873b9
Harden integrations outbound egress policy
rmusser01 Jul 4, 2026
13010ef
Preserve PubMed client reuse in egress audit
rmusser01 Jul 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions Docs/superpowers/plans/2026-07-04-audit-integrations-egress-remediation-plan.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
## Stage 1: Regression Tests
**Goal**: Capture the outbound-policy bypasses before changing production code.
**Success Criteria**: Tests fail for direct workflow `pdf_url` download and tokenizer `_http_post` bypassing the central HTTP policy.
**Tests**: Focused pytest cases in `tldw_Server_API/tests/Workflows/adapters/test_research_adapters.py` and `tldw_Server_API/tests/Writing/test_tokenizer_resolver_unit.py`.
**Status**: Complete

## Stage 2: Workflow Research HTTP Policy
**Goal**: Route project-owned research adapter HTTP calls through the central HTTP helper layer.
**Success Criteria**: Direct `pdf_url`, PubMed, Semantic Scholar, patent, and DOI requests use central egress/proxy/trust-env behavior while preserving existing response shapes.
**Tests**: Research adapter regression tests plus existing focused research adapter tests.
**Status**: Complete

## Stage 3: Tokenizer Resolver HTTP Policy
**Goal**: Replace raw tokenizer/counting `requests.post` use with the central sync HTTP helper path.
**Success Criteria**: Tokenizer provider calls inherit central egress denial, redirect checks, proxy validation, and `trust_env=False` defaults; explicit local provider behavior remains testable.
**Tests**: Tokenizer resolver regression tests plus existing unit tests for remote count/tokenizer adapters.
**Status**: Complete

## 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.

@qodo-code-review qodo-code-review Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

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

**Tests**: `python -m pytest` focused files, `git diff --check`, and Bandit on touched production files.
**Status**: Complete
74 changes: 74 additions & 0 deletions backlog/tasks/task-12146 - Harden-audit-Integrations-outbound-HTTP-policy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
---
id: TASK-12146
title: Harden audit Integrations outbound HTTP policy
status: Done
created_date: 2026-07-04 01:32
labels:
- audit-remediation
- security
- integrations
- egress
priority: medium
references:
- AUDIT-2026-06-27-INTEGRATIONS-001
- AUDIT-2026-06-27-INTEGRATIONS-002
- https://github.com/rmusser01/tldw_server/pull/2604
documentation:
- Docs/superpowers/reviews/2026-06-27-repo-audit/domains/integrations-providers.md
- Docs/superpowers/reviews/2026-06-27-repo-audit/remediation-backlog-draft.md
modified_files:
- Docs/superpowers/plans/2026-07-04-audit-integrations-egress-remediation-plan.md
- 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
- tldw_Server_API/tests/Workflows/adapters/test_research_adapters.py
- tldw_Server_API/tests/Writing/test_tokenizer_resolver_unit.py
updated_date: 2026-07-04 19:08
---

## Description

<!-- SECTION:DESCRIPTION:BEGIN -->
Remediate comprehensive audit findings AUDIT-2026-06-27-INTEGRATIONS-001 and AUDIT-2026-06-27-INTEGRATIONS-002. Route workflow research adapter outbound calls and tokenizer resolver provider/token-count requests through the central outbound HTTP policy instead of raw clients, while preserving explicit local-provider behavior where supported.
<!-- SECTION:DESCRIPTION:END -->

## Acceptance Criteria
<!-- AC:BEGIN -->
- [x] #1 Workflow research adapter direct pdf_url downloads go through central egress/proxy/trust_env controls and deny private/loopback targets by policy.
- [x] #2 Workflow research adapter first-party HTTP calls use central HTTP helpers or client factories rather than raw httpx clients.
- [x] #3 Tokenizer resolver _http_post uses the central sync HTTP helper/client path instead of requests.post directly.
- [x] #4 Regression tests cover private/loopback denial, direct pdf_url handling, central client/proxy defaults, and tokenizer base URL handling.
- [x] #5 Focused pytest, diff check, and Bandit verification are recorded before finalizing.
<!-- AC:END -->

## Implementation Plan

<!-- SECTION:PLAN:BEGIN -->
1. Add failing regression tests for arxiv_download direct pdf_url egress denial and tokenizer _http_post central egress/proxy behavior.
2. Replace workflow research raw httpx call sites with central async HTTP helpers or client factories, keeping existing response behavior.
3. Replace tokenizer_resolver._http_post requests.post usage with the central sync fetch path.
4. Run focused pytest, diff checks, and Bandit on touched production files; record results in the task final summary.
<!-- SECTION:PLAN:END -->

## Implementation Notes

<!-- SECTION:IMPLEMENTATION_NOTES:BEGIN -->
Implemented via isolated worktree codex/audit-integrations-egress-2026-07-04. Verification run: focused pytest for research adapter and tokenizer resolver tests passed (116 passed); git diff --check passed; Bandit on touched production files exited 0 and wrote /tmp/bandit_integrations_egress.json. Also confirmed targeted raw httpx.AsyncClient and requests.post patterns were removed from touched paths.
Latest-dev validation refresh (2026-07-04): original branch was stale at merge-base 800a81b5d7f8c941969c915274eb9f878bc2c207, then rebased cleanly onto origin/dev fd5c152b065c408e4e8ee5f08da41589f21cb7f5. Post-rebase merge-base matched origin/dev. Addressed PR review feedback by letting `_managed_afetch` accept an optional pre-existing async client and restoring PubMed search/summary to share one `create_async_client(timeout=30)` context while still calling central `afetch`. Added `test_pubmed_search_adapter_reuses_central_http_client`, which verifies one central client context and two fetch calls using the same client. Passed targeted PubMed reuse test (1 passed). 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` with 117 passed. Passed Bandit over touched production files with 0 findings in `/tmp/bandit_integrations_egress_latest_dev.json`. Passed `git diff --check`. Raw-client scan over touched production files for `httpx.AsyncClient`, `httpx.Client`, `requests.`, `urllib.request`, and `urlopen(` returned no matches.
<!-- SECTION:IMPLEMENTATION_NOTES:END -->

## Final Summary

<!-- SECTION:FINAL_SUMMARY:BEGIN -->
Routed workflow research adapter direct HTTP calls for direct arXiv PDF download, PubMed, Semantic Scholar, Google Patents, and DOI resolution through the central HTTP helper path with managed clients that inherit trust_env=False and central egress/proxy enforcement. Replaced tokenizer resolver raw requests.post usage with the central sync fetch helper. Added regression coverage for direct pdf_url central-helper use, private URL denial through central policy, tokenizer central fetch use, tokenizer private URL denial, and PubMed shared-client reuse. Latest validation was refreshed after rebasing onto origin/dev fd5c152b065c408e4e8ee5f08da41589f21cb7f5: 117 focused tests passed, Bandit over touched production files reported 0 findings, `git diff --check` passed, and touched production files no longer contain the scanned raw-client patterns.
<!-- SECTION:FINAL_SUMMARY:END -->

## Definition of Done
<!-- DOD:BEGIN -->
- [x] #1 Acceptance criteria completed
- [x] #2 Tests or verification recorded
- [x] #3 Documentation updated when relevant
- [x] #4 Bandit run for touched code when applicable or document non-code/environment skip
- [x] #5 Final summary added
- [x] #6 Known skips or blockers documented
<!-- DOD:END -->
16 changes: 11 additions & 5 deletions tldw_Server_API/app/core/LLM_Calls/tokenizer_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import json
import math
import os
from contextlib import suppress
from dataclasses import dataclass
from datetime import datetime, timezone
from functools import lru_cache
Expand Down Expand Up @@ -650,10 +651,17 @@ def strict_token_counting_enabled(default: bool = False) -> bool:

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,
)
Comment on lines 652 to +664

@qodo-code-review qodo-code-review Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

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



def normalize_provider_for_tokenizer(provider: str) -> str:
Expand Down Expand Up @@ -883,10 +891,8 @@ def _runtime_probe_exact_resolution(
)
finally:
if original_timeout is not None:
try:
with suppress(Exception):
setattr(encoding, "timeout_seconds", original_timeout)
Comment on lines +894 to 895

@qodo-code-review qodo-code-review Bot Jul 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

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

except Exception:
pass

return resolution

Expand Down
61 changes: 33 additions & 28 deletions tldw_Server_API/app/core/Workflows/adapters/research/bibliography.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

from loguru import logger

from tldw_Server_API.app.core.http_client import afetch, create_async_client
from tldw_Server_API.app.core.testing import is_test_mode
from tldw_Server_API.app.core.Workflows.adapters._common import _extract_openai_content
from tldw_Server_API.app.core.Workflows.adapters._registry import registry
Expand All @@ -26,6 +27,12 @@
)


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)


@registry.register(
"doi_resolve",
category="research",
Expand All @@ -43,8 +50,6 @@ async def run_doi_resolve_adapter(config: dict[str, Any], context: dict[str, Any
- metadata: dict - Paper metadata
- resolved: bool
"""
import httpx

from tldw_Server_API.app.core.Chat.prompt_template_manager import apply_template_to_string as _tmpl

if callable(context.get("is_cancelled")) and context["is_cancelled"]():
Expand Down Expand Up @@ -91,32 +96,32 @@ async def run_doi_resolve_adapter(config: dict[str, Any], context: dict[str, Any
}

try:
async with httpx.AsyncClient() as client:
response = await client.get(
f"https://doi.org/{doi}",
headers={"Accept": "application/vnd.citationstyles.csl+json"},
follow_redirects=True,
timeout=30,
)
response.raise_for_status()
data = response.json()

metadata = {
"doi": doi,
"title": data.get("title", ""),
"authors": [
f"{a.get('given', '')} {a.get('family', '')}".strip()
for a in data.get("author", [])
],
"container_title": data.get("container-title", ""),
"publisher": data.get("publisher", ""),
"issued": data.get("issued", {}).get("date-parts", [[]])[0],
"type": data.get("type", ""),
"abstract": data.get("abstract", ""),
"url": data.get("URL", ""),
}

return {"metadata": metadata, "resolved": True}
response = await _managed_afetch(
method="GET",
url=f"https://doi.org/{doi}",
headers={"Accept": "application/vnd.citationstyles.csl+json"},
allow_redirects=True,
timeout=30,
)
response.raise_for_status()
data = response.json()

metadata = {
"doi": doi,
"title": data.get("title", ""),
"authors": [
f"{a.get('given', '')} {a.get('family', '')}".strip()
for a in data.get("author", [])
],
"container_title": data.get("container-title", ""),
"publisher": data.get("publisher", ""),
"issued": data.get("issued", {}).get("date-parts", [[]])[0],
"type": data.get("type", ""),
"abstract": data.get("abstract", ""),
"url": data.get("URL", ""),
}

return {"metadata": metadata, "resolved": True}

except Exception:
logger.error("DOI resolve error")
Expand Down
Loading
Loading