Skip to content

Commit f3a3f65

Browse files
committed
fix(mcp): mcp auth header, "bearer" duplication
1 parent 52805fa commit f3a3f65

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
@@ -114,6 +114,7 @@
114114
McpHeaders,
115115
build_mcp_headers,
116116
find_unresolved_auth_headers,
117+
strip_bearer_prefix,
117118
)
118119
from utils.model_list import parse_model_list_response
119120
from utils.otel_tracing import (
@@ -777,6 +778,13 @@ async def get_mcp_tools(
777778
continue
778779

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

781789
require_approval = (
782790
mcp_server.require_approval
@@ -856,6 +864,10 @@ def apply_mcp_headers_to_explicit_tools(
856864
continue
857865

858866
authorization = headers.pop("Authorization", None)
867+
if authorization is not None:
868+
# See the equivalent comment in get_mcp_tools: OGX prepends its
869+
# own "Bearer " scheme, so strip any scheme already present here.
870+
authorization = strip_bearer_prefix(authorization)
859871
out.append(
860872
mcp_tool.model_copy(
861873
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
@@ -482,7 +482,10 @@ async def test_get_mcp_tools_with_kubernetes_auth(
482482
mocker.patch("utils.responses.configuration", mock_config)
483483
tools_k8s = await get_mcp_tools(token="user-k8s-token")
484484
assert len(tools_k8s) == 1
485-
assert tools_k8s[0].authorization == "Bearer user-k8s-token"
485+
# The Bearer scheme is stripped here since OGX adds its own "Bearer "
486+
# prefix before forwarding the token to the MCP server; passing the
487+
# scheme through would result in a duplicated "Bearer Bearer <token>".
488+
assert tools_k8s[0].authorization == "user-k8s-token"
486489

487490
@pytest.mark.asyncio
488491
async def test_get_mcp_tools_with_mcp_headers(self, mocker: MockerFixture) -> None:
@@ -620,7 +623,7 @@ async def test_get_mcp_tools_with_mixed_headers(
620623

621624
tools = await get_mcp_tools(token="k8s-token", mcp_headers=mcp_headers)
622625
assert len(tools) == 1
623-
assert tools[0].authorization == "Bearer k8s-token"
626+
assert tools[0].authorization == "k8s-token"
624627
assert tools[0].headers == {
625628
"X-API-Key": "secret-api-key",
626629
"X-Custom": "client-custom-value",
@@ -705,6 +708,40 @@ async def test_get_mcp_tools_with_propagated_headers(
705708
"x-request-id": "req-456",
706709
}
707710

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

818855
@pytest.mark.asyncio
@@ -931,6 +968,48 @@ async def test_apply_mcp_headers_preserves_type(
931968
dumped = out[0].model_dump(exclude_unset=True)
932969
assert dumped.get("type") == "mcp"
933970

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

9351014
class TestGetTopicSummary:
9361015
"""Tests for get_topic_summary function."""

0 commit comments

Comments
 (0)