feat: implement self-connection guard for MCP endpoints

[sathyapramod]

Summary

Close product-security finding H7 (AAP-82830): implement a real MCP self-connection guard so the daemon cannot connect to its own HTTP/SSE /mcp (or gRPC listen port) endpoints and recurse/amplify tool calls.

Previously isSelfConnection() always returned false.

Changes

• Track daemon HTTP/gRPC listen ports via McpClientPool.setListenEndpoints()
• Add exported isSelfConnectionUrl() (local hosts × listen ports)
• Register ports early in startDaemon (httpPort / grpcPort) and refresh when the embedded web server binds
• Fail-closed: when no listen ports are registered yet, local hosts are refused (remote hosts still allowed)
• Normalize IPv4-mapped IPv6 loopback (::ffff:127.0.0.1 / Node’s ::ffff:7f00:1)
• Reject self URLs in connect() / connectDynamic() with a recorded failed status; skip self in connectAll without aborting other servers
• Document as DR-039 in docs/decisions.md + ARCHITECTURE note
• Unit + E2E coverage for localhost variants and non-self remote/mock URLs

Test plan

• Unit: localhost / 127.0.0.1 / [::1] / hostname + matching port → rejected
• Unit: IPv4-mapped loopback ([::ffff:127.0.0.1] / [::ffff:7f00:1]) → rejected
• Unit: remote host or localhost different port → allowed
• Unit: empty listen-port set + local host → rejected (fail-closed); remote still allowed
• Unit/E2E: guard cannot remain a permanent no-op (connect / connectDynamic throw); status records error
• E2E: self /mcp rejected; different-port mock MCP allowed
• CI green (lint-and-test, build matrix, container, package-python)
• Manual (optional): configure MCP URL to this daemon’s /mcp → clear self-connection error

[sudhirverma]

Contributor review (maintainer assist)

Verdict: needs work before merge — H7 guard implementation and tests look solid and CI is green, but there are merge-blocking doc/coordination items and one fail-open gap vs the stated DR.

Checklist

• Up to date with main (0 behind / 3 ahead)
• CI green (lint-and-test, build matrix, container, package-python)
• PR description updated to Summary / Changes / Test plan
• DR numbers coordinated with PR #81 (both claim DR-038)

What looks solid

• Closes the real pre-PR gap: isSelfConnection() is no longer a permanent return false
• isSelfConnectionUrl() compares host (loopback / hostname / local iface IPs) + daemon HTTP/gRPC listen ports
• Wired early via setListenEndpoints() in startDaemon (httpPort / grpcPort) and refreshed when the embedded web server binds
• connect() / connectDynamic() throw a clear self-connection error; connectAll skips self without aborting other servers
• Unit coverage for localhost / 127.0.0.1 / [::1] / hostname / remote-allowed; E2E in mcp-self-connection.test.ts rejects self /mcp and allows a different-port mock
• Exported helper + regression tests make a silent no-op hard to reintroduce
• ARCHITECTURE + DR entry document the behavior

Blockers / must coordinate before merge

1. DR-038 collision with PR #81 (fix(daemon): gate dynamic stdio MCP spawn) — #81 also adds DR-038 (stdio allowlist). Renumber this PR’s decision (e.g. self-connection → DR-039 if #81 lands first, or the reverse) and update ARCHITECTURE / cross-refs.
2. PR description format — body is prose bullets only. Per project PR readiness, rewrite to Summary / Changes / Test plan (map to H7 acceptance: self URL rejected; localhost variants detected; remote/mock allowed; guard not a no-op).

Should-fix (security posture vs DR text)

• Empty listen-port set fail-opens — if httpPort is not passed to startDaemon, httpPorts stays [] until the web server binds; isSelfConnectionUrl then returns false for loopback self URLs. abbenay web passes httpPort, so the main path is OK, but any MCP init before ports are registered can still self-connect. DR-038 implies self URLs are refused; prefer fail-closed for local hosts when HTTP MCP is enabled and no listen ports are known yet (or always register expected HTTP port before initMcpConnections()).

Non-blocking follow-ups

Topic	Note
Fail-closed when listen ports unknown	Should-fix above
IPv4-mapped IPv6 (::ffff:127.0.0.1)	Not normalized to 127.0.0.1; easy unit case
Config connect() status UX	Self-reject throws before status is recorded, so dashboard may show “no status” rather than the error
Persist-time validation	Self URLs can still be saved in config; rejected only at connect (acceptable if documented)

