From 7e60ef7cc5f72174b5fd2767fe717051757e4894 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:13:29 -0400 Subject: [PATCH 1/5] fix(plugin): verify install checksums against the artifact actually downloaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A plugin with a binaryName has downloadPluginPackageForTier try its per-platform release asset first, falling back to the source tarball only when that download fails. But the registry carried exactly one checksum per plugin (the source tarball's), and installLocked always verified against it regardless of which artifact was actually on disk — so any binaryName plugin whose per-platform asset downloaded successfully had its platform tarball's bytes hashed and compared against the source tarball's checksum, which can never match. downloadPluginPackageForTier now reports which artifact it fetched (ArtifactKindSource, or a platform string from PlatformArch()) alongside the path. installLocked resolves the checksum that matches via the new resolveArtifactChecksum: the source artifact uses manifest.Checksum exactly as before (including its existing FIX-CLI-6 warn-and-proceed leniency when empty); a platform artifact uses the matching entry in the new PluginManifest.PlatformChecksums map, parsed from the registry's checksums.platforms object. A platform artifact with no matching registry checksum is refused unconditionally — it never falls through to the source-checksum leniency, and no env var (NSELF_PLUGIN_REQUIRE_CHECKSUM included) changes that. That leniency exists for the documented, tracked source-checksum coverage gap; it was never a license to install a downloaded executable with zero verification. A release that predates PlatformChecksums, or one platform whose checksum was never backfilled, is a registry data gap to fix upstream, not a flag to bypass here. PlatformChecksums round-trips through the registry cache (Registry.MarshalJSON) the same way every other field in this package must, per TestRegistryRoundTripLosesNoField. --- internal/plugin/download.go | 32 +++++++---- internal/plugin/download_platform_test.go | 15 +++++- internal/plugin/installer_locked.go | 25 ++++++--- internal/plugin/interfaces.go | 21 ++++++++ internal/plugin/registry_cache.go | 13 +++++ internal/plugin/registry_json.go | 50 +++++++++-------- internal/plugin/registry_parse.go | 65 ++++++++++++++++++----- internal/plugin/security.go | 38 +++++++++++++ 8 files changed, 205 insertions(+), 54 deletions(-) diff --git a/internal/plugin/download.go b/internal/plugin/download.go index a6042221f..9189fb4d9 100644 --- a/internal/plugin/download.go +++ b/internal/plugin/download.go @@ -24,6 +24,14 @@ import ( "github.com/nself-org/cli/internal/license" ) +// ArtifactKindSource is the artifact kind downloadPluginPackageForTier +// reports when it fetched the source tarball. Any other value it returns is +// one of PlatformArch()'s platform strings (darwin-arm64, darwin-amd64, +// linux-amd64, linux-arm64, windows-amd64), identifying a per-platform +// binary tarball instead. Callers use this to pick the checksum that +// actually matches the bytes on disk — see installer_locked.go's Step 5. +const ArtifactKindSource = "source" + // downloadPlugin fetches the plugin tarball to a temporary file. // For paid plugins, it sends the X-License-Key header required by ping.nself.org. // For free plugins, it tries the R2-backed worker URL first and falls back to @@ -35,18 +43,24 @@ import ( // see license.go's isPaidPluginManifest doc comment for why the name map // drifts and the registry fields do not. func downloadPlugin(ctx context.Context, name, version, repository string) (string, error) { //nolint:unused // kept: name-only fallback entry point retained deliberately; see qa/bugs/declared-but-never-wired-symbols.md - return downloadPluginPackage(ctx, name, version, repository, "") + path, _, err := downloadPluginPackage(ctx, name, version, repository, "") + return path, err } // downloadPluginPackage fetches a plugin package using the name-only // isPaidPlugin fallback. Prefer downloadPluginPackageForTier when a manifest // is available (see its doc comment). -func downloadPluginPackage(ctx context.Context, name, version, repository, binaryName string) (string, error) { +func downloadPluginPackage(ctx context.Context, name, version, repository, binaryName string) (string, string, error) { return downloadPluginPackageForTier(ctx, name, version, repository, binaryName, isPaidPlugin(name)) } // downloadPluginPackageForTier fetches a plugin package, preferring a build -// for the running platform when the plugin ships a command binary. +// for the running platform when the plugin ships a command binary. It +// returns the local temp file path and the kind of artifact it actually +// fetched (ArtifactKindSource or a platform string) — the caller needs the +// kind to know which registry checksum applies, since a source tarball and a +// platform tarball for the same plugin+version have different bytes and +// therefore different checksums (see PluginManifest.PlatformChecksums). // // paid must come from the registry manifest (isPaidPluginManifest), not the // static paidPlugins name map — that map only lists 59 of 127 registered paid @@ -61,7 +75,7 @@ func downloadPluginPackage(ctx context.Context, name, version, repository, binar // someone on macOS. Those live as per-platform release assets, so they are // tried first, with the generic package as the fallback for a plugin whose // release predates per-platform assets. -func downloadPluginPackageForTier(ctx context.Context, name, version, repository, binaryName string, paid bool) (string, error) { +func downloadPluginPackageForTier(ctx context.Context, name, version, repository, binaryName string, paid bool) (string, string, error) { // Platform-specific package first, for a plugin that provides a command. if binaryName != "" { if platform, err := PlatformArch(); err == nil { @@ -71,7 +85,7 @@ func downloadPluginPackageForTier(ctx context.Context, name, version, repository } platformURL := binaryPluginDownloadURL(strings.TrimSuffix(repo, ".git"), name, version, platform) if tmp, err := downloadFromURL(ctx, platformURL, nil); err == nil { - return tmp, nil + return tmp, platform, nil } // Fall through: no per-platform asset for this release. } @@ -91,7 +105,7 @@ func downloadPluginPackageForTier(ctx context.Context, name, version, repository tmp, err := downloadFromURL(ctx, primaryURL, extraHeaders) if err == nil { - return tmp, nil + return tmp, ArtifactKindSource, nil } // If not a paid plugin, attempt GitHub Releases fallback on primary failure. @@ -100,13 +114,13 @@ func downloadPluginPackageForTier(ctx context.Context, name, version, repository if fallbackURL != primaryURL { tmp2, fallbackErr := downloadFromURL(ctx, fallbackURL, nil) if fallbackErr == nil { - return tmp2, nil + return tmp2, ArtifactKindSource, nil } - return "", fmt.Errorf("download failed: primary %s: %w; fallback %s: %v", primaryURL, err, fallbackURL, fallbackErr) + return "", "", fmt.Errorf("download failed: primary %s: %w; fallback %s: %v", primaryURL, err, fallbackURL, fallbackErr) } } - return "", err + return "", "", err } // downloadFromURL fetches a single URL to a temp file and returns the file path. diff --git a/internal/plugin/download_platform_test.go b/internal/plugin/download_platform_test.go index debef889d..0806fc12f 100644 --- a/internal/plugin/download_platform_test.go +++ b/internal/plugin/download_platform_test.go @@ -31,16 +31,19 @@ func TestDownloadPluginPackagePrefersPlatformAsset(t *testing.T) { name string binaryName string wantPath string + wantKind string }{ { name: "cli plugin asks for its platform build", binaryName: "nself-example", wantPath: "/releases/download/v1.0.0/example-1.0.0-" + platform + ".tar.gz", + wantKind: platform, }, { name: "service plugin uses the generic package", binaryName: "", wantPath: "/plugins/example/tarball", + wantKind: ArtifactKindSource, }, } @@ -62,12 +65,16 @@ func TestDownloadPluginPackagePrefersPlatformAsset(t *testing.T) { t.Setenv("NSELF_PLUGIN_REGISTRY", srv.URL) - path, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, tt.binaryName) + path, kind, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, tt.binaryName) if err != nil { t.Fatalf("download: %v", err) } _ = path + if kind != tt.wantKind { + t.Errorf("artifact kind = %q, want %q", kind, tt.wantKind) + } + mu.Lock() defer mu.Unlock() if len(seen) == 0 { @@ -106,9 +113,13 @@ func TestDownloadPluginPackageFallsBackWhenNoPlatformAsset(t *testing.T) { t.Setenv("NSELF_PLUGIN_REGISTRY", srv.URL) - if _, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, "nself-example"); err != nil { + _, kind, err := downloadPluginPackage(context.Background(), "example", "1.0.0", srv.URL, "nself-example") + if err != nil { t.Fatalf("expected fallback to succeed, got: %v", err) } + if kind != ArtifactKindSource { + t.Errorf("artifact kind = %q, want %q (fell back to the generic package)", kind, ArtifactKindSource) + } mu.Lock() defer mu.Unlock() diff --git a/internal/plugin/installer_locked.go b/internal/plugin/installer_locked.go index ecdb630da..1b98fca3c 100644 --- a/internal/plugin/installer_locked.go +++ b/internal/plugin/installer_locked.go @@ -166,20 +166,31 @@ func installLocked(ctx context.Context, cfg *config.Config, name string, pluginD // Tier comes from the registry manifest, not the paidPlugins name map, // which has drifted — see isPaidPluginManifest in license.go. paid := isPaidPluginManifest(manifest) - archivePath, err := downloadPluginPackageForTier(ctx, name, manifest.Version, manifest.Repository, firstBinary, paid) + archivePath, artifactKind, err := downloadPluginPackageForTier(ctx, name, manifest.Version, manifest.Repository, firstBinary, paid) if err != nil { return fmt.Errorf("downloading plugin %q: %w", name, err) } defer func() { _ = os.Remove(archivePath) }() // Step 5: Verify checksum before extraction. + // + // Which checksum applies depends on WHICH artifact Step 4 actually + // downloaded — see resolveArtifactChecksum's doc comment. + expectedChecksum, err := resolveArtifactChecksum(*manifest, artifactKind) + if err != nil { + _ = os.Remove(archivePath) + return err + } + // A checksum that IS present and wrong always refuses the install. A - // MISSING checksum only refuses when NSELF_PLUGIN_REQUIRE_CHECKSUM=1 is - // set (default: warn and proceed, for every publishStatus including an - // effectively-stable one — registry coverage is 47/177 as of 2026-09-04, - // see verifyChecksum's doc comment; FIX-CLI-6). - if manifest.Checksum != "" { - if err := verifyChecksum(archivePath, manifest.Checksum, manifest.PublishStatus); err != nil { + // MISSING checksum for the SOURCE artifact only refuses when + // NSELF_PLUGIN_REQUIRE_CHECKSUM=1 is set (default: warn and proceed, for + // every publishStatus including an effectively-stable one — registry + // coverage is 47/177 as of 2026-09-04, see verifyChecksum's doc comment; + // FIX-CLI-6). A platform artifact never reaches this branch with an + // empty expectedChecksum — the block above already refused it. + if expectedChecksum != "" { + if err := verifyChecksum(archivePath, expectedChecksum, manifest.PublishStatus); err != nil { _ = os.Remove(archivePath) return fmt.Errorf("checksum verification for plugin %q: %w", name, err) } diff --git a/internal/plugin/interfaces.go b/internal/plugin/interfaces.go index 90eb9cc8f..55d3fd2a8 100644 --- a/internal/plugin/interfaces.go +++ b/internal/plugin/interfaces.go @@ -240,4 +240,25 @@ type PluginManifest struct { // --detailed` displays it when present (registry entry or local // plugin.json) and shows nothing invented when it is not. UpdatedAt string `json:"updated_at,omitempty"` + + // PlatformChecksums holds one SHA-256 checksum (lowercase hex, no + // "sha256:" prefix) per per-platform binary tarball, keyed by the exact + // platform string PlatformArch() returns (darwin-arm64, darwin-amd64, + // linux-amd64, linux-arm64, windows-amd64). Populated from the registry's + // nested `checksums.platforms` object. + // + // Checksum above is ALWAYS the source tarball's checksum, never a + // platform one — the two packages have different bytes, so one flat + // field can never validate both (FIX-CLI-plugins-83: a platform tarball + // downloaded via binaryPluginDownloadURL was being checked against + // Checksum, which could never match). downloadPluginPackageForTier + // reports which artifact it fetched; installLocked picks the matching + // entry from this map, or Checksum for the source artifact — see + // verifyChecksum's caller in installer_locked.go. + // + // Only present for a plugin with a binaryName. A plugin whose release + // predates this field (or a platform the release never built a checksum + // for) has no entry here — see downloadPluginPackageForTier's doc + // comment for what happens then. + PlatformChecksums map[string]string `json:"platform_checksums,omitempty"` } diff --git a/internal/plugin/registry_cache.go b/internal/plugin/registry_cache.go index 481bfa255..201043538 100644 --- a/internal/plugin/registry_cache.go +++ b/internal/plugin/registry_cache.go @@ -101,6 +101,18 @@ func (r Registry) MarshalJSON() ([]byte, error) { } rawDeps = b } + // Checksums re-serialises PlatformChecksums into the nested registry + // shape, matching how it was parsed in — see pluginChecksumsEntry. + // SHA256 is left empty: the cache's source-tarball checksum lives in + // the flat Checksum field below (and always has), so re-populating + // the nested duplicate here would just be inventing a value never + // read back by anything (entryToManifest sources PlatformChecksums + // from Checksums.Platforms only). + var checksums *pluginChecksumsEntry + if len(p.PlatformChecksums) > 0 { + checksums = &pluginChecksumsEntry{Platforms: p.PlatformChecksums} + } + entries = append(entries, pluginEntry{ Name: p.Name, Version: p.Version, @@ -112,6 +124,7 @@ func (r Registry) MarshalJSON() ([]byte, error) { Tier: p.Tier, Repository: p.Repository, Checksum: p.Checksum, + Checksums: checksums, Tags: p.Tags, Tables: p.Tables, Port: p.Port, diff --git a/internal/plugin/registry_json.go b/internal/plugin/registry_json.go index 0b6dc9cf8..8e6bbfa28 100644 --- a/internal/plugin/registry_json.go +++ b/internal/plugin/registry_json.go @@ -130,29 +130,35 @@ func entryToManifest(e pluginEntry) PluginManifest { implFramework = e.Implementation.Framework } + var platformChecksums map[string]string + if e.Checksums != nil { + platformChecksums = normalizePlatformChecksums(e.Checksums.Platforms) + } + return PluginManifest{ - Name: e.Name, - Version: e.Version, - Description: e.Description, - Category: e.Category, - License: e.License, - LicenseType: e.LicenseType, - Tier: tier, - Repository: e.Repository, - Checksum: e.Checksum, - Tags: e.Tags, - RequiresLicense: e.RequiresLicense, - Tables: e.Tables, - Port: port, - TierPair: e.TierPair, - Bundles: e.Bundles, - Dependencies: parseDependencies(e.Dependencies), - APIEndpoints: parseAPIEndpoints(e.APIEndpoints), - Language: language, - Runtime: runtime, - PluginType: pluginType, - BinaryName: binaryName, - CLICommands: e.CLICommands, + Name: e.Name, + Version: e.Version, + Description: e.Description, + Category: e.Category, + License: e.License, + LicenseType: e.LicenseType, + Tier: tier, + Repository: e.Repository, + Checksum: e.Checksum, + PlatformChecksums: platformChecksums, + Tags: e.Tags, + RequiresLicense: e.RequiresLicense, + Tables: e.Tables, + Port: port, + TierPair: e.TierPair, + Bundles: e.Bundles, + Dependencies: parseDependencies(e.Dependencies), + APIEndpoints: parseAPIEndpoints(e.APIEndpoints), + Language: language, + Runtime: runtime, + PluginType: pluginType, + BinaryName: binaryName, + CLICommands: e.CLICommands, Author: e.Author, Homepage: e.Homepage, diff --git a/internal/plugin/registry_parse.go b/internal/plugin/registry_parse.go index 5b936bc97..8748d2f4b 100644 --- a/internal/plugin/registry_parse.go +++ b/internal/plugin/registry_parse.go @@ -62,26 +62,42 @@ type pluginEndpointEntry struct { Description string `json:"description"` } +// pluginChecksumsEntry holds the registry's nested `checksums` object. +// `sha256` duplicates the flat `checksum` field (source tarball, kept for +// backwards compatibility with older registry readers); `platforms` carries +// one checksum per per-platform binary tarball, keyed by the exact platform +// string internal/plugin/arch.go's PlatformArch() returns. Only present for +// a plugin with a binaryName — see PluginManifest.PlatformChecksums. +type pluginChecksumsEntry struct { + SHA256 string `json:"sha256,omitempty"` + Platforms map[string]string `json:"platforms,omitempty"` +} + // pluginEntry matches the fields present in the array-format (pro) // registry as well as the object-format (free) registry. // APIEndpoints is kept as json.RawMessage because the live registry returns // it as an array of objects while older/local registries use an array of // strings. We normalise both into []string during entryToManifest conversion. type pluginEntry struct { - Name string `json:"name"` - Version string `json:"version"` - Description string `json:"description"` - Category string `json:"category"` - Tier string `json:"tier"` - License string `json:"license"` - LicenseType string `json:"licenseType"` - Repository string `json:"repository"` - Checksum string `json:"checksum"` - DownloadURL string `json:"download_url"` - RequiresLicense bool `json:"requires_license"` - Tags []string `json:"tags"` - Tables []string `json:"tables,omitempty"` - Port int `json:"port,omitempty"` + Name string `json:"name"` + Version string `json:"version"` + Description string `json:"description"` + Category string `json:"category"` + Tier string `json:"tier"` + License string `json:"license"` + LicenseType string `json:"licenseType"` + Repository string `json:"repository"` + Checksum string `json:"checksum"` + // Checksums is the nested form carrying per-platform binary tarball + // checksums (checksums.platforms) alongside the source-tarball checksum + // (checksums.sha256, a duplicate of the flat Checksum field above). See + // pluginChecksumsEntry and PluginManifest.PlatformChecksums. + Checksums *pluginChecksumsEntry `json:"checksums,omitempty"` + DownloadURL string `json:"download_url"` + RequiresLicense bool `json:"requires_license"` + Tags []string `json:"tags"` + Tables []string `json:"tables,omitempty"` + Port int `json:"port,omitempty"` // TierPair and Bundles support install-time tier resolution for a slug // served twice (free + pro) as one product — see PluginManifest's doc // comments (interfaces.go) and tier_resolve.go. @@ -205,6 +221,27 @@ func parseAPIEndpoints(raw json.RawMessage) []string { return out } +// normalizePlatformChecksums copies a registry's checksums.platforms map, +// stripping an optional "sha256:" prefix from each value. The plugins repo +// writes the sibling checksums.sha256 field with that prefix (see +// release-tarballs.yml's backfill step); platform checksums are documented +// to be written as plain hex, but stripping a stray prefix defensively here +// costs nothing and avoids a checksum that LOOKS present but can never +// match verifyChecksum's raw hex comparison — which would otherwise surface +// as an install-time checksum mismatch instead of the registry data error it +// actually is. Returns nil for an empty/nil input, matching the omitempty +// contract used throughout this package. +func normalizePlatformChecksums(raw map[string]string) map[string]string { + if len(raw) == 0 { + return nil + } + out := make(map[string]string, len(raw)) + for platform, checksum := range raw { + out[platform] = strings.TrimPrefix(strings.ToLower(strings.TrimSpace(checksum)), "sha256:") + } + return out +} + // parseDependencies extracts the list of plugin-name dependencies from the // raw `dependencies` JSON value. The registry has shipped several shapes over // time, so we accept all of them and ignore non-plugin keys (npm/system/python) diff --git a/internal/plugin/security.go b/internal/plugin/security.go index fd20afa2d..fd04bfccd 100644 --- a/internal/plugin/security.go +++ b/internal/plugin/security.go @@ -116,6 +116,44 @@ func verifyChecksum(filePath string, expectedHash string, publishStatus string) return nil } +// resolveArtifactChecksum picks the registry checksum that matches the +// artifact downloadPluginPackageForTier actually fetched, identified by +// artifactKind (ArtifactKindSource, or a platform string from PlatformArch()). +// +// The source tarball and each platform's binary tarball are different bytes +// for the same plugin+version, so a source checksum can never validate a +// platform download or vice versa — comparing the wrong one is not a +// stricter check, it is a guaranteed-wrong one (plugins#83, the bug this +// function exists to close). +// +// For the source artifact this returns manifest.Checksum verbatim, including +// when it is empty — verifyChecksum's own warn-and-proceed leniency for a +// missing SOURCE checksum (FIX-CLI-6, a documented and tracked registry +// coverage gap) still applies, unchanged, to whatever this returns. +// +// For a platform artifact, a missing checksum in +// manifest.PlatformChecksums[artifactKind] is refused HERE, unconditionally +// — it never reaches verifyChecksum's lenient empty-string path, and no env +// var (NSELF_PLUGIN_REQUIRE_CHECKSUM included) changes that. The FIX-CLI-6 +// leniency exists for the source-checksum coverage gap; it was never a +// license to install an actual downloaded EXECUTABLE with zero +// verification. A release that predates PlatformChecksums, or one platform's +// checksum that was never backfilled, are both registry data gaps — the fix +// is a registry checksum, not a flag that makes the install proceed anyway. +func resolveArtifactChecksum(manifest PluginManifest, artifactKind string) (string, error) { + if artifactKind == ArtifactKindSource || artifactKind == "" { + return manifest.Checksum, nil + } + + checksum, ok := manifest.PlatformChecksums[artifactKind] + if !ok || checksum == "" { + return "", fmt.Errorf( + "plugin %q: registry has no checksum for platform %q at version %s — refusing to install an unverified binary (report to nself-org/plugins)", + manifest.Name, artifactKind, manifest.Version) + } + return checksum, nil +} + // verifyPluginSignature verifies that the Ed25519 signature stored in the // plugin's registry manifest matches the SHA-256 hash of the downloaded // tarball. The public key is pinned in the registry (never fetched at verify From 2a2d99e3e642bf1af8085da367c65305b068426f Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:13:34 -0400 Subject: [PATCH 2/5] test(plugin): cover per-platform checksum verification and the absent policy resolveArtifactChecksum: the source artifact always uses manifest.Checksum (including the empty-string case, left for verifyChecksum's own leniency to handle); a platform artifact resolves to its matching PlatformChecksums entry, never the source checksum or a different platform's; a platform artifact with no matching entry is refused regardless of NSELF_PLUGIN_REQUIRE_CHECKSUM, a present source checksum, or whether the map is nil versus just missing that one key. Three end-to-end tests exercise the same policy through verifyChecksum: a correct platform checksum passes, a mismatched one is rejected (mirroring the existing source-checksum mismatch test), and a missing one never reaches verifyChecksum's lenient empty-string branch at all. --- internal/plugin/checksum_artifact_test.go | 196 ++++++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 internal/plugin/checksum_artifact_test.go diff --git a/internal/plugin/checksum_artifact_test.go b/internal/plugin/checksum_artifact_test.go new file mode 100644 index 000000000..f8773c32d --- /dev/null +++ b/internal/plugin/checksum_artifact_test.go @@ -0,0 +1,196 @@ +package plugin + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + "testing" +) + +// TestResolveArtifactChecksum_SourceUsesManifestChecksum verifies that for +// the source artifact (ArtifactKindSource, and the zero value "" — a +// download that never got as far as reporting a kind), resolveArtifactChecksum +// returns manifest.Checksum verbatim, unaffected by whatever is in +// PlatformChecksums. +func TestResolveArtifactChecksum_SourceUsesManifestChecksum(t *testing.T) { + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "deadbeef", + PlatformChecksums: map[string]string{"linux-amd64": "should-not-be-used"}, + } + + for _, kind := range []string{ArtifactKindSource, ""} { + t.Run("kind="+kind, func(t *testing.T) { + got, err := resolveArtifactChecksum(manifest, kind) + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if got != "deadbeef" { + t.Errorf("got %q, want manifest.Checksum %q", got, manifest.Checksum) + } + }) + } +} + +// TestResolveArtifactChecksum_SourceEmptyChecksumPassesThrough verifies that +// an empty manifest.Checksum for the source artifact comes back as "", not +// an error — the FIX-CLI-6 warn-and-proceed leniency for a missing SOURCE +// checksum is verifyChecksum's decision to make, not this function's. +func TestResolveArtifactChecksum_SourceEmptyChecksumPassesThrough(t *testing.T) { + manifest := PluginManifest{Name: "example", Version: "1.0.1"} + + got, err := resolveArtifactChecksum(manifest, ArtifactKindSource) + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if got != "" { + t.Errorf("got %q, want empty string", got) + } +} + +// TestResolveArtifactChecksum_PlatformUsesMatchingEntry verifies that a +// platform artifact resolves to PlatformChecksums[platform], the entry +// matching the artifact actually downloaded — not manifest.Checksum (the +// SOURCE tarball's checksum, different bytes entirely) and not a different +// platform's entry. +func TestResolveArtifactChecksum_PlatformUsesMatchingEntry(t *testing.T) { + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "source-checksum-must-not-be-used", + PlatformChecksums: map[string]string{ + "darwin-arm64": "darwin-arm64-checksum", + "linux-amd64": "linux-amd64-checksum", + }, + } + + got, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if got != "linux-amd64-checksum" { + t.Errorf("got %q, want the linux-amd64 entry, not the source checksum or a different platform's", got) + } +} + +// TestResolveArtifactChecksum_PlatformMissingEntryRefusesUnconditionally +// verifies the absent-checksum policy (plugins#83 item 8): a platform +// artifact with NO matching registry entry is refused with an error — +// regardless of NSELF_PLUGIN_REQUIRE_CHECKSUM, regardless of whether +// manifest.Checksum (the source checksum) happens to be present, and +// regardless of whether PlatformChecksums is nil or just missing this one +// platform's key. This is what makes the policy impossible to bypass: there +// is no code path, env var, or registry shape that turns a missing platform +// checksum into an empty string reaching verifyChecksum's lenient branch. +func TestResolveArtifactChecksum_PlatformMissingEntryRefusesUnconditionally(t *testing.T) { + cases := []struct { + name string + platformChecksums map[string]string + requireChecksum bool + }{ + {name: "nil map, default mode", platformChecksums: nil, requireChecksum: false}, + {name: "nil map, hard mode", platformChecksums: nil, requireChecksum: true}, + {name: "other platforms present, default mode", platformChecksums: map[string]string{"darwin-arm64": "abc123"}, requireChecksum: false}, + {name: "other platforms present, hard mode", platformChecksums: map[string]string{"darwin-arm64": "abc123"}, requireChecksum: true}, + {name: "empty-string entry for this platform", platformChecksums: map[string]string{"linux-amd64": ""}, requireChecksum: false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if tc.requireChecksum { + t.Setenv(pluginRequireChecksumEnv, "1") + } + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "source-checksum-present-but-irrelevant", + PlatformChecksums: tc.platformChecksums, + } + + _, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err == nil { + t.Fatal("expected an error refusing the install, got nil") + } + if !strings.Contains(err.Error(), "linux-amd64") { + t.Errorf("error should name the platform that has no checksum: %v", err) + } + }) + } +} + +// TestPlatformArtifact_VerifiesAgainstMatchingPlatformChecksum is the +// end-to-end shape of Step 5 for a platform download: resolve the checksum +// for the artifact actually downloaded, then verify the archive bytes +// against it. A correct platform checksum passes. +func TestPlatformArtifact_VerifiesAgainstMatchingPlatformChecksum(t *testing.T) { + content := []byte("fake linux-amd64 binary tarball bytes") + archivePath := writeTempFile(t, content) + sum := sha256.Sum256(content) + platformChecksum := hex.EncodeToString(sum[:]) + + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + Checksum: "unrelated-source-checksum", + PlatformChecksums: map[string]string{ + "linux-amd64": platformChecksum, + }, + } + + expected, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if err := verifyChecksum(archivePath, expected, "stable"); err != nil { + t.Fatalf("correct platform checksum should verify: %v", err) + } +} + +// TestPlatformArtifact_MismatchRejected verifies that a platform download +// whose bytes do not match its recorded platform checksum is rejected — the +// same as any other present-but-wrong checksum (mirrors +// TestVerifyChecksum_MismatchAlwaysRefusesRegardlessOfStatus, for the +// platform-checksum path specifically). +func TestPlatformArtifact_MismatchRejected(t *testing.T) { + content := []byte("fake linux-amd64 binary tarball bytes") + archivePath := writeTempFile(t, content) + wrongChecksum := hex.EncodeToString(make([]byte, sha256.Size)) // all-zero, guaranteed wrong + + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + PlatformChecksums: map[string]string{ + "linux-amd64": wrongChecksum, + }, + } + + expected, err := resolveArtifactChecksum(manifest, "linux-amd64") + if err != nil { + t.Fatalf("resolveArtifactChecksum: %v", err) + } + if err := verifyChecksum(archivePath, expected, "stable"); err == nil { + t.Fatal("tampered/mismatched platform artifact should fail verification, got nil error") + } +} + +// TestPlatformArtifact_AbsentChecksumNeverReachesLenientPath verifies the +// absent-checksum policy end to end: a platform artifact with no registry +// checksum never reaches verifyChecksum's warn-and-proceed branch at all — +// resolveArtifactChecksum refuses first, before any checksum comparison +// (lenient or not) would run. +func TestPlatformArtifact_AbsentChecksumNeverReachesLenientPath(t *testing.T) { + content := []byte("fake linux-amd64 binary tarball bytes") + archivePath := writeTempFile(t, content) + _ = archivePath // the archive is never reached; resolveArtifactChecksum fails first + + manifest := PluginManifest{ + Name: "example", + Version: "1.0.1", + PlatformChecksums: nil, + } + + if _, err := resolveArtifactChecksum(manifest, "linux-amd64"); err == nil { + t.Fatal("expected resolveArtifactChecksum to refuse before any verification would run") + } +} From db680d673f7265890108b5fc9d22b148c485f50d Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 10:21:54 -0400 Subject: [PATCH 3/5] refactor(plugin): trim installer_locked.go comments to the 300-line file cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-artifact checksum change pushed this file to 309 lines, past the engineering-standard cap enforced by internal/repoqa's TestFileSizeBudgetNotExceeded (budget: 0 files allowed over). Tightened the new Step 5 comments and removed a pre-existing duplicated sentence in the Step 4 comment — no behavior change, same logic, back to 300 lines exactly. --- internal/plugin/installer_locked.go | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/internal/plugin/installer_locked.go b/internal/plugin/installer_locked.go index 1b98fca3c..62d12ccb7 100644 --- a/internal/plugin/installer_locked.go +++ b/internal/plugin/installer_locked.go @@ -153,12 +153,9 @@ func installLocked(ctx context.Context, cfg *config.Config, name string, pluginD fmt.Fprintf(os.Stderr, " ✓ %s installed\n", dep) } - // Step 4: Download the plugin archive. - // A plugin that provides a command needs a package built for this platform; - // one that does not is source and works anywhere. cliBinaryName returns "" - // for the latter, which is most plugins. - // A plugin providing a command needs a package built for this platform; one - // that does not is source and works anywhere. + // Step 4: Download the plugin archive. A plugin that provides a command + // needs a package built for this platform; one that does not is source + // and works anywhere. cliBinaryName returns "" for the latter (most plugins). var firstBinary string if names := cliBinaryNames(name, manifest); len(names) > 0 { firstBinary = names[0] @@ -172,23 +169,17 @@ func installLocked(ctx context.Context, cfg *config.Config, name string, pluginD } defer func() { _ = os.Remove(archivePath) }() - // Step 5: Verify checksum before extraction. - // - // Which checksum applies depends on WHICH artifact Step 4 actually - // downloaded — see resolveArtifactChecksum's doc comment. + // Step 5: Verify checksum before extraction — resolveArtifactChecksum + // picks the checksum matching the artifact Step 4 downloaded. expectedChecksum, err := resolveArtifactChecksum(*manifest, artifactKind) if err != nil { _ = os.Remove(archivePath) return err } - // A checksum that IS present and wrong always refuses the install. A - // MISSING checksum for the SOURCE artifact only refuses when - // NSELF_PLUGIN_REQUIRE_CHECKSUM=1 is set (default: warn and proceed, for - // every publishStatus including an effectively-stable one — registry - // coverage is 47/177 as of 2026-09-04, see verifyChecksum's doc comment; - // FIX-CLI-6). A platform artifact never reaches this branch with an - // empty expectedChecksum — the block above already refused it. + // A present-but-wrong checksum always refuses; an empty one here is + // always the SOURCE artifact's — see verifyChecksum's FIX-CLI-6 doc + // comment (a missing platform checksum already refused above). if expectedChecksum != "" { if err := verifyChecksum(archivePath, expectedChecksum, manifest.PublishStatus); err != nil { _ = os.Remove(archivePath) From 500cbf4cc096d1440d4cf26e933818b182273858 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 17:40:33 -0400 Subject: [PATCH 4/5] feat(compose): extend CS_N custom services with image, env-file, and volume mounts nself build could not express a pinned image digest, injected SMTP env vars, or an extra volume mount for a custom service, so any stack needing them had to be hand-authored into a docker-compose.override.yml that nself build would silently drop on a rebuild (G-013). Adds three env vars to the CS_N model, all declarable in .env: - CS_N_IMAGE: run a pre-built image (optionally digest-pinned via @sha256:...) instead of building from a Dockerfile. Mutually exclusive with CS_N_PATH. - CS_N_ENV_FILE: inject KEY=VALUE pairs from a dotenv-format file, applied after CS_N_ENV_PASSTHROUGH and before CS_N_ENV. Handles values CS_N_ENV's comma-joined format cannot represent safely (e.g. a password containing a comma), and many-var cases like SMTP config. - CS_N_VOLUMES: comma-separated host:container[:mode] bind mounts, appended to the generated service. Generator gains WithWorkDir to anchor CS_N_ENV_FILE reads to the project root; the build orchestrator now threads st.workdir through so CS_N_ENV_FILE resolves correctly regardless of invocation directory. buildCustomService and Generate() now return an error on a missing/ unreadable CS_N_ENV_FILE rather than silently omitting the vars. --- .github/wiki/Config-Custom-Services.md | 8 +- internal/build/orchestrator_build_compose.go | 2 +- internal/compose/custom_service_extras.go | 57 ++++++ .../compose/custom_service_extras_test.go | 186 ++++++++++++++++++ internal/compose/custom_service_test.go | 91 ++++++--- internal/compose/custom_services.go | 132 +++++++++---- internal/compose/generator.go | 22 ++- internal/config/custom_services.go | 40 +++- .../custom_services_image_env_volumes_test.go | 159 +++++++++++++++ internal/config/custom_services_validate.go | 67 +++++++ internal/config/types_ops_plugins.go | 26 +++ 11 files changed, 710 insertions(+), 80 deletions(-) create mode 100644 internal/compose/custom_service_extras.go create mode 100644 internal/compose/custom_service_extras_test.go create mode 100644 internal/config/custom_services_image_env_volumes_test.go create mode 100644 internal/config/custom_services_validate.go diff --git a/.github/wiki/Config-Custom-Services.md b/.github/wiki/Config-Custom-Services.md index 3c86e4355..1668c20ec 100644 --- a/.github/wiki/Config-Custom-Services.md +++ b/.github/wiki/Config-Custom-Services.md @@ -82,7 +82,10 @@ All variables use the pattern `CS_N_*` where `N` is the slot number (1–10). Va | `CS_N_HEALTHCHECK` | string | `/health` | Healthcheck override. A path (e.g. `/auth/health`) probes that path instead of `/health` on the service's own port. A full `CMD ...` / `CMD-SHELL ...` command is passed through verbatim (split on whitespace) for services that need curl, a non-HTTP probe, or a different port. `disabled` / `none` / `false` omits the healthcheck entirely. | | `CS_N_TABLE_PREFIX` | string | *(empty)* | Database table prefix for this service's migrations | | `CS_N_ENV_PASSTHROUGH` | string | *(empty)* | Comma-separated allowlist of project `.env` var names to forward into this container in addition to the fixed core set. `CS_N_ENV` still wins on a name conflict. | -| `CS_N_ENV` | string | *(empty)* | Additional env vars to inject, in `KEY=VALUE,KEY=VALUE` format. Always applied last — overrides both the fixed core set and `CS_N_ENV_PASSTHROUGH`. | +| `CS_N_ENV_FILE` | string | *(empty)* | Project-relative path to a dotenv-format file whose `KEY=VALUE` lines are injected into this container. Applied after `CS_N_ENV_PASSTHROUGH`, before `CS_N_ENV`. Use this instead of `CS_N_ENV` when a value itself contains a comma (e.g. some SMTP passwords) or when there are too many vars for one line. A missing file fails `nself build` rather than silently starting the service without those vars. | +| `CS_N_ENV` | string | *(empty)* | Additional env vars to inject, in `KEY=VALUE,KEY=VALUE` format. Always applied last — overrides the fixed core set, `CS_N_ENV_PASSTHROUGH`, and `CS_N_ENV_FILE`. | +| `CS_N_IMAGE` | string | *(empty)* | Run a pre-built image instead of building from a Dockerfile — e.g. `minio/minio:RELEASE.2024-01-16T16-07-38Z@sha256:...` to pin an exact digest. Mutually exclusive with `CS_N_PATH`; when set, no `build:` block is emitted at all. | +| `CS_N_VOLUMES` | string | *(empty)* | Comma-separated extra bind mounts in `host:container[:mode]` form, e.g. `./email-templates:/app/templates:ro`. Appended to the service's generated volume list. | All `CS_*` variables are automatically exempt from "unknown env var" warnings. @@ -251,11 +254,12 @@ CS_2_REPLICAS=3 ## Notes -- Custom service images are built from the scaffolded Dockerfile in `./services/{name}/`. To use a pre-built image instead, set `CS_N_IMAGE` directly (advanced usage, see [[Guide-Custom-Services]]). +- Custom service images are built from the scaffolded Dockerfile in `./services/{name}/`. To use a pre-built image instead, set `CS_N_IMAGE` directly — a full image reference, optionally digest-pinned with `@sha256:...`. - The `CS_N_TABLE_PREFIX` variable is used by `nself migrate` to scope migrations to a subdirectory, keeping custom service migrations separate from core schema changes. - If a service's health endpoint isn't `/health` on its own port (e.g. an auth service serving `/auth/health`), set `CS_N_HEALTHCHECK=/auth/health` — otherwise Docker probes the wrong path and reports the service unhealthy forever regardless of its actual state. - Custom services participate in `nself backup`, the backup bundle includes a dump of any tables matching the `CS_N_TABLE_PREFIX`. - Logs from all custom service slots are included in `nself logs --all`. +- `CS_N_IMAGE`, `CS_N_ENV_FILE`, and `CS_N_VOLUMES` cover the cases that previously forced a hand-authored `docker-compose.override.yml`: a pinned third-party image digest, many/complex injected env vars (e.g. SMTP credentials), and an extra bind mount (e.g. an email-template directory) — see the reference table above. --- diff --git a/internal/build/orchestrator_build_compose.go b/internal/build/orchestrator_build_compose.go index 239e9b94c..6f695e736 100644 --- a/internal/build/orchestrator_build_compose.go +++ b/internal/build/orchestrator_build_compose.go @@ -33,7 +33,7 @@ func (st *buildState) generateCompose() error { if profile == "" { profile = compose.ProfileApp } - composeGen := compose.NewGeneratorWithProfile(st.cfg, profile) + composeGen := compose.NewGeneratorWithProfile(st.cfg, profile).WithWorkDir(st.workdir) composeYAML, err := composeGen.Generate() if err != nil { return fmt.Errorf("generating docker-compose.yml: %w", err) diff --git a/internal/compose/custom_service_extras.go b/internal/compose/custom_service_extras.go new file mode 100644 index 000000000..e4ab664bd --- /dev/null +++ b/internal/compose/custom_service_extras.go @@ -0,0 +1,57 @@ +package compose + +import ( + "fmt" + "path/filepath" + "strings" + + "github.com/joho/godotenv" +) + +// Purpose: filesystem/parsing helpers for the CS_N_ENV_FILE and CS_N_VOLUMES +// custom-service extensions (G-013). Split out of custom_services.go so that +// file keeps its focus on the fixed env-var set and the ServiceConfig +// builders. +// Inputs: a project-relative path (CS_N_ENV_FILE) or a raw CS_N_VOLUMES +// string, plus the Generator's workDir for resolving the former on disk. +// Outputs: a parsed env map or volume-mount slice ready to attach to a +// ServiceConfig. +// Constraints: CS_N_ENV_FILE's path traversal/absolute-path safety was +// already checked by config.parseCustomServices — this layer only resolves +// and reads it. CS_N_VOLUMES entries were similarly pre-validated; this +// layer only splits them into the []string form ServiceConfig.Volumes wants. + +// loadCustomServiceEnvFile reads a dotenv-format file named by CS_N_ENV_FILE +// and returns its KEY=VALUE pairs. workDir anchors the (already-validated, +// project-relative) path; an empty workDir falls back to resolving relative +// to the process's current directory, matching how CS_N_PATH build contexts +// are implicitly resolved when no explicit project root is threaded through. +func loadCustomServiceEnvFile(workDir, relPath string) (map[string]string, error) { + path := relPath + if workDir != "" { + path = filepath.Join(workDir, relPath) + } + vars, err := godotenv.Read(path) + if err != nil { + return nil, fmt.Errorf("reading %s: %w", path, err) + } + return vars, nil +} + +// parseCustomServiceVolumes splits a CS_N_VOLUMES value ("host:container[:mode]" +// entries, comma-separated) into the []string form docker-compose's `volumes:` +// list expects. Returns nil for an empty input so ServiceConfig.Volumes stays +// unset (omitempty) rather than an empty-but-present list. +func parseCustomServiceVolumes(raw string) []string { + if raw == "" { + return nil + } + var out []string + for _, entry := range strings.Split(raw, ",") { + entry = strings.TrimSpace(entry) + if entry != "" { + out = append(out, entry) + } + } + return out +} diff --git a/internal/compose/custom_service_extras_test.go b/internal/compose/custom_service_extras_test.go new file mode 100644 index 000000000..957be73cc --- /dev/null +++ b/internal/compose/custom_service_extras_test.go @@ -0,0 +1,186 @@ +package compose + +import ( + "os" + "path/filepath" + "testing" + + "github.com/nself-org/cli/internal/config" +) + +// Purpose: buildCustomService/coreEnvVars coverage for the G-013 additions — +// CS_N_IMAGE (pre-built image instead of Dockerfile build), CS_N_ENV_FILE +// (dotenv-sourced env injection), and CS_N_VOLUMES (extra bind mounts). +// Inputs: config.CustomService fixtures built via testCS() (custom_service_test.go). +// Outputs: none (t.Fatal/t.Error on mismatch). +// Constraints: co-located with custom_service_test.go's existing fixtures; +// reuses minimalConfigWithCS/testCS rather than redefining them. + +// ── CS_N_IMAGE ─────────────────────────────────────────────────────────────── + +// TestBuildCustomService_ImageSkipsBuild verifies that setting CS_N_IMAGE +// emits `image:` and omits `build:` entirely. +func TestBuildCustomService_ImageSkipsBuild(t *testing.T) { + cfg := minimalConfigWithCS() + g := NewGenerator(cfg) + cs := testCS() + cs.Image = "minio/minio:RELEASE.2024-01-16T16-07-38Z@sha256:abc123" + + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } + if svc.Image != cs.Image { + t.Errorf("Image = %q, want %q", svc.Image, cs.Image) + } + if svc.Build != nil { + t.Errorf("Build = %+v, want nil when CS_N_IMAGE is set", svc.Build) + } +} + +// TestBuildCustomService_NoImageStillBuilds is a regression check that the +// default (no CS_N_IMAGE) path is unchanged: it still emits a build: block. +func TestBuildCustomService_NoImageStillBuilds(t *testing.T) { + cfg := minimalConfigWithCS() + g := NewGenerator(cfg) + cs := testCS() + + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } + if svc.Image != "" { + t.Errorf("Image = %q, want empty when CS_N_IMAGE is unset", svc.Image) + } + if svc.Build == nil { + t.Fatal("Build is nil, want a build context when CS_N_IMAGE is unset") + } +} + +// ── CS_N_VOLUMES ───────────────────────────────────────────────────────────── + +// TestBuildCustomService_VolumesAppended verifies CS_N_VOLUMES entries are +// split and passed through to ServiceConfig.Volumes. +func TestBuildCustomService_VolumesAppended(t *testing.T) { + cfg := minimalConfigWithCS() + g := NewGenerator(cfg) + cs := testCS() + cs.Volumes = "./email-templates:/app/templates:ro, my_data:/data" + + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } + want := []string{"./email-templates:/app/templates:ro", "my_data:/data"} + if len(svc.Volumes) != len(want) { + t.Fatalf("Volumes = %v, want %v", svc.Volumes, want) + } + for i, v := range want { + if svc.Volumes[i] != v { + t.Errorf("Volumes[%d] = %q, want %q", i, svc.Volumes[i], v) + } + } +} + +// TestBuildCustomService_NoVolumesIsNil verifies that an unset CS_N_VOLUMES +// leaves ServiceConfig.Volumes nil (so it's omitted from the generated YAML, +// not emitted as an empty list). +func TestBuildCustomService_NoVolumesIsNil(t *testing.T) { + cfg := minimalConfigWithCS() + g := NewGenerator(cfg) + cs := testCS() + + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } + if svc.Volumes != nil { + t.Errorf("Volumes = %v, want nil", svc.Volumes) + } +} + +// ── CS_N_ENV_FILE ──────────────────────────────────────────────────────────── + +// TestBuildCustomService_EnvFileInjected verifies CS_N_ENV_FILE vars are +// read from disk (resolved against the Generator's workDir) and merged into +// the container environment. +func TestBuildCustomService_EnvFileInjected(t *testing.T) { + dir := t.TempDir() + envFile := "smtp.env" + content := "SMTP_HOST=smtp.example.com\nSMTP_PASS=has,a,comma\n" + if err := os.WriteFile(filepath.Join(dir, envFile), []byte(content), 0600); err != nil { + t.Fatalf("writing fixture env file: %v", err) + } + + cfg := minimalConfigWithCS() + g := NewGenerator(cfg).WithWorkDir(dir) + cs := testCS() + cs.EnvFile = envFile + + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } + if got := svc.Environment["SMTP_HOST"]; got != "smtp.example.com" { + t.Errorf("SMTP_HOST = %q, want %q", got, "smtp.example.com") + } + // The value containing commas is exactly the case CS_N_ENV (a single + // comma-joined line) cannot represent safely — proves the env-file path + // handles it correctly. + if got := svc.Environment["SMTP_PASS"]; got != "has,a,comma" { + t.Errorf("SMTP_PASS = %q, want %q", got, "has,a,comma") + } +} + +// TestBuildCustomService_EnvFilePrecedence verifies CS_N_ENV still wins over +// a conflicting CS_N_ENV_FILE value (fixed precedence order documented on +// coreEnvVars). +func TestBuildCustomService_EnvFilePrecedence(t *testing.T) { + dir := t.TempDir() + envFile := "extra.env" + if err := os.WriteFile(filepath.Join(dir, envFile), []byte("SHARED_KEY=from_file\n"), 0600); err != nil { + t.Fatalf("writing fixture env file: %v", err) + } + + cfg := minimalConfigWithCS() + g := NewGenerator(cfg).WithWorkDir(dir) + cs := testCS() + cs.EnvFile = envFile + cs.ExtraEnv = "SHARED_KEY=from_cs_n_env" + + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } + if got := svc.Environment["SHARED_KEY"]; got != "from_cs_n_env" { + t.Errorf("SHARED_KEY = %q, want %q (CS_N_ENV must win over CS_N_ENV_FILE)", got, "from_cs_n_env") + } +} + +// TestBuildCustomService_EnvFileMissingErrors verifies a CS_N_ENV_FILE +// naming a nonexistent file fails the build loudly rather than silently +// dropping the vars the service needs. +func TestBuildCustomService_EnvFileMissingErrors(t *testing.T) { + cfg := minimalConfigWithCS() + g := NewGenerator(cfg).WithWorkDir(t.TempDir()) + cs := testCS() + cs.EnvFile = "does-not-exist.env" + + if _, err := g.buildCustomService(cs); err == nil { + t.Fatal("expected an error for a missing CS_N_ENV_FILE, got nil") + } +} + +// TestGenerate_CustomServiceEnvFileError verifies a bad CS_N_ENV_FILE fails +// the whole Generate() call with a clear error rather than a partial compose. +func TestGenerate_CustomServiceEnvFileError(t *testing.T) { + cfg := minimalConfigWithCS() + cs := testCS() + cs.EnvFile = "does-not-exist.env" + cfg.CustomServices = []config.CustomService{cs} + + g := NewGenerator(cfg).WithWorkDir(t.TempDir()) + if _, err := g.Generate(); err == nil { + t.Fatal("expected Generate() to fail on a missing CS_N_ENV_FILE") + } +} diff --git a/internal/compose/custom_service_test.go b/internal/compose/custom_service_test.go index 6b507b0bf..bd5199276 100644 --- a/internal/compose/custom_service_test.go +++ b/internal/compose/custom_service_test.go @@ -36,7 +36,7 @@ func TestCoreEnvVars_ProjectFields(t *testing.T) { cfg := minimalConfigWithCS() cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if env["PROJECT_NAME"] != cfg.ProjectName { t.Errorf("PROJECT_NAME = %q, want %q", env["PROJECT_NAME"], cfg.ProjectName) @@ -55,7 +55,7 @@ func TestCoreEnvVars_PostgresVars(t *testing.T) { cfg := minimalConfigWithCS() cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if env["POSTGRES_HOST"] != "postgres" { t.Errorf("POSTGRES_HOST = %q, want %q", env["POSTGRES_HOST"], "postgres") @@ -79,7 +79,7 @@ func TestCoreEnvVars_DatabaseURL(t *testing.T) { cfg := minimalConfigWithCS() cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) dbURL := env["DATABASE_URL"] if !strings.HasPrefix(dbURL, "postgresql://") { @@ -97,7 +97,7 @@ func TestCoreEnvVars_HasuraEndpoint(t *testing.T) { cfg.Hasura.Port = 8080 cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) want := "http://hasura:8080/v1/graphql" if env["HASURA_GRAPHQL_ENDPOINT"] != want { @@ -115,7 +115,7 @@ func TestCoreEnvVars_HasuraEndpoint_IgnoresHostPortOverride(t *testing.T) { cfg.Hasura.Port = 8181 // host-mapped port override cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) want := "http://hasura:8080/v1/graphql" if env["HASURA_GRAPHQL_ENDPOINT"] != want { @@ -130,7 +130,7 @@ func TestCoreEnvVars_AuthServerURL(t *testing.T) { cfg.Auth.Port = 4000 cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) want := "http://auth:4000" if env["AUTH_SERVER_URL"] != want { @@ -144,7 +144,7 @@ func TestCoreEnvVars_ServiceFields(t *testing.T) { cfg := minimalConfigWithCS() cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if env["SERVICE_NAME"] != cs.Name { t.Errorf("SERVICE_NAME = %q, want %q", env["SERVICE_NAME"], cs.Name) @@ -164,7 +164,7 @@ func TestCoreEnvVars_RedisAbsent(t *testing.T) { cfg.Redis.Enabled = false cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if _, ok := env["REDIS_URL"]; ok { t.Error("REDIS_URL should not be present when Redis is disabled") @@ -180,7 +180,7 @@ func TestCoreEnvVars_RedisPresent(t *testing.T) { cfg.Redis.Port = 6379 cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) redisURL, ok := env["REDIS_URL"] if !ok { @@ -198,7 +198,7 @@ func TestCoreEnvVars_MinioAbsent(t *testing.T) { cfg.Minio.Enabled = false cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) for _, key := range []string{"S3_ENDPOINT", "S3_ACCESS_KEY", "S3_SECRET_KEY", "S3_BUCKET"} { if _, ok := env[key]; ok { @@ -218,7 +218,7 @@ func TestCoreEnvVars_MinioPresent(t *testing.T) { cfg.Minio.DefaultBuckets = "uploads" cs := testCS() - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if !strings.Contains(env["S3_ENDPOINT"], "minio:9000") { t.Errorf("S3_ENDPOINT should reference minio:9000, got %q", env["S3_ENDPOINT"]) @@ -241,7 +241,7 @@ func TestCoreEnvVars_ExtraEnvOverrides(t *testing.T) { cs := testCS() cs.ExtraEnv = "CUSTOM_KEY=custom_value,ENV=override" - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if env["CUSTOM_KEY"] != "custom_value" { t.Errorf("CUSTOM_KEY = %q, want %q", env["CUSTOM_KEY"], "custom_value") @@ -259,7 +259,7 @@ func TestCoreEnvVars_ExtraEnvMalformed(t *testing.T) { cs := testCS() cs.ExtraEnv = "NOEQUALS,VALID_KEY=val" - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) // NOEQUALS has no '=' so it should be ignored. if _, ok := env["NOEQUALS"]; ok { @@ -280,7 +280,10 @@ func TestBuildCustomService_ContainerName(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } want := cfg.ProjectName + "_" + cs.Name if svc.ContainerName != want { @@ -295,7 +298,10 @@ func TestBuildCustomService_BuildContext(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } if svc.Build == nil { t.Fatal("buildCustomService: Build config is nil") @@ -317,7 +323,10 @@ func TestBuildCustomService_PortMapping(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } want := fmt.Sprintf("127.0.0.1:%d:%d", cs.Port, cs.Port) found := false @@ -341,7 +350,10 @@ func TestBuildCustomService_HealthcheckPort(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } if svc.Healthcheck == nil { t.Fatal("buildCustomService: Healthcheck is nil") @@ -361,7 +373,10 @@ func TestBuildCustomService_DependsOnPostgres(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } dep, ok := svc.DependsOn["postgres"] if !ok { @@ -379,7 +394,10 @@ func TestBuildCustomService_Restart(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } if svc.Restart != "unless-stopped" { t.Errorf("Restart = %q, want %q", svc.Restart, "unless-stopped") @@ -395,7 +413,10 @@ func TestBuildCustomService_ResourceLimits(t *testing.T) { cs.CPU = "0.5" g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } if svc.Deploy == nil || svc.Deploy.Resources == nil || svc.Deploy.Resources.Limits == nil { t.Fatal("buildCustomService: Deploy resource limits are nil") @@ -416,7 +437,10 @@ func TestBuildCustomService_Network(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } found := false for _, n := range svc.Networks { @@ -442,7 +466,7 @@ func TestCoreEnvVars_EnvPassthrough_Forwarded(t *testing.T) { cs := testCS() cs.EnvPassthrough = "MY_API_KEY" - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if env["MY_API_KEY"] != "secret-value" { t.Errorf("MY_API_KEY = %q, want %q", env["MY_API_KEY"], "secret-value") @@ -459,7 +483,7 @@ func TestCoreEnvVars_EnvPassthrough_MultipleNames(t *testing.T) { cs := testCS() cs.EnvPassthrough = "FOO_VAR, BAR_VAR" - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if env["FOO_VAR"] != "foo" { t.Errorf("FOO_VAR = %q, want %q", env["FOO_VAR"], "foo") @@ -477,7 +501,7 @@ func TestCoreEnvVars_EnvPassthrough_AbsentSkipped(t *testing.T) { cs := testCS() cs.EnvPassthrough = "DOES_NOT_EXIST_VAR" - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if _, ok := env["DOES_NOT_EXIST_VAR"]; ok { t.Error("DOES_NOT_EXIST_VAR should not be present when unset in the process env") @@ -495,7 +519,7 @@ func TestCoreEnvVars_EnvPassthrough_ExtraEnvWins(t *testing.T) { cs.EnvPassthrough = "SHARED_VAR" cs.ExtraEnv = "SHARED_VAR=from-extra-env" - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) if env["SHARED_VAR"] != "from-extra-env" { t.Errorf("SHARED_VAR = %q, want %q (CS_N_ENV must win over CS_N_ENV_PASSTHROUGH)", env["SHARED_VAR"], "from-extra-env") @@ -511,7 +535,7 @@ func TestCoreEnvVars_EnvPassthrough_Absent(t *testing.T) { cs := testCS() cs.EnvPassthrough = "" - env := coreEnvVars(cfg, cs) + env := coreEnvVars(cfg, cs, nil) // The fixed core set only (PROJECT_NAME..TABLE_PREFIX), per coreEnvVars' // documented design: no Redis/Minio (disabled) and no passthrough/extra-env. @@ -623,7 +647,10 @@ func TestBuildCustomService_HealthcheckDisabled(t *testing.T) { cs.HealthCheck = "disabled" g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } if svc.Healthcheck != nil { t.Errorf("Healthcheck = %+v, want nil when CS_N_HEALTHCHECK=disabled", svc.Healthcheck) @@ -641,7 +668,10 @@ func TestBuildCustomService_HealthcheckCustomPath(t *testing.T) { cs.HealthCheck = "/auth/health" g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } if svc.Healthcheck == nil { t.Fatal("buildCustomService: Healthcheck is nil") @@ -660,7 +690,10 @@ func TestBuildCustomService_CoreEnvInjected(t *testing.T) { cs := testCS() g := NewGenerator(cfg) - svc := g.buildCustomService(cs) + svc, err := g.buildCustomService(cs) + if err != nil { + t.Fatalf("buildCustomService returned error: %v", err) + } for _, key := range []string{ "PROJECT_NAME", diff --git a/internal/compose/custom_services.go b/internal/compose/custom_services.go index 5711d341e..f12eecbe8 100644 --- a/internal/compose/custom_services.go +++ b/internal/compose/custom_services.go @@ -20,10 +20,30 @@ import ( // decision. // // Precedence (lowest to highest): fixed defaults → CS_N_ENV_PASSTHROUGH -// (named allowlist forwarded from the project's resolved env) → CS_N_ENV -// (explicit overrides, always win). -func coreEnvVars(cfg *config.Config, svc config.CustomService) map[string]string { - env := map[string]string{ +// (named allowlist forwarded from the project's resolved env) → CS_N_ENV_FILE +// (envFileVars, pre-loaded by the caller from the dotenv file CS_N_ENV_FILE +// names) → CS_N_ENV (explicit overrides, always win). +// +// envFileVars is nil when the service has no CS_N_ENV_FILE — callers pass +// the map already resolved (rather than a file path) so this function stays +// pure and easy to unit test without touching the filesystem. +func coreEnvVars(cfg *config.Config, svc config.CustomService, envFileVars map[string]string) map[string]string { + env := fixedCoreEnvVars(cfg, svc) + addOptionalStoreEnvVars(env, cfg) + applyEnvPassthrough(env, svc) + // CS_N_ENV_FILE — merged after passthrough, before CS_N_ENV, so an + // explicit CS_N_ENV entry still wins on conflict. + for k, v := range envFileVars { + env[k] = v + } + applyExtraEnv(env, svc) + return env +} + +// fixedCoreEnvVars returns the always-present base set: project identity, +// Postgres, Hasura, Auth, and this service's own identity fields. +func fixedCoreEnvVars(cfg *config.Config, svc config.CustomService) map[string]string { + return map[string]string{ "PROJECT_NAME": cfg.ProjectName, "BASE_DOMAIN": cfg.BaseDomain, "ENV": cfg.Env, @@ -47,6 +67,11 @@ func coreEnvVars(cfg *config.Config, svc config.CustomService) map[string]string "SERVICE_ROUTE": svc.Route, "TABLE_PREFIX": svc.TablePrefix, } +} + +// addOptionalStoreEnvVars adds REDIS_URL / S3_* connection vars when the +// corresponding optional service is enabled on the project. +func addOptionalStoreEnvVars(env map[string]string, cfg *config.Config) { if cfg.Redis.Enabled { env["REDIS_URL"] = fmt.Sprintf("redis://:%s@redis:%d", cfg.Redis.Password, cfg.Redis.Port) } @@ -56,33 +81,40 @@ func coreEnvVars(cfg *config.Config, svc config.CustomService) map[string]string env["S3_SECRET_KEY"] = cfg.Minio.RootPassword env["S3_BUCKET"] = cfg.Minio.DefaultBuckets } - // CS_N_ENV_PASSTHROUGH — explicit allowlist of extra project env vars to - // forward into this container beyond the fixed core set above. Applied - // before CS_N_ENV so an explicit override still wins on conflict. Names - // not present in the resolved env are silently skipped (not an error) so - // an allowlist can be shared across environments where a var may be - // optional. - if svc.EnvPassthrough != "" { - for _, name := range strings.Split(svc.EnvPassthrough, ",") { - name = strings.TrimSpace(name) - if name == "" { - continue - } - if val, ok := os.LookupEnv(name); ok { - env[name] = val - } +} + +// applyEnvPassthrough forwards the CS_N_ENV_PASSTHROUGH allowlist of project +// env var names into env. Applied before CS_N_ENV_FILE/CS_N_ENV so either +// still wins on a name conflict. Names not present in the resolved env are +// silently skipped (not an error) so an allowlist can be shared across +// environments where a var may be optional. +func applyEnvPassthrough(env map[string]string, svc config.CustomService) { + if svc.EnvPassthrough == "" { + return + } + for _, name := range strings.Split(svc.EnvPassthrough, ",") { + name = strings.TrimSpace(name) + if name == "" { + continue + } + if val, ok := os.LookupEnv(name); ok { + env[name] = val } } - // CS_N_ENV overrides applied last — user wins - if svc.ExtraEnv != "" { - for _, pair := range strings.Split(svc.ExtraEnv, ",") { - parts := strings.SplitN(pair, "=", 2) - if len(parts) == 2 { - env[strings.TrimSpace(parts[0])] = strings.TrimSpace(parts[1]) - } +} + +// applyExtraEnv merges CS_N_ENV "KEY=VALUE,KEY=VALUE" pairs into env. Always +// applied last — an explicit CS_N_ENV entry wins over every other source. +func applyExtraEnv(env map[string]string, svc config.CustomService) { + if svc.ExtraEnv == "" { + return + } + for _, pair := range strings.Split(svc.ExtraEnv, ",") { + parts := strings.SplitN(pair, "=", 2) + if len(parts) == 2 { + env[strings.TrimSpace(parts[0])] = strings.TrimSpace(parts[1]) } } - return env } // buildHealthcheck renders the Docker healthcheck for a custom service, @@ -134,30 +166,39 @@ func buildHealthcheck(cs config.CustomService) *Healthcheck { // buildCustomService returns the service configuration for a user-defined // custom service (CS_1..CS_10). Each custom service is built from a Dockerfile -// in ./services/{name}/ by default, or from CS_N_PATH when set. -func (g *Generator) buildCustomService(cs config.CustomService) ServiceConfig { +// in ./services/{name}/ by default, or from CS_N_PATH when set — unless +// CS_N_IMAGE names a pre-built image, in which case the service pulls that +// image and no build: block is emitted at all (G-013). +// +// Inputs: cs — the parsed CustomService (CS_N_* env vars already validated +// by config.parseCustomServices). g.workDir anchors CS_N_ENV_FILE reads. +// Outputs: the ServiceConfig to emit, or an error if CS_N_ENV_FILE names a +// file that cannot be read/parsed — a build-time failure is preferred over +// silently omitting the vars a service needs (e.g. SMTP credentials). +func (g *Generator) buildCustomService(cs config.CustomService) (ServiceConfig, error) { cfg := g.cfg - buildContext := cs.BuildPath - if buildContext == "" { - buildContext = fmt.Sprintf("./services/%s", cs.Name) + var envFileVars map[string]string + if cs.EnvFile != "" { + vars, err := loadCustomServiceEnvFile(g.workDir, cs.EnvFile) + if err != nil { + return ServiceConfig{}, fmt.Errorf("CS_%d_ENV_FILE: %w", cs.Index, err) + } + envFileVars = vars } - return ServiceConfig{ - Build: &BuildConfig{ - Context: buildContext, - Dockerfile: "Dockerfile", - }, + svc := ServiceConfig{ ContainerName: fmt.Sprintf("%s_%s", cfg.ProjectName, cs.Name), Restart: "unless-stopped", Networks: []string{cfg.DockerNetwork}, DependsOn: map[string]DepOn{ "postgres": {Condition: "service_healthy"}, }, - Environment: coreEnvVars(cfg, cs), + Environment: coreEnvVars(cfg, cs, envFileVars), Ports: []string{ fmt.Sprintf("127.0.0.1:%d:%d", cs.Port, cs.Port), }, + Volumes: parseCustomServiceVolumes(cs.Volumes), Healthcheck: buildHealthcheck(cs), Deploy: &DeployConfig{ Resources: &Resources{ @@ -168,4 +209,19 @@ func (g *Generator) buildCustomService(cs config.CustomService) ServiceConfig { }, }, } + + if cs.Image != "" { + svc.Image = cs.Image + } else { + buildContext := cs.BuildPath + if buildContext == "" { + buildContext = fmt.Sprintf("./services/%s", cs.Name) + } + svc.Build = &BuildConfig{ + Context: buildContext, + Dockerfile: "Dockerfile", + } + } + + return svc, nil } diff --git a/internal/compose/generator.go b/internal/compose/generator.go index 078662f39..2d183760f 100644 --- a/internal/compose/generator.go +++ b/internal/compose/generator.go @@ -20,6 +20,13 @@ const NginxSitesDir = "nginx/sites" type Generator struct { cfg *config.Config profile ServiceSet + + // workDir anchors CS_N_ENV_FILE reads to the project root. Empty by + // default (falls back to the process's current directory — see + // loadCustomServiceEnvFile) so existing callers that never set it are + // unaffected. Set via WithWorkDir when the caller has an explicit + // project directory (e.g. the build orchestrator's st.workdir). + workDir string } // NewGenerator creates a compose Generator from the given config using the @@ -42,6 +49,15 @@ func NewGeneratorWithProfile(cfg *config.Config, name ProfileName) *Generator { return &Generator{cfg: cfg, profile: set} } +// WithWorkDir sets the project root used to resolve CS_N_ENV_FILE paths and +// returns the same Generator for chaining. Callers that build compose from a +// known project directory (rather than assuming os.Getwd()) should always +// set this — see internal/build/orchestrator_build_compose.go. +func (g *Generator) WithWorkDir(dir string) *Generator { + g.workDir = dir + return g +} + // Generate produces the complete docker-compose.yml as YAML bytes. // It marshals a DockerCompose struct via gopkg.in/yaml.v3. func (g *Generator) Generate() ([]byte, error) { @@ -143,7 +159,11 @@ func (g *Generator) buildDockerCompose() (*DockerCompose, error) { // Custom services (always pass-through — per-project overrides). for _, cs := range g.cfg.CustomServices { - dc.AddService(cs.Name, g.buildCustomService(cs)) + svcCfg, err := g.buildCustomService(cs) + if err != nil { + return nil, fmt.Errorf("building custom service %q: %w", cs.Name, err) + } + dc.AddService(cs.Name, svcCfg) } // Nginx — profile-gated (always last — depends on other services). diff --git a/internal/config/custom_services.go b/internal/config/custom_services.go index afaba1fdf..091aea2f3 100644 --- a/internal/config/custom_services.go +++ b/internal/config/custom_services.go @@ -14,8 +14,8 @@ import ( // // If port is omitted or zero, it auto-assigns 8000+N. // Per-service overrides are read from CS_N_PUBLIC, CS_N_MEMORY, CS_N_CPU, -// CS_N_PORT, CS_N_ROUTE, CS_N_HEALTHCHECK, and CS_N_ENV_PASSTHROUGH -// environment variables. +// CS_N_PORT, CS_N_ROUTE, CS_N_HEALTHCHECK, CS_N_ENV_PASSTHROUGH, +// CS_N_IMAGE, CS_N_ENV_FILE, and CS_N_VOLUMES environment variables. func parseCustomServices() ([]CustomService, error) { var services []CustomService for i := 1; i <= 10; i++ { @@ -74,17 +74,39 @@ func parseCustomServices() ([]CustomService, error) { // Optional build context path override. Rejects absolute paths and // path traversal so a misconfigured env can't escape the project root. if p := os.Getenv(fmt.Sprintf("CS_%d_PATH", i)); p != "" { - if strings.HasPrefix(p, "/") { - return nil, fmt.Errorf("CS_%d_PATH must be a relative path, got %q", i, p) - } - for _, seg := range strings.Split(p, "/") { - if seg == ".." { - return nil, fmt.Errorf("CS_%d_PATH must not contain '..', got %q", i, p) - } + if err := validateRelativePath(p); err != nil { + return nil, fmt.Errorf("CS_%d_PATH %w", i, err) } cs.BuildPath = p } + // CS_N_IMAGE: run a pre-built (optionally digest-pinned) image instead + // of building from a Dockerfile. Mutually exclusive with CS_N_PATH, + // which only makes sense for the build path (G-013). + cs.Image = os.Getenv(fmt.Sprintf("CS_%d_IMAGE", i)) + if cs.Image != "" && cs.BuildPath != "" { + return nil, fmt.Errorf("CS_%d_IMAGE and CS_%d_PATH are mutually exclusive: a service either builds from a Dockerfile (CS_%d_PATH) or runs a pre-built image (CS_%d_IMAGE), not both", i, i, i, i) + } + + // CS_N_ENV_FILE: dotenv-format file of extra env vars, injected at + // build time (see coreEnvVars). Same relative-path rules as CS_N_PATH. + if p := os.Getenv(fmt.Sprintf("CS_%d_ENV_FILE", i)); p != "" { + if err := validateRelativePath(p); err != nil { + return nil, fmt.Errorf("CS_%d_ENV_FILE %w", i, err) + } + cs.EnvFile = p + } + + // CS_N_VOLUMES: comma-separated "host:container[:mode]" bind mounts, + // appended to the generated service. Each relative host path is + // subject to the same traversal check as CS_N_PATH. + if v := os.Getenv(fmt.Sprintf("CS_%d_VOLUMES", i)); v != "" { + if err := validateCustomServiceVolumes(v); err != nil { + return nil, fmt.Errorf("CS_%d_VOLUMES %w", i, err) + } + cs.Volumes = v + } + // Override port/route if explicitly set if p := getEnvInt(fmt.Sprintf("CS_%d_PORT", i), 0); p != 0 { cs.Port = p diff --git a/internal/config/custom_services_image_env_volumes_test.go b/internal/config/custom_services_image_env_volumes_test.go new file mode 100644 index 000000000..8b5860aad --- /dev/null +++ b/internal/config/custom_services_image_env_volumes_test.go @@ -0,0 +1,159 @@ +package config + +import "testing" + +// Purpose: parse-time coverage for the three CS_N_* additions that close +// G-013 (nself build cannot express a pinned image digest, injected SMTP +// env vars, or a volume mount) — CS_N_IMAGE, CS_N_ENV_FILE, CS_N_VOLUMES. +// Mirrors the existing CS_N_PATH tests in parse_services_test.go. +// Inputs: environment variables set via t.Setenv per test. +// Outputs: none (t.Fatal/t.Error on unexpected parseCustomServices results). +// Constraints: every test clears CS_2..CS_10 so slots don't leak state. + +func clearOtherCSSlots(t *testing.T, keep int) { + t.Helper() + for i := 1; i <= 10; i++ { + if i == keep { + continue + } + t.Setenv("CS_"+itoa(i), "") + } +} + +// TestCustomServicesImage_Valid verifies CS_N_IMAGE (with a digest suffix) +// is parsed through untouched — this is the exact shape needed to express a +// pinned minio image (G-013 evidence row 1). +func TestCustomServicesImage_Valid(t *testing.T) { + t.Setenv("CS_1", "email-storage:go") + t.Setenv("CS_1_IMAGE", "minio/minio:RELEASE.2024-01-16T16-07-38Z@sha256:abc123") + clearOtherCSSlots(t, 1) + + services, err := parseCustomServices() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(services) == 0 { + t.Fatal("expected at least one custom service") + } + want := "minio/minio:RELEASE.2024-01-16T16-07-38Z@sha256:abc123" + if got := services[0].Image; got != want { + t.Errorf("Image = %q, want %q", got, want) + } +} + +// TestCustomServicesImage_ConflictsWithPath verifies CS_N_IMAGE and CS_N_PATH +// together is rejected — a service either builds or pulls, never both. +func TestCustomServicesImage_ConflictsWithPath(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_IMAGE", "myorg/myimage:latest") + t.Setenv("CS_1_PATH", "./services/myservice") + clearOtherCSSlots(t, 1) + + if _, err := parseCustomServices(); err == nil { + t.Fatal("expected error when CS_1_IMAGE and CS_1_PATH are both set") + } +} + +// TestCustomServicesEnvFile_Valid verifies CS_N_ENV_FILE accepts a clean +// relative path. +func TestCustomServicesEnvFile_Valid(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_ENV_FILE", "./secrets/smtp.env") + clearOtherCSSlots(t, 1) + + services, err := parseCustomServices() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(services) == 0 { + t.Fatal("expected at least one custom service") + } + if got := services[0].EnvFile; got != "./secrets/smtp.env" { + t.Errorf("EnvFile = %q, want %q", got, "./secrets/smtp.env") + } +} + +// TestCustomServicesEnvFile_AbsoluteRejected verifies an absolute +// CS_N_ENV_FILE path is rejected, same as CS_N_PATH. +func TestCustomServicesEnvFile_AbsoluteRejected(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_ENV_FILE", "/etc/secrets/smtp.env") + clearOtherCSSlots(t, 1) + + if _, err := parseCustomServices(); err == nil { + t.Fatal("expected error for absolute CS_1_ENV_FILE, got nil") + } +} + +// TestCustomServicesEnvFile_TraversalRejected verifies a CS_N_ENV_FILE +// containing ".." is rejected. +func TestCustomServicesEnvFile_TraversalRejected(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_ENV_FILE", "../../outside/smtp.env") + clearOtherCSSlots(t, 1) + + if _, err := parseCustomServices(); err == nil { + t.Fatal("expected error for traversal CS_1_ENV_FILE, got nil") + } +} + +// TestCustomServicesVolumes_Valid verifies CS_N_VOLUMES parses a +// comma-separated list, covering the ntask email-templates mount +// (G-013 evidence row 3). +func TestCustomServicesVolumes_Valid(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_VOLUMES", "./email-templates:/app/templates:ro,my_data:/data") + clearOtherCSSlots(t, 1) + + services, err := parseCustomServices() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(services) == 0 { + t.Fatal("expected at least one custom service") + } + want := "./email-templates:/app/templates:ro,my_data:/data" + if got := services[0].Volumes; got != want { + t.Errorf("Volumes = %q, want %q", got, want) + } +} + +// TestCustomServicesVolumes_TraversalRejected verifies a relative host path +// containing ".." inside CS_N_VOLUMES is rejected. +func TestCustomServicesVolumes_TraversalRejected(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_VOLUMES", "../../outside/templates:/app/templates") + clearOtherCSSlots(t, 1) + + if _, err := parseCustomServices(); err == nil { + t.Fatal("expected error for traversal host path in CS_1_VOLUMES, got nil") + } +} + +// TestCustomServicesVolumes_MissingContainerPathRejected verifies an entry +// without a container path (no ":") is rejected. +func TestCustomServicesVolumes_MissingContainerPathRejected(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_VOLUMES", "./just-a-host-path") + clearOtherCSSlots(t, 1) + + if _, err := parseCustomServices(); err == nil { + t.Fatal("expected error for CS_1_VOLUMES entry missing a container path") + } +} + +// TestCustomServicesVolumes_AbsoluteHostAllowed verifies an absolute host +// bind mount is accepted (permissive by design, per validateCustomServiceVolumes). +func TestCustomServicesVolumes_AbsoluteHostAllowed(t *testing.T) { + t.Setenv("CS_1", "myservice:go") + t.Setenv("CS_1_VOLUMES", "/srv/shared-templates:/app/templates:ro") + clearOtherCSSlots(t, 1) + + services, err := parseCustomServices() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(services) == 0 || services[0].Volumes == "" { + t.Fatal("expected CS_1_VOLUMES to be accepted") + } +} diff --git a/internal/config/custom_services_validate.go b/internal/config/custom_services_validate.go new file mode 100644 index 000000000..ec30a4f47 --- /dev/null +++ b/internal/config/custom_services_validate.go @@ -0,0 +1,67 @@ +package config + +import ( + "fmt" + "strings" +) + +// Purpose: path/volume validation helpers for CS_N custom-service env vars, +// split out of custom_services.go to keep that file's parse loop readable. +// Shared by CS_N_PATH, CS_N_ENV_FILE (both single relative paths) and +// CS_N_VOLUMES (comma-separated host:container[:mode] triples whose host +// half is checked the same way). Extracted once a third caller needed the +// same traversal check (G-013), per the repo's DRY-on-third-copy convention. +// Inputs: raw string values read directly from os.Getenv by the caller. +// Outputs: nil on a safe value, otherwise an error naming the problem — +// callers wrap it with the specific CS_N_* var name for context. +// Constraints: intentionally permissive on everything except escaping the +// project root — these are operator-authored env vars, not untrusted input, +// so the goal is catching mistakes, not adversarial hardening. + +// validateRelativePath rejects an absolute path or one containing a ".." +// path-traversal segment. Used for any CS_N_* value that names a location +// inside the project tree (CS_N_PATH, CS_N_ENV_FILE). +func validateRelativePath(p string) error { + if strings.HasPrefix(p, "/") { + return fmt.Errorf("must be a relative path, got %q", p) + } + for _, seg := range strings.Split(p, "/") { + if seg == ".." { + return fmt.Errorf("must not contain '..', got %q", p) + } + } + return nil +} + +// validateCustomServiceVolumes checks a CS_N_VOLUMES value: a comma-separated +// list of "host:container[:mode]" entries. Each entry must have at least a +// host and container path; a relative host path (one not starting with "/" +// and not a bare named-volume identifier containing no "/") is checked for +// traversal via validateRelativePath. Named Docker volumes (e.g. +// "my_data:/data") and absolute bind mounts (e.g. "/srv/x:/data") are left to +// the operator's judgment, matching Docker Compose's own permissive stance. +func validateCustomServiceVolumes(raw string) error { + for _, entry := range strings.Split(raw, ",") { + entry = strings.TrimSpace(entry) + if entry == "" { + return fmt.Errorf("contains an empty entry in %q", raw) + } + parts := strings.Split(entry, ":") + if len(parts) < 2 { + return fmt.Errorf("entry %q must be host:container[:mode]", entry) + } + host := parts[0] + if host == "" { + return fmt.Errorf("entry %q is missing a host path", entry) + } + // Only check paths that look like a project-relative bind mount + // ("./x", "../x", or a bare relative segment containing "/"). + // Absolute paths and bare named-volume names (no "/") are exempt. + if !strings.HasPrefix(host, "/") && strings.Contains(host, "/") { + if err := validateRelativePath(host); err != nil { + return fmt.Errorf("entry %q: %w", entry, err) + } + } + } + return nil +} diff --git a/internal/config/types_ops_plugins.go b/internal/config/types_ops_plugins.go index 6ce883080..7bc01c49e 100644 --- a/internal/config/types_ops_plugins.go +++ b/internal/config/types_ops_plugins.go @@ -154,6 +154,32 @@ type CustomService struct { // project .env var names to forward into this container in addition to // the fixed core set from coreEnvVars. CS_N_ENV still wins on conflict. EnvPassthrough string + + // Image is CS_N_IMAGE: a pre-built image reference (optionally digest-pinned, + // e.g. "minio/minio:RELEASE.2024-01-16T16-07-38Z@sha256:...") to run instead + // of building from a Dockerfile. When set, the compose generator emits + // `image:` and omits `build:` entirely — mutually exclusive with + // CS_N_PATH (G-013: closes the gap where a pinned third-party image had + // no CS_N representation and had to be hand-authored into + // docker-compose.override.yml). + Image string + + // EnvFile is CS_N_ENV_FILE: a project-relative path to a dotenv-format + // file whose KEY=VALUE lines are injected into this container. Unlike + // CS_N_ENV (a single comma-joined line), a file has no comma/newline + // escaping problem, so it is the right vehicle for many vars or values + // that themselves contain commas (e.g. SMTP credentials). Precedence: + // applied after CS_N_ENV_PASSTHROUGH, before CS_N_ENV (CS_N_ENV always + // wins on conflict, per the existing coreEnvVars contract). Same + // relative-path rules as BuildPath (no absolute paths, no ".."). + EnvFile string + + // Volumes is CS_N_VOLUMES: a comma-separated list of extra Docker volume + // mounts in "host:container[:mode]" form (e.g. + // "./email-templates:/app/templates:ro"), appended to the service's + // generated volume list. Closes the gap where a required bind mount + // (e.g. a template directory) had no CS_N representation. + Volumes string } // FrontendApp represents a frontend application (FRONTEND_APP_1..FRONTEND_APP_20). From 3be31cb9785f18ea48b7ceadb2402c29fe100b81 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Fri, 11 Sep 2026 17:54:31 -0400 Subject: [PATCH 5/5] docs(wiki): document CS_N_IMAGE, CS_N_ENV_FILE, CS_N_VOLUMES env vars --- .github/wiki/Config-Env-Vars.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/wiki/Config-Env-Vars.md b/.github/wiki/Config-Env-Vars.md index d24fc3f16..0b3aace2d 100644 --- a/.github/wiki/Config-Env-Vars.md +++ b/.github/wiki/Config-Env-Vars.md @@ -258,6 +258,9 @@ These boolean flags enable optional bundled services. Each defaults to `false`. | `CS_N_REPLICAS` | int | `1` | No | Number of container instances to run. | | `CS_N_HEALTHCHECK` | string | `/health` | No | Healthcheck override: a path, a full `CMD ...`/`CMD-SHELL ...` command, or `disabled`/`none`/`false` to omit it. See [[Config-Custom-Services]]. | | `CS_N_ENV_PASSTHROUGH` | string | *(empty)* | No | Comma-separated allowlist of project `.env` var names to forward into this service. `CS_N_ENV` wins on conflict. See [[Config-Custom-Services]]. | +| `CS_N_IMAGE` | string | *(empty)* | No | Run a pre-built image (optionally digest-pinned, e.g. `repo/name@sha256:...`) instead of building from a Dockerfile. Mutually exclusive with `CS_N_PATH`. | +| `CS_N_ENV_FILE` | string | *(empty)* | No | Project-relative path to a dotenv-format file of extra env vars, injected at build time. Applied after `CS_N_ENV_PASSTHROUGH`; `CS_N_ENV` always wins on conflict. Subject to the same path-traversal check as `CS_N_PATH`. | +| `CS_N_VOLUMES` | string | *(empty)* | No | Comma-separated `host:container[:mode]` bind mounts, subject to the same traversal check as `CS_N_PATH`. | **Example** (from `web/`, `nself.org` infrastructure):