Skip to content

Fix: Accept Hermes addresses typed without https:// - #67

Merged
rayl15 merged 1 commit into
rayl15:mainfrom
raphaelbarreiros:fix/hermes-address-scheme
Oct 4, 2026
Merged

rayl15 merged 1 commit into
rayl15:mainfrom
raphaelbarreiros:fix/hermes-address-scheme

Conversation

@raphaelbarreiros

@raphaelbarreiros raphaelbarreiros commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

What & why

Found during the live API-key check reported on #65. A Hermes address typed without a scheme (myhost.ts.net, 100.88.1.2:8642) failed with "Enter the server address, e.g. https://hermes.example.com." It's easy to trip on, since most people type just the host.

Both Hermes address fields (the API server, and the web UI for username & password) now add the scheme themselves:

  • http:// for loopback, .local, private (RFC 1918) and Tailscale (100.64.0.0/10) IPs, where servers rarely have a certificate. The Hermes API server listens on plain http by default.
  • https:// for everything else, including *.ts.net (what tailscale serve gives you).

A scheme the user typed is kept. The plain-http rule from #63 is unchanged: no sign-in over plain http to a public host, and the API key warning still shows. The private-range check moved into isLocalHost so both paths share it. The leftover error now says what a valid address looks like.

How it was tested

