Skip to content

Commit 66a0d53

Browse files
he-yufengYuan325
andauthored
fix(parameters): return an error instead of panicking on a non-string type field (#3516)
## Description `parseParamFromDelayedUnmarshaler` reads the `type` field from a parameter map and passes it straight to `ParseParameter` with an unchecked type assertion: ```go return ParseParameter(ctx, p, t.(string)) ``` If a tools file declares a parameter whose `type` is not a string (for example `type: 123` from a YAML typo, or `type:` left as a mapping), `t.(string)` panics with `interface conversion: interface {} is int, not string` instead of surfacing a config error. The panic propagates out of `UnmarshalYAML`, so a single malformed parameter takes down config loading with a stack trace rather than a readable message. This is inconsistent with the rest of the function and with `ParseParameter` itself: the missing-`type` case right above already returns a clean error, and `ParseParameter`'s switch has a default branch that reports unknown types as errors. Only the string assertion was unguarded. The fix uses the comma-ok form and returns the same style of error the surrounding code already uses: ```go typeStr, ok := t.(string) if !ok { return nil, fmt.Errorf("parameter 'type' field must be a string, got %T", t) } return ParseParameter(ctx, p, typeStr) ``` Added a `TestFailParametersUnmarshal` case ("common parameter with non-string type") that feeds an integer `type` and expects the error. I confirmed it panics on `main` before the change and passes after; the full `internal/util/parameters` package still passes and `gofmt`/`go vet` are clean. Note this is a separate concern from #3512, which also touches `parameters.go` but in a different function (the array/map element parsing paths). No overlap with that change. ## PR Checklist - [x] Make sure you reviewed [CONTRIBUTING.md](https://github.com/googleapis/mcp-toolbox/blob/main/CONTRIBUTING.md) - [x] Ensure the tests and linter pass - [x] Code coverage does not decrease (added a test covering the new branch) - [ ] Appropriate docs were updated (no behavior/doc change, internal robustness fix) Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>
1 parent 5cee0d2 commit 66a0d53

2 files changed

Lines changed: 17 additions & 1 deletion

File tree

‎internal/util/parameters/parameters.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,12 @@ func parseParamFromDelayedUnmarshaler(ctx context.Context, u *util.DelayedUnmars
364364
return nil, fmt.Errorf("parameter is missing 'type' field")
365365
}
366366

367-
return ParseParameter(ctx, p, t.(string))
367+
typeStr, ok := t.(string)
368+
if !ok {
369+
return nil, fmt.Errorf("parameter 'type' field must be a string, got %T", t)
370+
}
371+
372+
return ParseParameter(ctx, p, typeStr)
368373
}
369374

370375
// ParseParameter parses a raw map into a Parameter object based on its "type" field.

‎internal/util/parameters/parameters_test.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1900,6 +1900,17 @@ func TestFailParametersUnmarshal(t *testing.T) {
19001900
},
19011901
err: "parameter is missing 'type' field",
19021902
},
1903+
{
1904+
name: "common parameter with non-string type",
1905+
in: []map[string]any{
1906+
{
1907+
"name": "my_string",
1908+
"type": 123,
1909+
"description": "this is a param with a numeric type",
1910+
},
1911+
},
1912+
err: "parameter 'type' field must be a string",
1913+
},
19031914
{
19041915
name: "common parameter missing description",
19051916
in: []map[string]any{

0 commit comments

Comments
 (0)