feat(plugin-mcp): the endpoint knows which caller it is serving - #1850
Merged
mobeenabdullah merged 1 commit intoSep 13, 2026
Merged
mobeenabdullah merged 1 commit into
mobeenabdullah merged 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Collaborator
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
type: docs
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
A plugin route handler is called with the request AND the route context, and the endpoint declared only the first. The services facade and the authenticated user were discarded before the protocol layer could see them. That costs nothing while the endpoint exposes no tools, and would cost a great deal the moment one arrives: the tool would read and write with no user attached, and the defect would be invisible because the endpoint answers correctly in every other way. The context now travels as itself, scoped to the request with `AsyncLocalStorage` for the reason `auth/caller-scope.ts` gives: requests are served concurrently and a module variable hands one request's caller to another. The handler is built once on purpose, so the factory cannot close over a caller who did not exist when it was built, and the library's own seam for this is `authInfo`, an OAuth token with a flat scope list. A key's own grants are deliberately NOT carried here. They are already ambient: the plugin dispatcher runs every handler inside `runWithCallerScope`, so a service call made anywhere in the request is judged on the grants stamped on the key rather than on the roles of whoever minted it. Flattening that into `authInfo.scopes` would answer the authorization question a second time, beside the routes the rest of the CMS is reached through, and the two would drift while both looked correct. A request whose caller cannot be established is refused at construction rather than served a server built for nobody, so the guarantee is in place before the first tool depends on it. The auth proof matrix comes with it, driven through the real dispatcher with a real API key rather than a fixture standing in for one: an unauthenticated caller is refused, an unverifiable credential is refused, a real key is served, and the address guard is shown refusing that same key on a foreign Host and a foreign Origin while serving it on the configured site's own. Those last cases are the first time the guard has been observable at all, because an unauthenticated probe is answered before it ever runs.
mobeenabdullah
force-pushed
the
feat/the-endpoint-knows-who-is-asking
branch
from
September 13, 2026 10:20
2bf3146 to
edfccf9
Compare
mobeenabdullah
added a commit
that referenced
this pull request
Sep 13, 2026
…it is served (#1848) * fix(plugin-mcp): say what the package does, and test the endpoint as it is served Follow-up to #1837, which merged before this round's findings were worked. ## Two summaries said the opposite of what the package does The npm description and the root package catalogue both still called it a placeholder serving no endpoint. That is what a reader discovering the package through the registry, or an agent reading the indexed root README, was told about a package that now serves one. Both say what it is. ## `path` refused nothing and promised too much A value that cannot address a single endpoint is refused where it is written rather than surfacing later as a 404 somebody has to explain: a missing leading slash, a trailing one, a `:param` pattern, or the mount itself. A plain error, not `NextlyError` — that type's public messages are canonical because they shape an HTTP response, and this fires while the developer's own config is being evaluated, where the message IS the fix. What it deliberately does NOT do is check whether core already serves the path. Core keeps no list of reserved prefixes on purpose: its own answer coming first IS the mechanism that stops a plugin claiming `/collections`, so a list here would be a second one, drifting behind every route core adds. The option's documentation now states the consequence instead, including that the shadowing is per method — a path core serves for `GET` and not `DELETE` would send half this endpoint's traffic elsewhere. ## The tests could not see what the dispatcher does They call the route handler directly, which is right for the handler and wrong for the endpoint: core authenticates BEFORE the handler runs, so a caller with no credential never reaches the address guard at all and is answered `401` — where the transport specification names `403`. The package gains an integration lane and a suite that goes through `createDynamicHandlers`, the dispatcher that actually serves it, and pins that `401`. Two controls sit beside it, because "the endpoint refuses" is otherwise equally satisfied by there being no endpoint: an install without the plugin answers `400`, and so does one with the plugin off. The gap is recorded rather than closed. The request is still refused, and by the check that does not depend on the attacker's cooperation; the guard covers what authentication cannot, which is a request carrying a real credential from a page that had no business sending it. Making the route public would put the guard first and answer `403` — and would mean authenticating here instead, which is the one thing this endpoint exists not to do. ## Also A browser request from the configured site is now asserted to be SERVED. Every Origin case was a refusal, so none of them could tell a guard that refuses the wrong origin from one that refuses every origin — which is what an allowlist in the wrong shape produces. A second case covers an operator configuring a full origin rather than a hostname, since the check compares hostnames and an entry carrying a scheme would match nothing. * fix(plugin-mcp): apply the matcher's own capture rule to the endpoint path The path check refused every `:`, while the matcher it validates for treats only a segment that BEGINS with `:` as a capture. A validation stricter than its matcher refuses paths that would have worked: `/mcp:v1` addresses exactly one URL and was rejected as a pattern. The rule is now applied per segment, and the dispatcher test mounts such a path and shows it answering at that address and at no other, so the claim is about the matcher rather than about this package. The path check had no test at all, which is why the disagreement could exist. Both the accepted and the refused shapes are now covered, including that the refusal happens while the config is evaluated rather than as a 404 to explain. `mcpPlugin`'s documentation was separated from the function by a helper defined between them, so the emitted `dist/index.d.ts` declared the package's only public export with no description and an editor showed nothing on hover. The helper now sits above that block. * fix(plugin-mcp): publish the route grammar, and ask it instead of restating it Whether a path names one address or a family of them is the matcher's question. The endpoint's own check answered it separately, and a separate predicate that agrees today is still separate: a change to what a capture looks like reaches the matcher and leaves the plugin refusing paths that route, or accepting patterns that do not name one address. `routePathIsLiteral` is that classification, derived from the same `isCapture` the matcher applies and published through `@nextlyhq/plugin-sdk`, which is the surface a plugin author is meant to import from. `isCapture` itself stays private: a caller given it has to know the rule is per SEGMENT, and splitting on the wrong thing is the divergence this exists to prevent. The core-compat range stays at a released version rather than the one this ships in, because core validates it against its own version at boot and a floor naming an unreleased version refuses the plugin inside this repository. What makes the lower bound sufficient is the release train: every published package versions in lockstep, so the core an install receives beside this plugin is built from the same commit. * feat(plugin-mcp): the endpoint knows which caller it is serving (#1850) A plugin route handler is called with the request AND the route context, and the endpoint declared only the first. The services facade and the authenticated user were discarded before the protocol layer could see them. That costs nothing while the endpoint exposes no tools, and would cost a great deal the moment one arrives: the tool would read and write with no user attached, and the defect would be invisible because the endpoint answers correctly in every other way. The context now travels as itself, scoped to the request with `AsyncLocalStorage` for the reason `auth/caller-scope.ts` gives: requests are served concurrently and a module variable hands one request's caller to another. The handler is built once on purpose, so the factory cannot close over a caller who did not exist when it was built, and the library's own seam for this is `authInfo`, an OAuth token with a flat scope list. A key's own grants are deliberately NOT carried here. They are already ambient: the plugin dispatcher runs every handler inside `runWithCallerScope`, so a service call made anywhere in the request is judged on the grants stamped on the key rather than on the roles of whoever minted it. Flattening that into `authInfo.scopes` would answer the authorization question a second time, beside the routes the rest of the CMS is reached through, and the two would drift while both looked correct. A request whose caller cannot be established is refused at construction rather than served a server built for nobody, so the guarantee is in place before the first tool depends on it. The auth proof matrix comes with it, driven through the real dispatcher with a real API key rather than a fixture standing in for one: an unauthenticated caller is refused, an unverifiable credential is refused, a real key is served, and the address guard is shown refusing that same key on a foreign Host and a foreign Origin while serving it on the configured site's own. Those last cases are the first time the guard has been observable at all, because an unauthenticated probe is answered before it ever runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 5.4 of the AI-native programme: the auth proof matrix, and the seam it proves something about.
Stacked on #1848. Its base is
fix/mcp-endpoint-follow-ups, notmain, because the integration lane this suite runs in arrives with that PR. Branching offmainwould have addedvitest.integration.config.tsand thetest:integrationscript a second time, which is an added-by-both conflict on exactly the files only these two branches touch.Two consequences worth stating rather than discovering later:
ci.yml,integration.yml,secret-scan.ymlandpreview.ymlare all declaredpull_request: branches: [main], so none of them runs while the base is a feature branch. The green checks visible here are thepull_request_targetpair and any path-filtered build, which is not a CI verdict. Retarget after fix(plugin-mcp): say what the package does, and test the endpoint as it is served #1848 merges, then push an empty commit, because retargeting alone emitseditedand nothing subscribes to it.What was wrong
A plugin route handler is called with
(req, ctx). The endpoint declared onlyrequest, so the services facade and the authenticated user were discarded before the protocol layer could see them.That costs nothing today, because the endpoint exposes no tools. It would cost a great deal at 5.5: the first tool would read and write with no user attached, and nothing about the endpoint's behaviour would look wrong.
What changed
The route context now travels as itself, scoped to the request.
AsyncLocalStorage, for the reasonauth/caller-scope.tsalready gives in core: requests are served concurrently and a module variable hands one request's caller to another. The handler is built once on purpose, so the factory cannot close over a caller who did not exist when it was built.A key's own grants are deliberately not carried here. They are already ambient:
plugins/routes/dispatch.tsruns every handler insiderunWithCallerScope, so a service call made anywhere in the request is judged on the grants stamped on the key rather than on the roles of whoever minted it. The library's own seam,authInfo, is an OAuth token with a flat scope list; flattening Nextly's model into it would answer the authorization question a second time, beside the routes the rest of the CMS is reached through, and the two would drift while both looked correct.A request whose caller cannot be established is refused at construction rather than served a server built for nobody, so the guarantee is in place before the first tool depends on it.
The matrix
Driven through the real dispatcher with a real API key minted through
apiKeyService, not a fixture standing in for authentication:HostHostOriginOriginHostandOriginThe two 403 rows are the first time the address guard has been observable at all. #1837 recorded that an unauthenticated probe is answered before the guard runs, which is true and meant the guard had no test that reached it. A credential is what makes it reachable, and that is also the attack it exists for: a real session or key sent from a page that had no business sending it.
The last row pins the ordering as a case rather than a comment: both checks would refuse it, and the status a client sees says which one answered.
Evidence
check-changesetspasses on the changeset, and a deliberately short control list fails itThe third of those survived the first round. Removing
callerNow()from the factory broke nothing, because with the window always open the call has no observable effect, so the fail-closed precondition had mutation evidence and no standing test.buildServeris now exported for its own test, with a control proving the refusal is not blanket. Reported because a guard nobody has watched refuse is a guard nobody has seen work.