Optional nits (not blocking)

• Blocking any path on matching host:port (not only /mcp) is correct/stricter — keep it
• localHostAddresses() rescans interfaces per check — fine at connect frequency
• package-lock.json one-line churn — confirm intentional / rebase artifact

[cidrblock]

Maintainer assist — agent-executable follow-up

Context: This PR closes product-security finding H7 / AAP-82830 (epic AAP-80944): isSelfConnection() previously always returned false, so the daemon could register its own /mcp endpoint as an external MCP server and recurse/amplify tool calls.

Current status: Implementation + unit/E2E tests look solid and CI is green (0 behind / 3 ahead of main). Merge is blocked on coordination/docs and one fail-open gap vs the stated DR. Aligns with @sudhirverma’s earlier review; this comment expands each item into concrete, executable work.

How to use this comment: Execute items in order. Treat P0 as merge-blocking. Do not invent extra scope. Do not open GitHub issues unless a human explicitly asks. When done, reply on this thread with commit SHAs and a short checklist of what landed.

---

P0-1 — Resolve DR-038 collision with PR #81

Problem: On current main, the last decision is DR-037. Both open PRs add a new DR-038:

• #81 — DR-038: Stdio MCP spawn allowlist + operator approval (H6)
• This PR (feat: implement self-connection guard for MCP endpoints #83) — DR-038: Block MCP self-connections… (H7)

Why it matters: Duplicate DR numbers break the decision log as an ordered record. Reviewers and future agents cannot cite “DR-038” unambiguously. Merge of either PR without renumbering will force a conflict or silent overwrite risk for the other.

What to do:

1. Check whether fix(daemon): gate dynamic stdio MCP spawn (H6 / DR-038) #81 has merged. Prefer leaving whichever merges first as DR-038.
2. If fix(daemon): gate dynamic stdio MCP spawn (H6 / DR-038) #81 is still open or lands first: renumber this PR’s decision to the next free ID (almost certainly DR-039).
3. Update every self-connection reference in this branch:
   • docs/decisions.md — heading ## DR-038: Block MCP self-connections… → ## DR-039: … (or next free)
   • docs/ARCHITECTURE.md — any DR cross-ref if present
   • PR body / commit message text if they mention DR-038 for self-connection
4. Also clean the accidental double --- separator introduced above DR-029 in docs/decisions.md while editing that file.
5. Re-read docs/decisions.md on latest main (or after rebasing onto fix(daemon): gate dynamic stdio MCP spawn (H6 / DR-038) #81 if merged) and confirm no duplicate DR numbers remain.

Acceptance:

• Exactly one unique DR number for self-connection guard
• No collision with fix(daemon): gate dynamic stdio MCP spawn (H6 / DR-038) #81’s stdio-allowlist DR
• rg 'DR-038|DR-039' docs/ shows consistent numbering

---

P0-2 — Rewrite PR description to project template + Jira

Problem: Body is unstructured prose bullets. Project PR readiness expects Summary / Changes / Test plan. Jira AAP-82830 is not linked in the description.

Why it matters: Reviewers and release archaeology need the H7 acceptance criteria and the security ticket in the PR record, not only in chat/Jira.

What to do: Replace the PR body with something equivalent to:

## Summary

Close product-security finding **H7** ([AAP-82830](https://redhat.atlassian.net/browse/AAP-82830)): implement a real MCP self-connection guard so the daemon cannot connect to its own HTTP/SSE `/mcp` (or gRPC listen port) endpoints and recurse/amplify tool calls.

Previously `isSelfConnection()` always returned `false`.

## Changes

- Track daemon HTTP/gRPC listen ports via `McpClientPool.setListenEndpoints()`
- Add exported `isSelfConnectionUrl()` (local hosts × listen ports)
- Register ports early in `startDaemon` (`httpPort` / `grpcPort`) and refresh when the embedded web server binds
- Reject self URLs in `connect()` / `connectDynamic()`; skip self in `connectAll` without aborting other servers
- Document as **DR-0XX** (number after collision resolution) in `docs/decisions.md` + ARCHITECTURE note
- Unit + E2E coverage for localhost variants and non-self remote/mock URLs

## Test plan

- [x] Unit: `localhost` / `127.0.0.1` / `[::1]` / hostname + matching port → rejected
- [x] Unit: remote host or localhost different port → allowed
- [x] Unit/E2E: guard cannot remain a permanent no-op (`connect` / `connectDynamic` throw)
- [x] E2E: self `/mcp` rejected; different-port mock MCP allowed
- [x] CI green (`lint-and-test`, build matrix, container, package-python)
- [ ] Manual (optional): configure MCP URL to this daemon’s `/mcp` → clear self-connection error

Use gh pr edit 83 --body-file … (or GitHub UI). Fill DR-0XX with the number chosen in P0-1.

Acceptance:

• Body has Summary / Changes / Test plan
• Links AAP-82830
• DR number matches docs/decisions.md

---

P0-3 — Fail-closed when listen ports are unknown (security posture)

Problem: isSelfConnectionUrl() returns false when both httpPorts and grpcPorts are empty — even for obvious loopback self URLs. A unit test currently locks that fail-open in:

it('returns false when no listen ports are configured', () => {
  expect(isSelfConnectionUrl('http://127.0.0.1:8787/mcp', {
    httpPorts: [],
    grpcPorts: [],
  })).toBe(false);
});

Why it matters: DR text says self URLs targeting the daemon’s own endpoints are refused. abbenay web passes httpPort into startDaemon (good), but abbenay start does not pass httpPort. MCP init is kicked off asynchronously inside startDaemon before the web server may call setListenEndpoints(). In that window (or any path that never registers ports), a config/dynamic MCP URL to http://127.0.0.1:<our-port>/mcp can still connect — defeating H7.

Preferred fix (pick one; A is preferred):

Option A — Fail-closed for local hosts when no listen ports are known

1. In isSelfConnectionUrl() (packages/daemon/src/daemon/mcp-client-pool.ts):
   • After parsing URL + host normalization, if listenPorts is empty and host is local (localHostAddresses()), return true (treat as self / refuse).
   • If host is remote, still return false.
2. Update the unit test that currently expects false for empty ports + loopback → expect true / rejection instead.
3. Add/adjust tests:
   • empty ports + http://127.0.0.1:8787/mcp → self
   • empty ports + http://mcp.example.com:8787/mcp → not self
4. Keep existing behavior when ports are registered (port must match).

Option B — Always register expected HTTP port before MCP init

1. Ensure every code path that can serve /mcp or load MCP config passes httpPort into startDaemon before initMcpConnections().
2. Audit packages/daemon/src/daemon/index.ts (abbenay start vs abbenay web) and any other startDaemon( call sites.
3. Still consider Option A as defense-in-depth if ports can ever be empty.

Justification for preferring A: Port registration order is easy to regress; fail-closed on “local host, unknown listen set” matches secure-by-default and the H7 DoD (“self-connection … is detected and blocked”). Legitimate other-localhost MCP servers on different ports still work once ports are registered; when ports are unknown, refusing local URLs is the safer temporary posture.

Acceptance:

• Empty listen-port set no longer allows loopback self URLs
• Remote URLs still allowed when ports empty
• With ports registered: matching local host+port rejected; different port allowed
• Unit tests updated (remove/replace the fail-open assertion)
• E2E still passes

---

P1 — Optional hardening (non-blocking; do in this PR only if cheap)

Do these only if they fit naturally while touching isSelfConnectionUrl / tests. Otherwise leave for a later human-directed follow-up (do not file issues from this comment alone).

P1-a — Normalize IPv4-mapped IPv6 loopback

• Gap: http://[::ffff:127.0.0.1]:<port>/mcp may not match 127.0.0.1 / ::1.
• Fix: In normalizeHost(), map ::ffff:127.0.0.1 (and similar) to 127.0.0.1, or treat IPv4-mapped loopback as loopback in localHostAddresses() / the local-host check.
• Test: unit case for [::ffff:127.0.0.1] + matching port → self.
• Why: Same loopback, different textual form — classic bypass for host-based guards.

P1-b — Record status on self-reject for config connect()

• Gap: assertNotSelfConnection() throws before status is written in connect(), so dashboard/status may show “no status” rather than the error. connectAll swallows the throw.
• Fix (minimal): Set/update status with connected: false and error: <self-connection msg> before throwing (or in a catch path that still rethrows for connectDynamic).
• Why: Operators debugging a bad config deserve a visible error, not a silent skip.

P1-c — Confirm package-lock.json one-line engines churn

• File shows node engines change under a nested package (^20.19.0 || >=22.12.0).
• If unintentional rebase noise: drop it. If required by a dependency bump already on the branch: leave and note in the PR Changes section.

---

Out of scope (explicit non-goals for this follow-up)

• Persist-time validation rejecting self URLs at config save (connect-time reject is acceptable for H7; document in DR/PR if useful).
• Filing GitHub issues for P1 items (human will ask if needed).
• Expanding beyond self-connection detection (stdio allowlist is fix(daemon): gate dynamic stdio MCP spawn (H6 / DR-038) #81 / H6).

---

Verification commands (after code changes)

From repo root (use project’s usual npm/vitest workspace commands; do not invent tox if unused here):

# Prefer project CI-equivalent local gates used in this repo, e.g.:
npm test -w packages/daemon -- mcp-client-pool.test.ts mcp-self-connection.test.ts
npm run lint
# or full: npm test / CI-equivalent before push

Rebase onto latest main if needed so DR numbering and CI stay clean:

git fetch upstream
git rebase upstream/main
# resolve DR conflicts with #81 if both touch docs/decisions.md

---

Done definition for the agent executing this comment

1. P0-1, P0-2, P0-3 landed (commits + pushed to fix/mcp-self-connection-guard).
2. PR body updated.
3. CI green.
4. Reply on this thread with:
   • Commit SHAs
   • Chosen DR number
   • Whether Option A or B was used for P0-3
   • Which P1 items (if any) were included
5. Do not mark Sudhir’s review threads resolved unless those items are actually fixed; leave disputed items open.

[sathyapramod]

Follow-up complete (maintainer assist)

Landed on fix/mcp-self-connection-guard:

Item	Result
P0-1 DR collision	Renumbered to DR-039 (PR #81 keeps DR-038). Cleaned double --- above DR-029.
P0-2 PR body	Rewritten to Summary / Changes / Test plan with AAP-82830.
P0-3 fail-closed	Option A — empty listen-port set + local host → self; remote still allowed.
P1-a IPv4-mapped	Normalized ::ffff:127.0.0.1 and Node’s ::ffff:7f00:1 → 127.0.0.1.
P1-b status UX	Self-reject records connected: false + error before throw.
P1-c package-lock	Dropped unintentional engines churn (restored to main).

Commit: e225ee3 (fix: address H7 review — DR-039, fail-closed local hosts, hardening)

Local gates before push: lint (0 errors), daemon tests 718 passed, vscode 24 passed, ci:build OK.

[sudhirverma]

Re-review — follow-up verified

Verified against e225ee3 on fix/mcp-self-connection-guard. All P0/P1 items from the prior review and @cidrblock’s maintainer-assist comment are addressed.

Item	Status
P0-1 DR collision	Self-connection is DR-039; no DR-038 on this branch (#81 keeps DR-038). Single --- above DR-029.
P0-2 PR body	Summary / Changes / Test plan + AAP-82830; DR-039 matches docs.
P0-3 fail-closed	Option A: empty listen-port set + local host → self; remote still allowed. Unit tests updated.
P1-a IPv4-mapped	normalizeHost maps ::ffff:127.0.0.1 / ::ffff:7f00:1 → 127.0.0.1 + tests.
P1-b status UX	assertNotSelfConnection records connected: false + error before throw; unit asserts status.
P1-c package-lock	No package-lock.json diff vs main.

Verdict: LGTM — ready to merge once remaining CI (container) is green. Persist-time validation remains an acceptable non-goal for H7.

[sudhirverma]

Manual verification (LGTM)

Validated H7 / DR-039 end-to-end with Ollama on fix/mcp-self-connection-guard.

Verified

• Self URLs refused for 127.0.0.1 / localhost / [::1] / ::ffff:127.0.0.1 / hostname + port 8787; status records Refusing self-connection
• Reconnect of self URL still refused
• Fail-closed: aby daemon (no HTTP listen ports) refuses local MCP URL
• Non-self localhost different port (:9876) is not classified as self-connection (transport/protocol error only)
• Remote host (mcp.example.com) fails with DNS/fetch — not self-connection
• Legitimate stdio MCP (filesystem) connects with tools while self entry stays refused
• Ollama chat still works with self MCP present in config

Also confirmed from review follow-up: DR-039 (no clash with #81), PR body format + AAP-82830, fail-closed Option A, IPv4-mapped normalize, status on self-reject.

Approve — ready to merge once CI is green.