Skip to content

fix: webhook and MCP URLs use https behind an HTTPS proxy (#3601, #2907) - #3811

Merged
vybe merged 2 commits into
devfrom
feature/3601-proxy-https-urls
Oct 19, 2026
Merged

vybe merged 2 commits into
devfrom
feature/3601-proxy-https-urls

Conversation

@obasilakis

@obasilakis obasilakis commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • The schedule webhook URL and the auto-detected MCP URL now start with https:// when the reverse proxy reports an HTTPS connection (X-Forwarded-Proto: https).
  • Both use the existing utils/public_url.public_base_url helper from fix(nevermined): card (fiat) plans can't be paid — the 402 and verify/settle always use the nvm:erc4337 scheme #3215. It reads the raw header and only ever upgrades http to https, so the payment links, the webhook URL and the MCP URL follow one rule.
  • Behind an HTTPS proxy the MCP default is https://<host>/mcp, with no :8080, because the frontend nginx serves /mcp on the same origin. Without the header, or on localhost, the MCP URL is unchanged.
  • The API Keys page uses the server's detected MCP URL when it is https, and keeps its browser-hostname guess otherwise. Behind the Vite dev proxy the backend sees its own host, so an http value from the server would be wrong there.

@AndriiPasternak31 — the compose / proxy-trust part of #3601 is not in this PR

Your comment on #3601 and its second acceptance criterion ask for --proxy-headers with a forwarded-allow-ips value scoped to the proxy. This PR leaves that out on purpose. The reasons:

  • Turning on uvicorn proxy trust also rewrites request.client.host from X-Forwarded-For. That changes the IP every per-IP limiter and audit row sees (CSO 2026-09-30, finding 16).
  • The frontend nginx sits on trinity-agent-network next to the agents and has no fixed IP. A value that trusts nginx therefore also trusts any agent, unless we first pin static IPs for frontend and cloudflared.
  • The webhook and MCP URLs only need the scheme. The helper gives them the scheme without that trust change. A forged header can only change the scheme of a URL returned to the same caller, and is never stored.

So #3601's first criterion (https:// webhook URL and curl sample) is met here, and its second is not. Can you either agree to split the compose change into its own issue, or say the scoped trust is still needed for 1.0? This PR leaves #3601 open. #2907 closes.

X-Forwarded-Host: the hostname stays the Host header. The frontend nginx forwards the client's $host, and no other route in the codebase trusts X-Forwarded-Host.

Changes

  • src/backend/routers/schedules.py: _build_webhook_url(request, token) uses public_base_url, and returns None with no usable host. All four call sites pass the request.
  • src/backend/routers/settings/mcp_url.py: _get_default_mcp_url returns https://<host>/mcp behind an HTTPS proxy. A stored override still wins in resolve_mcp_url.
  • src/frontend/src/components/settings/McpKeysTab.vue: uses the https default_url from the server.
  • src/frontend/src/views/Settings.vue: the MCP panel hint names the proxy scheme.
  • Docs: docs/DEPLOYMENT.md (TLS section), feature-flows/webhook-triggers.md, feature-flows/api-keys-page.md, and the CSO diff report.

Test Plan

  • New tests pass: cd tests && pytest unit/test_3601_2907_proxy_scheme_urls.py -v gives 14 passed. 8 were red before the fix.
  • Related unit tests on the branch: 1036 passed, 3 skipped (filter webhook or mcp_url or public_base_url or settings_package or connector or schedule). test_3215_plan_scheme.py and test_3215_task_metadata.py were excluded because they cannot be imported with the local payments_py version. That is the same on dev.
  • Manual: on an HTTPS proxied install, open Schedules → Webhook → Enable, and Settings → MCP Server URL. Both should show https://.

Fixes #2907
Refs #3601

🤖 Generated with Claude Code

obasilakis and others added 2 commits October 19, 2026 16:43
The schedule webhook URL came from request.base_url and the auto-detected
MCP URL hardcoded http://<host>:8080, so a TLS install behind a proxy
advertised http:// for both. Both now take their origin from
utils.public_url.public_base_url, which upgrades to https on the proxy's
X-Forwarded-Proto and never downgrades. Behind an HTTPS proxy the MCP
default is https://<host>/mcp, since the frontend nginx serves /mcp on the
same origin.

The API Keys page now takes the server-detected MCP URL when it is https,
and keeps its hostname guess otherwise (behind the Vite dev proxy the
backend sees its own host).

uvicorn proxy-header trust is unchanged: enabling it would also change
request.client.host for every per-IP limiter, and agents share the network
with the frontend nginx.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@obasilakis obasilakis added the ui PR touches the frontend UI — triggers Playwright e2e tests label Oct 19, 2026
@obasilakis

Copy link
Copy Markdown
Contributor Author

/review: pre-landing review

Branch: feature/3601-proxy-https-urls → dev. Files: 10 (+202/−16; most of it tests and docs).
Scope: REQUIREMENTS MISSING for #3601, by decision. Clean for #2907.
Plan completion: 9 done / 0 partial / 1 not done / 1 changed / 1 unverifiable.

Item Status
#3601: webhook URL and curl sample are https:// behind an HTTPS proxy DONE. routers/schedules.py _build_webhook_url. The frontend builds the curl sample from the same webhook_url
#3601: compose keeps --proxy-headers with a forwarded-allow-ips value scoped to the proxy NOT DONE, scope cut (see the PR body for why). Impact HIGH for closing #3601, so the PR uses Refs #3601
#2907: forwarded https gives https://<host>/mcp DONE. routers/settings/mcp_url.py _get_default_mcp_url
#2907: forwarded http or no header is unchanged DONE (test parametrization)
#2907: X-Forwarded-Host decision CHANGED. Host is kept, and the decision is documented in the PR body
#2907: override wins; the response has both url and default_url DONE (test_stored_override_wins; the response shape is untouched)
#2907: connector snippets pick up the default through resolve_mcp_url DONE (no change in connector.py)
#2907: tests for 3 header cases + override DONE
#2907: Settings panel status is honest DONE. Hint text in Settings.vue; the panel already showed the server default_url
#2907: DEPLOYMENT.md note DONE
Manual check on a TLS install UNVERIFIABLE from the diff

Execution coverage

Changed symbol Executed by Live consumer Verdict
schedules._build_webhook_url test_webhook_url_* (5 cases) 4 routes in routers/schedules.py (generate, status, secret generate, secret disable) ✅ executed
call-site wiring test_every_webhook_route_passes_the_request (AST over the call args) same 🛡 guard. It pins arg shape, which a call test can't see without a TestClient
mcp_url._get_default_mcp_url test_default_mcp_url (5 cases) get_mcp_url, resolve_mcp_url → connector.py:56,115 ✅ executed
McpKeysTab.vue fetchMcpUrl — API Keys page MCP snippet ❌ no test (I1)

Fix mutation: with schedules.py + mcp_url.py reverted in a scratch worktree, 9 of 14 new tests go red. The 5 still green are the unchanged-behaviour cases (http/no-header MCP, localhost ×2, override wins).

Critical findings

None.

Informational

[I1] Test gap: the McpKeysTab.vue https-only selection has no unit test (Confidence 7/10)
configuredMcpUrl.value = response.data.url || (detected.startsWith('https://') ? detected : null). This is a three-branch choice. A swapped operand would show the http server value on dev and regress the Vite case, and nothing would catch it. Suggestion: extract it into a pure utils/ function and add one vitest. It is left as is because the logic is one line; say if you want it.

[I2] Header trust is upgrade-only and caller-scoped (Confidence 8/10, no action)
X-Forwarded-Proto is caller-controlled. Its only effect is the scheme of a URL returned to the same caller, and the URL is never persisted (connector.py returns it in the response only). This is the same posture as the #3215 payment links.

Clean categories

  • SQL: none touched.
  • Concurrency: none.
  • Auth: no new route; existing dependencies are unchanged.
  • Credentials: none in the diff.
  • Enterprise docs: the guard pattern run over the added docs/ lines gives 0 hits.
  • Enums: none.
  • Frontend XSS: the value is rendered as text, with no v-html.
  • Doc staleness: webhook-triggers.md and api-keys-page.md are updated in the diff.

Summary

🤖 Generated with Claude Code

@obasilakis

Copy link
Copy Markdown
Contributor Author

PR Validation Report

PR: #3811 - fix: webhook and MCP URLs use https behind an HTTPS proxy (#3601, #2907)
Branch: feature/3601-proxy-https-urls → dev · Files: 10 · Lane: B (no lane-C paths, no schema)

Category Status Notes
Commit Messages ✅ fix: + docs(security):, both descriptive
Base Branch ✅ dev
PR Size ✅ 10 files
Status Auto-Promotion ⚠️ Fixes #2907 promotes on merge. #3601 is Refs on purpose: its compose criterion is not done (see the PR body), so it stays status-in-progress until AndriiPasternak31 rules
Requirements / Architecture ➖ Bug fix, no new capability or endpoint
Feature Flows ✅ webhook-triggers.md, api-keys-page.md updated
Security Check ✅ §4.1–4.6, 4.10: no keys, emails, IPs, secrets, symlinks or host paths. The enterprise-docs pattern gives 0 hits on added doc lines
Infrastructure / Build / Config Packaging ➖ No Docker, compose or new env var; no new backend directory
Structural Review ✅ /review posted above: 0 critical, 2 informational
Merge Gate (Tier 1) ✅ 22 pass / 3 skipped. pytest (head, seed 12345) and regression diff green; required contexts green. dev-ci: success. The cancelled frontend-e2e / report is the duplicate run the ui label triggers; the live e2e run is green
Test Adequacy ✅ Named regression file test_3601_2907_proxy_scheme_urls.py covers both issues, the three header cases, the override and the missing host
Code Quality ✅ Reuses public_base_url (#3215). No new trust primitive
Requirements Trace ✅ Every #2907 criterion and #3601's first are met; #3601's second is deliberately not done

Execution coverage (§5.4)

Changed symbol Executed by Live consumer Verdict
_build_webhook_url test_webhook_url_* 4 routes in routers/schedules.py ✅
call-site arg shape AST guard same 🛡 guard
_get_default_mcp_url / resolve_mcp_url test_default_mcp_url, test_*override* get_mcp_url, connector.py:56,115 ✅
McpKeysTab.vue fetchMcpUrl — API Keys page ⚠️ untested one-liner (review I1)

Fix mutation: with the two backend files reverted, 9 of 14 new tests go red.

Issues

Recommendation

APPROVE for #2907 and the webhook half of #3601, pending a human review. #3601 stays open for the decision in the PR body.

🤖 Generated with Claude Code

@vybe vybe 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.

merge-train: batch validated on train/20261019-1651 (train PR #3818 green on Tier 1 + Tier 2)

@vybe
vybe merged commit 9b865d2 into dev Oct 19, 2026
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants