Sanitization, limits, enums on all free-form fields

[tadasant]

Fields like name, description, and anything else free-form in the server.json` file should be strictly limited to minimum necessary content so as to limit the possible vectors for spam and/or security risks (e.g. injection attacks).

[Avish34]

Hey @tadasant , Can I pick this up? Also, do we have a document capturing the constraint on fields like name, description etc.?

[tadasant]

@Avish34 thank you! Yes happy for you to take this on. Some more guidance:

1. First step is to enumerate and add those constraints to the registry-schema.json. We don't want them in the core schema.json - the latter is fine to be more permissive.
2. Then it's to actually implement those enforcements in the Go code

Note that #198 is not yet done, so I expect you'll find inconsistencies in the implementation vs. the docs. The docs are the source of truth, so any work you do to get them closer to alignment is much appreciated.

do we have a document capturing the constraint on fields like name, description etc.?

We don't have this, so part of the scope for addressing this issue is to review and make recommendations. Recommend just going through field by field and asking yourself what constraints might be reasonable to add, and document your thinking for each one.

[tadasant]

@Avish34 this issue is specifically about "free-form fields". At the moment, that is:

• Repository.url
• Repository.id
• VersionDetail.version though there is an issue floating around to better define this, probably blocking
• Server.name
• Server.description
• Package.name
• Package.version
• Input.*... need to think through whether we can/should lock this one down at all
• PositionalArgument.value_hint
• NamedArgument.name
• KeyValueInput.name
• Remote.url

Basically we want to add guards for those in the same way we already added guards for Repository.source, Package.registry_name, Package.runtime_hint.

Some of these are obvious, e.g. URL's need to be valid URL's. Repository.id should be locked down to the format of GitHub's format (until we expand beyond GitHub being a Repositoryoption).

The goal here is to protect against spam/malicious activity that might try to inject data, by locking down the free form input to the things we expect to be there. It's ok if we don't have a mechanism for every single field; might not make sense to be foolproof here.

[Avish34]

Ah, make sense. Thanks @tadasant for clarifying. I get better sense now. I will start on these then.

[Avish34]

@tadasant I have listed down some validation in the doc before I go and I implement. Wanted to run it through you or wider forum. https://docs.google.com/document/d/1EcX6wnLb6nKMZTmMvmWXL5tccNIPRtbXeIjLKnMu1CA/edit?usp=sharing
I will grant the permission once you request it, does not want to make it open for everyone, Tight security :)

[Avish34]

@tadasant I have given the access, sorry for the delay. Also, I can't see validations for Repository.source and Package.registry_name. Is it yet to be added?

[tadasant]

@Avish34 bringing comments here for visibility --

Also, I can't see validations for Repository.source and Package.registry_name. Is it yet to be added?

For Repository.source, let's do github and gitlab out of the gate. Most are on GitHub, but I've seen the community using Gitlab too, so will be good to have precedent for something other than GitHub here so we can easily extend down the line.

Package.registry_name might be in flux per discussions on #249 (reply in thread)

For now it should just be the list of specific string enums we already have in there.

Repository.Url - It should be a valid github url.

If source=github, yes. Should be different int he source=gitlab case and easily extendable to new sources.

Repository.Id - Is it github repo id? Shall we use the token to get the value and check.

Yes let's only allow the format of GitHub repo IDs and Gitlab repo IDs here. I think they are UUID's? But would verify with docs.

I don't think we need to hit the GitHub/Gitlab API's to verify they are correct; just verify the format.

Server.Name - Is 50 characters good? Any concrete format for a name?

See modelcontextprotocol/modelcontextprotocol#1086. Could be longer than 50 chars (could imagine some long reverse DNS names). Let's do 200.

Should validate the format as per that SEP (reverse.dns/name).

Server.Description - Shall we constraint it to 100 characters? Or shall we divide into long and short descriptions.

Yeah let's go 100 chars. It'll be easier to increase than decrease in the future.

Package.Name - same as of Server.Name

Let's do 200 as well, and no spaces. This is e.g. @pulsemcp/pulse-fetch from https://npmjs.com/package/@pulsemcp/pulse-fetch.

VersionDetails.Version - Should it be validated against the list of versions present in the registry?

Something we need to figure out: #158

Maybe just leave this unvalidated for now and drop a note in that Issue when you're done explaining how to add validation (e.g. formatting must be semantic versioning style).

Remote.URL - Check for valid mcp URL for not. Any naming convention is followed for mcp remote server URL or need to be introduced?

I think we can just leave this validation as generic validation that it is indeed some URL format, no need for something more (like actually connecting to the URL).

---

In general I'd view this ticket as focusing on formatting of the fields, rather than heavy-handed validation of things like "connect to URL / hit an API to ensure it is correct".

[Avish34]

Thanks a ton for the info. I will get started.
For the githud ID - I could see it's int64. I checked in the API schema. https://docs.github.com/en/rest/repos/repos?apiget-a-repository=&apiVersion=2022-11-28#get-a-repository

For package name - It could be different registries (npm, nuget etc), so naming convention might vary. So we are just having a simple validation of checking length and spaces, is the understanding right?

We have a PR for Server name validation too I guess though constraints are missing, should this PR go in first - #218?

[tadasant]

Yes to all the above, thanks @Avish34 !