mcp: validate tool inputs - #310
Open
bhcopeland wants to merge 4 commits into
Open
Conversation
The MCP query tools take status as a free-form string and hand it to StatusFilter, which only ever compares against lowercase 'pass', 'fail' and 'inconclusive'. Anything else matched nothing and returned an empty page with no error, so a caller could not tell a rejected value from a genuine absence of results. That includes the uppercase 'FAIL' the dashboard itself reports, which made echoing an observed status back as a filter look like a clean "no failures" answer. Normalise the value and reject anything outside the set the filter understands. The CLI is unaffected, since click.Choice already constrains it there. Also list 'all' in the tool docstrings, which the filter accepts but none of them mentioned. Signed-off-by: Ben Copeland <ben.copeland@linaro.org>
limit and offset reach a plain list slice, so Python's negative index handling quietly reinterpreted them: a negative limit returned nearly the whole result set, defeating the pagination that exists to keep responses small, and a negative offset returned an empty page while still reporting the full total. list_nodes passed both straight to the Maestro API instead. Reject negative values in both paths. Signed-off-by: Ben Copeland <ben.copeland@linaro.org>
Filters are split on '=' and passed to requests as query parameter pairs. A filter string without '=' produced a one-element tuple that failed deep in the request layer, where the generic handler turned it into "Maestro nodes request failed" with no mention of the filter that caused it. Check the syntax before the request and name the offending filter. Signed-off-by: Ben Copeland <ben.copeland@linaro.org>
The issue fetchers interpolate the id into their endpoint, so an empty id turns "issue/<id>" into "issue/", which is the collection endpoint dashboard_fetch_issue_list already uses. The request then succeeded and returned every known issue in place of the one that was asked for. Over MCP that surfaced as a 150KB result reported as success. On the command line, "kci-dev results issue --id ''" printed an issue with every field None and a dashboard link to /issue/None, then exited 0. Guard the three issue fetchers, which is the point the CLI and the library client share. The builds and tests siblings requested "issue//builds", which 404s, so those failed already but without saying why. Signed-off-by: Ben Copeland <ben.copeland@linaro.org>
bhcopeland
changed the title
bhcopeland
changed the title
bhcopeland
requested review from
aliceinwire and
nuclearcat
and removed request for
aliceinwire
August 27, 2026 07:56
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.
Four input-validation fixes at the MCP boundary. The MCP tools take free-form values where the CLI relies on click.Choice and required arguments to reject them, and nothing replaced those guards.
status was compared only against lowercase pass/fail/inconclusive, so status="FAIL" matched nothing and returned matched=0 with no error. The dashboard reports status in uppercase, so a caller feeding back a value it had just been given got a confident "no failures": on one mainline commit, matched=0 out of total=45788. An invalid value was indistinguishable from a real empty result.
limit/offset reach a plain list slice, so a negative limit returned nearly the whole result set, defeating the pagination that keeps responses small, and a negative offset returned an empty page while still reporting the
full total.
list_nodes filters without = produced a one-element tuple that failed deep in the request layer, surfacing as a generic "Maestro nodes request failed" with no mention of the filter at fault.
An empty issue id turned issue/ into issue/, the collection endpoint, so get_issue("") returned every known issue as though it were one. On the CLI, kci-dev results issue --id '' printed an issue with every field None and a dashboard link to /issue/None, and exited 0. Guarded in kcidev/libs/dashboard.py, which the CLI and the client share.