iPhone 16 Pro Max, iOS 27.2.

  • Built and ran on a physical device (simulator can't do Bluetooth / on-device MLX)
  • Verified the specific behavior this PR changes: testAddressWithoutSchemeGetsOne covers host names, *.ts.net, Tailscale and LAN IPs with a port and profile path, the web UI base, and keeping a typed scheme. HermesTests: 25 tests, 0 failures on the device. API-key mode was also checked live against my Hermes server (result on Store API keys in the Keychain instead of settings.json #65).

Checklist

  • Code matches the style of the surrounding files (naming, comments, idioms)
  • No secrets, API keys, or personal data committed
  • Ran xcodegen generate if files were added/removed (no files added)
  • Updated docs / README / CHANGELOG if behavior changed (no doc says the scheme is required; CHANGELOG left for your release notes)

Breaking changes

None. Addresses that worked before parse the same way.

Notes for reviewers

No layout changes, only an error message's wording, so no screenshots.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Server addresses entered without a scheme now use HTTPS for hostnames and HTTP for localhost, private IPv4 addresses, and qualifying Tailscale IPs. Ports and profile paths are preserved.
    • Dashboard addresses also support automatic scheme selection, while explicitly supplied schemes remain unchanged.
  • Bug Fixes
    • Tailscale hostnames are no longer treated as local addresses; warnings for other remote HTTP connections remain.

An address without a scheme (myhost.ts.net, 100.88.1.2:8642) used to
fail with "Enter the server address". It now gets one: http:// for
loopback, .local, private and Tailscale IPs, https:// otherwise.
Applies to both the API server and the web UI address; the plain-http
rules for sign-in are unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: rayl15/OpenVision/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b1ce0260-42fc-41a7-8989-8984f9174684
📥 Commits

Reviewing files that changed from the base of the PR and between 89aba56 and b755069.

📒 Files selected for processing (3)
  • OpenVision/Services/AIBackend/HermesGateway.swift
  • OpenVision/Services/AIBackend/HermesService.swift
  • OpenVisionTests/HermesTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Walkthrough

Walkthrough

Hermes API and dashboard address handling now adds a scheme when none is supplied. Local addresses use HTTP, and other addresses use HTTPS. Explicit schemes remain unchanged. Tests cover inferred schemes, ports, profile paths, and URL rejection.

Changes

Hermes URL normalization

Layer / File(s) Summary
Address normalization and integration
OpenVision/Services/AIBackend/HermesService.swift, OpenVision/Services/AIBackend/HermesGateway.swift, OpenVisionTests/HermesTests.swift
withScheme adds HTTP for recognized local addresses and HTTPS for other scheme-free addresses. apiBase and HermesDashboard.base use the normalized address. .ts.net hosts are excluded from unencrypted-remote warnings but are no longer classified as local by isLocalHost. The .badURL examples and address tests reflect the updated handling.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Merge Risk: 🔵 Low · up to b7550

Scheme-free IPv6 loopback addresses may not connect to an HTTP-only Hermes server. This narrow case can be fixed in follow-up; the PR is otherwise mergeable.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to b7550

Address auto-detection can mistake certain public hostnames for private IP addresses and silently select unencrypted HTTP. This can expose credentials and conversation traffic to network interception. Exposure requires configuring an affected address; ordinary public hostnames still default to HTTPS.

Retained concerns

  • Medium · security · inferred: Automatic scheme selection treats public DNS names containing four private-looking numeric labels as local. For example, 10.1.2.3.attacker.example receives HTTP, is not flagged as unencrypted remote, and passes dashboard validation. This extends a pre-existing classification weakness into silent plaintext transport selection for newly accepted scheme-free inputs, exposing API credentials or dashboard authentication traffic to network interception.
Security review details

Security Blast Radius

  • inferred — The demonstrated exposure concerns the configured Hermes destination and the user's credentials and traffic sent there. API-key interception could transfer the key's server-side authority; the settings warning describes access to Hermes tools, including the terminal, but actual server permissions and tenant scope were not verified.

Security Findings and Attack Paths

  • inferred — A configured public hostname such as 10.1.2.3.attacker.example is reduced to four numeric labels, classified as local, and automatically given HTTP. The same classification suppresses the API warning and permits dashboard sign-in. A network observer able to intercept that connection could read or modify plaintext traffic. The pre-existing explicit-HTTP weakness is worsened by automatic selection; no live exploitation was demonstrated.

Trust Boundaries and Controls

  • observed — Ordinary public hostnames default to HTTPS, and dashboard validation rejects ordinary remote HTTP. API-key mode instead displays an informational warning without disabling connection tests. Explicit .ts.net HTTP remains exempt from the remote warning, as before this PR.

Resilience and Maintainability Implications

  • observed — Normal dashboard settings prevent address editing while signed in; sign-out disconnects the gateway and clears saved sessions. Failed WebSocket handshakes disconnect the partially established socket, and ticket refresh retries retain the same base/provider. However, socket reuse does not compare server identity, and saved sessions are keyed by conversation. These mechanisms predate the PR; reachability through settings changes outside normal cleanup remains unresolved.

Hardening Proposals

  • proposed — Base transport decisions on a parsed destination authority and strict IP-literal validation, without discarding nonnumeric labels. Reject ambiguous userinfo-bearing addresses and add regression cases for numeric-label public DNS names and bracketed IPv6.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: Hermes addresses can be entered without a scheme.
Description check ✅ Passed The description covers the change and reason, testing, checklist, breaking changes, and reviewer notes. It provides the required information from the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@raphaelbarreiros

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@raphaelbarreiros

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@rayl15 rayl15 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks, this is a clear win for the first-run experience. withScheme keeps a typed scheme, isUnencryptedRemote is behaviour-preserving (I checked each branch of the new isLocalHost against the old switch), and the dashboard base gets the same treatment, with the plain-http-to-a-public-host rule still enforced.

Two non-blocking nits for a later pass:

  • The two TextField placeholders in HermesSettingsView still read https://hermes.example.com. Now that the scheme is optional, hermes.example.com would match what the new error message teaches.
  • A dot-less host (hermes-box, nuc) is almost always a LAN machine without a certificate, so defaulting it to http:// like .local would help. Likewise a *.ts.net host typed with an explicit port is probably not tailscale serve (which answers on 443), so http is the better guess there.

Merging.

@rayl15 rayl15 closed this Oct 4, 2026
@rayl15 rayl15 reopened this Oct 4, 2026
@rayl15
rayl15 merged commit b0f65ec into rayl15:main Oct 4, 2026
2 checks passed
raphaelbarreiros added a commit to raphaelbarreiros/OpenVision that referenced this pull request Oct 4, 2026
Fork main = upstream main + the open PR rayl15#66 (rayl15#64 and rayl15#67 are merged upstream).
raphaelbarreiros added a commit to raphaelbarreiros/OpenVision that referenced this pull request Oct 4, 2026
rayl15 pushed a commit that referenced this pull request Oct 4, 2026
* Fix: Better scheme guesses for Hermes addresses

Follow-up to the #67 review:
- A dot-less host (nuc, hermes-box) is a LAN machine: it gets http://
  and counts as local, like .local names, so its web UI can sign in.
- A *.ts.net name typed with a port gets http:// (tailscale serve
  answers https on 443, so a port means the server itself).
- The address fields' placeholders drop https://, matching the error
  message.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* Fix: Don't trust dot-less Hermes hosts for plain http

A dot-less name still gets http:// as the guess, but it no longer
counts as local: it can also resolve through a search domain. So the
API key warning shows for it and the web UI can't sign in over http,
as before this PR.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants