Skip to content

fix: accept header accessor function in ServerTransportSecurityValidator#982

Open
neerajbhatt wants to merge 3 commits into
modelcontextprotocol:mainfrom
neerajbhatt:fix/870-security-validator-header-accessor
Open

fix: accept header accessor function in ServerTransportSecurityValidator#982
neerajbhatt wants to merge 3 commits into
modelcontextprotocol:mainfrom
neerajbhatt:fix/870-security-validator-header-accessor

Conversation

@neerajbhatt
Copy link
Copy Markdown

@neerajbhatt neerajbhatt commented May 24, 2026

Summary

  • Introduce a HeaderAccessor interface with getHeader(String) and getHeaderNames() methods, replacing the raw Function<String, List<String>> parameter in ServerTransportSecurityValidator.validateHeaders(), as proposed in ServerTransportSecurityValidator accepts an accessor function for header #870 and per reviewer feedback
  • Servlet transports now pass an HttpServletHeaderAccessor wrapping the request directly, leveraging the servlet's native case-insensitive header lookup instead of extracting all headers into a Map upfront
  • Deprecated validateHeaders(Map) is preserved as a default method that bridges to the new validateHeaders(HeaderAccessor), ensuring backward compatibility for existing custom implementations
  • Remove HttpServletRequestUtils utility class (no longer needed)

Changes

  • HeaderAccessor — new public interface with getHeader(String) and getHeaderNames()
  • HttpServletHeaderAccessor — internal implementation wrapping HttpServletRequest
  • ServerTransportSecurityValidator — new validateHeaders(HeaderAccessor) method; deprecated validateHeaders(Map) default bridges to it; both defaults cross-delegate so existing impls work either way
  • DefaultServerTransportSecurityValidator — overrides validateHeaders(HeaderAccessor), queries Origin and Host via accessor directly
  • 3 servlet transports — pass new HttpServletHeaderAccessor(request) instead of extractHeaders(request)
  • Unit tests — updated helpers to return HeaderAccessor; added accessorDefaultBridgesToMapOverride test for the reverse bridge direction

Test plan

  • All 49 DefaultServerTransportSecurityValidatorTests pass
  • CI pipeline passes

Closes #870

Replace Map<String, List<String>> parameter with
Function<String, List<String>> in validateHeaders(), allowing callers
to pass a header accessor instead of extracting all headers upfront.

This is more efficient (only requested headers are looked up) and
delegates case-insensitive header matching to the underlying request
implementation (e.g. HttpServletRequest.getHeaders).

- Update DefaultServerTransportSecurityValidator to use the accessor
  directly for Origin and Host headers
- Update all three servlet transport providers to pass
  name -> Collections.list(request.getHeaders(name))
- Remove HttpServletRequestUtils (no longer needed)
- Update unit tests to use accessor-based API

Closes modelcontextprotocol#870
@Kehrlann
Copy link
Copy Markdown
Contributor

The proposed changed would be breaking. The original ServerTransportSecurityValidator has been shipped in a previous major. If you'd to contribute this, we need a deprecation path.

@Kehrlann Kehrlann self-assigned this May 26, 2026
@Kehrlann Kehrlann added area/server waiting for user Waiting for user feedback or more details enhancement New feature or request P2 Moderate issues affecting some users, edge cases, potentially valuable feature labels May 26, 2026
Instead of a breaking change, introduce a deprecation path:
- Keep validateHeaders(Map) as deprecated default method that bridges
  to the new validateHeaders(Function) via case-insensitive lookup
- New implementations override validateHeaders(Function) for efficiency
- Transport providers continue calling validateHeaders(Map) for backward
  compatibility with existing custom implementations
- Restore HttpServletRequestUtils for header extraction in transports
- Add tests for deprecated Map-based API and interface bridge behavior
@neerajbhatt
Copy link
Copy Markdown
Author

Updated with a deprecation path as suggested. The changes:

  • validateHeaders(Map) is now a deprecated default method that bridges to the new validateHeaders(Function) via case-insensitive lookup
  • Transport providers continue calling validateHeaders(Map), so existing custom implementations that override it won't break
  • Added tests for both the deprecated Map-based API and the interface bridge behavior

@neerajbhatt
Copy link
Copy Markdown
Author

@Kehrlann I've updated the PR with the deprecation path as you suggested. Would you mind taking another look when you get a chance? Thanks!

@Kehrlann
Copy link
Copy Markdown
Contributor

Kehrlann commented Jun 4, 2026

Hey @neerajbhatt ! Thanks for the update. So this adds a deprecation path but the transport does not use the new API. But that's the whole point in introducing this new API.

Let's try introducing a HeaderAccessor interface, with:

  1. List<String> getHeader(String)
  2. List<String> getHeaderNames()

And then ServerTransportSecurityValidator becomes (more or less):

@FunctionalInterface
public interface ServerTransportSecurityValidator {

	@Deprecated
	void validateHeaders(Map<String, List<String>> headers) throws ServerTransportSecurityException;

	default void validateHeaders(HeaderAccessor accessor) throws ServerTransportSecurityException {
		var collectedHeaders = accessor.getHeaderNames()
			.stream()
			.collect(Collectors.<String, String, List<String>>toUnmodifiableMap(String::toLowerCase,
					accessor::getHeader, (l1, l2) -> {
						var merged = new ArrayList<>(l1);
						merged.addAll(l2);
						return Collections.unmodifiableList(merged);
					}));
		validateHeaders(collectedHeaders);
	}
}

Replace Function<String, List<String>> with a dedicated HeaderAccessor
interface (getHeader + getHeaderNames) as suggested in review. Transports
now pass an HttpServletHeaderAccessor wrapping the request directly,
leveraging the servlet's native case-insensitive header lookup instead of
extracting all headers into a Map upfront.
@neerajbhatt neerajbhatt force-pushed the fix/870-security-validator-header-accessor branch from b16ccde to cee7a22 Compare June 6, 2026 15:08
@neerajbhatt
Copy link
Copy Markdown
Author

@Kehrlann Updated with the HeaderAccessor interface as you suggested. Here's a summary of the changes:

New files:

  • HeaderAccessor — public interface with getHeader(String) and getHeaderNames()
  • HttpServletHeaderAccessor — internal implementation wrapping HttpServletRequest, leveraging its native case-insensitive header lookup

Modified files:

  • ServerTransportSecurityValidator — new validateHeaders(HeaderAccessor) default method; deprecated validateHeaders(Map) bridges to it by wrapping the map as a HeaderAccessor; the new method's default collects headers into a Map and calls the deprecated one — so existing custom implementations that override either method continue to work
  • DefaultServerTransportSecurityValidator — overrides validateHeaders(HeaderAccessor) directly, querying Origin and Host via accessor.getHeader()
  • 3 servlet transports — now call validateHeaders(new HttpServletHeaderAccessor(request)) instead of extractHeaders(request) + Map-based call

Removed:

  • HttpServletRequestUtils — no longer needed

Tests:

  • All 49 DefaultServerTransportSecurityValidatorTests pass
  • Added accessorDefaultBridgesToMapOverride test verifying the reverse bridge (HeaderAccessor → Map) works for existing implementations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/server enhancement New feature or request P2 Moderate issues affecting some users, edge cases, potentially valuable feature waiting for user Waiting for user feedback or more details

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ServerTransportSecurityValidator accepts an accessor function for header

2 participants