Skip to content

fix(providers): use hostname comparison for api.openai.com api_mode detection - #43341

Closed
cjakma wants to merge 1 commit into
NousResearch:mainfrom
cjakma:fix/providers-openai-hostname-api-mode
Closed

fix(providers): use hostname comparison for api.openai.com api_mode detection#43341
cjakma wants to merge 1 commit into
NousResearch:mainfrom
cjakma:fix/providers-openai-hostname-api-mode

Conversation

@cjakma

@cjakma cjakma commented Jun 10, 2026

Copy link
Copy Markdown

What changed and why

determine_api_mode was using "api.openai.com" in url_lower (string containment) which can match URLs that merely include that string as a substring — for example a reverse-proxy whose hostname ends with .api.openai.com.evil.com.

Switch to base_url_hostname(base_url) == "api.openai.com" so only the exact hostname triggers codex_responses mode.

How to test

from hermes_cli.providers import determine_api_mode
assert determine_api_mode("openai", "https://api.openai.com/v1") == "codex_responses"
assert determine_api_mode("openai", "https://proxy.api.openai.com.evil.com/v1") != "codex_responses"
assert determine_api_mode("openai", "https://my-api.openai.com.proxy.internal/v1") != "codex_responses"

Platforms tested

  • Linux (Ubuntu 22.04)

Changes

  • hermes_cli/providers.py: 1-line change, in url_lowerbase_url_hostname(base_url) ==

…etection

String containment ("api.openai.com" in url_lower) can match URLs that
merely include the string as a substring (e.g. a proxy whose hostname ends
in .api.openai.com.evil.com). Switch to base_url_hostname(base_url) ==
"api.openai.com" so only the exact hostname triggers codex_responses mode.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard provider/openai OpenAI / Codex Responses API duplicate This issue or pull request already exists labels Jun 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #41136 — that open PR makes the identical api.openai.com hostname-hardening change in the same determine_api_mode known-provider branch ("api.openai.com" in url_lowerbase_url_hostname(...) == "api.openai.com"), and additionally hardens the sibling api.kimi.com/coding and api.anthropic.com checks in the same branch. #41136 is the earlier, more comprehensive fix.

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification

Reviewed the diff — this is a clean and correct fix.

Before: substring match "api.openai.com" in url_lower would match any URL containing that string (e.g. https://my-proxy.com/api.openai.com/v1, https://api.openai.com.evil.com).

After: exact hostname comparison via base_url_hostname(base_url) == "api.openai.com" restricts to the actual OpenAI endpoint.

Single-line change with clear security improvement. LGTM.

@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 hostname hardening. The submitted line correctly addresses a live false positive in the known-provider branch at hermes_cli/providers.py:560.

Problems

  • No regression test covers this branch: tests/hermes_cli/test_determine_api_mode_hostname.py:15-43 uses provider="", while this PR changes the get_provider(provider) path at hermes_cli/providers.py:550-562.
  • The same branch still has raw substring checks for Kimi and Anthropic at hermes_cli/providers.py:556-558. The earlier open PR #41136 applies parsed-host matching to all three sibling checks while preserving /anthropic behavior.

Suggested changes

  • Add known-provider cases for the native OpenAI host, a path-style proxy, and a lookalike hostname.
  • Consolidate the sibling hostname hardening so this branch has one consistent parsed-host policy.

This is an automated hermes-sweeper review.

Comment thread hermes_cli/providers.py
if url_lower.endswith("/anthropic") or "api.anthropic.com" in url_lower:
return "anthropic_messages"
if "api.openai.com" in url_lower:
if base_url_hostname(base_url) == "api.openai.com":

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.

Please add a regression test through the known-provider path (get_provider(provider) is not None); the existing hostname tests use provider="" and only cover the adjacent unknown-provider branch.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Thanks @cjakma — you had this right: substring matching on api.openai.com was the spoofable pattern, and hostname-parsed comparison is the correct fix.

Current main already carries the exact-hostname check in host_mandated_api_mode() (landed via the #32243 hardening), and as of PR #74958 (merged in 008f1ef) the whole lane now routes through a shared is_official_openai_host() predicate that keeps the hostname-parsed spoof rejection you proposed while also recognizing OpenAI's data-residency regional hosts (us./eu.api.openai.com).

Closing as implemented on main — your PR identified the right defect first. Appreciated!

@teknium1 teknium1 closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists provider/openai OpenAI / Codex Responses API sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants