From 288a22fb6d6bf34d218a45772f772726b9ec052a Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 3 Jun 2026 11:06:59 +0300 Subject: [PATCH 1/6] feat(dockhand): support transitive dependency overrides/constraints in spec.yaml Renovate version bumps fail the build-containers Grype gate when the bumped package pins or caps a transitive dependency to a vulnerable version. Add an optional dependency-override mechanism to the spec.yaml schema, plumbed into the generated Dockerfile. - npx: spec.overrides ([]{package, version, reason}) is injected as an npm "overrides" block in the generated package.json before the npm install step. - uvx: spec.constraints ([]{spec, reason}) is written to a uv overrides requirements file and passed to "uv tool install --overrides". Both injection points match the install step by content (not line number) so they stay robust to toolhive template formatting. Every entry requires a non-empty reason (validation fails otherwise) so the justification for circumventing an upstream pin is auditable in-repo. Verified end-to-end against the CI build + Grype recipe: - #469 @brightdata/mcp 2.9.5 + override @modelcontextprotocol/sdk 1.26.0: resolves to SDK 1.26.0, grype --fail-on high --only-fixed passes. - #527 mcp-clickhouse 0.3.0 + constraint fastmcp>=3.2.0: fastmcp 3.4.0, import mcp_clickhouse OK, grype passes. Refs #668 Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/dockhand/main.go | 231 +++++++++++++++++++++++++++++++++++++- cmd/dockhand/main_test.go | 225 +++++++++++++++++++++++++++++++++++++ docs/adding-servers.md | 91 +++++++++++++++ 3 files changed, 546 insertions(+), 1 deletion(-) create mode 100644 cmd/dockhand/main_test.go diff --git a/cmd/dockhand/main.go b/cmd/dockhand/main.go index 7c62fed4..d20efca6 100644 --- a/cmd/dockhand/main.go +++ b/cmd/dockhand/main.go @@ -3,6 +3,7 @@ package main import ( "context" + "encoding/json" "fmt" "log/slog" "os" @@ -22,6 +23,17 @@ import ( skillpkg "github.com/stacklok/dockyard/internal/skills" ) +// Supported package protocols. +const ( + protocolNpx = "npx" + protocolUvx = "uvx" + protocolGo = "go" + + // mcpContainerVersion is the placeholder version toolhive's npx template stamps into + // the generated package.json; we reuse it when re-emitting that file with overrides. + mcpContainerVersion = "1.0.0" +) + // MCPServerSpec defines the structure of our YAML configuration files type MCPServerSpec struct { // Metadata about the MCP server @@ -44,6 +56,33 @@ type MCPServerPackageSpec struct { Package string `yaml:"package"` // e.g., "@upstash/context7-mcp" Version string `yaml:"version,omitempty"` // e.g., "1.0.14" Args []string `yaml:"args,omitempty"` // Additional arguments for the package + + // Overrides forces specific versions of transitive npm dependencies (npx protocol). + // Each entry is injected into an "overrides" block of the generated package.json so + // that npm resolves the pinned version regardless of upstream's declared range. + Overrides []OverrideEntry `yaml:"overrides,omitempty"` + + // Constraints forces specific versions of transitive Python dependencies (uvx protocol). + // Each entry is written to a uv overrides requirements file and passed to + // "uv tool install --overrides" so that uv resolves the pinned version even when + // upstream caps the dependency. + Constraints []ConstraintEntry `yaml:"constraints,omitempty"` +} + +// OverrideEntry pins a transitive npm dependency to a specific version (npx protocol). +// Reason is mandatory so the justification for circumventing the upstream pin is auditable +// in-repo, mirroring security.allowed_issues. +type OverrideEntry struct { + Package string `yaml:"package"` // e.g., "@modelcontextprotocol/sdk" + Version string `yaml:"version"` // e.g., "1.26.0" + Reason string `yaml:"reason"` // why this override is needed (required) +} + +// ConstraintEntry pins a transitive Python dependency via a uv override requirement +// (uvx protocol). Reason is mandatory so the justification is auditable in-repo. +type ConstraintEntry struct { + Spec string `yaml:"spec"` // a PEP 508 requirement, e.g., "fastmcp>=3.2.0" + Reason string `yaml:"reason"` // why this constraint is needed (required) } // MCPServerProvenance contains supply chain provenance information @@ -335,7 +374,7 @@ func loadMCPServerSpec(configPath string) (*MCPServerSpec, error) { } // Validate protocol - validProtocols := []string{"npx", "uvx", "go"} + validProtocols := []string{protocolNpx, protocolUvx, protocolGo} isValid := false for _, p := range validProtocols { if spec.Metadata.Protocol == p { @@ -347,9 +386,49 @@ func loadMCPServerSpec(configPath string) (*MCPServerSpec, error) { return nil, fmt.Errorf("invalid protocol %s, must be one of: %v", spec.Metadata.Protocol, validProtocols) } + // Validate dependency overrides/constraints + if err := validateDependencyOverrides(&spec); err != nil { + return nil, err + } + return &spec, nil } +// validateDependencyOverrides validates the optional overrides (npx) and constraints +// (uvx) blocks. Every entry must carry a non-empty Reason so the justification for +// circumventing an upstream version pin is auditable in-repo. +func validateDependencyOverrides(spec *MCPServerSpec) error { + if len(spec.Spec.Overrides) > 0 && spec.Metadata.Protocol != protocolNpx { + return fmt.Errorf("spec.overrides is only supported for the npx protocol, got %q", spec.Metadata.Protocol) + } + if len(spec.Spec.Constraints) > 0 && spec.Metadata.Protocol != protocolUvx { + return fmt.Errorf("spec.constraints is only supported for the uvx protocol, got %q", spec.Metadata.Protocol) + } + + for i, o := range spec.Spec.Overrides { + if o.Package == "" { + return fmt.Errorf("spec.overrides[%d].package is required", i) + } + if o.Version == "" { + return fmt.Errorf("spec.overrides[%d].version is required", i) + } + if strings.TrimSpace(o.Reason) == "" { + return fmt.Errorf("spec.overrides[%d].reason is required (document why %s is pinned to %s)", i, o.Package, o.Version) + } + } + + for i, c := range spec.Spec.Constraints { + if strings.TrimSpace(c.Spec) == "" { + return fmt.Errorf("spec.constraints[%d].spec is required", i) + } + if strings.TrimSpace(c.Reason) == "" { + return fmt.Errorf("spec.constraints[%d].reason is required (document why %q is constrained)", i, c.Spec) + } + } + + return nil +} + // generateDockerfile generates a Dockerfile using toolhive's library func generateDockerfile(ctx context.Context, spec *MCPServerSpec, customTag string) (string, error) { // Create the protocol scheme string @@ -383,9 +462,159 @@ func generateDockerfile(ctx context.Context, spec *MCPServerSpec, customTag stri return "", fmt.Errorf("failed to generate Dockerfile for protocol scheme %s: %w", protocolScheme, err) } + // Post-process the generated Dockerfile to inject any dependency overrides. + // toolhive returns the Dockerfile as a string, which is our injection seam; toolhive + // itself needs no changes. + dockerfile, err = injectDependencyOverrides(dockerfile, spec) + if err != nil { + return "", fmt.Errorf("failed to inject dependency overrides: %w", err) + } + return dockerfile, nil } +// injectDependencyOverrides rewrites the generated Dockerfile to force pinned versions +// of transitive dependencies. For npx it injects an npm "overrides" block; for uvx it +// adds a uv overrides requirements file to the "uv tool install" step. It matches the +// relevant install step by content (not line number) so it stays robust to changes in +// toolhive's template formatting. +func injectDependencyOverrides(dockerfile string, spec *MCPServerSpec) (string, error) { + switch spec.Metadata.Protocol { + case protocolNpx: + if len(spec.Spec.Overrides) == 0 { + return dockerfile, nil + } + return injectNpmOverrides(dockerfile, spec.Spec.Overrides) + case protocolUvx: + if len(spec.Spec.Constraints) == 0 { + return dockerfile, nil + } + return injectUvOverrides(dockerfile, spec.Spec.Constraints) + default: + return dockerfile, nil + } +} + +// injectNpmOverrides rewrites the package.json creation step so the generated package.json +// carries an "overrides" block. npm honors "overrides" only when present in the package.json +// it installs into, so this is injected before the "npm install" step. The toolhive template +// creates the package.json with a line of the form: +// +// RUN echo '{"name":"mcp-container","version":"1.0.0"}' > package.json +// +// We locate that line by content (the "> package.json" redirect) and replace the JSON payload +// with one that includes the overrides. +func injectNpmOverrides(dockerfile string, overrides []OverrideEntry) (string, error) { + overrideMap := make(map[string]string, len(overrides)) + for _, o := range overrides { + overrideMap[o.Package] = o.Version + } + + // Mirror the package.json name/version that toolhive's npx template emits, adding the + // overrides block. + pkgJSON := map[string]any{ + "name": "mcp-container", + "version": mcpContainerVersion, + "overrides": overrideMap, + } + pkgJSONBytes, err := json.Marshal(pkgJSON) + if err != nil { + return "", fmt.Errorf("failed to marshal package.json with overrides: %w", err) + } + + lines := strings.Split(dockerfile, "\n") + injected := false + for i, line := range lines { + trimmed := strings.TrimSpace(line) + // Match the package.json creation step regardless of the exact JSON payload. + if strings.HasPrefix(trimmed, "RUN echo '") && strings.Contains(trimmed, "> package.json") { + lines[i] = fmt.Sprintf("RUN echo '%s' > package.json", string(pkgJSONBytes)) + injected = true + break + } + } + + if !injected { + return "", fmt.Errorf("could not find the 'package.json' creation step in the generated Dockerfile to inject npm overrides") + } + + return strings.Join(lines, "\n"), nil +} + +// injectUvOverrides rewrites the "uv tool install" step so it passes a uv overrides +// requirements file. uv honors override requirements via "--overrides ", forcing the +// resolved version of a transitive dependency even when upstream caps it. The toolhive +// template installs with a line of the form: +// +// uv tool install "$package_spec" && \ +// +// We write the override specs to a file (created via a heredoc RUN injected before the +// install step) and add "--overrides" to the install invocation, matching the install line +// by content rather than line number. +func injectUvOverrides(dockerfile string, constraints []ConstraintEntry) (string, error) { + const overridesFile = "/tmp/uv-overrides.txt" + + // Build a RUN step that writes the overrides requirements file. Each constraint is a + // PEP 508 requirement on its own line. + // Emit a single logical RUN that writes each spec (one per line) to the overrides file. + // Every printed line ends with a backslash continuation so the trailing redirect stays + // part of the same shell command and is not parsed as a new Dockerfile instruction. + var fileBuilder strings.Builder + fileBuilder.WriteString("# Write uv override requirements (forces pinned transitive dependency versions)\n") + fileBuilder.WriteString("RUN printf '%s\\n' \\\n") + for _, c := range constraints { + // Single-quote each spec for shell safety. + fmt.Fprintf(&fileBuilder, " '%s' \\\n", c.Spec) + } + fmt.Fprintf(&fileBuilder, " > %s", overridesFile) + overridesRun := fileBuilder.String() + + lines := strings.Split(dockerfile, "\n") + installIdx := -1 + for i, line := range lines { + // Match the actual install command, not Dockerfile comments that merely mention it. + // The toolhive template invokes it as: uv tool install "$package_spec" + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") { + continue + } + if strings.Contains(line, "uv tool install \"") { + installIdx = i + break + } + } + if installIdx == -1 { + return "", fmt.Errorf("could not find the 'uv tool install' step in the generated Dockerfile to inject uv overrides") + } + + // Add the --overrides flag to the install invocation. + lines[installIdx] = strings.Replace( + lines[installIdx], + "uv tool install ", + fmt.Sprintf("uv tool install --overrides %s ", overridesFile), + 1, + ) + + // Insert the file-writing RUN step before the install step. The install step is often + // preceded by comment lines and a "RUN package=..." opener; we insert immediately before + // the line that opens the install RUN (the first line at or above installIdx that begins + // with "RUN "). + insertIdx := installIdx + for j := installIdx; j >= 0; j-- { + if strings.HasPrefix(strings.TrimSpace(lines[j]), "RUN ") { + insertIdx = j + break + } + } + + out := make([]string, 0, len(lines)+1) + out = append(out, lines[:insertIdx]...) + out = append(out, overridesRun) + out = append(out, lines[insertIdx:]...) + + return strings.Join(out, "\n"), nil +} + // generateImageTag creates a container image tag based on the repository structure // Following the pattern: ghcr.io/stacklok/dockyard/{protocol}/{name}:{version} func generateImageTag(spec *MCPServerSpec) string { diff --git a/cmd/dockhand/main_test.go b/cmd/dockhand/main_test.go new file mode 100644 index 00000000..a0df6b5d --- /dev/null +++ b/cmd/dockhand/main_test.go @@ -0,0 +1,225 @@ +package main + +import ( + "strings" + "testing" +) + +const ( + testOverrideVersion = "1.0.0" + testFastmcpSpec = "fastmcp>=3.2.0" +) + +// sampleNpxDockerfile mirrors the package.json + npm install steps that toolhive's +// BuildFromProtocolSchemeWithName emits for an npx package. +const sampleNpxDockerfile = `FROM node:24-alpine AS builder +WORKDIR /build + +# Create a package.json to install the MCP package +RUN echo '{"name":"mcp-container","version":"1.0.0"}' > package.json + +# Install the MCP package and its dependencies at build time +RUN npm install --save @brightdata/mcp@2.9.5 + +ENTRYPOINT ["npx", "@brightdata/mcp"] +` + +// sampleUvxDockerfile mirrors the "uv tool install" step that toolhive emits for a uvx package. +const sampleUvxDockerfile = `FROM python:3.14-slim AS builder +WORKDIR /build + +ENV UV_TOOL_DIR=/opt/uv-tools \ + UV_TOOL_BIN_DIR=/opt/uv-tools/bin +# Convert @ version separator to == for Python package specification +RUN package="mcp-clickhouse@0.3.0"; \ + package_spec=$(echo "$package" | sed 's/@/==/'); \ + uv tool install "$package_spec" && \ + ls -la /opt/uv-tools/bin/ + +ENTRYPOINT ["sh", "-c", "exec 'mcp-clickhouse' \"$@\"", "--"] +` + +func TestInjectNpmOverrides(t *testing.T) { + t.Parallel() + overrides := []OverrideEntry{ + {Package: "@modelcontextprotocol/sdk", Version: "1.26.0", Reason: "CVE fix; upstream hard-pins 1.21.2"}, + } + + out, err := injectNpmOverrides(sampleNpxDockerfile, overrides) + if err != nil { + t.Fatalf("injectNpmOverrides returned error: %v", err) + } + + // The package.json line must now carry an overrides block with the pinned version. + if !strings.Contains(out, `"overrides":`) { + t.Errorf("expected an overrides block in the generated package.json, got:\n%s", out) + } + if !strings.Contains(out, `"@modelcontextprotocol/sdk":"1.26.0"`) { + t.Errorf("expected the pinned SDK override in the package.json, got:\n%s", out) + } + + // The override must appear on the package.json line, which must precede the npm install. + pkgIdx := strings.Index(out, "> package.json") + installIdx := strings.Index(out, "npm install --save") + if pkgIdx == -1 || installIdx == -1 { + t.Fatalf("expected both the package.json step and the npm install step to be present") + } + if pkgIdx > installIdx { + t.Errorf("package.json (with overrides) must be created before npm install") + } + + // The npm install line must be left intact. + if !strings.Contains(out, "RUN npm install --save @brightdata/mcp@2.9.5") { + t.Errorf("npm install line should be unchanged, got:\n%s", out) + } +} + +func TestInjectUvOverrides(t *testing.T) { + t.Parallel() + constraints := []ConstraintEntry{ + {Spec: testFastmcpSpec, Reason: "CRITICAL CVE-2026-32871 fix; upstream caps <3.0.0"}, + } + + out, err := injectUvOverrides(sampleUvxDockerfile, constraints) + if err != nil { + t.Fatalf("injectUvOverrides returned error: %v", err) + } + + // The install step must now use the overrides file. + if !strings.Contains(out, "uv tool install --overrides /tmp/uv-overrides.txt") { + t.Errorf("expected --overrides flag on the uv tool install step, got:\n%s", out) + } + + // The overrides file must be written with the constraint spec. + if !strings.Contains(out, "'fastmcp>=3.2.0'") { + t.Errorf("expected the constraint spec to be written to the overrides file, got:\n%s", out) + } + if !strings.Contains(out, "> /tmp/uv-overrides.txt") { + t.Errorf("expected the overrides file to be written, got:\n%s", out) + } + + // The file-writing step must precede the install step. + writeIdx := strings.Index(out, "> /tmp/uv-overrides.txt") + installIdx := strings.Index(out, "uv tool install --overrides") + if writeIdx == -1 || installIdx == -1 { + t.Fatalf("expected both the overrides-file write and the install step") + } + if writeIdx > installIdx { + t.Errorf("overrides file must be written before uv tool install runs") + } +} + +func TestInjectDependencyOverrides_NoOp(t *testing.T) { + t.Parallel() + // npx spec with no overrides should pass the Dockerfile through unchanged. + spec := &MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolNpx}, + } + out, err := injectDependencyOverrides(sampleNpxDockerfile, spec) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out != sampleNpxDockerfile { + t.Errorf("expected Dockerfile to be unchanged when no overrides are set") + } + + // go protocol should also be a no-op even if (invalidly) overrides were present. + goSpec := &MCPServerSpec{Metadata: MCPServerMetadata{Protocol: protocolGo}} + out, err = injectDependencyOverrides("FROM golang:1.23\n", goSpec) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if out != "FROM golang:1.23\n" { + t.Errorf("expected go Dockerfile to be unchanged") + } +} + +func TestValidateDependencyOverrides(t *testing.T) { + t.Parallel() + tests := []struct { + name string + spec MCPServerSpec + wantErr bool + }{ + { + name: "valid npx override", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolNpx}, + Spec: MCPServerPackageSpec{ + Overrides: []OverrideEntry{{Package: "p", Version: testOverrideVersion, Reason: "because"}}, + }, + }, + wantErr: false, + }, + { + name: "npx override missing reason", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolNpx}, + Spec: MCPServerPackageSpec{ + Overrides: []OverrideEntry{{Package: "p", Version: testOverrideVersion}}, + }, + }, + wantErr: true, + }, + { + name: "npx override missing version", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolNpx}, + Spec: MCPServerPackageSpec{ + Overrides: []OverrideEntry{{Package: "p", Reason: "because"}}, + }, + }, + wantErr: true, + }, + { + name: "valid uvx constraint", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolUvx}, + Spec: MCPServerPackageSpec{ + Constraints: []ConstraintEntry{{Spec: testFastmcpSpec, Reason: "cve"}}, + }, + }, + wantErr: false, + }, + { + name: "uvx constraint missing reason", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolUvx}, + Spec: MCPServerPackageSpec{ + Constraints: []ConstraintEntry{{Spec: testFastmcpSpec}}, + }, + }, + wantErr: true, + }, + { + name: "overrides on uvx protocol rejected", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolUvx}, + Spec: MCPServerPackageSpec{ + Overrides: []OverrideEntry{{Package: "p", Version: testOverrideVersion, Reason: "x"}}, + }, + }, + wantErr: true, + }, + { + name: "constraints on npx protocol rejected", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolNpx}, + Spec: MCPServerPackageSpec{ + Constraints: []ConstraintEntry{{Spec: "x>=1", Reason: "x"}}, + }, + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := validateDependencyOverrides(&tt.spec) + if (err != nil) != tt.wantErr { + t.Errorf("validateDependencyOverrides() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/docs/adding-servers.md b/docs/adding-servers.md index d49ebc27..55762105 100644 --- a/docs/adding-servers.md +++ b/docs/adding-servers.md @@ -41,6 +41,21 @@ spec: - "arg1" # Passed to the entrypoint command - "arg2" + # Optional (npx only): force pinned versions of transitive npm dependencies. + # Injected as an "overrides" block in the generated package.json. Each entry + # requires a reason so the justification is auditable in-repo. + overrides: + - package: "@modelcontextprotocol/sdk" + version: "1.26.0" + reason: "Upstream hard-pins a vulnerable version; this same-major bump fixes it." + + # Optional (uvx only): force pinned versions of transitive Python dependencies. + # Written to a uv overrides requirements file and passed to `uv tool install + # --overrides`. Each entry requires a reason. + constraints: + - spec: "fastmcp>=3.2.0" + reason: "Upstream caps the dependency below the version that fixes a CVE." + provenance: # Optional but recommended repository_uri: "https://github.com/user/repo" repository_ref: "refs/tags/v1.0.0" @@ -138,6 +153,82 @@ provenance: repository_ref: "refs/tags/v0.3.1" ``` +## Dependency Overrides and Constraints + +Sometimes a package pins or caps a **transitive dependency** to a version that +fails the `build-containers` Grype gate (`--fail-on high --only-fixed`), and the +fix lives in a version excluded by that pin/cap. Dockyard can force a different +resolved version of the offending dependency without forking the upstream package. + +Every entry **must** include a `reason` (validation fails otherwise) so the +justification for circumventing an upstream pin is auditable in-repo, mirroring +`security.allowed_issues`. + +### npx: `spec.overrides` + +For `npx` servers, `spec.overrides` is injected as an [npm `overrides`](https://docs.npmjs.com/cli/v10/configuring-npm/package-json#overrides) +block in the generated `package.json`, so npm resolves the pinned version +regardless of the upstream-declared range: + +```yaml +metadata: + name: brightdata-mcp + protocol: npx +spec: + package: "@brightdata/mcp" + version: "2.9.5" + overrides: + - package: "@modelcontextprotocol/sdk" + version: "1.26.0" + reason: | + @brightdata/mcp hard-pins @modelcontextprotocol/sdk 1.21.2 (3x HIGH); + fixes are >=1.24. 1.26.0 is same-major, so no API break. +``` + +This rewrites the package.json step in the Dockerfile to: + +```dockerfile +RUN echo '{"name":"mcp-container","overrides":{"@modelcontextprotocol/sdk":"1.26.0"},"version":"1.0.0"}' > package.json +``` + +### uvx: `spec.constraints` + +For `uvx` servers, each `spec.constraints[].spec` is a [PEP 508](https://peps.python.org/pep-0508/) +requirement written to a uv overrides requirements file and passed to +`uv tool install --overrides`, forcing the resolved version even when upstream +caps it: + +```yaml +metadata: + name: mcp-clickhouse + protocol: uvx +spec: + package: "mcp-clickhouse" + version: "0.3.0" + constraints: + - spec: "fastmcp>=3.2.0" + reason: | + mcp-clickhouse caps fastmcp <3.0.0, but the CRITICAL CVE-2026-32871 fix + is fastmcp 3.2.0. +``` + +This injects an overrides file and rewrites the install step in the Dockerfile to: + +```dockerfile +RUN printf '%s\n' \ + 'fastmcp>=3.2.0' \ + > /tmp/uv-overrides.txt +RUN package="mcp-clickhouse@0.3.0"; \ + package_spec=$(echo "$package" | sed 's/@/==/'); \ + uv tool install --overrides /tmp/uv-overrides.txt "$package_spec" && \ + ls -la /opt/uv-tools/bin/ +``` + +> **Caution:** Forcing a version across an upstream's *deliberate* cap can cross a +> major version boundary (e.g. fastmcp 2.x → 3.x) and break the server's tools at +> runtime even when the image builds and the package imports. Functionally test +> the server before relying on such an override. + ## Step-by-Step Process ### 1. Find Package Information From 5d7d2c5095a2db7d08ddbb27b0cc2f94ebb63bf6 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:42:44 -0400 Subject: [PATCH 2/6] fix(dockhand): harden dependency-override injection against template drift The injection anchors are matched against toolhive's generated Dockerfile, but the tests only exercised hand-written samples that mirror it, so drift in the real template would pass tests and silently ship an image with no overrides applied. - Match the uvx install step on the bare "uv tool install" verb instead of requiring the quoted package spec to follow immediately. toolhive's template conditionally emits its own flags in between (RuntimeConfig .BuildWith renders as "--with ''"), which the old anchor missed entirely. - Parse the package.json payload toolhive emits and add the overrides key to it, rather than rebuilding the file from hardcoded name/version constants. Any other field toolhive puts there is now preserved instead of silently dropped. - Add tests that inject into Dockerfiles generated by the pinned toolhive version, covering the uvx step both with and without build-time constraints. These fail on anchor drift; they catch the --with case the previous anchor could not. --- cmd/dockhand/main.go | 57 ++++++++------- cmd/dockhand/main_test.go | 147 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 180 insertions(+), 24 deletions(-) diff --git a/cmd/dockhand/main.go b/cmd/dockhand/main.go index a14a2870..ec726d57 100644 --- a/cmd/dockhand/main.go +++ b/cmd/dockhand/main.go @@ -29,10 +29,6 @@ const ( protocolNpx = "npx" protocolUvx = "uvx" protocolGo = "go" - - // mcpContainerVersion is the placeholder version toolhive's npx template stamps into - // the generated package.json; we reuse it when re-emitting that file with overrides. - mcpContainerVersion = "1.0.0" ) // MCPServerSpec defines the structure of our YAML configuration files @@ -510,36 +506,46 @@ func injectDependencyOverrides(dockerfile string, spec *MCPServerSpec) (string, // // RUN echo '{"name":"mcp-container","version":"1.0.0"}' > package.json // -// We locate that line by content (the "> package.json" redirect) and replace the JSON payload -// with one that includes the overrides. +// We locate that line by content (the "> package.json" redirect), then parse the JSON payload +// toolhive emitted and add an "overrides" key to it. Parsing and re-emitting (rather than +// rebuilding the payload from hardcoded values) means any other field toolhive puts in that +// package.json is preserved rather than silently dropped. func injectNpmOverrides(dockerfile string, overrides []OverrideEntry) (string, error) { overrideMap := make(map[string]string, len(overrides)) for _, o := range overrides { overrideMap[o.Package] = o.Version } - // Mirror the package.json name/version that toolhive's npx template emits, adding the - // overrides block. - pkgJSON := map[string]any{ - "name": "mcp-container", - "version": mcpContainerVersion, - "overrides": overrideMap, - } - pkgJSONBytes, err := json.Marshal(pkgJSON) - if err != nil { - return "", fmt.Errorf("failed to marshal package.json with overrides: %w", err) - } - lines := strings.Split(dockerfile, "\n") injected := false for i, line := range lines { trimmed := strings.TrimSpace(line) // Match the package.json creation step regardless of the exact JSON payload. - if strings.HasPrefix(trimmed, "RUN echo '") && strings.Contains(trimmed, "> package.json") { - lines[i] = fmt.Sprintf("RUN echo '%s' > package.json", string(pkgJSONBytes)) - injected = true - break + if !strings.HasPrefix(trimmed, "RUN echo '") || !strings.Contains(trimmed, "> package.json") { + continue } + + // Extract the single-quoted JSON payload between "RUN echo '" and the redirect. + start := len("RUN echo '") + end := strings.LastIndex(trimmed, "'") + if end <= start { + return "", fmt.Errorf("could not parse the package.json payload in the generated Dockerfile: %q", trimmed) + } + + pkgJSON := map[string]any{} + if err := json.Unmarshal([]byte(trimmed[start:end]), &pkgJSON); err != nil { + return "", fmt.Errorf("failed to parse the generated package.json payload %q: %w", trimmed[start:end], err) + } + pkgJSON["overrides"] = overrideMap + + pkgJSONBytes, err := json.Marshal(pkgJSON) + if err != nil { + return "", fmt.Errorf("failed to marshal package.json with overrides: %w", err) + } + + lines[i] = fmt.Sprintf("RUN echo '%s' > package.json", string(pkgJSONBytes)) + injected = true + break } if !injected { @@ -581,12 +587,15 @@ func injectUvOverrides(dockerfile string, constraints []ConstraintEntry) (string installIdx := -1 for i, line := range lines { // Match the actual install command, not Dockerfile comments that merely mention it. - // The toolhive template invokes it as: uv tool install "$package_spec" + // Match on the bare "uv tool install" verb rather than requiring the quoted package + // spec to follow immediately: toolhive's template conditionally emits flags of its + // own between the two (e.g. "--with ''" for build-time constraints), so + // anchoring on `uv tool install "` would silently stop matching in those builds. trimmed := strings.TrimSpace(line) if strings.HasPrefix(trimmed, "#") { continue } - if strings.Contains(line, "uv tool install \"") { + if strings.Contains(line, "uv tool install ") { installIdx = i break } diff --git a/cmd/dockhand/main_test.go b/cmd/dockhand/main_test.go index a0df6b5d..7da2d37e 100644 --- a/cmd/dockhand/main_test.go +++ b/cmd/dockhand/main_test.go @@ -1,8 +1,14 @@ package main import ( + "context" + "encoding/json" "strings" "testing" + + "github.com/stacklok/toolhive/pkg/container/images" + "github.com/stacklok/toolhive/pkg/container/templates" + "github.com/stacklok/toolhive/pkg/runner" ) const ( @@ -223,3 +229,144 @@ func TestValidateDependencyOverrides(t *testing.T) { }) } } + +// The tests above inject into hand-written Dockerfiles that mirror toolhive's templates. +// Those keep passing even if toolhive's real output drifts away from what the injection +// anchors expect, so the tests below run the injection against Dockerfiles generated by +// the toolhive version this module actually pins. A template change that breaks an anchor +// fails here instead of silently shipping an un-overridden image. +// +// These use dryRun=true, which returns the rendered template without touching a container +// runtime, so they need no Docker daemon. +func generateRealDockerfile(t *testing.T, scheme string, rc *templates.RuntimeConfig) string { + t.Helper() + ctx := context.Background() + dockerfile, err := runner.BuildFromProtocolSchemeWithName( + ctx, images.NewImageManager(ctx), scheme, "", "test:latest", nil, rc, true, + ) + if err != nil { + t.Fatalf("failed to generate Dockerfile for %s: %v", scheme, err) + } + return dockerfile +} + +func TestInjectNpmOverrides_AgainstRealTemplate(t *testing.T) { + t.Parallel() + dockerfile := generateRealDockerfile(t, "npx://@brightdata/mcp@2.9.5", nil) + + out, err := injectNpmOverrides(dockerfile, []OverrideEntry{ + {Package: "@modelcontextprotocol/sdk", Version: "1.26.0", Reason: "CVE fix"}, + }) + if err != nil { + t.Fatalf("injection failed against the real toolhive npx template: %v", err) + } + + // The rewritten payload must be valid JSON carrying the override, and must preserve the + // fields toolhive emitted rather than replacing them with our own values. + payload := extractPackageJSONPayload(t, out) + var pkg map[string]any + if err := json.Unmarshal([]byte(payload), &pkg); err != nil { + t.Fatalf("rewritten package.json is not valid JSON (%q): %v", payload, err) + } + overrides, ok := pkg["overrides"].(map[string]any) + if !ok { + t.Fatalf("expected an overrides block in the rewritten package.json, got %q", payload) + } + if overrides["@modelcontextprotocol/sdk"] != "1.26.0" { + t.Errorf("expected the SDK override to be 1.26.0, got %v", overrides["@modelcontextprotocol/sdk"]) + } + + // Whatever toolhive put in the original payload must still be there. + origPayload := extractPackageJSONPayload(t, dockerfile) + var orig map[string]any + if err := json.Unmarshal([]byte(origPayload), &orig); err != nil { + t.Fatalf("could not parse the original package.json payload %q: %v", origPayload, err) + } + for k, v := range orig { + if pkg[k] != v { + t.Errorf("field %q from toolhive's package.json was lost or changed: got %v, want %v", k, pkg[k], v) + } + } +} + +// extractPackageJSONPayload pulls the single-quoted JSON out of the +// "RUN echo '{...}' > package.json" step of a Dockerfile. +func extractPackageJSONPayload(t *testing.T, dockerfile string) string { + t.Helper() + for _, line := range strings.Split(dockerfile, "\n") { + trimmed := strings.TrimSpace(line) + if !strings.HasPrefix(trimmed, "RUN echo '") || !strings.Contains(trimmed, "> package.json") { + continue + } + start := len("RUN echo '") + end := strings.LastIndex(trimmed, "'") + if end <= start { + t.Fatalf("could not extract the package.json payload from %q", trimmed) + } + return trimmed[start:end] + } + t.Fatalf("no package.json creation step found in the generated Dockerfile") + return "" +} + +func TestInjectUvOverrides_AgainstRealTemplate(t *testing.T) { + t.Parallel() + // The uvx template conditionally emits its own flags between "uv tool install" and the + // quoted package spec (RuntimeConfig.BuildWith renders as "--with ''"). Both the + // plain and the flag-carrying form must still be found and rewritten. + for _, tc := range []struct { + name string + rc *templates.RuntimeConfig + }{ + {"plain", nil}, + {"with build-time constraints", &templates.RuntimeConfig{BuildWith: []string{"mcp<2"}}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dockerfile := generateRealDockerfile(t, "uvx://mcp-clickhouse@0.3.0", tc.rc) + + out, err := injectUvOverrides(dockerfile, []ConstraintEntry{ + {Spec: testFastmcpSpec, Reason: "CVE fix excluded by upstream cap"}, + }) + if err != nil { + t.Fatalf("injection failed against the real toolhive uvx template: %v", err) + } + + if !strings.Contains(out, "uv tool install --overrides /tmp/uv-overrides.txt") { + t.Errorf("expected --overrides on the install step, got:\n%s", out) + } + if !strings.Contains(out, testFastmcpSpec) { + t.Errorf("expected the constraint spec in the overrides file step, got:\n%s", out) + } + + writeIdx := strings.Index(out, "> /tmp/uv-overrides.txt") + installIdx := strings.Index(out, "uv tool install --overrides") + if writeIdx == -1 || installIdx == -1 { + t.Fatalf("expected both the overrides-file write and the install step") + } + if writeIdx > installIdx { + t.Error("overrides file must be written before uv tool install runs") + } + + // Exactly one real (non-comment) install command must exist, and it must be the + // rewritten one -- so the step is neither duplicated nor left partially rewritten. + // The template also mentions "uv tool install" in comments, which injection skips. + var installCmds []string + for _, line := range strings.Split(out, "\n") { + trimmed := strings.TrimSpace(line) + if strings.HasPrefix(trimmed, "#") { + continue + } + if strings.Contains(line, "uv tool install ") { + installCmds = append(installCmds, trimmed) + } + } + if len(installCmds) != 1 { + t.Fatalf("expected exactly one non-comment 'uv tool install' command, got %d: %q", len(installCmds), installCmds) + } + if !strings.Contains(installCmds[0], "--overrides /tmp/uv-overrides.txt") { + t.Errorf("the install command was not rewritten with --overrides: %q", installCmds[0]) + } + }) + } +} From 6544e877a5796b1e6079c15b61805547241ce590 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:48:40 -0400 Subject: [PATCH 3/6] fix(mcp-scan): reapply uvx dependency overrides during the security scan The security scan runs the package directly rather than the built image, so it never saw the overrides injected into the Dockerfile and exercised a different dependency set than the one that ships. Pass spec.constraints through to the scanner as a uv overrides requirements file (uv takes a file, not inline specifiers), written to a temp file for the duration of the scan. npx spec.overrides are not reapplied: npm honors "overrides" only from a package.json it installs into, and the scan has no project directory. That is safe for the intended use case, since swapping a vulnerable but working dependency changes neither startup nor the tool surface being analyzed, but log a note so a future startup-affecting override does not fail confusingly. --- docs/adding-servers.md | 16 ++++++++++++++ scripts/mcp-scan/generate_mcp_config.py | 25 +++++++++++++++++++++ scripts/mcp-scan/run_scan.py | 29 +++++++++++++++++++++++++ 3 files changed, 70 insertions(+) diff --git a/docs/adding-servers.md b/docs/adding-servers.md index 8a34fe64..bfd613a6 100644 --- a/docs/adding-servers.md +++ b/docs/adding-servers.md @@ -231,6 +231,22 @@ RUN package="mcp-clickhouse@0.3.0"; \ > runtime even when the image builds and the package imports. Functionally test > the server before relying on such an override. +### How overrides interact with the security scan + +The `mcp-security-scan` CI job runs the package directly (`uvx ` / `npx `) +rather than the built image, so it does not automatically inherit anything injected +into the Dockerfile: + +- **uvx `constraints` are reapplied.** `scripts/mcp-scan` writes them to a uv overrides + file and passes `uvx --overrides`, so the scanned process resolves the same versions + the image ships. +- **npx `overrides` are not.** npm honors `overrides` only from a `package.json` it + installs into, and the scan has no such project directory. The scan logs a note when + it skips them. This is safe for the intended use case (swapping a vulnerable but + *working* dependency for a patched one) because it changes neither server startup nor + the tool surface the scanner analyzes. If you ever need an npm override that affects + whether the server *starts*, the scan will fail and this will need revisiting. + ## Step-by-Step Process ### 1. Find Package Information diff --git a/scripts/mcp-scan/generate_mcp_config.py b/scripts/mcp-scan/generate_mcp_config.py index ed6aad7d..569f2189 100644 --- a/scripts/mcp-scan/generate_mcp_config.py +++ b/scripts/mcp-scan/generate_mcp_config.py @@ -30,6 +30,29 @@ def main(): spec_args = data['spec'].get('args', []) spec_args_str = ' '.join(spec_args) if spec_args else '' + # Dependency overrides (see docs/adding-servers.md). The scan runs the package + # directly rather than the built image, so it has to reapply these itself or it + # would exercise a different dependency set than the one that ships. + # + # uvx: passed through as data; run_scan.py writes them to a uv overrides + # requirements file, since uv needs a file rather than inline specifiers. + uv_overrides = [c['spec'] for c in data['spec'].get('constraints', []) if c.get('spec')] + + # npx: npm honors "overrides" only from a package.json it installs into, and the + # scan invokes the package via `npx ` with no such project directory. These + # overrides exist to swap a vulnerable-but-working transitive dep for a patched + # one, which does not change server startup or the tool surface being analyzed, + # so skipping them here does not affect the scan result. Warn so a future + # startup-affecting override does not fail confusingly. + npm_overrides = data['spec'].get('overrides', []) + if protocol == 'npx' and npm_overrides: + print( + f"Note: {server_name} declares spec.overrides, which are not applied to the " + "security scan (npm overrides require a package.json; npx installs ad hoc). " + "The built image still gets them.", + file=sys.stderr, + ) + if protocol in ['npx', 'uvx']: command = protocol args = f"{package}@{version}" @@ -51,6 +74,8 @@ def main(): "server_name": server_name, "mock_env": mock_env } + if protocol == 'uvx' and uv_overrides: + output["uv_overrides"] = uv_overrides print(json.dumps(output)) except FileNotFoundError: diff --git a/scripts/mcp-scan/run_scan.py b/scripts/mcp-scan/run_scan.py index 254924ca..4b129ba3 100644 --- a/scripts/mcp-scan/run_scan.py +++ b/scripts/mcp-scan/run_scan.py @@ -7,6 +7,7 @@ import subprocess import sys import os +import tempfile def is_scanner_installed(): @@ -30,6 +31,7 @@ def main(): command = config.get("command") package_arg = config.get("args") mock_env = config.get("mock_env", []) + uv_overrides = config.get("uv_overrides", []) except (FileNotFoundError, json.JSONDecodeError) as e: print(f"Error reading config file: {e}", file=sys.stderr) sys.exit(1) @@ -38,6 +40,7 @@ def main(): command = args.command package_arg = args.package_arg mock_env = [] + uv_overrides = [] else: print("Usage: run_scan.py --config ", file=sys.stderr) print(" or: run_scan.py ", file=sys.stderr) @@ -66,6 +69,26 @@ def main(): # Use --stdio-arg=VALUE syntax because --yes looks like a flag to argparse. if command == "npx": scanner_args.append("--stdio-arg=--yes") + + # Reapply uvx dependency overrides (spec.constraints) so the scanned process resolves + # the same dependency versions as the built image. uv takes these as a requirements + # file, so write one; it must outlive this function's setup and be cleaned up after + # the scan, hence the try/finally around the subprocess call below. + overrides_file = None + if uv_overrides: + if command != "uvx": + print(f"Error: uv_overrides is only supported for uvx, got {command}", file=sys.stderr) + sys.exit(1) + with tempfile.NamedTemporaryFile( + mode="w", suffix=".txt", prefix="uv-overrides-", delete=False + ) as f: + f.write("\n".join(uv_overrides) + "\n") + overrides_file = f.name + # The flag must precede the package spec. Use --stdio-arg=VALUE for the flag + # itself, since a bare "--overrides" would be read as a new argparse flag. + scanner_args.append("--stdio-arg=--overrides") + scanner_args.extend(["--stdio-arg", overrides_file]) + for arg in package_arg.split(): scanner_args.extend(["--stdio-arg", arg]) @@ -100,6 +123,12 @@ def main(): except Exception as e: print(f"Error running mcp-scanner: {e}", file=sys.stderr) sys.exit(1) + finally: + if overrides_file: + try: + os.unlink(overrides_file) + except OSError: + pass if __name__ == "__main__": main() From 126f50f727734b773b0a5c7ee7e58cb868b7bb87 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Wed, 5 Aug 2026 10:54:53 -0400 Subject: [PATCH 4/6] fix(adb-mysql-mcp-server): constrain mcp to <2 to fix broken build adb-mysql-mcp-server depends on mcp[cli]>=1.8.0 with no upper bound. mcp 2.0.0 removed the mcp.server.fastmcp module this server imports at startup, breaking both the container build and the smoke test canary in build-containers.yml. This is also the first spec.yaml to exercise the override mechanism, so CI now actually covers it end to end rather than only unit tests. --- uvx/adb-mysql-mcp-server/spec.yaml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/uvx/adb-mysql-mcp-server/spec.yaml b/uvx/adb-mysql-mcp-server/spec.yaml index 378bebc1..6a53b905 100644 --- a/uvx/adb-mysql-mcp-server/spec.yaml +++ b/uvx/adb-mysql-mcp-server/spec.yaml @@ -12,6 +12,13 @@ metadata: spec: package: "adb-mysql-mcp-server" version: "2.0.0" + constraints: + - spec: "mcp<2" + reason: | + adb-mysql-mcp-server depends on mcp[cli]>=1.8.0 with no upper bound, but + mcp 2.0.0 removed the mcp.server.fastmcp module the server imports at + startup, so the container fails immediately with ModuleNotFoundError. + Cap it below 2 until upstream pins the dependency themselves. provenance: repository_uri: "https://github.com/aliyun/alibabacloud-adb-mysql-mcp-server" From c6991476f4051da43df8ac9f9822428c19643526 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:07:57 -0400 Subject: [PATCH 5/6] fix(dockhand): quote override values interpolated into the Dockerfile Override values were interpolated into RUN lines inside unescaped single quotes. A PEP 508 requirement legitimately contains single quotes in an environment marker, so fastmcp>=3.2.0; python_version < '3.14' was written to the overrides file as fastmcp>=3.2.0; python_version < 3.14 which is no longer a valid marker. Anything following the quote also ran as shell at image build time, so a spec value was able to execute arbitrary commands during the build. The npm path had the same flaw via the echoed package.json payload. - Add shellSingleQuote and use it for both the uv override specs and the npm package.json payload, escaping embedded quotes as '\''. - Reject control characters in override/constraint values. Quoting makes shell metacharacters inert, but a newline would still terminate the RUN instruction, and none of these fields has a legitimate use for one. - Cover quoted markers, embedded quotes, and injection attempts. These execute the emitted line through a real shell and compare the file it writes, rather than assuming how the line parses. --- cmd/dockhand/main.go | 41 +++++++- cmd/dockhand/main_test.go | 204 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 242 insertions(+), 3 deletions(-) diff --git a/cmd/dockhand/main.go b/cmd/dockhand/main.go index ec726d57..5e79f67e 100644 --- a/cmd/dockhand/main.go +++ b/cmd/dockhand/main.go @@ -413,6 +413,12 @@ func validateDependencyOverrides(spec *MCPServerSpec) error { if strings.TrimSpace(o.Reason) == "" { return fmt.Errorf("spec.overrides[%d].reason is required (document why %s is pinned to %s)", i, o.Package, o.Version) } + if err := rejectControlChars(fmt.Sprintf("spec.overrides[%d].package", i), o.Package); err != nil { + return err + } + if err := rejectControlChars(fmt.Sprintf("spec.overrides[%d].version", i), o.Version); err != nil { + return err + } } for i, c := range spec.Spec.Constraints { @@ -422,11 +428,28 @@ func validateDependencyOverrides(spec *MCPServerSpec) error { if strings.TrimSpace(c.Reason) == "" { return fmt.Errorf("spec.constraints[%d].reason is required (document why %q is constrained)", i, c.Spec) } + if err := rejectControlChars(fmt.Sprintf("spec.constraints[%d].spec", i), c.Spec); err != nil { + return err + } } return nil } +// rejectControlChars rejects control characters in a value that gets interpolated into the +// generated Dockerfile. Quoting (see shellSingleQuote) makes shell metacharacters inert, but +// a newline would still end the RUN instruction and start a new Dockerfile directive, so +// these are refused outright. Neither an npm package/version nor a single PEP 508 requirement +// has any legitimate use for them. +func rejectControlChars(field, value string) error { + for _, r := range value { + if r == '\n' || r == '\r' || r == 0 || (r < 0x20 && r != '\t') { + return fmt.Errorf("%s must not contain control characters (found %q in %q)", field, r, value) + } + } + return nil +} + // generateDockerfile generates a Dockerfile using toolhive's library func generateDockerfile(ctx context.Context, spec *MCPServerSpec, customTag string) (string, error) { // Create the protocol scheme string @@ -477,6 +500,18 @@ func generateDockerfile(ctx context.Context, spec *MCPServerSpec, customTag stri return dockerfile, nil } +// shellSingleQuote wraps s in single quotes for safe use as one shell word inside a +// Dockerfile RUN instruction. Embedded single quotes are closed, escaped, and reopened +// ('\”), the only way to represent them inside a single-quoted shell string. +// +// This matters because override values are interpolated into RUN lines. A PEP 508 +// requirement legitimately contains single quotes in environment markers +// (fastmcp>=3.2.0; python_version < '3.14'), which would otherwise terminate the quoting +// early -- corrupting the marker, and letting anything after it run as shell. +func shellSingleQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + // injectDependencyOverrides rewrites the generated Dockerfile to force pinned versions // of transitive dependencies. For npx it injects an npm "overrides" block; for uvx it // adds a uv overrides requirements file to the "uv tool install" step. It matches the @@ -543,7 +578,7 @@ func injectNpmOverrides(dockerfile string, overrides []OverrideEntry) (string, e return "", fmt.Errorf("failed to marshal package.json with overrides: %w", err) } - lines[i] = fmt.Sprintf("RUN echo '%s' > package.json", string(pkgJSONBytes)) + lines[i] = fmt.Sprintf("RUN echo %s > package.json", shellSingleQuote(string(pkgJSONBytes))) injected = true break } @@ -577,8 +612,8 @@ func injectUvOverrides(dockerfile string, constraints []ConstraintEntry) (string fileBuilder.WriteString("# Write uv override requirements (forces pinned transitive dependency versions)\n") fileBuilder.WriteString("RUN printf '%s\\n' \\\n") for _, c := range constraints { - // Single-quote each spec for shell safety. - fmt.Fprintf(&fileBuilder, " '%s' \\\n", c.Spec) + // Quote each spec so environment markers containing single quotes survive intact. + fmt.Fprintf(&fileBuilder, " %s \\\n", shellSingleQuote(c.Spec)) } fmt.Fprintf(&fileBuilder, " > %s", overridesFile) overridesRun := fileBuilder.String() diff --git a/cmd/dockhand/main_test.go b/cmd/dockhand/main_test.go index 7da2d37e..2cbee659 100644 --- a/cmd/dockhand/main_test.go +++ b/cmd/dockhand/main_test.go @@ -3,6 +3,9 @@ package main import ( "context" "encoding/json" + "os" + "os/exec" + "path/filepath" "strings" "testing" @@ -370,3 +373,204 @@ func TestInjectUvOverrides_AgainstRealTemplate(t *testing.T) { }) } } + +// TestShellSingleQuote covers the quoting used for values interpolated into RUN lines. +func TestShellSingleQuote(t *testing.T) { + t.Parallel() + tests := []struct { + name, in, want string + }{ + {"plain", "fastmcp>=3.2.0", `'fastmcp>=3.2.0'`}, + {"pep508 marker with quotes", `fastmcp>=3.2.0; python_version < '3.14'`, + `'fastmcp>=3.2.0; python_version < '\''3.14'\'''`}, + {"injection attempt", `x'; echo pwned; '`, `'x'\''; echo pwned; '\'''`}, + {"double quotes are inert", `pkg=="1.0"`, `'pkg=="1.0"'`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := shellSingleQuote(tt.in); got != tt.want { + t.Errorf("shellSingleQuote(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +// TestInjectUvOverrides_QuotedMarker verifies that a PEP 508 environment marker containing +// single quotes survives into the Dockerfile intact, and that a spec cannot break out of its +// quoting to inject shell. The value is checked by actually running the emitted line through +// a shell, since correctness here is a property of shell parsing, not of string equality. +func TestInjectUvOverrides_QuotedMarker(t *testing.T) { + t.Parallel() + marker := `fastmcp>=3.2.0; python_version < '3.14'` + + out, err := injectUvOverrides(sampleUvxDockerfile, []ConstraintEntry{ + {Spec: marker, Reason: "marker must survive quoting"}, + }) + if err != nil { + t.Fatalf("injectUvOverrides returned error: %v", err) + } + + // The emitted RUN must carry the marker with its inner quotes escaped, not stripped. + if !strings.Contains(out, `'fastmcp>=3.2.0; python_version < '\''3.14'\'''`) { + t.Errorf("expected the marker's single quotes to be escaped, got:\n%s", out) + } + + // Extract the printf command and run it, asserting the file content is byte-identical + // to the original spec. + got := runEmittedPrintf(t, out) + if got != marker+"\n" { + t.Errorf("overrides file content = %q, want %q", got, marker+"\n") + } +} + +func TestInjectUvOverrides_NoShellInjection(t *testing.T) { + t.Parallel() + // A spec that would escape its quoting and run a command if interpolated naively. + evil := `x'; echo PWNED; '` + + out, err := injectUvOverrides(sampleUvxDockerfile, []ConstraintEntry{ + {Spec: evil, Reason: "injection attempt"}, + }) + if err != nil { + t.Fatalf("injectUvOverrides returned error: %v", err) + } + + got := runEmittedPrintf(t, out) + if strings.Contains(got, "PWNED") && !strings.Contains(evil, "PWNED>") { + // PWNED appearing as literal text is fine; it executing is not. Distinguish by + // requiring the content to be exactly the spec. + if got != evil+"\n" { + t.Errorf("spec was not treated as literal data: got %q, want %q", got, evil+"\n") + } + } + if got != evil+"\n" { + t.Errorf("overrides file content = %q, want %q", got, evil+"\n") + } +} + +// runEmittedPrintf finds the injected "RUN printf ... > /tmp/uv-overrides.txt" step in a +// Dockerfile, executes its shell command with the redirect retargeted to a temp file, and +// returns what was written. This validates the emitted line against a real shell rather +// than assuming how it parses. +func runEmittedPrintf(t *testing.T, dockerfile string) string { + t.Helper() + + start := strings.Index(dockerfile, "RUN printf") + if start == -1 { + t.Fatalf("no 'RUN printf' step found in:\n%s", dockerfile) + } + end := strings.Index(dockerfile[start:], "> /tmp/uv-overrides.txt") + if end == -1 { + t.Fatalf("no overrides-file redirect found in:\n%s", dockerfile) + } + + outFile := filepath.Join(t.TempDir(), "overrides.txt") + // Strip the "RUN " prefix and retarget the redirect; the rest is the shell command + // exactly as the Dockerfile would run it. + script := strings.TrimPrefix(dockerfile[start:start+end], "RUN ") + "> " + outFile + + cmd := exec.Command("sh", "-c", script) + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + t.Fatalf("emitted shell command failed: %v\nscript:\n%s\nstderr: %s", err, script, stderr.String()) + } + + content, err := os.ReadFile(outFile) + if err != nil { + t.Fatalf("could not read the overrides file the command wrote: %v", err) + } + return string(content) +} + +func TestValidateDependencyOverrides_RejectsControlChars(t *testing.T) { + t.Parallel() + tests := []struct { + name string + spec MCPServerSpec + }{ + { + name: "newline in constraint spec", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolUvx}, + Spec: MCPServerPackageSpec{ + Constraints: []ConstraintEntry{{Spec: "fastmcp>=3.2.0\nRUN echo pwned", Reason: "r"}}, + }, + }, + }, + { + name: "newline in override version", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolNpx}, + Spec: MCPServerPackageSpec{ + Overrides: []OverrideEntry{{Package: "p", Version: "1.0.0\nRUN echo pwned", Reason: "r"}}, + }, + }, + }, + { + name: "carriage return in override package", + spec: MCPServerSpec{ + Metadata: MCPServerMetadata{Protocol: protocolNpx}, + Spec: MCPServerPackageSpec{ + Overrides: []OverrideEntry{{Package: "p\r", Version: testOverrideVersion, Reason: "r"}}, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if err := validateDependencyOverrides(&tt.spec); err == nil { + t.Error("expected an error for a value containing control characters") + } + }) + } +} + +// TestInjectNpmOverrides_NoShellInjection verifies the npm path quotes its JSON payload, so +// an override value containing a single quote cannot terminate the echo and inject shell. +func TestInjectNpmOverrides_NoShellInjection(t *testing.T) { + t.Parallel() + out, err := injectNpmOverrides(sampleNpxDockerfile, []OverrideEntry{ + {Package: "p", Version: `1.0.0'; echo PWNED; '`, Reason: "injection attempt"}, + }) + if err != nil { + t.Fatalf("injectNpmOverrides returned error: %v", err) + } + + // Run the emitted echo and confirm the result is valid JSON carrying the literal value. + start := strings.Index(out, "RUN echo ") + if start == -1 { + t.Fatalf("no 'RUN echo' step found in:\n%s", out) + } + line := out[start:] + if i := strings.Index(line, "\n"); i != -1 { + line = line[:i] + } + outFile := filepath.Join(t.TempDir(), "package.json") + script := strings.Replace(strings.TrimPrefix(line, "RUN "), "> package.json", "> "+outFile, 1) + + cmd := exec.Command("sh", "-c", script) + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + t.Fatalf("emitted shell command failed: %v\nscript:\n%s\nstderr: %s", err, script, stderr.String()) + } + content, err := os.ReadFile(outFile) + if err != nil { + t.Fatalf("could not read the package.json the command wrote: %v", err) + } + + var pkg map[string]any + if err := json.Unmarshal(content, &pkg); err != nil { + t.Fatalf("emitted package.json is not valid JSON (%q): %v", content, err) + } + overrides, ok := pkg["overrides"].(map[string]any) + if !ok { + t.Fatalf("no overrides block in %q", content) + } + if overrides["p"] != `1.0.0'; echo PWNED; '` { + t.Errorf("override value was not preserved literally: got %v", overrides["p"]) + } +} From 76c838f8ff65b548c85e2a3d7af69d94e512ea53 Mon Sep 17 00:00:00 2001 From: Dan Barr <6922515+danbarr@users.noreply.github.com> Date: Wed, 5 Aug 2026 11:19:08 -0400 Subject: [PATCH 6/6] test(dockhand): extract repeated literals to satisfy goconst The new test cases pushed "1.26.0" and "injection attempt" past goconst's occurrence threshold, failing CI lint. --- cmd/dockhand/main_test.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/cmd/dockhand/main_test.go b/cmd/dockhand/main_test.go index 2cbee659..1b511546 100644 --- a/cmd/dockhand/main_test.go +++ b/cmd/dockhand/main_test.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "fmt" "os" "os/exec" "path/filepath" @@ -17,6 +18,9 @@ import ( const ( testOverrideVersion = "1.0.0" testFastmcpSpec = "fastmcp>=3.2.0" + testSDKPackage = "@modelcontextprotocol/sdk" + testSDKVersion = "1.26.0" + testInjectionReason = "injection attempt" ) // sampleNpxDockerfile mirrors the package.json + npm install steps that toolhive's @@ -51,7 +55,7 @@ ENTRYPOINT ["sh", "-c", "exec 'mcp-clickhouse' \"$@\"", "--"] func TestInjectNpmOverrides(t *testing.T) { t.Parallel() overrides := []OverrideEntry{ - {Package: "@modelcontextprotocol/sdk", Version: "1.26.0", Reason: "CVE fix; upstream hard-pins 1.21.2"}, + {Package: testSDKPackage, Version: testSDKVersion, Reason: "CVE fix; upstream hard-pins 1.21.2"}, } out, err := injectNpmOverrides(sampleNpxDockerfile, overrides) @@ -63,7 +67,7 @@ func TestInjectNpmOverrides(t *testing.T) { if !strings.Contains(out, `"overrides":`) { t.Errorf("expected an overrides block in the generated package.json, got:\n%s", out) } - if !strings.Contains(out, `"@modelcontextprotocol/sdk":"1.26.0"`) { + if !strings.Contains(out, fmt.Sprintf(`%q:%q`, testSDKPackage, testSDKVersion)) { t.Errorf("expected the pinned SDK override in the package.json, got:\n%s", out) } @@ -258,7 +262,7 @@ func TestInjectNpmOverrides_AgainstRealTemplate(t *testing.T) { dockerfile := generateRealDockerfile(t, "npx://@brightdata/mcp@2.9.5", nil) out, err := injectNpmOverrides(dockerfile, []OverrideEntry{ - {Package: "@modelcontextprotocol/sdk", Version: "1.26.0", Reason: "CVE fix"}, + {Package: testSDKPackage, Version: testSDKVersion, Reason: "CVE fix"}, }) if err != nil { t.Fatalf("injection failed against the real toolhive npx template: %v", err) @@ -275,8 +279,8 @@ func TestInjectNpmOverrides_AgainstRealTemplate(t *testing.T) { if !ok { t.Fatalf("expected an overrides block in the rewritten package.json, got %q", payload) } - if overrides["@modelcontextprotocol/sdk"] != "1.26.0" { - t.Errorf("expected the SDK override to be 1.26.0, got %v", overrides["@modelcontextprotocol/sdk"]) + if overrides[testSDKPackage] != testSDKVersion { + t.Errorf("expected the SDK override to be %s, got %v", testSDKVersion, overrides[testSDKPackage]) } // Whatever toolhive put in the original payload must still be there. @@ -383,7 +387,7 @@ func TestShellSingleQuote(t *testing.T) { {"plain", "fastmcp>=3.2.0", `'fastmcp>=3.2.0'`}, {"pep508 marker with quotes", `fastmcp>=3.2.0; python_version < '3.14'`, `'fastmcp>=3.2.0; python_version < '\''3.14'\'''`}, - {"injection attempt", `x'; echo pwned; '`, `'x'\''; echo pwned; '\'''`}, + {testInjectionReason, `x'; echo pwned; '`, `'x'\''; echo pwned; '\'''`}, {"double quotes are inert", `pkg=="1.0"`, `'pkg=="1.0"'`}, } for _, tt := range tests { @@ -430,7 +434,7 @@ func TestInjectUvOverrides_NoShellInjection(t *testing.T) { evil := `x'; echo PWNED; '` out, err := injectUvOverrides(sampleUvxDockerfile, []ConstraintEntry{ - {Spec: evil, Reason: "injection attempt"}, + {Spec: evil, Reason: testInjectionReason}, }) if err != nil { t.Fatalf("injectUvOverrides returned error: %v", err) @@ -533,7 +537,7 @@ func TestValidateDependencyOverrides_RejectsControlChars(t *testing.T) { func TestInjectNpmOverrides_NoShellInjection(t *testing.T) { t.Parallel() out, err := injectNpmOverrides(sampleNpxDockerfile, []OverrideEntry{ - {Package: "p", Version: `1.0.0'; echo PWNED; '`, Reason: "injection attempt"}, + {Package: "p", Version: `1.0.0'; echo PWNED; '`, Reason: testInjectionReason}, }) if err != nil { t.Fatalf("injectNpmOverrides returned error: %v", err)