Skip to content

fix(providers): bound the /models catalog response size in fetch_models - #42930

Open
youngstar-eth wants to merge 1 commit into
NousResearch:mainfrom
youngstar-eth:fix/fetch-models-size-limit
Open

fix(providers): bound the /models catalog response size in fetch_models#42930
youngstar-eth wants to merge 1 commit into
NousResearch:mainfrom
youngstar-eth:fix/fetch-models-size-limit

Conversation

@youngstar-eth

Copy link
Copy Markdown
Contributor

What & why

ProviderProfile.fetch_models did json.loads(resp.read().decode()) with no size limit. The endpoint URL comes from the provider profile's base_url/models_url, which is operator/config-controlled (e.g. a self-hosted or community OpenAI-compatible relay). A misconfigured or compromised endpoint could stream a multi-hundred-MB body within the request timeout and force the CLI/gateway to buffer and JSON-parse an unbounded blob (memory spike / OOM); a timeout bounds wall-clock, not size.

Check Content-Length and cap the read at _MAX_MODELS_RESPONSE_BYTES (16 MiB), returning None on an oversized response so callers degrade gracefully to the static fallback model list — mirroring the existing bounded-download pattern in tools/vision_tools.py.

Robustness / DoS-hardening fix.

How to test

pytest tests/providers/test_fetch_models_size_limit.py

Platforms

macOS (stdlib urllib, no platform-specific behaviour).

🤖 Generated with Claude Code

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists labels Jun 9, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Positive verification — clean size-bound guard for catalog responses.

The dual-check pattern (Content-Length header pre-check + post-read length guard) correctly handles both honest and adversarial servers. Using resp.read(max_bytes + 1) and then checking len(raw) > max_bytes is the standard bounded-read idiom — one byte over the limit triggers rejection without buffering an unbounded body.

Observations:

  • 16 MiB is generous for a model catalog (even providers with thousands of models produce ~100 KB JSON). The cap is defensive, not restrictive.
  • Returning None on oversize matches the documented contract: callers fall back to the static model list.
  • Tests are well-structured: _FakeResp with configurable Content-Length, monkeypatched _MAX_MODELS_RESPONSE_BYTES to keep allocations small.

No issues found.

fetch_models did `json.loads(resp.read().decode())` with no size limit. The
endpoint URL comes from the provider profile's base_url/models_url, which is
operator/config-controlled (e.g. a self-hosted or community OpenAI-compatible
relay). A misconfigured or compromised endpoint could stream a multi-hundred-
MB body within the request timeout and force the CLI/gateway to buffer and
JSON-parse an unbounded blob (memory spike / OOM). A timeout bounds
wall-clock, not size.

Check Content-Length and cap the read at _MAX_MODELS_RESPONSE_BYTES (16 MiB),
returning None on an oversized response so the caller degrades gracefully to
the static fallback model list — mirroring the existing bounded-download
pattern in tools/vision_tools.py.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@youngstar-eth
youngstar-eth force-pushed the fix/fetch-models-size-limit branch from 03ebfec to 53f8a21 Compare June 9, 2026 16:40

@egilewski egilewski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Recommendation: approve

I reviewed this against current GitHub main d1383a6b1450c6c139720b1b01f8b99cc130453f and PR head 53f8a21e8c828819c9032fe706676de6e80b822a.

Validation:

  • git rev-list --left-right --count upstream/main...refs/remotes/upstream/pr/42930 => 125 1; git merge-tree --write-tree upstream/main refs/remotes/upstream/pr/42930 wrote tree 40455b8434c91c3cb16661b8c277f13170bb5368; git diff --check upstream/main...refs/remotes/upstream/pr/42930 passed.
  • GitHub checks were all successful or skipped at final recheck.
  • Current main uses an unbounded resp.read().decode() in ProviderProfile.fetch_models(); this PR changes it to reject oversized Content-Length and read at most _MAX_MODELS_RESPONSE_BYTES + 1 bytes before JSON parsing.
  • python -B -m pytest -q tests/providers/test_fetch_models_size_limit.py -p no:cacheprovider passed: 3 tests.
  • python -B -m py_compile providers/base.py tests/providers/test_fetch_models_size_limit.py passed.
  • A direct probe with _MAX_MODELS_RESPONSE_BYTES = 16 confirmed fetch_models() calls read(17) rather than an unbounded read.
  • CodeRabbit ran successfully and reported only non-blocking test-harness/boundary suggestions; I did not find a blocker in the reviewed security scope.

Signed: GPT-5.5-xhigh in Codex

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the focused catalog-response hardening. The unbounded read remains on current main at providers/base.py:214, so the underlying issue is real.

Problems

  • The patch is based on an older implementation and changes the request open to urllib.request.urlopen (providers/base.py:214 in this PR). Current main intentionally uses open_credentialed_url at providers/base.py:213; that helper enforces credential stripping across cross-origin redirects (hermes_cli/urllib_security.py:112-132). Please retain that helper when adding the bounded read.
  • The added tests patch urllib.request.urlopen, but current main no longer invokes it from fetch_models; use the secure-opener path or the existing local-server style in tests/providers/test_fetch_models_base_url.py:11-39.
  • plugins/model-providers/anthropic/__init__.py:33 has a separate unbounded catalog read. Please cover it with the same cap or share the bounded-read implementation.

Suggested changes

  • Bound resp.read(max_bytes + 1) after open_credentialed_url(...), preserving the existing redirect-security behavior.
  • Add tests for declared and undeclared oversized bodies through the current production path.

Automated hermes-sweeper review.

Comment thread providers/base.py

# Bound the response so a misconfigured/hostile catalog endpoint
# can't force an unbounded in-memory buffer. A timeout limits
# wall-clock, not size. Oversized responses fall back to the static

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main opens this credential-bearing request through open_credentialed_url, which strips secrets on cross-origin redirects. Please apply the bounded read while retaining that helper rather than switching to urllib.request.urlopen; otherwise this stale patch regresses the redirect-security boundary.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants