Skip to content

feat(url_safety): add allow_benchmark_ips to exempt RFC 2544 benchmark range 198.18.0.0/15 - #26064

Open
Jensenwgd wants to merge 2 commits into
NousResearch:mainfrom
Jensenwgd:feat/allow-benchmark-ips
Open

feat(url_safety): add allow_benchmark_ips to exempt RFC 2544 benchmark range 198.18.0.0/15#26064
Jensenwgd wants to merge 2 commits into
NousResearch:mainfrom
Jensenwgd:feat/allow-benchmark-ips

Conversation

@Jensenwgd

Copy link
Copy Markdown

Summary

In DNS fake IP environments (common in China), / can resolve to IPs in (RFC 2544 benchmark range). Hermes correctly identifies these as non-private, but had no way to exempt them, causing false positives.

This PR adds an independent allow_benchmark_ips toggle to precisely control 198.18.0.0/15 without affecting other reserved ranges or SSRF protection.

Config

security:
  allow_benchmark_ips: false  # default off

Priority

  1. HERMES_ALLOW_BENCHMARK_IPS env var (highest)
  2. security.allow_benchmark_ips in config.yaml
  3. Default: false (benchmarks blocked)

What is NOT affected

  • allow_private_urls remains fully independent
  • 10.x / 192.168.x / 100.64.0.0/10 / 169.254.0.0/16 / cloud metadata retain full SSRF protection
  • 110/110 tests passing

…k range 198.18.0.0/15

Solves DNS fake IP misidentification where github.com/pypi.org resolve
to 198.18.x.x (IANA reserved for benchmark testing) and get blocked
as non-private reserved IPs.

Changes:
- Add allow_benchmark_ips config toggle (default false)
- Add HERMES_ALLOW_BENCHMARK_IPS env var (highest priority)
- 198.18.0.0/15 is now controllable independently from private URL protection
- Existing SSRF protection for 10.x, 192.168.x, 100.64.0.0/10 etc. unchanged
- 110 tests passing
@liuhao1024

Copy link
Copy Markdown
Contributor

Security: allow_benchmark silently disables SSRF protection for all private IPs

Nice work on the RFC 2544 benchmark range support -- the tests and toggle design are thorough. However, there is a logic bug in the condition:

if not allow_all_private and not allow_private_ip and not allow_benchmark and _is_blocked_ip(ip):

Because not allow_benchmark is evaluated before _is_blocked_ip(ip), setting allow_benchmark=true short-circuits the entire block for all IPs that _is_blocked_ip would reject -- including private IPs (10.x, 192.168.x), CGNAT (100.64.0.0/10), loopback, link-local, etc.

This means a user who only wants to allow 198.18.0.0/15 (benchmark) inadvertently opens access to the entire private address space.

Suggested fix -- narrow the bypass to only the benchmark range:

if not allow_all_private and not allow_private_ip and _is_blocked_ip(ip):
    if allow_benchmark and ip in _BENCHMARK_NETWORK:
        logger.debug(
            "Allowing benchmark-IP resolution (security.allow_benchmark_ips=true): %s",
            hostname,
        )
    else:
        logger.warning(
            "Blocked request to private/internal address: %s -> %s",
            hostname, ip_str,
        )
        return False

And keep the existing elif allow_all_private: / elif allow_private_ip: branches unchanged.

The existing tests don't catch this because they only test benchmark IPs -- a test with allow_benchmark=true and a private IP (e.g., 192.168.1.1) would expose the regression. Happy to provide a test diff if helpful.

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets area/config Config system, migrations, profiles labels May 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

The underlying issue (RFC 2544 range blocked) was already resolved by merged #14054 (security.allow_private_urls toggle). See #3777 and #19992. This PR adds a more granular allow_benchmark_ips toggle specifically for 198.18.0.0/15 — a narrower knob than the existing allow_private_urls, which is a valid enhancement but not urgent.

… for all private IPs

The original allow_benchmark shortcut was:

    if not allow_all_private and not allow_private_ip and not allow_benchmark and _is_blocked_ip(ip):

This means 'allow_benchmark=True' skips the entire _is_blocked_ip() check,
opening all private ranges (10.x, 192.168.x, CGNAT, etc.) — not just
198.18.0.0/15.

Correct structure (reviewer liuhao10264):

    if not allow_all_private and not allow_private_ip and _is_blocked_ip(ip):
        if allow_benchmark and ip in _BENCHMARK_NETWORK:
            ...  # benchmark IP only
        else:
            return False  # all other blocked IPs still blocked

Also: add tests/conftest.py to prevent HERMES_ALLOW_BENCHMARK_IPS env var
leaking into pytest-xdist worker processes during parallel test runs.
@Jensenwgd

Copy link
Copy Markdown
Author

@liuhao1024 Good catch — you are right. The original short-circuit not allow_benchmark before _is_blocked_ip() meant allow_benchmark=true bypassed the entire blocked-IP check, opening all private ranges (10.x, 192.168.x, CGNAT, etc.) in addition to 198.18.0.0/15.

Fixed in the latest push. The correct structure now:

if not allow_all_private and not allow_private_ip and _is_blocked_ip(ip):
    if allow_benchmark and ip in _BENCHMARK_NETWORK:
        ...  # only benchmark IPs bypass here
    else:
        return False  # all other blocked IPs still protected

Added regression tests:

  • test_allow_benchmark_does_not_bypass_private_ssrf: confirms 10.x / 192.168.x / 172.x are still blocked when allow_benchmark_ips=true
  • test_allow_benchmark_only_affects_benchmark_range: confirms CGNAT and link-local remain blocked

Also fixed test isolation issue in conftest.py where HERMES_ALLOW_BENCHMARK_IPS=true in the host environment was leaking into pytest-xdist worker processes.

@Jensenwgd

Copy link
Copy Markdown
Author

@alt-glitch Agreed — allow_private_urls (from #14054) is the broader solution. allow_benchmark_ips is a narrower, more targeted toggle specifically for the 198.18.0.0/15 range, so users who want DNS fake IP support don't need to fully disable SSRF protection for private address space. Happy to close if the maintainers prefer to extend the existing toggle rather than add a new one.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing a real granularity gap: current main still rejects a normal host resolving to 198.18.0.23 (tests/tools/test_url_safety.py:247-251), while the existing security.allow_private_urls path bypasses ordinary private-IP blocking (tools/url_safety.py:411-448).

Problems

  • The new HERMES_ALLOW_BENCHMARK_IPS mechanism is a user-facing, non-secret behavioral env var. AGENTS.md requires behavioral settings to use config.yaml; please keep security.allow_benchmark_ips as the supported surface.
  • The new cache reset in tests/conftest.py does not isolate a host-set env var. The autouse fixture clears only _HERMES_BEHAVIORAL_VARS (tests/conftest.py:341-343), and HERMES_ALLOW_BENCHMARK_IPS is not in that set; resetting the cache makes the host value get read again.
  • Add the default to DEFAULT_CONFIG beside security.allow_private_urls (hermes_cli/config.py:2607-2614) and document it in the security configuration reference (website/docs/user-guide/configuration.md:1826-1847).

Suggested changes

  • Remove the new env-var precedence path and its tests; preserve the narrowly scoped config toggle and SSRF regression coverage.
  • If env support remains, add it to _HERMES_BEHAVIORAL_VARS rather than relying on cache resets.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Jul 13, 2026
@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 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have 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 sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants