Skip to content

Commit 27064ec

Browse files
authored
Merge pull request #2693 from jrobertboos/lcore-4088
LCORE-4088: Fix MCP auth header "Bearer" duplication
2 parents cc85efc + f3a3f65 commit 27064ec

6 files changed

Lines changed: 190 additions & 18 deletions

File tree

‎src/utils/mcp/mcp_headers.py‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,50 @@ def extract_propagated_headers(
129129
return propagated
130130

131131

132+
def strip_bearer_prefix(value: str) -> str:
133+
"""Strip a leading ``Bearer`` scheme from an Authorization header value.
134+
135+
OGX's MCP tool ``authorization`` field expects a raw token/credential and
136+
unconditionally prepends its own ``Bearer `` prefix before forwarding the
137+
request to the downstream MCP server. Values built by this module (e.g.
138+
the resolved Kubernetes token, or an ``Authorization`` header propagated
139+
verbatim from the incoming request) already include the ``Bearer`` scheme
140+
since that's the correct on-the-wire header representation. Passing such
141+
a value straight through to OGX's ``authorization`` field would result in
142+
a duplicated ``Bearer Bearer <token>`` header at the downstream MCP
143+
server, so callers that populate that field must strip the scheme first.
144+
145+
Args:
146+
value: The raw Authorization header value, with or without a
147+
``Bearer`` scheme.
148+
149+
Returns:
150+
The value with a leading ``Bearer`` scheme (exact casing) and its
151+
separating whitespace removed. Values without the scheme, including
152+
those with a differently-cased scheme (e.g. ``bearer``), are
153+
returned unchanged.
154+
"""
155+
return value.removeprefix("Bearer ")
156+
157+
158+
def ensure_bearer_prefix(value: str) -> str:
159+
"""Ensure an Authorization header value carries the ``Bearer`` scheme.
160+
161+
Idempotent counterpart to :func:`strip_bearer_prefix`: a value that
162+
already has the exact ``Bearer`` scheme is not double-prefixed, while a
163+
raw token (or a value with a differently-cased scheme, e.g. ``bearer``,
164+
which is treated as a raw token) gets the ``Bearer`` scheme added.
165+
166+
Args:
167+
value: A raw token, or a value that may already start with the
168+
exact ``Bearer`` scheme.
169+
170+
Returns:
171+
The value guaranteed to start with a single ``Bearer `` prefix.
172+
"""
173+
return f"Bearer {strip_bearer_prefix(value)}"
174+
175+
132176
def find_unresolved_auth_headers(
133177
configured: Mapping[str, str],
134178
resolved: Mapping[str, str],

‎src/utils/mcp/mcp_oauth_probe.py‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
from configuration import AppConfig
1616
from log import get_logger
1717
from models.api.responses.error import UnauthorizedResponse
18-
from utils.mcp.mcp_headers import McpHeaders, build_mcp_headers
18+
from utils.mcp.mcp_headers import McpHeaders, build_mcp_headers, ensure_bearer_prefix
1919

2020
logger = get_logger(__name__)
2121

@@ -57,14 +57,9 @@ async def check_mcp_auth(
5757
for mcp_server in configuration.mcp_servers:
5858
headers = complete_headers.get(mcp_server.name, {})
5959
auth_header = headers.get("Authorization")
60-
if auth_header is not None:
61-
authorization = (
62-
auth_header
63-
if auth_header.startswith("Bearer ")
64-
else f"Bearer {auth_header}"
65-
)
66-
else:
67-
authorization = None
60+
authorization = (
61+
ensure_bearer_prefix(auth_header) if auth_header is not None else None
62+
)
6863

6964
if (
7065
authorization

‎src/utils/mcp/mcp_tools.py‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
from log import get_logger
1313
from models.common.tools import ListedMcpTool
14+
from utils.mcp.mcp_headers import ensure_bearer_prefix
1415

1516
logger = get_logger(__name__)
1617

@@ -106,12 +107,8 @@ def _prepare_mcp_request_headers(headers: dict[str, str]) -> dict[str, str]:
106107
"""
107108
prepared = dict(headers)
108109
for header_name, value in list(prepared.items()):
109-
if (
110-
header_name.lower() == "authorization"
111-
and value
112-
and not value.startswith("Bearer ")
113-
):
114-
prepared[header_name] = f"Bearer {value}"
110+
if header_name.lower() == "authorization" and value:
111+
prepared[header_name] = ensure_bearer_prefix(value)
115112
return prepared
116113

117114

‎src/utils/responses.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@
118118
McpHeaders,
119119
build_mcp_headers,
120120
find_unresolved_auth_headers,
121+
strip_bearer_prefix,
121122
)
122123
from utils.model_list import parse_model_list_response
123124
from utils.otel_tracing import (
@@ -781,6 +782,13 @@ async def get_mcp_tools(
781782
continue
782783

783784
authorization = headers.pop("Authorization", None)
785+
if authorization is not None:
786+
# OGX's "authorization" field expects a raw token/credential and
787+
# prepends its own "Bearer " scheme downstream; strip any scheme
788+
# already present (e.g. from Kubernetes auth or a propagated
789+
# request header) to avoid a duplicated "Bearer Bearer <token>"
790+
# header at the MCP server.
791+
authorization = strip_bearer_prefix(authorization)
784792

785793
require_approval = (
786794
mcp_server.require_approval
@@ -860,6 +868,10 @@ def apply_mcp_headers_to_explicit_tools(
860868
continue
861869

862870
authorization = headers.pop("Authorization", None)
871+
if authorization is not None:
872+
# See the equivalent comment in get_mcp_tools: OGX prepends its
873+
# own "Bearer " scheme, so strip any scheme already present here.
874+
authorization = strip_bearer_prefix(authorization)
863875
out.append(
864876
mcp_tool.model_copy(
865877
update={

‎tests/unit/utils/mcp/test_mcp_headers.py‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,10 @@
1111
from utils.mcp import mcp_headers
1212
from utils.mcp.mcp_headers import (
1313
build_server_headers,
14+
ensure_bearer_prefix,
1415
extract_propagated_headers,
1516
find_unresolved_auth_headers,
17+
strip_bearer_prefix,
1618
)
1719

1820

@@ -335,6 +337,49 @@ def test_empty_resolved_returns_all_configured(self) -> None:
335337
assert sorted(result) == ["Authorization", "X-Api-Key"]
336338

337339

340+
class TestStripBearerPrefix:
341+
"""Test cases for strip_bearer_prefix function."""
342+
343+
def test_strips_bearer_prefix(self) -> None:
344+
"""Test that a leading 'Bearer ' scheme is removed."""
345+
assert strip_bearer_prefix("Bearer abc123") == "abc123"
346+
347+
def test_does_not_strip_differently_cased_prefix(self) -> None:
348+
"""Test that only the exact 'Bearer ' scheme is matched, not other casings."""
349+
assert strip_bearer_prefix("bearer abc123") == "bearer abc123"
350+
assert strip_bearer_prefix("BEARER abc123") == "BEARER abc123"
351+
assert strip_bearer_prefix("BeArEr abc123") == "BeArEr abc123"
352+
353+
def test_value_without_prefix_is_unchanged(self) -> None:
354+
"""Test that a raw token without a scheme is returned as-is."""
355+
assert strip_bearer_prefix("abc123") == "abc123"
356+
357+
def test_empty_string_is_unchanged(self) -> None:
358+
"""Test that an empty string is returned unchanged."""
359+
assert not strip_bearer_prefix("")
360+
361+
def test_only_strips_leading_prefix(self) -> None:
362+
"""Test that only the first 'Bearer ' scheme is stripped, not repeated ones."""
363+
assert strip_bearer_prefix("Bearer Bearer abc123") == "Bearer abc123"
364+
365+
366+
class TestEnsureBearerPrefix:
367+
"""Test cases for ensure_bearer_prefix function."""
368+
369+
def test_adds_bearer_prefix_to_raw_token(self) -> None:
370+
"""Test that a raw token gets the 'Bearer ' scheme added."""
371+
assert ensure_bearer_prefix("abc123") == "Bearer abc123"
372+
373+
def test_does_not_double_prefix_already_scoped_value(self) -> None:
374+
"""Test that a value already carrying 'Bearer ' is left unchanged."""
375+
assert ensure_bearer_prefix("Bearer abc123") == "Bearer abc123"
376+
377+
def test_double_prefixes_differently_cased_scheme(self) -> None:
378+
"""Test that a differently-cased scheme is treated as a raw token and prefixed."""
379+
assert ensure_bearer_prefix("bearer abc123") == "Bearer bearer abc123"
380+
assert ensure_bearer_prefix("BEARER abc123") == "Bearer BEARER abc123"
381+
382+
338383
class TestBuildServerHeaders:
339384
"""Test cases for build_server_headers function."""
340385

‎tests/unit/utils/test_responses.py‎

Lines changed: 82 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,10 @@ async def test_get_mcp_tools_with_kubernetes_auth(
485485
mocker.patch("utils.responses.configuration", mock_config)
486486
tools_k8s = await get_mcp_tools(token="user-k8s-token")
487487
assert len(tools_k8s) == 1
488-
assert tools_k8s[0].authorization == "Bearer user-k8s-token"
488+
# The Bearer scheme is stripped here since OGX adds its own "Bearer "
489+
# prefix before forwarding the token to the MCP server; passing the
490+
# scheme through would result in a duplicated "Bearer Bearer <token>".
491+
assert tools_k8s[0].authorization == "user-k8s-token"
489492

490493
@pytest.mark.asyncio
491494
async def test_get_mcp_tools_with_mcp_headers(self, mocker: MockerFixture) -> None:
@@ -623,7 +626,7 @@ async def test_get_mcp_tools_with_mixed_headers(
623626

624627
tools = await get_mcp_tools(token="k8s-token", mcp_headers=mcp_headers)
625628
assert len(tools) == 1
626-
assert tools[0].authorization == "Bearer k8s-token"
629+
assert tools[0].authorization == "k8s-token"
627630
assert tools[0].headers == {
628631
"X-API-Key": "secret-api-key",
629632
"X-Custom": "client-custom-value",
@@ -708,6 +711,40 @@ async def test_get_mcp_tools_with_propagated_headers(
708711
"x-request-id": "req-456",
709712
}
710713

714+
@pytest.mark.asyncio
715+
async def test_get_mcp_tools_propagated_authorization_strips_bearer_scheme(
716+
self, mocker: MockerFixture
717+
) -> None:
718+
"""Regression test for a duplicated 'Bearer Bearer <token>' MCP header.
719+
720+
When an MCP server allowlists the incoming ``Authorization`` header
721+
via ``headers: [Authorization]`` (no ``authorization_headers``
722+
config), the propagated value already includes the ``Bearer``
723+
scheme. Since OGX's MCP ``authorization`` field expects a raw token
724+
and adds its own ``Bearer `` prefix, the scheme must be stripped
725+
before it is assigned to that field, or the downstream MCP server
726+
receives ``Authorization: Bearer Bearer <token>``.
727+
"""
728+
servers = [
729+
ModelContextProtocolServer(
730+
name="subscription-watch",
731+
url="http://subscription-watch:8080",
732+
headers=["Authorization"],
733+
provider_id="provider",
734+
),
735+
]
736+
mock_config = mocker.Mock()
737+
mock_config.mcp_servers = servers
738+
mocker.patch("utils.responses.configuration", mock_config)
739+
740+
request_headers = {"Authorization": "Bearer user-jwt-token"}
741+
tools = await get_mcp_tools(
742+
token=None, mcp_headers=None, request_headers=request_headers
743+
)
744+
assert len(tools) == 1
745+
assert tools[0].authorization == "user-jwt-token"
746+
assert tools[0].headers is None
747+
711748
@pytest.mark.asyncio
712749
async def test_get_mcp_tools_propagated_headers_do_not_overwrite_auth_headers(
713750
self, tmp_path: Path, mocker: MockerFixture
@@ -815,7 +852,7 @@ async def test_get_mcp_tools_propagated_headers_additive_with_mcp_headers(
815852
token=None, mcp_headers=mcp_hdrs, request_headers=request_headers
816853
)
817854
assert len(tools) == 1
818-
assert tools[0].authorization == "Bearer client-token"
855+
assert tools[0].authorization == "client-token"
819856
assert tools[0].headers == {"x-rh-identity": "identity-value"}
820857

821858
@pytest.mark.asyncio
@@ -934,6 +971,48 @@ async def test_apply_mcp_headers_preserves_type(
934971
dumped = out[0].model_dump(exclude_unset=True)
935972
assert dumped.get("type") == "mcp"
936973

974+
def test_apply_mcp_headers_strips_bearer_from_propagated_authorization(
975+
self, mocker: MockerFixture
976+
) -> None:
977+
"""apply_mcp_headers_to_explicit_tools must strip the Bearer scheme.
978+
979+
Same regression as ``test_get_mcp_tools_propagated_authorization_strips_bearer_scheme``
980+
but for the explicit-tools path: a propagated ``Authorization`` header
981+
must not retain its ``Bearer`` scheme when assigned to OGX's
982+
``authorization`` field, or the MCP server receives a duplicated
983+
``Bearer Bearer <token>`` header.
984+
"""
985+
from utils.responses import ( # pylint: disable=import-outside-toplevel
986+
apply_mcp_headers_to_explicit_tools,
987+
)
988+
989+
servers = [
990+
ModelContextProtocolServer(
991+
name="subscription-watch",
992+
url="http://subscription-watch:8080",
993+
headers=["Authorization"],
994+
provider_id="mcp",
995+
),
996+
]
997+
mock_config = mocker.Mock()
998+
mock_config.mcp_servers = servers
999+
mocker.patch("utils.responses.configuration", mock_config)
1000+
1001+
explicit = InputToolMCP(
1002+
server_label="subscription-watch",
1003+
server_url="http://subscription-watch:8080",
1004+
)
1005+
1006+
out = apply_mcp_headers_to_explicit_tools(
1007+
[explicit],
1008+
token=None,
1009+
mcp_headers=None,
1010+
request_headers={"Authorization": "Bearer user-jwt-token"},
1011+
)
1012+
1013+
assert len(out) == 1
1014+
assert out[0].authorization == "user-jwt-token"
1015+
9371016

9381017
class TestGetTopicSummary:
9391018
"""Tests for get_topic_summary function."""

0 commit comments

Comments
 (0)