fix(plugin-mcp): say what the package does, and test the endpoint as it is served - #1848
Conversation
…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.
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. |
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (18)
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 |
|
@codex review Follow-up to #1837, which merged before these findings were worked. Three fixed, one refuted with a measurement. The one to look at hardest is the dispatcher ordering. You were right that the handler-level tests could not see it, so the package has an integration lane now and a suite through On the Origin allowlist: the helper compares hostnames, and passing full origins is what would reject the configured site. The table is in the description. Your point about the missing positive case was right and is fixed. |
|
@coderabbitai review |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cd437481a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
@nextlyhq/adapter-drizzle
@nextlyhq/adapter-mysql
@nextlyhq/adapter-postgres
@nextlyhq/adapter-sqlite
@nextlyhq/admin
@nextlyhq/admin-css
@nextlyhq/blocks-engine
@nextlyhq/blocks-react
@nextlyhq/builder
create-nextly-app
@nextlyhq/eslint-plugin
nextly
@nextlyhq/plugin-form-builder
@nextlyhq/plugin-mcp
@nextlyhq/plugin-page-builder
@nextlyhq/plugin-sdk
@nextlyhq/plugin-seo
@nextlyhq/storage-s3
@nextlyhq/storage-uploadthing
@nextlyhq/storage-vercel-blob
@nextlyhq/ui
commit: |
github-actions
Bot
added
type: docs
… 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9377508a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…tating 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.
|
@codex review |
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ace2964db9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| @@ -0,0 +1,53 @@ | |||
| --- | |||
There was a problem hiding this comment.
This commit adds both the-endpoint-knows-who-is-asking.md and an-endpoint-that-says-what-it-is.md, each covering the complete lockstep package group. When the Version PR consumes this change, both bodies become separate release entries for the same PR; merge their contents into the single changeset required for each PR and remove the other file.
AGENTS.md reference: AGENTS.md:L327-L331
Useful? React with 👍 / 👎.
| it("serves it on the configured site's own Origin", async () => { | ||
| // The control on the case above, and the finding this programme refuted | ||
| // once already: an allowlist compared as serialized origins refuses the | ||
| // configured site's own browser clients. Hostnames are what it compares, |
There was a problem hiding this comment.
This newly added comment explicitly refers to “the finding this programme refuted once already,” so its meaning depends on review history rather than documenting the hostname-comparison behavior in the code. Rephrase it as the invariant the test protects; repository conventions require code comments to describe only the code and never review findings or conversations.
AGENTS.md reference: AGENTS.md:L288-L290
Useful? React with 👍 / 👎.
1 participant
Follow-up to #1837, which merged before this round's findings were worked. Four findings: three real and fixed, one I believe is wrong and I have the measurement.
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 it through the registry, or an agent reading the Context7-indexed root README, was told about a package that now serves one. Both corrected.
pathrefused nothing and promised too muchA value that cannot address a single endpoint is now refused where it is written — a missing leading slash, a trailing one, a
:parampattern, or the mount itself — rather than surfacing later as a 404 somebody has to explain.A plain
Error, notNextlyError: that type's public messages are canonical by design because they shape an HTTP response (validationflattens to"Validation failed."), 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 comment says the ordering is the guard, "so there is no list of reserved prefixes to keep in step with the routes core adds later". A list here would be a second one, drifting behind every route core adds, which is the proxy
derived-checks.mdwarns about. The option's documentation now states the consequence instead, including that the shadowing is per method.The tests could not see what the dispatcher does — correct, and now they can
This criticism was right. The package gains an integration lane and a suite that goes through
createDynamicHandlers, the dispatcher that actually serves the endpoint. It confirms the finding: an unauthenticated caller is answered401and the address guard never runs, because core authenticates before the handler.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, and the reasoning is in the code. The request is still refused, by the check that does not depend on the attacker's cooperation. The guard covers what authentication cannot — a request carrying a real credential from a page that had no business sending it, which is the browser half of the attack. Making the route
publicwould put the guard first and answer403; it would also mean authenticating here instead, which is the one thing this endpoint exists not to do.One finding I believe is wrong, measured
…1ADrsays the origin allowlist must carry serialized origins and that passing hostnames rejects the configured site. Measured against the library:Originsentcms.example.com(what this does)https://cms.example.comcms.example.comhttps://evil.example.comhttps://cms.example.com(what the finding suggests)https://cms.example.comThe helper compares hostnames and is port-agnostic, which is what its own documentation says and what
localhostAllowedOrigins()returns. Adopting the suggested remedy would introduce the bug the finding describes.Its second half was right and is fixed. 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. A browser request from the configured site is now asserted to be served, plus a case for an operator who configures a full origin rather than a hostname.
Gates
35 unit, 4 integration (new lane, wired into
lane:test:integration:sqliteand verified bycheck:test-lanes, which refused until it was), types, lint, doc-samples, docs-claims,fallowzero introduced, changeset covers all 26 lockstep packages.check:commentsis red frommain— a comment merged with #1834 names the pull request it arrived in. Not touched here to avoid a conflict: #1847 carries that one-line fix.