Skip to content

fix(mcp): accept environment-based endpoints without exposing resolved URLs - #919

Open
breedx wants to merge 2 commits into
mpfaffenberger:mainfrom
breedx:fix/upstream-expanded-mcp-url
Open

fix(mcp): accept environment-based endpoints without exposing resolved URLs#919
breedx wants to merge 2 commits into
mpfaffenberger:mainfrom
breedx:fix/upstream-expanded-mcp-url

Conversation

@breedx

@breedx breedx commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

MCP endpoint templates can fail validation even when their environment variables resolve to valid HTTP URLs. Validate the resolved value so these configurations work, without including the resolved endpoint in validation errors or replacing the stored template.

Problem

Startup expands environment variables, but registry validation checks the unexpanded URL. A configuration such as ${MCP_BASE}/mcp is rejected before startup even when MCP_BASE contains a valid HTTP(S) base URL.

Change

  • Reuse startup's existing environment expansion before the HTTP/SSE scheme check.
  • Preserve the original template in memory and persisted configuration.
  • Keep validation errors generic: no resolved endpoints or environment values.
  • Cover actual register/persist/reload and invalid-registration error paths, not only the validation helper.

No connector lifecycle or schema changes; this patch is independent of other pending MCP fixes. It does not change configuration file reading/writing policy (#802 addresses separate configuration I/O).

Validation

Base: 1d25d696; Linux / Python 3.13.13 with the unchanged upstream lock. Tests use disposable HOME/XDG, no inherited credentials and blocked socket connect/DNS/bind.

  • Regression file on unpatched base: 6 failed, 8 passed. HTTP/SSE template acceptance and actual registration fail before this patch.
  • python -m pytest -q -o addopts= tests/mcp/test_expanded_url_validation.py tests/mcp/test_registry_comprehensive.py tests/mcp/test_registry_coverage.py tests/mcp/test_registry_sync_drops_unconfigured.py: 82 passed.
  • python -m pytest -q -o addopts= tests/mcp: 487 passed, 6 skipped, no warnings. The skips cover explicitly unimplemented MCP search.
  • Ruff lint, formatting and diff checks pass on changed files.

Compatibility and limits

No dependency or package-version changes. This retains the existing HTTP(S) scheme-prefix policy; it is not full URL validation, SSRF protection, authorization, or a guarantee that unresolved variables elsewhere in an otherwise prefixed URL will be rejected. Environment changes between validation and startup remain possible. No live-connector qualification is claimed.

The base currently fails the unrelated tilde-completion assertion corrected separately in #916. This PR neither incorporates that change nor claims the full upstream suite is green.

@breedx

breedx commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

This fix remains independently reviewable, but full CI hits the existing quoted-tilde completion assertion already addressed separately in #916. I reproduced that exact failure on unpatched 1d25d696; this PR does not change the completion code or test, so I am keeping the unrelated correction out of this patch.

  • Quality and Windows encoding checks passed.
  • The macOS suite's only failure is tests/test_completions_and_small_modules.py::TestFilePathCompleterMissedLines::test_tilde_prefix_display: the insertion contains quoting around ~/..., so .startswith("~") fails.
  • Reproduction on unpatched base: python -m pytest -q -o addopts= tests/test_completions_and_small_modules.py::TestFilePathCompleterMissedLines::test_tilde_prefix_display1 failed, same assertion.
  • The independently reviewed focused and broader local results remain documented in the PR body. No checks were suppressed, and no green-full-suite claim is made.
  • CI again uses Python 3.14.7 despite the job's 3.13 label. Its deprecation/resource/coroutine warnings were inspected, but not all have independent base controls; local validation used Python 3.13.13.

Once the separate completion correction lands, this topic can be requalified on the updated base without widening its scope.

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.

1 participant