fix(compliance): require explicit mount mode in volume mount validation#1231
fix(compliance): require explicit mount mode in volume mount validation#1231
Conversation
…ce gap Section 4.1.5) Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens MCP Gateway stdin configuration validation to require an explicit volume mount mode (ro/rw) in the 3-part source:dest:mode format, aligning mount parsing/regex validation and test fixtures with the spec’s least-privilege requirements.
Changes:
- Require exactly 3 mount segments in
rules.MountFormatand always validate the mode. - Update schema-adjacent
mountPatternregex to require an explicit:ro/:rw. - Update tests/fixtures to treat 2-part mounts as invalid and to use 3-part mounts in valid configs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/config/rules/rules.go | Enforces source:dest:mode strictly and validates mode unconditionally. |
| internal/config/validation_schema.go | Updates mountPattern to require explicit mount mode. |
| internal/config/rules/rules_test.go | Updates mount-format unit tests to expect errors for missing mode. |
| internal/config/validation_test.go | Updates stdio server validation tests to reject mounts without mode. |
| internal/config/validation_string_patterns_test.go | Updates pattern-validation tests to treat missing-mode mounts as invalid. |
| internal/config/validation_schema_test.go | Updates string-pattern validation tests to reject missing-mode mounts. |
| internal/config/config_stdin_test.go | Updates stdin conversion fixtures to use valid 3-part mount strings. |
Comments suppressed due to low confidence (1)
internal/config/rules/rules.go:158
- MountFormat splits the mount string using strings.Split(mount, ":"), which makes it impossible to support Windows drive-letter paths (e.g., "C:\data:/container:ro") or any source path containing a colon. Also, the absolute-path check for source/dest only accepts paths starting with "/", even though this package already has an AbsolutePath() validator that supports Windows-style absolute paths. Consider parsing from the right to extract the mode, then splitting the remaining "source:dest" more carefully (including drive letters), and reusing AbsolutePath() for the host path validation (or explicitly documenting that mounts are Unix-only).
func MountFormat(mount, jsonPath string, index int) *ValidationError {
log.Printf("Validating mount format: mount=%s, jsonPath=%s, index=%d", mount, jsonPath, index)
parts := strings.Split(mount, ":")
if len(parts) != 3 {
log.Printf("Mount format validation failed: invalid part count=%d", len(parts))
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("invalid mount format '%s' (expected 'source:dest:mode')", mount),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Use format 'source:dest:mode' where mode is 'ro' (read-only) or 'rw' (read-write), e.g. '/host/path:/container/path:ro'",
}
}
source := parts[0]
dest := parts[1]
mode := parts[2]
// Validate source is not empty
if source == "" {
return &ValidationError{
Field: "mounts",
Message: fmt.Sprintf("mount source cannot be empty in '%s'", mount),
JSONPath: fmt.Sprintf("%s.mounts[%d]", jsonPath, index),
Suggestion: "Provide a valid absolute source path (e.g., '/host/path')",
}
}
// Validate source is an absolute path (MCP spec requirement)
if !strings.HasPrefix(source, "/") {
return &ValidationError{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| containerPattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9./_-]*(:([a-zA-Z0-9._-]+|latest))?$`) | ||
| urlPattern = regexp.MustCompile(`^https?://.+`) | ||
| mountPattern = regexp.MustCompile(`^[^:]+:[^:]+(:(ro|rw))?$`) | ||
| mountPattern = regexp.MustCompile(`^[^:]+:[^:]+:(ro|rw)$`) |
There was a problem hiding this comment.
mountPattern (used by validateStringPatterns) rejects Windows drive-letter host paths because it forbids additional ':' characters in the source segment. If Windows host paths are intended to be supported (other validators in this package accept them), the regex should be updated accordingly (or this pre-check should be removed in favor of MountFormat doing the authoritative parsing/validation).
| mountPattern = regexp.MustCompile(`^[^:]+:[^:]+:(ro|rw)$`) | |
| // mountPattern validates "source:target:mode" structure but allows colons within source/target | |
| mountPattern = regexp.MustCompile(`^.+:.+:(ro|rw)$`) |
Per MCP Gateway Specification v1.8.0 §4.1.5, volume mounts MUST use the
host:container:mode3-part format — mode is not optional. The implementation was accepting 2-parthost:containermounts, which silently defaults to Docker'srwmode, violating least-privilege principles.Validation logic (
internal/config/rules/rules.go,validation_schema.go)MountFormat: changedlen(parts) < 2 || len(parts) > 3→len(parts) != 3; removed optional-mode branch; mode is always validated againstro/rwmountPatternregex:^[^:]+:[^:]+(:(ro|rw))?$→^[^:]+:[^:]+:(ro|rw)$Tests
"valid mount without mode"cases to"invalid mount without mode"(now expect error) acrossrules_test.go,validation_test.go,validation_string_patterns_test.go,validation_schema_test.goconfig_stdin_test.gofixtures to use valid 3-part mounts (/tmp:/tmp:rw,/host:/container:ro)Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build2535648412/b275/launcher.test /tmp/go-build2535648412/b275/launcher.test -test.testlogfile=/tmp/go-build2535648412/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go flate/deflate.go--64 64/pkg/tool/linu-o pull.rebase(dns block)/tmp/go-build2026882574/b279/launcher.test /tmp/go-build2026882574/b279/launcher.test -test.testlogfile=/tmp/go-build2026882574/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 5648412/b223/cmd.test -I ker/cli-plugins/docker-buildx --gdwarf-5 --64 -o ker/cli-plugins/docker-buildx n-me�� ry=1 /opt/hostedtoolcache/go/1.25.6/x64/src/net ker/docker-init /tmp/go-build253/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/compile -imultiarch x86_64-linux-gnu/tmp/go-build2026882574/b278/_pkg_.a ker/docker-init(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build2535648412/b260/config.test /tmp/go-build2535648412/b260/config.test -test.testlogfile=/tmp/go-build2535648412/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go ternal/fips140/c--64 64/pkg/tool/linu-o r(dns block)/tmp/go-build3414178539/b260/config.test /tmp/go-build3414178539/b260/config.test -test.testlogfile=/tmp/go-build3414178539/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/cgo main x_amd64/vet(dns block)/tmp/go-build2026882574/b264/config.test /tmp/go-build2026882574/b264/config.test -test.testlogfile=/tmp/go-build2026882574/b264/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true se stmain.go ache/go/1.25.6/x64/pkg/tool/linux_amd64/link . ctor p=/opt/hostedtoo/tmp/go-build2784548960/b211/vet.cfg ache/go/1.25.6/x64/pkg/tool/linux_amd64/link -I 5648412/b223/cmd.test -I ker/cli-plugins/docker-buildx --gdwarf-5 --64 -o ker/cli-plugins/docker-buildx(dns block)nonexistent.local/tmp/go-build2535648412/b275/launcher.test /tmp/go-build2535648412/b275/launcher.test -test.testlogfile=/tmp/go-build2535648412/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go flate/deflate.go--64 64/pkg/tool/linu-o pull.rebase(dns block)/tmp/go-build2026882574/b279/launcher.test /tmp/go-build2026882574/b279/launcher.test -test.testlogfile=/tmp/go-build2026882574/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 5648412/b223/cmd.test -I ker/cli-plugins/docker-buildx --gdwarf-5 --64 -o ker/cli-plugins/docker-buildx n-me�� ry=1 /opt/hostedtoolcache/go/1.25.6/x64/src/net ker/docker-init /tmp/go-build253/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/compile -imultiarch x86_64-linux-gnu/tmp/go-build2026882574/b278/_pkg_.a ker/docker-init(dns block)slow.example.com/tmp/go-build2535648412/b275/launcher.test /tmp/go-build2535648412/b275/launcher.test -test.testlogfile=/tmp/go-build2535648412/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go flate/deflate.go--64 64/pkg/tool/linu-o pull.rebase(dns block)/tmp/go-build2026882574/b279/launcher.test /tmp/go-build2026882574/b279/launcher.test -test.testlogfile=/tmp/go-build2026882574/b279/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 5648412/b223/cmd.test -I ker/cli-plugins/docker-buildx --gdwarf-5 --64 -o ker/cli-plugins/docker-buildx n-me�� ry=1 /opt/hostedtoolcache/go/1.25.6/x64/src/net ker/docker-init /tmp/go-build253/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/compile -imultiarch x86_64-linux-gnu/tmp/go-build2026882574/b278/_pkg_.a ker/docker-init(dns block)this-host-does-not-exist-12345.com/tmp/go-build2535648412/b284/mcp.test /tmp/go-build2535648412/b284/mcp.test -test.testlogfile=/tmp/go-build2535648412/b284/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/runtime/c-p o ache/go/1.25.6/x-lang=go1.25 user.name(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
This section details on the original issue you should resolve
<issue_title>[compliance] Compliance Gap: Volume Mount Mode Not Required by Validation</issue_title>
<issue_description>## MCP Gateway Compliance Review - 2026-02-21
Summary
Found 1 compliance issue (MUST violation) during daily review of commit
e3e8080.Recent Changes Reviewed
e3e8080affected workflow/CI files; no changes tointernal/,main.go, or core gateway logic in the last 10 commitsCritical Issues (MUST violations)
1. Volume Mount Mode Is Accepted As Optional But Spec Requires It
Specification Section: 4.1.5 Volume Mounts for Stdio Servers
Deep Link: https://github.com/github/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md#415-volume-mounts-for-stdio-servers
Requirement:
Current State:
In
internal/config/rules/rules.go:123-131, the function is documented and validated as accepting either the 2-part"source:dest"format or the 3-part"source:dest:mode"format:This behavior is tested and confirmed in multiple test files:
internal/config/rules/rules_test.go:178:"valid mount without mode"passes validationinternal/config/validation_test.go:313:"valid mount without mode"passes validationinternal/config/validation_string_patterns_test.go:139: sameinternal/config/validation_schema_test.go:395: sameGap:
The specification in Section 4.1.5 unambiguously requires the 3-part
"host:container:mode"format with mode being mandatory. The compliance test T-CFG-015 ("Valid volume mount format (host:container:mode)") is defined in Section 10.1.1 and describes the required format with an explicit mode component.The implementation diverges from this by treating mode as optional, accepting
"/host:/container"where the spec requires"/host:/container:ro"or"/host:/container:rw".Security Impact:
This gap has security implications: without a required mode, a user may omit mode and rely on an undocumented default. Docker defaults to
rwfor mounts without a mode, meaning read-write access may be granted when read-only was intended. The spec explicitly recommends: "Read-only mounts ('ro') SHOULD be preferred when the server only needs to read data." Forcing explicit mode declaration aligns with the principle of least privilege.Severity: Critical (MUST violation)
File References:
internal/config/rules/rules.go:123-128— comment and function signature treating mode as optionalinternal/config/rules/rules.go:131— conditionlen(parts) < 2 || len(parts) > 3accepts 2-part formatinternal/config/rules/rules.go:178-180— mode-only validation is skipped if emptyinternal/config/rules/rules_test.go:178-186— "valid mount without mode" test caseinternal/config/validation_test.go:313-320— "valid mount without mode" test caseSuggested Fix:
MountFormatininternal/config/rules/rules.goto require exactly 3 parts:Update the related test cases to remove "valid mount without mode" cases and instead add tests that verify 2-part mounts are rejected.
Update the error message suggestion in
rules.go:136to only show the 3-part format.Compliance Status
| Section | Status |
|---------|--------|...
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.