Skip to content

Added the possibility to block the MCP server from outside.#18

Open
romankassebaum wants to merge 1 commit into
GDKsoftware:mainfrom
romankassebaum:Security
Open

Added the possibility to block the MCP server from outside.#18
romankassebaum wants to merge 1 commit into
GDKsoftware:mainfrom
romankassebaum:Security

Conversation

@romankassebaum

Copy link
Copy Markdown
Contributor

Sometimes it is necessary to block the MSC server from outside.

@cmgeuze cmgeuze left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi Roman,

Thanks for this! Binding the server to a single host is a really useful addition, and the hooks are a nice approach. A few things I'd like to sort out before we merge.

  1. The allow/deny check ends up in two places that can disagree. InternalAllowTool is applied when FTools gets built (constructor and now ListTools), but CallTool just reads FTools as it stands and never rebuilds. So if the filter is dynamic, a call can run against a stale list. I think the cleaner fix is to not rebuild FTools at all: register every tool once, then apply InternalAllowTool at the two read points, skip disallowed tools in the list response and reject them in CallTool. That keeps list and call consistent and drops the per-list rebuild below.

  2. Related to that, ListTools now clears and recreates the whole dictionary on every call. With the approach above that rebuild isn't needed anymore.

  3. When InternalVerify returns False it just Exits, and Indy defaults ResponseNo to 200, so a blocked request gets an empty 200 instead of a 403. Could you document that an override needs to set ResponseInfo itself, and maybe give the base a safe default?

  4. In Start, GStack.ResolveHost runs before FHTTPServer.Active, where GStack can still be nil and cause an AV when OnlyHost is on. That same block also reads FSettings.Host without the Assigned(FSettings) guard you use just above it. Could you guard both, and resolve once the stack is up? It's also hardcoded to IPv4.

  5. Small one: OnlyHost can only be set in code right now, exposing it as a [Server] setting would make it usable out of the box.

Thanks!

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