Skip to content

Fix/apikey placement npe#316

Merged
eskenazit merged 2 commits intomainfrom
fix/apikey-placement-npe
Apr 14, 2026
Merged

Fix/apikey placement npe#316
eskenazit merged 2 commits intomainfrom
fix/apikey-placement-npe

Conversation

@jlouvel
Copy link
Copy Markdown
Contributor

@jlouvel jlouvel commented Apr 13, 2026

Related Issue

Closes #212


What does this PR do?

When an apikey authentication spec omits the placement field, HttpClientAdapter.setChallengeResponse() threw a NullPointerException on placement.equals("header").

This PR adds a null guard that throws a clear IllegalArgumentException with the message "placement is required for apikey authentication (expected: header or query)".


Tests

Added setChallengeResponseShouldThrowWhenApiKeyPlacementIsMissing in HttpClientAdapterTest — verifies the exception type and message when placement is omitted.


Checklist

  • CI is green (build, tests, schema validation, security scans)
  • Rebased on latest main
  • Small and focused — one concern per PR
  • Commit messages follow Conventional Commits

Agent Context (optional)

agent_name: GitHub Copilot
llm: claude-opus-4-5
tool: VS Code Chat
confidence: high
source_event: "#212"
discovery_method: user_report
review_focus: HttpClientAdapter.java:136-140

@jlouvel jlouvel requested a review from eskenazit April 13, 2026 19:44
@jlouvel jlouvel self-assigned this Apr 13, 2026
@eskenazit
Copy link
Copy Markdown
Contributor

It seems this branch has way too much files for just a fix, it looks like this branch was rebased from chore/mcp-restlet-transport and not main, or that chore/mcp-restlet-transport was merged into it. This makes the PR hard to review, since we need to explore the commit list to find what is related to the fix, and what it noise from other branches. Plus this generates conflicts ^^'

Let us discuss branch management and rules to avoid that in the future =)

@jlouvel jlouvel force-pushed the fix/apikey-placement-npe branch from 0f0278c to 58a73aa Compare April 14, 2026 11:51
@jlouvel jlouvel force-pushed the fix/apikey-placement-npe branch from 58a73aa to d10b80a Compare April 14, 2026 12:52
@jlouvel
Copy link
Copy Markdown
Contributor Author

jlouvel commented Apr 14, 2026

@eskenazit There was no need to initially create this PR based on the mcp-auth one. I've rebased on main to it only has the 2 useful commits in

Copy link
Copy Markdown
Contributor

@eskenazit eskenazit left a comment

Choose a reason for hiding this comment

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

LGTM

@eskenazit eskenazit merged commit f7113b9 into main Apr 14, 2026
2 checks passed
@eskenazit eskenazit deleted the fix/apikey-placement-npe branch April 14, 2026 12:55
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.

NPE in HttpClientAdapter when apikey auth placement is not set

2 participants