fix(upstream): stop misclassifying transport errors as auth failures#464
Merged
Conversation
The auth-strategy loop in connection_http.go used `isAuthError` to decide
whether to fall back to the next strategy. The substring list was too
permissive — `"auth"` matched the strategy-name wrapper itself
("...headers-auth strategy: ...", "...no-auth strategy: ..."), so any
wrapped transport/parse error was treated as an auth failure and silently
triggered the OAuth fallback.
The visible symptom: a server configured with a static Bearer token whose
upstream returns HTTP 502 (Cloudflare error body, not JSON) failed with
"OAuth authentication required" — pointing the user at the wrong fix.
Tighten the substring list to genuine 403 / "authentication failed" /
"authorization failed" markers. 401/Unauthorized continues to be routed
through isOAuthError, unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
8141a7b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://149f16a6.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-auth-strategy-misclassif.mcpproxy-docs.pages.dev |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 25846212472 --repo smart-mcp-proxy/mcpproxy-go
|
7 tasks
electrolobzik
added a commit
to HaloCollar/mcpproxy-go
that referenced
this pull request
May 16, 2026
Brings in upstream's 9 commits since 2b9b5f9: - 0597762 fix(upstream): stop misclassifying transport errors as auth failures (smart-mcp-proxy#464) - aaec117 fix(diagnostics): correct bug-report URL (smart-mcp-proxy#465) - d27fa38 feat(server-detail): display + edit headers/env, with reveal & convert-to-secret (smart-mcp-proxy#466) - c086770 feat(webui): per-tool enable/disable + bulk Enable All/Disable All (smart-mcp-proxy#463) - 0ef75a8 chore(docs): remove stale root-level docs - 4b4b62a fix(ui): respect engaged flag in sidebar Setup pulse (smart-mcp-proxy#462) - 24aab3d docs(installation): add migrating-from-manual-install section (smart-mcp-proxy#459) - 9b79254 feat(doctor): surface snap-docker override hint when host needs it (smart-mcp-proxy#460) - be927b6 packaging(deb): ship unattended-upgrades whitelist so installs auto-update (smart-mcp-proxy#458) Conflicts resolved: 1. internal/management/service_test.go — purely additive on upstream's side (3 new headers/env t.Run blocks). Halo-main had no overlapping test work. Took upstream's additions verbatim. 2. oas/docs.go + oas/swagger.yaml — auto-generated by swaggo from Go annotations in source. Took --theirs on the conflict then regenerated with `make swagger` against the merged source so both halo-main- and upstream-introduced REST endpoints are reflected. Note on upstream's c086770 (per-tool enable/disable, upstream's version of smart-mcp-proxy#463): halo-main already has its own per-tool enable/disable work (via feat/per-tool-enable-disable + the security-hardening stack on top — admin gating, isToolCallable fail-closed, sentinel-error quarantine synthesis, etc.). The upstream version produced no merge conflicts because the file-level diffs aligned cleanly — the fork's hardening sits atop the same surface upstream landed. Sanity-checked: - `go build ./...` succeeds. - `go test -short ./internal/management/ ./internal/runtime/ ./internal/httpapi/ ./internal/storage/ ./cmd/generate-types/` passes. - internal/server pre-existing sandbox-environment flake (TestBinaryAPIEndpoints/GET_/servers) was verified to fail on pre-merge halo-main (ad31fde) too — not a regression.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isAuthErrorsubstring list so wrapper text like"headers-auth strategy"/"no-auth strategy"no longer matches"auth"and triggers an unwanted OAuth fallback.Why
A server configured with a static
Authorization: Bearer …header against an upstream that returned HTTP 502 (Cloudflare error body, non-JSON) failed with the misleading error:The headers-auth attempt actually failed with
unexpected end of JSON input, but because the wrapper string contained the substring"auth", the strategy loop treated it as an auth failure and silently fell through to OAuth — which then produced the user-facing message and pointed them at the wrong remediation (mcpproxy auth login).After this change the underlying transport/parse error is surfaced directly, so the UI can tell the user the real cause (upstream 5xx, malformed response, etc.).
Test plan
go test ./internal/upstream/core/ -run TestIsAuthError -v— three new tests pass.go test ./internal/upstream/core/ ./internal/upstream/managed/ ./internal/diagnostics/— green.go build ./...— green.🤖 Generated with Claude Code