Skip to content

Comments

LCORE-1366: Fixed MCP "Authorization" header not using the authorization parameter#1203

Open
jrobertboos wants to merge 2 commits intolightspeed-core:mainfrom
jrobertboos:lcore-1366
Open

LCORE-1366: Fixed MCP "Authorization" header not using the authorization parameter#1203
jrobertboos wants to merge 2 commits intolightspeed-core:mainfrom
jrobertboos:lcore-1366

Conversation

@jrobertboos
Copy link
Contributor

@jrobertboos jrobertboos commented Feb 23, 2026

Description

Fixed the bug where authorization header must be passed separately from other headers in responses API.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: N/A
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #
  • Closes LCORE-1366

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Bug Fixes
    • Authorization headers are now captured in a dedicated authorization field and excluded from general headers, ensuring consistent handling of auth tokens while preserving other request headers. Tests updated to reflect this behavior.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 23, 2026

Walkthrough

A new processing block extracts the Authorization header from request headers and relocates it into the tool definition under the "authorization" key before existing header handling occurs.

Changes

Cohort / File(s) Summary
Authorization Header Relocation
src/utils/responses.py
Added logic to detect Authorization in request headers, move its value into tool["authorization"], and remove it from the headers prior to the existing header attachment flow.
Tests Updated for Authorization Key
tests/unit/utils/test_responses.py
Updated test assertions to expect authorization key on tool dicts and removed expectations that Authorization remains inside headers. Adjusted multiple test cases to assert bearer token placement.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: moving the Authorization header from the generic headers collection to a separate authorization parameter in MCP tool definitions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jrobertboos jrobertboos changed the title LCORE-1366: LCORE-1366: Fixed MCP "Authorization" header not using the authorization parameter Feb 23, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tests/unit/utils/test_responses.py (2)

363-374: ⚠️ Potential issue | 🟡 Minor

Missing negative assertion for "headers" key absence.

The test asserts the new "authorization" field but omits verifying that the "headers" key is absent. Since the server config carries only "Authorization": "kubernetes" (no other headers), the implementation should produce no headers key at all after the fix. The analogous test_get_mcp_tools_without_auth already uses this pattern.

🛡️ Proposed addition
     assert len(tools_k8s) == 1
     assert tools_k8s[0]["authorization"] == "Bearer user-k8s-token"
+    assert "headers" not in tools_k8s[0]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/utils/test_responses.py` around lines 363 - 374, The test
test_get_mcp_tools_with_kubernetes_auth should also assert that the resulting
tool dict does not include a "headers" key; update the test (which calls
get_mcp_tools with a ModelContextProtocolServer named "k8s-server" and
token="user-k8s-token") to add an assertion like ensuring "headers" not in
tools_k8s[0] so the case where only kubernetes-style Authorization results in an
"authorization" field and no "headers" key is explicitly validated.

443-458: ⚠️ Potential issue | 🟡 Minor

Missing negative assertion for "headers" key absence.

Same gap as the k8s test: the server config has only "Authorization" mapped to a file path, so after the fix no headers key should remain. The test only checks the "authorization" value.

🛡️ Proposed addition
     assert len(tools) == 1
     assert tools[0]["authorization"] == "static-secret-token"
+    assert "headers" not in tools[0]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/utils/test_responses.py` around lines 443 - 458, The test
test_get_mcp_tools_with_static_headers is missing an assertion that the original
"headers" key is removed from the returned tool dict; update the test (which
constructs ModelContextProtocolServer and calls get_mcp_tools) to assert that
"headers" is not present on tools[0] (e.g., use "headers" not in tools[0] or
tools[0].get("headers") is None) in addition to the existing authorization value
check so the post-fix shape is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tests/unit/utils/test_responses.py`:
- Around line 363-374: The test test_get_mcp_tools_with_kubernetes_auth should
also assert that the resulting tool dict does not include a "headers" key;
update the test (which calls get_mcp_tools with a ModelContextProtocolServer
named "k8s-server" and token="user-k8s-token") to add an assertion like ensuring
"headers" not in tools_k8s[0] so the case where only kubernetes-style
Authorization results in an "authorization" field and no "headers" key is
explicitly validated.
- Around line 443-458: The test test_get_mcp_tools_with_static_headers is
missing an assertion that the original "headers" key is removed from the
returned tool dict; update the test (which constructs ModelContextProtocolServer
and calls get_mcp_tools) to assert that "headers" is not present on tools[0]
(e.g., use "headers" not in tools[0] or tools[0].get("headers") is None) in
addition to the existing authorization value check so the post-fix shape is
validated.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e458968 and a38dcbf.

📒 Files selected for processing (1)
  • tests/unit/utils/test_responses.py

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please update documentation as well.

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.

2 participants