From db6d31504da5950e29f73b7fcde98e04cd1f9dd3 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:06:00 -0400 Subject: [PATCH 1/5] fix(build): list the plugin.json name candidates in the core env no-match warning (D-0604) --- internal/build/plugins_core_env.go | 16 +++++++++++++--- internal/build/plugins_core_env_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/internal/build/plugins_core_env.go b/internal/build/plugins_core_env.go index bb53a9bd9..15b0c2706 100644 --- a/internal/build/plugins_core_env.go +++ b/internal/build/plugins_core_env.go @@ -30,6 +30,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/compose" @@ -188,9 +189,7 @@ func normalizeComposePluginCoreEnv(content []byte, pluginDir, pluginName string) // A pure third-party stack has no port (free/monitoring): nothing to // inject by design, so no warning. if port > 0 { - warnOnce(fmt.Sprintf( - "plugin %q: %s has several services and none is named %s, nself-%s or plugin-%s, so the core env vars were not injected", - pluginName, filepath.Join(pluginDir, pluginName, pluginComposeFilename), pluginName, pluginName, pluginName)) + warnOnce(noMatchServiceWarning(pluginDir, pluginName, manifestName)) } return content } @@ -217,6 +216,17 @@ func normalizeComposePluginCoreEnv(content []byte, pluginDir, pluginName string) return append(out, content[end:]...) } +// noMatchServiceWarning names every service name the lookup tried, built from +// pluginServiceCandidates so the message cannot drift from the lookup (D-0604): +// the manifest-name forms appear when plugin.json's name differs from the +// directory name. +func noMatchServiceWarning(pluginDir, pluginName, manifestName string) string { + return fmt.Sprintf( + "plugin %q: %s has several services and none is named one of %s, so the core env vars were not injected", + pluginName, filepath.Join(pluginDir, pluginName, pluginComposeFilename), + strings.Join(pluginServiceCandidates(pluginName, manifestName), ", ")) +} + // warnUnrecognisedEnvBlock reports a plugin compose fragment whose // environment configuration normalizeComposePluginCoreEnv cannot safely // rewrite: an env_file-only fragment with no environment: block and no diff --git a/internal/build/plugins_core_env_test.go b/internal/build/plugins_core_env_test.go index 0eafcaff4..9a16f28ea 100644 --- a/internal/build/plugins_core_env_test.go +++ b/internal/build/plugins_core_env_test.go @@ -442,3 +442,27 @@ func TestAddPluginCoreEnvVars_Populated(t *testing.T) { t.Errorf("empty NotifySecret must not be written, got: %v", vars["NOTIFY_INTERNAL_SECRET"]) } } + +// TestNormalizeComposePluginCoreEnv_NoMatchWarningListsManifestNames: when +// plugin.json's name differs from the directory name, the no-match warning +// lists the manifest-name candidates the lookup also tried (D-0604). +func TestNormalizeComposePluginCoreEnv_NoMatchWarningListsManifestNames(t *testing.T) { + resetWarnOnce() + pluginDir := t.TempDir() + dir := filepath.Join(pluginDir, "dirname") + if err := os.MkdirAll(dir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "plugin.json"), []byte(`{"name":"manifestname","port":3100}`), 0644); err != nil { + t.Fatal(err) + } + in := "services:\n a:\n image: x\n b:\n image: y\n" + stderr := captureStderr(t, func() { + normalizeComposePluginCoreEnv([]byte(in), pluginDir, "dirname") + }) + for _, want := range []string{"dirname", "nself-dirname", "plugin-dirname", "manifestname", "nself-manifestname", "plugin-manifestname"} { + if !strings.Contains(stderr, want) { + t.Errorf("warning must name candidate %q, got: %q", want, stderr) + } + } +} From 49590c5ea88b0bff2090a1e09e485bca22a089da Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:06:05 -0400 Subject: [PATCH 2/5] fix(nginx): record the flat snippet cert path as unmodelled instead of ssl_dir (D-0571) --- .../build/plugin_nginx_routes_build_test.go | 4 ++-- internal/build/plugin_nginx_routes_test.go | 4 ++-- .../nginx/routemodel/snippet_phase_test.go | 6 ++--- internal/nginx/routemodel/snippet_test.go | 6 ++--- internal/nginx/routemodel/snippet_tls.go | 22 +++++++------------ internal/nginx/routemodel/snippet_tls_test.go | 7 +++--- .../compliance/compliance.routes.json | 4 +++- .../snippets/geocoding/geocoding.routes.json | 4 +++- .../testdata/snippets/idme/idme.routes.json | 4 +++- .../object-storage/object-storage.routes.json | 7 ++++-- 10 files changed, 36 insertions(+), 32 deletions(-) diff --git a/internal/build/plugin_nginx_routes_build_test.go b/internal/build/plugin_nginx_routes_build_test.go index 8c5975e64..43a8bdaf2 100644 --- a/internal/build/plugin_nginx_routes_build_test.go +++ b/internal/build/plugin_nginx_routes_build_test.go @@ -25,8 +25,8 @@ server { listen 80; server_name idme.${BASE_DOMAIN}; return 301 https://$host$re server { listen 443 ssl http2; server_name idme.${BASE_DOMAIN}; - ssl_certificate /etc/nginx/ssl/${BASE_DOMAIN}/fullchain.pem; - ssl_certificate_key /etc/nginx/ssl/${BASE_DOMAIN}/privkey.pem; + ssl_certificate /etc/nginx/ssl/certificates/${BASE_DOMAIN}/fullchain.pem; + ssl_certificate_key /etc/nginx/ssl/certificates/${BASE_DOMAIN}/privkey.pem; location / { proxy_pass http://idme_backend; proxy_read_timeout 60s; weird_thing on; } } ` diff --git a/internal/build/plugin_nginx_routes_test.go b/internal/build/plugin_nginx_routes_test.go index c7f9afeb4..e9886cba3 100644 --- a/internal/build/plugin_nginx_routes_test.go +++ b/internal/build/plugin_nginx_routes_test.go @@ -25,8 +25,8 @@ server { listen 80; server_name idme.${BASE_DOMAIN}; return 301 https://$host$re server { listen 443 ssl http2; server_name idme.${BASE_DOMAIN}; - ssl_certificate /etc/nginx/ssl/${BASE_DOMAIN}/fullchain.pem; - ssl_certificate_key /etc/nginx/ssl/${BASE_DOMAIN}/privkey.pem; + ssl_certificate /etc/nginx/ssl/certificates/${BASE_DOMAIN}/fullchain.pem; + ssl_certificate_key /etc/nginx/ssl/certificates/${BASE_DOMAIN}/privkey.pem; location / { proxy_pass http://idme_backend; proxy_read_timeout 60s; weird_thing on; } } ` diff --git a/internal/nginx/routemodel/snippet_phase_test.go b/internal/nginx/routemodel/snippet_phase_test.go index b2be100ae..52ffcdc83 100644 --- a/internal/nginx/routemodel/snippet_phase_test.go +++ b/internal/nginx/routemodel/snippet_phase_test.go @@ -123,14 +123,14 @@ func TestSnippetWebSocketNeedsHTTP11(t *testing.T) { func TestSnippetCertDirRejectsDots(t *testing.T) { for _, dir := range []string{".", "..", ".hidden", "-x"} { - cert := "ssl_certificate /etc/nginx/ssl/" + dir + "/fullchain.pem;" - key := "ssl_certificate_key /etc/nginx/ssl/" + dir + "/privkey.pem;" + cert := "ssl_certificate /etc/nginx/ssl/certificates/" + dir + "/fullchain.pem;" + key := "ssl_certificate_key /etc/nginx/ssl/certificates/" + dir + "/privkey.pem;" r := route(t, "server { listen 443 ssl; server_name a.test; "+cert+" "+key+" }") if r.TLS == nil || r.TLS.SSLDir != "" || !reflect.DeepEqual(r.Unmodelled, []string{cert, key}) { t.Errorf("%s: tls=%+v unmodelled=%q", dir, r.TLS, r.Unmodelled) } } - r := route(t, "server { listen 443 ssl; server_name a.test; ssl_certificate /etc/nginx/ssl/a.test_1/fullchain.pem; ssl_certificate_key /etc/nginx/ssl/a.test_1/privkey.pem; }") + r := route(t, "server { listen 443 ssl; server_name a.test; ssl_certificate /etc/nginx/ssl/certificates/a.test_1/fullchain.pem; ssl_certificate_key /etc/nginx/ssl/certificates/a.test_1/privkey.pem; }") if r.TLS == nil || r.TLS.SSLDir != "a.test_1" || len(r.Unmodelled) != 0 { t.Errorf("valid dir: tls=%+v unmodelled=%q", r.TLS, r.Unmodelled) } diff --git a/internal/nginx/routemodel/snippet_test.go b/internal/nginx/routemodel/snippet_test.go index f3ff30f94..75e1f3767 100644 --- a/internal/nginx/routemodel/snippet_test.go +++ b/internal/nginx/routemodel/snippet_test.go @@ -23,7 +23,7 @@ var update = flag.Bool("update", false, "rewrite snippet goldens") // renderVars stands in for the build's template variables. var renderVars = strings.NewReplacer("${BASE_DOMAIN}", "example.test", "${DOMAIN}", "example.test", - "${NSELF_DOMAIN}", "example.test", "${SSL_DIR}", "example-test", "${GRAFANA_ROUTE}", "grafana", + "${NSELF_DOMAIN}", "example.test", "${SSL_DIR}", "certificates/example-test", "${GRAFANA_ROUTE}", "grafana", "${PROMETHEUS_ROUTE}", "prometheus", "${ALERTMANAGER_ROUTE}", "alertmanager") type corpusFile struct{ slug, name, path string } @@ -229,7 +229,7 @@ server { listen 443 ssl; server_name a.test; weird_directive 1 2 3; if ($x) { re } func TestSnippetMapping(t *testing.T) { - const head = "server { listen 443 ssl http2; server_name a.test; ssl_certificate /etc/nginx/ssl/d/fullchain.pem; ssl_certificate_key /etc/nginx/ssl/d/privkey.pem; " + const head = "server { listen 443 ssl http2; server_name a.test; ssl_certificate /etc/nginx/ssl/certificates/d/fullchain.pem; ssl_certificate_key /etc/nginx/ssl/certificates/d/privkey.pem; " loc := func(body string) Location { r := route(t, head+"location / { "+body+" } }") if len(r.Unmodelled) != 0 { @@ -444,7 +444,7 @@ func TestTokenizeAdapter(t *testing.T) { func TestHandManagedRoutes(t *testing.T) { text := "server { listen 80; server_name a.example.test; location / { proxy_pass http://a:80; } }\n" + - "server { listen 443 ssl; server_name b.example.test; ssl_certificate /etc/nginx/ssl/d/fullchain.pem; ssl_certificate_key /etc/nginx/ssl/d/privkey.pem; location /x { proxy_pass https://b.example.test; } }\n" + "server { listen 443 ssl; server_name b.example.test; ssl_certificate /etc/nginx/ssl/certificates/d/fullchain.pem; ssl_certificate_key /etc/nginx/ssl/certificates/d/privkey.pem; location /x { proxy_pass https://b.example.test; } }\n" rs, err := FromSnippet("hand_managed", "", "custom.conf", text) if err != nil || len(rs) != 2 { t.Fatalf("routes=%d err=%v", len(rs), err) diff --git a/internal/nginx/routemodel/snippet_tls.go b/internal/nginx/routemodel/snippet_tls.go index a0853329b..740110b8a 100644 --- a/internal/nginx/routemodel/snippet_tls.go +++ b/internal/nginx/routemodel/snippet_tls.go @@ -7,23 +7,17 @@ package routemodel // Outputs: the dir name, or "" with every certificate line recorded. // Constraints: a renderer joins ssl_dir into a path, so `.`, `..` and dot // dirs never match, and a half match (one line elsewhere) maps nothing. -// Two path shapes map: the nested one internal/ssl.Generator writes and the +// Only the nested shape maps: the one internal/ssl.Generator writes and the // CLI's own templates read (/etc/nginx/ssl/certificates//, which is what -// a plugin snippet's /etc/nginx/ssl/${SSL_DIR}/ renders to), and the older -// flat /etc/nginx/ssl//. Both lines must use the same shape. +// a plugin snippet's /etc/nginx/ssl/${SSL_DIR}/ renders to). The older flat +// /etc/nginx/ssl// names a different file than the renderer's +// certificates/, so it is recorded as unmodelled (D-0571). -import ( - "regexp" - "strings" -) +import "regexp" -// certSubdir is the level internal/ssl.Generator writes certificate dirs -// under, inside the nginx ssl mount. -const certSubdir = "certificates/" +var certRE = regexp.MustCompile(`^/etc/nginx/ssl/certificates/([A-Za-z0-9][A-Za-z0-9._-]*)/(fullchain|privkey)\.pem$`) -var certRE = regexp.MustCompile(`^/etc/nginx/ssl/((?:certificates/)?[A-Za-z0-9][A-Za-z0-9._-]*)/(fullchain|privkey)\.pem$`) - -// certDir maps exactly one ssl_certificate /etc/nginx/ssl/[certificates/]/fullchain.pem +// certDir maps exactly one ssl_certificate /etc/nginx/ssl/certificates//fullchain.pem // and one ssl_certificate_key ...//privkey.pem with the same path to the // route's ssl_dir . Anything else (another path or file name, two dirs, // mixed shapes, a missing half) records every certificate line and leaves @@ -48,5 +42,5 @@ func (s *srv) certDir() string { if !ok { return "" } - return strings.TrimPrefix(dir, certSubdir) + return dir } diff --git a/internal/nginx/routemodel/snippet_tls_test.go b/internal/nginx/routemodel/snippet_tls_test.go index bffa580f7..d3d0fa961 100644 --- a/internal/nginx/routemodel/snippet_tls_test.go +++ b/internal/nginx/routemodel/snippet_tls_test.go @@ -2,7 +2,8 @@ package routemodel // snippet_tls_test.go — D-0505: the nested certificate path internal/ssl // writes (and a plugin's /etc/nginx/ssl/${SSL_DIR}/ renders to) maps to the -// route's ssl_dir, the flat shape keeps mapping, mixed shapes do not. +// route's ssl_dir; the flat shape names a different file than the renderer's +// certificates/ and is unmodelled (D-0571), as are mixed shapes. import ( "reflect" @@ -14,8 +15,8 @@ func TestSnippetCertDirShapes(t *testing.T) { name, cert, key, want string }{ {"nested", "/etc/nginx/ssl/certificates/local-nself-org/fullchain.pem", "/etc/nginx/ssl/certificates/local-nself-org/privkey.pem", "local-nself-org"}, - {"flat", "/etc/nginx/ssl/local-nself-org/fullchain.pem", "/etc/nginx/ssl/local-nself-org/privkey.pem", "local-nself-org"}, - {"flat dir named certificates", "/etc/nginx/ssl/certificates/fullchain.pem", "/etc/nginx/ssl/certificates/privkey.pem", "certificates"}, + {"flat", "/etc/nginx/ssl/local-nself-org/fullchain.pem", "/etc/nginx/ssl/local-nself-org/privkey.pem", ""}, + {"flat dir named certificates", "/etc/nginx/ssl/certificates/fullchain.pem", "/etc/nginx/ssl/certificates/privkey.pem", ""}, {"mixed shapes", "/etc/nginx/ssl/certificates/a/fullchain.pem", "/etc/nginx/ssl/a/privkey.pem", ""}, {"nested two dirs", "/etc/nginx/ssl/certificates/a/fullchain.pem", "/etc/nginx/ssl/certificates/b/privkey.pem", ""}, {"nested dot dir", "/etc/nginx/ssl/certificates/.x/fullchain.pem", "/etc/nginx/ssl/certificates/.x/privkey.pem", ""}, diff --git a/internal/nginx/routemodel/testdata/snippets/compliance/compliance.routes.json b/internal/nginx/routemodel/testdata/snippets/compliance/compliance.routes.json index ce9273bdd..72235ba8d 100644 --- a/internal/nginx/routemodel/testdata/snippets/compliance/compliance.routes.json +++ b/internal/nginx/routemodel/testdata/snippets/compliance/compliance.routes.json @@ -62,7 +62,7 @@ "https": true }, "tls": { - "ssl_dir": "example.test", + "ssl_dir": "", "protocols": [ "TLSv1.2", "TLSv1.3" @@ -129,6 +129,8 @@ ], "shadowed_by": null, "unmodelled": [ + "ssl_certificate /etc/nginx/ssl/example.test/fullchain.pem;", + "ssl_certificate_key /etc/nginx/ssl/example.test/privkey.pem;", "proxy_pass http://compliance_backend/health;", "proxy_set_header Host $host;" ] diff --git a/internal/nginx/routemodel/testdata/snippets/geocoding/geocoding.routes.json b/internal/nginx/routemodel/testdata/snippets/geocoding/geocoding.routes.json index 271946bea..25d7e036b 100644 --- a/internal/nginx/routemodel/testdata/snippets/geocoding/geocoding.routes.json +++ b/internal/nginx/routemodel/testdata/snippets/geocoding/geocoding.routes.json @@ -62,7 +62,7 @@ "https": true }, "tls": { - "ssl_dir": "example.test", + "ssl_dir": "", "protocols": [], "ciphers": null }, @@ -126,6 +126,8 @@ ], "shadowed_by": null, "unmodelled": [ + "ssl_certificate /etc/nginx/ssl/example.test/fullchain.pem;", + "ssl_certificate_key /etc/nginx/ssl/example.test/privkey.pem;", "proxy_pass http://geocoding_backend/health;" ] } diff --git a/internal/nginx/routemodel/testdata/snippets/idme/idme.routes.json b/internal/nginx/routemodel/testdata/snippets/idme/idme.routes.json index 9d457242b..2253a563e 100644 --- a/internal/nginx/routemodel/testdata/snippets/idme/idme.routes.json +++ b/internal/nginx/routemodel/testdata/snippets/idme/idme.routes.json @@ -62,7 +62,7 @@ "https": true }, "tls": { - "ssl_dir": "example.test", + "ssl_dir": "", "protocols": [], "ciphers": null }, @@ -126,6 +126,8 @@ ], "shadowed_by": null, "unmodelled": [ + "ssl_certificate /etc/nginx/ssl/example.test/fullchain.pem;", + "ssl_certificate_key /etc/nginx/ssl/example.test/privkey.pem;", "proxy_pass http://idme_backend/health;" ] } diff --git a/internal/nginx/routemodel/testdata/snippets/object-storage/object-storage.routes.json b/internal/nginx/routemodel/testdata/snippets/object-storage/object-storage.routes.json index fd9791d5c..be76d497d 100644 --- a/internal/nginx/routemodel/testdata/snippets/object-storage/object-storage.routes.json +++ b/internal/nginx/routemodel/testdata/snippets/object-storage/object-storage.routes.json @@ -16,7 +16,7 @@ "https": true }, "tls": { - "ssl_dir": "example.test", + "ssl_dir": "", "protocols": [], "ciphers": null }, @@ -54,7 +54,10 @@ } ], "shadowed_by": null, - "unmodelled": [] + "unmodelled": [ + "ssl_certificate /etc/nginx/ssl/example.test/fullchain.pem;", + "ssl_certificate_key /etc/nginx/ssl/example.test/privkey.pem;" + ] } ] } From 63e14b9c975d96ae49494cbd862316c933e291fe Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:06:05 -0400 Subject: [PATCH 3/5] test(portable): pin that a same-size edit and a swapped parent dir are refused (D-0252) --- internal/portable/reader_race_test.go | 90 +++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 internal/portable/reader_race_test.go diff --git a/internal/portable/reader_race_test.go b/internal/portable/reader_race_test.go new file mode 100644 index 000000000..2e1a1dfa2 --- /dev/null +++ b/internal/portable/reader_race_test.go @@ -0,0 +1,90 @@ +package portable + +// reader_race_test.go — D-0252: a writer racing the reader cannot make it +// accept bytes the manifest does not hash. +// +// Open and Verify hash every byte they read and compare the descriptor to the +// snapshot taken at Open, so neither a same-size in-place edit with the mtime +// put back nor a parent directory swapped for a symlink is accepted. +// OpenFile alone hands out a descriptor whose bytes are not hashed again (its +// documented contract); it is covered by the snapshot checks in +// openfile_test.go. + +import ( + "io" + "os" + "path/filepath" + "testing" +) + +// TestSameSizeEditWithMtimeRestoredIsRefused edits a member in place, keeps +// its size, puts the old mtime back, and checks the stream and Verify still +// fail on the hash. +func TestSameSizeEditWithMtimeRestoredIsRefused(t *testing.T) { + dir := fixture(t) + r, err := Open(dir) + if err != nil { + t.Fatal(err) + } + p := filepath.Join(dir, "db", "schema.sql") + before := mustStat(t, p) + b, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + b[0] ^= 1 + if err := os.WriteFile(p, b, 0o600); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(p, before.ModTime(), before.ModTime()); err != nil { + t.Fatal(err) + } + + rc, err := r.Open("db/schema.sql") + if err != nil { + t.Fatalf("the snapshot cannot tell this edit apart, so Open must hand out the stream: %v", err) + } + _, err = io.ReadAll(rc) + _ = rc.Close() + wantCode(t, err, "E516", ErrChecksum) + wantCode(t, r.Verify(), "E516", ErrChecksum) +} + +// TestSwappedParentDirectoryIsRefused replaces a verified directory with a +// symlink to a copy holding a same-size file with other bytes. +func TestSwappedParentDirectoryIsRefused(t *testing.T) { + dir := fixture(t) + r, err := Open(dir) + if err != nil { + t.Fatal(err) + } + evil := t.TempDir() + orig, err := os.ReadFile(filepath.Join(dir, "db", "schema.sql")) + if err != nil { + t.Fatal(err) + } + orig[0] ^= 1 + for _, name := range []string{"schema.sql", "data.dump"} { + if err := os.WriteFile(filepath.Join(evil, name), orig, 0o600); err != nil { + t.Fatal(err) + } + } + if err := os.RemoveAll(filepath.Join(dir, "db")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(evil, filepath.Join(dir, "db")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + rc, err := r.Open("db/schema.sql") + if err == nil { + _, err = io.ReadAll(rc) + _ = rc.Close() + } + wantCode(t, err, "E516", nil) + if fh, err := r.OpenFile("db/schema.sql"); err == nil { + _ = fh.Close() + t.Fatal("OpenFile followed a swapped parent directory") + } + wantCode(t, r.Verify(), "E516", nil) +} From 01f4fc17749349220f101a5df12d3fab4f760a5f Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:06:14 -0400 Subject: [PATCH 4/5] refactor(config): one plugin dir resolver and one cert dir name rule for every reader (D-0573) --- .github/wiki/Compat-V15.md | 2 +- cmd/commands/plugin.go | 12 +--- internal/apidocs/openapi_render.go | 2 +- internal/build/orchestrator_nginx.go | 5 +- internal/build/plugin_configs.go | 47 +------------ internal/build/plugin_configs_test.go | 4 +- internal/build/plugin_nginx.go | 19 ++--- internal/build/plugins.go | 12 +--- internal/build/proxy_provider.go | 5 +- internal/build/shared_proxy.go | 11 ++- internal/config/plugin_dir.go | 80 ++++++++++++++++++++++ internal/config/plugin_dir_test.go | 76 ++++++++++++++++++++ internal/database/migrate_ledger.go | 7 +- internal/database/migrate_test.go | 22 ++++++ internal/nginx/routemodel/build.go | 10 +-- internal/nginx/routemodel/build_apidocs.go | 4 +- internal/nginx/ssl_detect.go | 22 +++--- internal/nginxtopo/ssl_path.go | 30 ++++++-- internal/nginxtopo/ssl_path_test.go | 19 +++++ internal/repoqa/layering_basis_test.go | 1 - internal/repoqa/testdata/layers.yaml | 1 - internal/ssl/generator_trust.go | 9 +-- 22 files changed, 262 insertions(+), 138 deletions(-) create mode 100644 internal/config/plugin_dir.go create mode 100644 internal/config/plugin_dir_test.go create mode 100644 internal/nginxtopo/ssl_path_test.go diff --git a/.github/wiki/Compat-V15.md b/.github/wiki/Compat-V15.md index df7efac1f..0ee73973b 100644 --- a/.github/wiki/Compat-V15.md +++ b/.github/wiki/Compat-V15.md @@ -66,7 +66,7 @@ The table is generated. Never hand-edit it; run `compat-markers.sh --write-wiki` | D-0330 | remove drops plugin data unless --keep-data | keeps it unless --delete-data, confirmed by --yes or a terminal | `cmd/commands/plugin_remove_data.go` | | D-0430 | "Error: trust warnings present (exit code 2)" and exit 1 | silent exit 2, the canon's warning class | `cmd/commands/trust_status_detailed.go` | | D-0437 | doctor only repairs generated files | diagnoses and repairs deferred boot-plugin table tracking | `cmd/commands/doctor_plugin_tables.go` | -| D-0478 | PLUGIN_DIR "~/..." used as written, no plugin snippets on default config | "~" expanded, installed plugins' snippets injected | `internal/build/plugin_configs.go` | +| D-0478 | PLUGIN_DIR "~/..." used as written, no plugin snippets on default config | "~" expanded, installed plugins' snippets injected | `internal/config/plugin_dir.go` | | D-0484 | pin store ancestor walk warns once and continues | refuses | `sdk/go/remote/knownhosts_compat.go` | | D-0491 | replacing another owner's or a core nginx/sites file warns | E055 fails the build | `internal/build/plugin_nginx_collision.go` | | D-0676 | bare stream path streams as path:// with a notice | refused | `internal/backup/stream_dest_compat.go` | diff --git a/cmd/commands/plugin.go b/cmd/commands/plugin.go index 9d5ddab32..0967c3c2b 100644 --- a/cmd/commands/plugin.go +++ b/cmd/commands/plugin.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os" - "path/filepath" "strings" "github.com/nself-org/cli/internal/config" @@ -189,16 +188,7 @@ func init() { // --- helpers --- // resolvePluginDir returns the plugin directory from config or a sensible default. -func resolvePluginDir() string { - if d := os.Getenv("NSELF_PLUGIN_DIR"); d != "" { - return d - } - home, err := os.UserHomeDir() - if err != nil { - return filepath.Join("/tmp", ".nself", "plugins") - } - return filepath.Join(home, ".nself", "plugins") -} +func resolvePluginDir() string { return config.DefaultPluginDir() } // loadConfig loads the project config from the current working directory. func loadConfig() (*config.Config, error) { diff --git a/internal/apidocs/openapi_render.go b/internal/apidocs/openapi_render.go index d35d73b57..65c5e8002 100644 --- a/internal/apidocs/openapi_render.go +++ b/internal/apidocs/openapi_render.go @@ -56,7 +56,7 @@ func NginxConf(docsServePath, baseDomain string) string { if baseDomain == "" { baseDomain = "localhost" } - certDir := strings.ReplaceAll(baseDomain, ".", "-") + certDir := nginxtopo.CertDirName(baseDomain) return fmt.Sprintf(`# Generated by nself build - DO NOT EDIT MANUALLY # Service: docs.%s diff --git a/internal/build/orchestrator_nginx.go b/internal/build/orchestrator_nginx.go index 09586af51..c0c6c31a1 100644 --- a/internal/build/orchestrator_nginx.go +++ b/internal/build/orchestrator_nginx.go @@ -168,10 +168,7 @@ func (s *snippetSet) parse() { // skipped (pluginSnippetSkip, hasSSL = the model's TLS) the same way; a // skipped snippet is recorded in global, not dropped. Read errors are left to the injection step, which reports them. func (s *snippetSet) plugins(workdir string, cfg *config.Config, hasSSL bool) { - pluginDir, err := resolvePluginNginxDir("", cfg) - if err != nil { - return - } + pluginDir := resolvePluginNginxDir("", cfg) entries, err := os.ReadDir(pluginDir) if err != nil { return diff --git a/internal/build/plugin_configs.go b/internal/build/plugin_configs.go index 492fca7da..55108c1a5 100644 --- a/internal/build/plugin_configs.go +++ b/internal/build/plugin_configs.go @@ -6,9 +6,8 @@ import ( "path/filepath" "strings" - "github.com/nself-org/cli/internal/compat" "github.com/nself-org/cli/internal/config" - "github.com/nself-org/cli/internal/ssl" + "github.com/nself-org/cli/internal/nginxtopo" ) // RenderPluginConfigs walks installed plugins' configs/ directories @@ -124,13 +123,7 @@ func buildTemplateVars(workdir string, cfg *config.Config) map[string]string { network = cfg.ProjectName + "_network" } - pluginDir := expandPluginDir(cfg.PluginSystem.Dir) - if pluginDir == "" { - home, _ := os.UserHomeDir() - if home != "" { - pluginDir = filepath.Join(home, ".nself", "plugins") - } - } + pluginDir := config.ResolvePluginDir(cfg.PluginSystem.Dir) return map[string]string{ "SSL_DIR": pluginSSLDir(cfg.BaseDomain), @@ -155,39 +148,5 @@ func buildTemplateVars(workdir string, cfg *config.Config) map[string]string { // certificates/ level. An empty domain falls back to localhost, as the // generator does. func pluginSSLDir(baseDomain string) string { - dir := ssl.DomainToDirName(baseDomain) - if dir == "" { - dir = "localhost" - } - return "certificates/" + dir -} - -// expandPluginDir is expandHomeDir under v1.5 only (D-0634). v1.4.13 used -// PLUGIN_DIR as written, so the default "~/.nself/plugins" never resolved: a -// default-config project got no plugin nginx snippets, and -// ${NSELF_PLUGIN_DIR} rendered as "~/.nself/plugins". Expanding it in a patch -// would publish every installed plugin at . on the first -// rebuild after an upgrade, and fail the build on a snippet the user had -// copied into nginx/sites by hand, so v1.4 keeps the old value. -func expandPluginDir(dir string) string { - // compat.V15(D-0478): PLUGIN_DIR "~/..." used as written, no plugin snippets on default config -> "~" expanded, installed plugins' snippets injected - if compat.V15() { - return expandHomeDir(dir) - } - return dir -} - -// expandHomeDir turns a leading "~" or "~/" into the user's home dir. -// config.ApplyDefaults sets PLUGIN_DIR to the literal "~/.nself/plugins", -// which os.Stat would otherwise read relative to the working dir, so no -// plugin snippet was ever found. Anything else is returned unchanged. -func expandHomeDir(dir string) string { - if dir != "~" && !strings.HasPrefix(dir, "~/") { - return dir - } - home, err := os.UserHomeDir() - if err != nil || home == "" { - return dir - } - return filepath.Join(home, strings.TrimPrefix(dir, "~")) + return "certificates/" + nginxtopo.CertDirNameOrLocalhost(baseDomain) } diff --git a/internal/build/plugin_configs_test.go b/internal/build/plugin_configs_test.go index 8d0c44602..ee8586e77 100644 --- a/internal/build/plugin_configs_test.go +++ b/internal/build/plugin_configs_test.go @@ -78,8 +78,8 @@ func TestExpandHomeDir(t *testing.T) { "": "", } for in, want := range cases { - if got := expandHomeDir(in); got != want { - t.Errorf("expandHomeDir(%q) = %q, want %q", in, got, want) + if got := config.ExpandHomeDir(in); got != want { + t.Errorf("ExpandHomeDir(%q) = %q, want %q", in, got, want) } } } diff --git a/internal/build/plugin_nginx.go b/internal/build/plugin_nginx.go index 0f72fdb7c..2f4ccf194 100644 --- a/internal/build/plugin_nginx.go +++ b/internal/build/plugin_nginx.go @@ -152,20 +152,12 @@ func pluginSnippetSkip(rendered string, vars map[string]string, hasSSL bool) str // resolvePluginNginxDir returns the plugin directory the nginx snippets are // read from: pluginDir, else the configured one, else ~/.nself/plugins. A -// leading "~" is expanded under v1.5 only (expandPluginDir, D-0634). -func resolvePluginNginxDir(pluginDir string, cfg *config.Config) (string, error) { +// leading "~" is expanded under v1.5 only (config.ExpandPluginDir, D-0634). +func resolvePluginNginxDir(pluginDir string, cfg *config.Config) string { if pluginDir == "" { pluginDir = cfg.PluginSystem.Dir } - pluginDir = expandPluginDir(pluginDir) - if pluginDir == "" { - home, err := os.UserHomeDir() - if err != nil { - return "", fmt.Errorf("resolving home directory: %w", err) - } - pluginDir = filepath.Join(home, ".nself", "plugins") - } - return pluginDir, nil + return config.ResolvePluginDir(pluginDir) } // pluginConfPaths lists a plugin's nginx/*.conf files (none when it ships no @@ -203,10 +195,7 @@ func renderPluginSnippet(content string, vars map[string]string) string { // injectPluginNginxRoutesVia is InjectPluginNginxRoutes writing through sink. func injectPluginNginxRoutesVia(sink Sink, workdir, pluginDir string, cfg *config.Config, hasSSL bool) (int, error) { - pluginDir, err := resolvePluginNginxDir(pluginDir, cfg) - if err != nil { - return 0, err - } + pluginDir = resolvePluginNginxDir(pluginDir, cfg) // If the plugin directory does not exist, there is nothing to inject. if _, err := os.Stat(pluginDir); os.IsNotExist(err) { diff --git a/internal/build/plugins.go b/internal/build/plugins.go index 3f4d8b5c8..2899ba0ae 100644 --- a/internal/build/plugins.go +++ b/internal/build/plugins.go @@ -21,6 +21,7 @@ import ( "sync" "github.com/nself-org/cli/internal/compat" + "github.com/nself-org/cli/internal/config" "github.com/nself-org/cli/internal/docker" "github.com/nself-org/cli/internal/errs" "github.com/nself-org/cli/internal/license/entitlement" @@ -44,16 +45,7 @@ var refreshPluginEntitlement = entitlement.RefreshForPlugins // (~/.nself/plugins). The NSELF_PLUGIN_DIR environment variable overrides the // default — used for per-project plugin sets, hermetic tests, and CI. Falls // back to /tmp/.nself/plugins when the home directory cannot be determined. -func DefaultPluginDir() string { - if dir := strings.TrimSpace(os.Getenv("NSELF_PLUGIN_DIR")); dir != "" { - return dir - } - home, err := os.UserHomeDir() - if err != nil { - return filepath.Join("/tmp", ".nself", "plugins") - } - return filepath.Join(home, ".nself", "plugins") -} +func DefaultPluginDir() string { return config.DefaultPluginDir() } // DiscoverPluginComposeFiles scans pluginDir for installed plugins that // contain a docker-compose.plugin.yml file. It returns absolute paths to diff --git a/internal/build/proxy_provider.go b/internal/build/proxy_provider.go index e6a7acc32..8c2e1c137 100644 --- a/internal/build/proxy_provider.go +++ b/internal/build/proxy_provider.go @@ -238,10 +238,7 @@ func proxyRouteLimitations(m *routemodel.Model) ([]string, map[string]bool) { // proxyPluginSiteNames matches injection's directory resolution and filenames. func proxyPluginSiteNames(cfg *config.Config, names map[string]bool) error { - dir, err := resolvePluginNginxDir("", cfg) - if err != nil { - return err - } + dir := resolvePluginNginxDir("", cfg) entries, err := os.ReadDir(dir) if err != nil && !os.IsNotExist(err) { return err diff --git a/internal/build/shared_proxy.go b/internal/build/shared_proxy.go index 9c01811d3..5f1e89a1e 100644 --- a/internal/build/shared_proxy.go +++ b/internal/build/shared_proxy.go @@ -199,12 +199,11 @@ func (st *buildState) sharedServices() map[string]string { for _, svc := range DetectServices(st.cfg) { out[svc] = st.cfg.ServiceContainerName(svc) } - if dir, err := resolvePluginNginxDir("", st.cfg); err == nil { - entries, _ := os.ReadDir(dir) - for _, e := range entries { - if _, err := os.Stat(filepath.Join(dir, e.Name(), pluginComposeFilename)); err == nil && e.IsDir() { - out[e.Name()] = st.cfg.ServiceContainerName(e.Name()) - } + dir := resolvePluginNginxDir("", st.cfg) + entries, _ := os.ReadDir(dir) + for _, e := range entries { + if _, err := os.Stat(filepath.Join(dir, e.Name(), pluginComposeFilename)); err == nil && e.IsDir() { + out[e.Name()] = st.cfg.ServiceContainerName(e.Name()) } } return out diff --git a/internal/config/plugin_dir.go b/internal/config/plugin_dir.go new file mode 100644 index 000000000..abf02b6de --- /dev/null +++ b/internal/config/plugin_dir.go @@ -0,0 +1,80 @@ +package config + +// plugin_dir.go — the one plugin directory rule (D-0573). +// +// Purpose: every reader of the global plugin directory resolves it the same +// way: NSELF_PLUGIN_DIR (or the configured PLUGIN_DIR), else ~/.nself/plugins, +// with a leading "~" expanded. Before this file compose discovery, the plugin +// migrations, the nginx injection and the plugin commands each carried their +// own copy, and only the nginx one expanded "~" (the D-0478 class of drift). +// Inputs: a configured dir (may be empty or start with "~") and the +// environment. +// Outputs: the directory to read plugins from. +// Constraints: the "~" expansion is v1.5 only (D-0634): v1.4.13 used PLUGIN_DIR +// as written, and expanding it in a patch would publish every installed +// plugin's nginx snippet on the first rebuild after an upgrade. + +import ( + "os" + "path/filepath" + "strings" + + "github.com/nself-org/cli/internal/compat" +) + +// pluginDirEnv is the environment variable that overrides the plugin dir. +const pluginDirEnv = "NSELF_PLUGIN_DIR" + +// DefaultPluginDir returns the global plugin installation directory: +// $NSELF_PLUGIN_DIR when set, else ~/.nself/plugins (/tmp/.nself/plugins when +// the home directory cannot be determined). +func DefaultPluginDir() string { + if dir := strings.TrimSpace(os.Getenv(pluginDirEnv)); dir != "" { + return ExpandPluginDir(dir) + } + home, err := os.UserHomeDir() + if err != nil || home == "" { + return filepath.Join("/tmp", ".nself", "plugins") + } + return filepath.Join(home, ".nself", "plugins") +} + +// ResolvePluginDir is a configured plugin dir (PLUGIN_DIR, which defaults to +// the literal "~/.nself/plugins") made usable: expanded, or DefaultPluginDir +// when nothing is configured. +func ResolvePluginDir(configured string) string { + if dir := ExpandPluginDir(configured); dir != "" { + return dir + } + return DefaultPluginDir() +} + +// ExpandPluginDir is ExpandHomeDir under v1.5 only (D-0634). v1.4.13 used +// PLUGIN_DIR as written, so the default "~/.nself/plugins" never resolved: a +// default-config project got no plugin nginx snippets, and +// ${NSELF_PLUGIN_DIR} rendered as "~/.nself/plugins". Expanding it in a patch +// would publish every installed plugin at . on the first +// rebuild after an upgrade, and fail the build on a snippet the user had +// copied into nginx/sites by hand, so v1.4 keeps the old value. +func ExpandPluginDir(dir string) string { + // compat.V15(D-0478): PLUGIN_DIR "~/..." used as written, no plugin snippets on default config -> "~" expanded, installed plugins' snippets injected + if compat.V15() { + return ExpandHomeDir(dir) + } + return dir +} + +// ExpandHomeDir turns a leading "~" or "~/" into the user's home dir. +// config.ApplyDefaults sets PLUGIN_DIR to the literal "~/.nself/plugins", +// which os.Stat would otherwise read relative to the working dir, so no +// plugin snippet was ever found. Anything else is returned unchanged. +func ExpandHomeDir(dir string) string { + if dir != "~" && !strings.HasPrefix(dir, "~/") { + return dir + } + home, err := os.UserHomeDir() + if err != nil || home == "" { + return dir + } + return filepath.Join(home, strings.TrimPrefix(dir, "~")) +} diff --git a/internal/config/plugin_dir_test.go b/internal/config/plugin_dir_test.go new file mode 100644 index 000000000..2749536d9 --- /dev/null +++ b/internal/config/plugin_dir_test.go @@ -0,0 +1,76 @@ +package config + +import ( + "path/filepath" + "testing" +) + +func setPluginDirHome(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + return home +} + +func TestDefaultPluginDir(t *testing.T) { + home := setPluginDirHome(t) + t.Setenv("NSELF_V15", "1") + + t.Setenv("NSELF_PLUGIN_DIR", "") + if got, want := DefaultPluginDir(), filepath.Join(home, ".nself", "plugins"); got != want { + t.Errorf("unset env: %q, want %q", got, want) + } + t.Setenv("NSELF_PLUGIN_DIR", " /abs/plugins ") + if got := DefaultPluginDir(); got != "/abs/plugins" { + t.Errorf("env is trimmed: %q", got) + } + t.Setenv("NSELF_PLUGIN_DIR", "~/custom") + if got, want := DefaultPluginDir(), filepath.Join(home, "custom"); got != want { + t.Errorf("v1.5 expands a literal ~ in the env value: %q, want %q", got, want) + } +} + +func TestResolvePluginDir(t *testing.T) { + home := setPluginDirHome(t) + t.Setenv("NSELF_PLUGIN_DIR", "") + def := filepath.Join(home, ".nself", "plugins") + + t.Setenv("NSELF_V15", "1") + for in, want := range map[string]string{ + "~/.nself/plugins": def, + "~": home, + "/abs/plugins": "/abs/plugins", + "rel/plugins": "rel/plugins", + "": def, + } { + if got := ResolvePluginDir(in); got != want { + t.Errorf("v1.5 ResolvePluginDir(%q) = %q, want %q", in, got, want) + } + } + + // v1.4 keeps a configured value as written (D-0634); empty still defaults. + t.Setenv("NSELF_V15", "") + if got := ResolvePluginDir("~/.nself/plugins"); got != "~/.nself/plugins" { + t.Errorf("v1.4 must not expand: %q", got) + } + if got := ResolvePluginDir(""); got != def { + t.Errorf("v1.4 empty: %q, want %q", got, def) + } +} + +func TestExpandHomeDir(t *testing.T) { + home := setPluginDirHome(t) + for in, want := range map[string]string{ + "~/.nself/plugins": filepath.Join(home, ".nself", "plugins"), + "~": home, + "/abs/plugins": "/abs/plugins", + "rel/plugins": "rel/plugins", + "~user/plugins": "~user/plugins", + "": "", + } { + if got := ExpandHomeDir(in); got != want { + t.Errorf("ExpandHomeDir(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/database/migrate_ledger.go b/internal/database/migrate_ledger.go index 14642f600..a128024b6 100644 --- a/internal/database/migrate_ledger.go +++ b/internal/database/migrate_ledger.go @@ -55,12 +55,7 @@ type MigrationStatus struct { // messages point the user at the canonical location. func migrationsDir(cfg *config.Config, plugin string) string { if plugin != "" { - pluginDir := cfg.PluginSystem.Dir - if pluginDir == "" { - home, _ := os.UserHomeDir() - pluginDir = filepath.Join(home, ".nself", "plugins") - } - return filepath.Join(pluginDir, plugin, "migrations") + return filepath.Join(config.ResolvePluginDir(cfg.PluginSystem.Dir), plugin, "migrations") } candidates := []string{ diff --git a/internal/database/migrate_test.go b/internal/database/migrate_test.go index 2b9fa1223..353e338a0 100644 --- a/internal/database/migrate_test.go +++ b/internal/database/migrate_test.go @@ -477,3 +477,25 @@ func TestMigrationsDir_LegacyBeatsPostgres(t *testing.T) { t.Errorf("expected migrations to win over postgres/migrations, got %q", got) } } + +// TestMigrationsDir_PluginHomeExpansion: a plugin's migrations come from the +// shared plugin dir rule (D-0573): the default config's literal +// "~/.nself/plugins" is expanded under v1.5, and kept as written under v1.4. +func TestMigrationsDir_PluginHomeExpansion(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("NSELF_PLUGIN_DIR", "") + cfg := &config.Config{} + cfg.PluginSystem.Dir = "~/.nself/plugins" + + t.Setenv("NSELF_V15", "1") + want := filepath.Join(home, ".nself", "plugins", "ai", "migrations") + if got := migrationsDir(cfg, "ai"); got != want { + t.Errorf("v1.5: %q, want %q", got, want) + } + t.Setenv("NSELF_V15", "") + if got, w := migrationsDir(cfg, "ai"), filepath.Join("~/.nself/plugins", "ai", "migrations"); got != w { + t.Errorf("v1.4: %q, want %q", got, w) + } +} diff --git a/internal/nginx/routemodel/build.go b/internal/nginx/routemodel/build.go index 8723fd57b..d675fb72f 100644 --- a/internal/nginx/routemodel/build.go +++ b/internal/nginx/routemodel/build.go @@ -50,10 +50,7 @@ func Build(cfg *config.Config, workdir string, hasSSL bool) (*Model, error) { TLS: TLSDefaults{Protocols: []string{"TLSv1.2", "TLSv1.3"}, Ciphers: ptr(ciphers)}}, DefaultServer: DefaultServer{HTTP: DefaultHTTP{ACMEWebroot: &webroot, HealthPath: &health, RedirectHTTPS: hasSSL}, HTTPS: DefaultHTTPS{Action: "close"}}, Zones: buildZones(cfg)} - sslDir := strings.ReplaceAll(cfg.BaseDomain, ".", "-") - if sslDir == "" { - sslDir = "localhost" - } + sslDir := nginxtopo.CertDirNameOrLocalhost(cfg.BaseDomain) var specs []routeSpec add := func(id, source, owner, file, name, legacy, upstream, zone string, burst, conn int, websocket bool) { specs = append(specs, routeSpec{id, source, owner, file, name, legacy, upstream, zone, burst, conn, websocket}) @@ -169,10 +166,7 @@ func ptr(s string) *string { return &s } // SetSSL refreshes TLS facts after the build's certificate phase. func SetSSL(m *Model, hasSSL bool) { m.DefaultServer.HTTP.RedirectHTTPS = hasSSL - sslDir := strings.ReplaceAll(m.BaseDomain, ".", "-") - if sslDir == "" { - sslDir = "localhost" - } + sslDir := nginxtopo.CertDirNameOrLocalhost(m.BaseDomain) for i := range m.Routes { r := &m.Routes[i] r.Listen = Listen{HTTP: !hasSSL, HTTPS: hasSSL} diff --git a/internal/nginx/routemodel/build_apidocs.go b/internal/nginx/routemodel/build_apidocs.go index b44fd5cc6..ffd0426f3 100644 --- a/internal/nginx/routemodel/build_apidocs.go +++ b/internal/nginx/routemodel/build_apidocs.go @@ -3,9 +3,9 @@ package routemodel import ( "fmt" "regexp" - "strings" "github.com/nself-org/cli/internal/config" + "github.com/nself-org/cli/internal/nginxtopo" ) // apiDocsPathRE is the set of API_DOCS_PATH values safe to write raw into an @@ -51,7 +51,7 @@ func APIDocs(cfg *config.Config) (*Route, error) { return &Route{ ID: "core:api-docs", Source: "core", File: "nginx/sites/api-docs.conf", ServerNames: []string{name}, Listen: Listen{HTTP: false, HTTPS: true}, - TLS: &RouteTLS{SSLDir: strings.ReplaceAll(base, ".", "-"), + TLS: &RouteTLS{SSLDir: nginxtopo.CertDirName(base), Protocols: []string{"TLSv1.2", "TLSv1.3"}, Ciphers: ptr(ciphers)}, SecurityHeaders: map[string]string{}, BlockedPaths: []BlockedPath{}, Locations: []Location{ diff --git a/internal/nginx/ssl_detect.go b/internal/nginx/ssl_detect.go index a674a4fec..d38f6885a 100644 --- a/internal/nginx/ssl_detect.go +++ b/internal/nginx/ssl_detect.go @@ -6,7 +6,6 @@ import ( "github.com/nself-org/cli/internal/config" "github.com/nself-org/cli/internal/nginxtopo" - "github.com/nself-org/cli/internal/ssl" ) // ssl_detect.go — decides whether generated nginx site confs should @@ -23,7 +22,7 @@ import ( // generated tree onto a serving nginx that already terminates TLS per // service would have dropped HTTPS stack-wide. // Inputs: cfg (BaseDomain, SSLMode) and workdir (project root — certs live -// under workdir/ssl/certificates//, see internal/ssl.DomainToDirName). +// under workdir/ssl/certificates//, see nginxtopo.CertDirName). // Outputs: sslShouldEmit reports whether nginx should emit the 443/ // ssl_certificate block; sslCertsPresent reports raw cert-file presence. // Constraints: see sslShouldEmit's own doc comment for the per-mode rules. @@ -85,19 +84,14 @@ func sslAssumed(cfg *config.Config, assumed []string) bool { // internal/ssl.Generator writes to and RenderServiceRoute reads from. // sslDirName converts a domain to the directory-safe name nginx's // ssl_certificate directives are rooted at. Delegates to -// internal/ssl.DomainToDirName — the same function internal/ssl.Generator -// uses to decide where on disk it writes certs — so the generated nginx -// path and the actual cert location can never independently drift (they -// used to be two hand-typed copies of the same replace-dots-with-dashes -// rule, one per package, kept in sync only by a comment cross-reference). -// The empty-domain fallback stays local to this package: BaseDomain is -// never empty by the time a real Build() runs (ApplyDefaults fills it), -// but generator tests and ad-hoc callers may still pass "". +// nginxtopo.CertDirNameOrLocalhost — CertDirName is the function +// internal/ssl.Generator uses to decide where on disk it writes certs — so the +// generated nginx path and the actual cert location can never independently +// drift. BaseDomain is never empty by the time a real Build() runs +// (ApplyDefaults fills it), but generator tests and ad-hoc callers may still +// pass "", which falls back to "localhost". func sslDirName(domain string) string { - if domain == "" { - return "localhost" - } - return ssl.DomainToDirName(domain) + return nginxtopo.CertDirNameOrLocalhost(domain) } func sslCertsPresent(cfg *config.Config, workdir string) bool { diff --git a/internal/nginxtopo/ssl_path.go b/internal/nginxtopo/ssl_path.go index bc7423b57..51276434e 100644 --- a/internal/nginxtopo/ssl_path.go +++ b/internal/nginxtopo/ssl_path.go @@ -11,12 +11,34 @@ package nginxtopo // internal/compose) with nothing forcing them to agree. NginxSSLContainerPath // lives here — a package with zero internal dependencies, so both can import // it without a cycle — as the single place either would need to change. -// Inputs: none (a constant). +// Inputs: none for the constant; a domain for CertDirName. // Outputs: NginxSSLContainerPath, the absolute in-container directory nginx's -// TLS material is mounted at. +// TLS material is mounted at, and the certificate directory name of a domain. // Constraints: this is the MOUNT TARGET only. The certificate subdirectory // name under "/certificates/" is a second, separate -// question — see internal/ssl.DomainToDirName, the one implementation of +// question — see CertDirName below, the one implementation of // domain-to-directory-name both internal/ssl (which writes the certs) and -// internal/nginx (which references them) now share. +// internal/nginx (which references them) share. + +import "strings" + const NginxSSLContainerPath = "/etc/nginx/ssl" + +// CertDirName is the directory name under "/certificates/" +// a domain's certificate lives in: dots become dashes. It is the one domain to +// directory rule: internal/ssl (which writes the certs), internal/nginx and +// internal/nginx/routemodel (which reference them) and the plugin snippet +// ${SSL_DIR} all call it, so the copies cannot drift (D-0573). +func CertDirName(domain string) string { + return strings.ReplaceAll(domain, ".", "-") +} + +// CertDirNameOrLocalhost is CertDirName with the empty-domain fallback +// "localhost", for readers of the certificate dir (BaseDomain is never empty +// in a real build, but generator tests and ad-hoc callers may pass ""). +func CertDirNameOrLocalhost(domain string) string { + if dir := CertDirName(domain); dir != "" { + return dir + } + return "localhost" +} diff --git a/internal/nginxtopo/ssl_path_test.go b/internal/nginxtopo/ssl_path_test.go new file mode 100644 index 000000000..703f6ce3a --- /dev/null +++ b/internal/nginxtopo/ssl_path_test.go @@ -0,0 +1,19 @@ +package nginxtopo + +import "testing" + +func TestCertDirName(t *testing.T) { + for _, c := range []struct{ in, dir, orLocalhost string }{ + {"local.nself.org", "local-nself-org", "local-nself-org"}, + {"example.test", "example-test", "example-test"}, + {"localhost", "localhost", "localhost"}, + {"", "", "localhost"}, + } { + if got := CertDirName(c.in); got != c.dir { + t.Errorf("CertDirName(%q) = %q, want %q", c.in, got, c.dir) + } + if got := CertDirNameOrLocalhost(c.in); got != c.orLocalhost { + t.Errorf("CertDirNameOrLocalhost(%q) = %q, want %q", c.in, got, c.orLocalhost) + } + } +} diff --git a/internal/repoqa/layering_basis_test.go b/internal/repoqa/layering_basis_test.go index ef1ad310c..5fab5a846 100644 --- a/internal/repoqa/layering_basis_test.go +++ b/internal/repoqa/layering_basis_test.go @@ -25,7 +25,6 @@ var acceptedBasis = acceptedEntries([]edgeEntry{ {From: "internal/doctor", To: "internal/plugin"}, {From: "internal/hasura", To: "internal/database"}, {From: "internal/runner", To: "internal/deploy"}, - {From: "internal/nginx", To: "internal/ssl"}, {From: "internal/trust", To: "internal/ssl"}, {From: "internal/ssl", To: "internal/compose"}, {From: "internal/reconcile", To: "internal/build"}, diff --git a/internal/repoqa/testdata/layers.yaml b/internal/repoqa/testdata/layers.yaml index 6ec8b7024..9ab164555 100644 --- a/internal/repoqa/testdata/layers.yaml +++ b/internal/repoqa/testdata/layers.yaml @@ -100,7 +100,6 @@ edges: - {from: internal/doctor, to: internal/plugin} # P7-PLUG-59 - {from: internal/hasura, to: internal/database} - {from: internal/runner, to: internal/deploy} - - {from: internal/nginx, to: internal/ssl} - {from: internal/trust, to: internal/ssl} - {from: internal/ssl, to: internal/compose} # P7-LIVE-17: lego image from the image lock - {from: internal/reconcile, to: internal/build} diff --git a/internal/ssl/generator_trust.go b/internal/ssl/generator_trust.go index fa8faafa8..5d7976855 100644 --- a/internal/ssl/generator_trust.go +++ b/internal/ssl/generator_trust.go @@ -15,6 +15,8 @@ import ( "path/filepath" "strings" "time" + + "github.com/nself-org/cli/internal/nginxtopo" ) // applyTrustAndHosts performs the mkcert CA trust and /etc/hosts steps. @@ -116,11 +118,10 @@ func hostsManualNote(hostnames []string) string { } // DomainToDirName converts a domain to a directory-safe name by replacing dots with dashes. -// Exported so callers outside this package (cmd/commands' ssl_add.go) can assert -// their own domain-to-directory conversion agrees with this one instead of -// duplicating the literal replacement rule, which could silently diverge. +// It is nginxtopo.CertDirName, kept here for the callers that already import +// this package: one rule for the writer and every reader of the cert dir. func DomainToDirName(domain string) string { - return strings.ReplaceAll(domain, ".", "-") + return nginxtopo.CertDirName(domain) } // fileExists returns true if the path exists and is a regular file. From a21c648f8949451d84f13f3764cc4eef384bcdd3 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 14:06:14 -0400 Subject: [PATCH 5/5] fix(build): refuse include, extends of another file, build.network, undeclared networks and core service names in plugin fragments (D-0490) --- .github/wiki/Compat-V15.md | 1 + .github/wiki/Plugins.md | 1 + internal/build/lockfile_test.go | 6 +- internal/build/plugins_fragment_isolation.go | 119 ++++++++++++++++++ .../build/plugins_fragment_isolation_test.go | 85 +++++++++++++ internal/build/plugins_network_normalize.go | 8 ++ internal/build/plugins_network_resolved.go | 3 + 7 files changed, 220 insertions(+), 3 deletions(-) create mode 100644 internal/build/plugins_fragment_isolation.go create mode 100644 internal/build/plugins_fragment_isolation_test.go diff --git a/.github/wiki/Compat-V15.md b/.github/wiki/Compat-V15.md index 0ee73973b..045b0956b 100644 --- a/.github/wiki/Compat-V15.md +++ b/.github/wiki/Compat-V15.md @@ -68,6 +68,7 @@ The table is generated. Never hand-edit it; run `compat-markers.sh --write-wiki` | D-0437 | doctor only repairs generated files | diagnoses and repairs deferred boot-plugin table tracking | `cmd/commands/doctor_plugin_tables.go` | | D-0478 | PLUGIN_DIR "~/..." used as written, no plugin snippets on default config | "~" expanded, installed plugins' snippets injected | `internal/config/plugin_dir.go` | | D-0484 | pin store ancestor walk warns once and continues | refuses | `sdk/go/remote/knownhosts_compat.go` | +| D-0490 | a fragment may include another file, extend a service of one, build on the host network, join a network it does not declare or redefine a core service (warning) | E129 fails the build | `internal/build/plugins_network_normalize.go` | | D-0491 | replacing another owner's or a core nginx/sites file warns | E055 fails the build | `internal/build/plugin_nginx_collision.go` | | D-0676 | bare stream path streams as path:// with a notice | refused | `internal/backup/stream_dest_compat.go` | | D-0752 | plugin disable never confirmed | prod-class and hand-edit confirmation before the marker is written. | `cmd/commands/plugin_disable_gate.go` | diff --git a/.github/wiki/Plugins.md b/.github/wiki/Plugins.md index 25c0227c0..edc6176f5 100644 --- a/.github/wiki/Plugins.md +++ b/.github/wiki/Plugins.md @@ -22,6 +22,7 @@ The reason a tier was chosen is one of `installed`, `override`, `entitlement`, ` - **Foreign networks (E129).** A fragment may use an external network only if it is the project network (`${DOCKER_NETWORK}`, or `_network`); aliases and merge keys are resolved before this check, `external` written as a string (`"true"`, `yes`, `${VAR}`) counts as external, and a network with an explicit `name:` that is not project-scoped counts too (the name must be `${COMPOSE_PROJECT_NAME}__`, and the suffix is not `default`), because compose would join an existing network of that name. Any other external network fails the build in v1.5 mode (E129 names the plugin and network) and warns in v1.4 mode. Other v1.5 network cases, each E129 with its own cause in the message (v1.4 warns for the first two and ignores the rest): - A fragment with more than one YAML document (`---`): compose merges them all, the check reads the first. - A service `network_mode` other than `none` or `service:` (`host`, `container:x`, a network name, a variable). + - A fragment that reaches outside its own stack (D-0490; v1.4 warns): a top-level `include:`, a service that `extends` a service of another `file:`, a `build.network` other than `none` or `default`, a service that lists a network the fragment does not declare (other than the project network or `default`), or a service named like a core service (`postgres`, `hasura`, `auth`, `nginx`). - A service supplied by a `<<` merge key under `services:` with no `networks:`: it has no place to attach the project network, so write it out. - A fragment that decodes as YAML but not as compose values, so the policy could not be checked. - A legacy project network written in a form the build cannot rewrite in place (see Networks). diff --git a/internal/build/lockfile_test.go b/internal/build/lockfile_test.go index 738ace4d6..6b225d6d5 100644 --- a/internal/build/lockfile_test.go +++ b/internal/build/lockfile_test.go @@ -578,11 +578,11 @@ func TestLockUsesRediscoveredComposeFiles(t *testing.T) { lockBuild(t, f) frag := filepath.Join(f.plugins, "tweak") writeFixtureFile(t, filepath.Join(frag, "plugin.json"), `{"name":"tweak","port":3999,"language":"go"}`, 0o644) - writeFixtureFile(t, filepath.Join(frag, pluginComposeFilename), "services:\n hasura:\n image: hasura/graphql-engine:v2.51.0\n", 0o644) + writeFixtureFile(t, filepath.Join(frag, pluginComposeFilename), "services:\n redis:\n image: redis:7.4.1\n", 0o644) lockBuild(t, f) l := readLock(t, f) - if got := l.Unpinned["hasura"] + l.Images["hasura"]; got != "hasura/graphql-engine:v2.51.0" { - t.Fatalf("a fragment added in this build is missing from the lock: hasura %q", got) + if got := l.Unpinned["redis"] + l.Images["redis"]; got != "redis:7.4.1" { + t.Fatalf("a fragment added in this build is missing from the lock: redis %q", got) } cur, err := CurrentLock(f.workdir) if err != nil { diff --git a/internal/build/plugins_fragment_isolation.go b/internal/build/plugins_fragment_isolation.go new file mode 100644 index 000000000..f35d80e5b --- /dev/null +++ b/internal/build/plugins_fragment_isolation.go @@ -0,0 +1,119 @@ +package build + +// Purpose: the fragment rules that keep a plugin's compose fragment inside its +// own stack, beside the network rules of plugins_network_resolved.go (D-0490). +// Inputs: the resolved fragment (aliases and merge keys expanded). +// Outputs: one violation string per breach, sorted; empty when the fragment +// keeps to itself. +// Constraints: each rule is a fact about the decoded fragment, never about +// the bytes, so a merge key or an alias cannot hide a breach. A breach is an +// E129 in v1.5 and a warning in v1.4 (plugins_network_normalize.go). + +import ( + "fmt" + "sort" + "strings" + + "github.com/nself-org/cli/internal/compose" +) + +// isolationViolations lists what the fragment does to reach outside itself: +// - include: pulls another compose file in, past every rule here; +// - extends with a file: inherits a service of another file, including its +// network_mode; +// - build.network other than none or default: the image build joins the +// host network or one by name; +// - a service listing a network its own fragment does not declare and that +// is not the project network: after the merge it joins the network of +// another plugin; +// - a service named like a core service: it merges into that service and +// is given the plugin core env. +func (r *resolvedFragment) isolationViolations() []string { + var out []string + if r.Include != nil { + out = append(out, "top-level include") + } + declared := map[string]bool{} + for key := range r.Networks { + declared[key] = true + } + core := coreServiceNames() + for name, s := range r.Services { + if core[name] { + out = append(out, fmt.Sprintf("%s: defines the core service %s", name, name)) + } + if s.hasExtendsFile() { + out = append(out, fmt.Sprintf("%s: extends another file", name)) + } + if mode := s.buildNetwork(); mode != "" && mode != "none" && mode != "default" { + out = append(out, fmt.Sprintf("%s: build.network %s", name, mode)) + } + for _, key := range s.networkKeys() { + if !declared[key] && !isProjectNetworkKey(key) { + out = append(out, fmt.Sprintf("%s: network %s is not declared by this fragment", name, key)) + } + } + } + sort.Strings(out) + return out +} + +// coreServiceNames is the set of services every stack carries; a plugin +// fragment may not define one. +func coreServiceNames() map[string]bool { + names := map[string]bool{} + for _, e := range compose.CoreServices() { + names[e.Name] = true + } + return names +} + +// isProjectNetworkKey reports whether key names the project network (or +// compose's own per-project default), which every service may join. +func isProjectNetworkKey(key string) bool { + return key == "default" || projectNetworkRE.MatchString(key) || isLegacyNetwork(key) +} + +// hasExtendsFile reports whether the service extends a service of another +// file. A short or unreadable extends is treated as one (fail closed); an +// extends of a service in the same file inherits nothing the fragment's own +// checks do not already see. +func (s resolvedService) hasExtendsFile() bool { + switch v := s.Extends.(type) { + case nil: + return false + case map[string]any: + file, ok := v["file"] + return ok && file != nil + } + return true +} + +// buildNetwork returns the lower-cased build.network, "" when the service +// builds no image or sets none. A non-string value comes back as its text. +func (s resolvedService) buildNetwork() string { + b, ok := s.Build.(map[string]any) + if !ok || b["network"] == nil { + return "" + } + return strings.ToLower(strings.TrimSpace(fmt.Sprint(b["network"]))) +} + +// networkKeys returns the network keys the service lists, in list or mapping +// form. +func (s resolvedService) networkKeys() []string { + var out []string + switch v := s.Networks.(type) { + case []any: + for _, e := range v { + if name, ok := e.(string); ok { + out = append(out, name) + } + } + case map[string]any: + for name := range v { + out = append(out, name) + } + } + return out +} diff --git a/internal/build/plugins_fragment_isolation_test.go b/internal/build/plugins_fragment_isolation_test.go new file mode 100644 index 000000000..671611511 --- /dev/null +++ b/internal/build/plugins_fragment_isolation_test.go @@ -0,0 +1,85 @@ +package build + +// Purpose: D-0490 tests for the fragment isolation rules: include, extends of +// another file, build.network, an undeclared network and a core service name +// are E129 in v1.5 (a warning in v1.4), and the first-party fragment shapes +// stay clean. + +import ( + "os" + "path/filepath" + "testing" + + "github.com/nself-org/cli/internal/compat/compattest" +) + +func TestFragmentIsolationRefusals(t *testing.T) { + cases := []struct { + name, frag, want string + }{ + {"include", "include:\n - other.yml\nservices:\n w:\n image: x\n networks: [\"${DOCKER_NETWORK}\"]\n", "top-level include"}, + {"extends file", "services:\n w:\n extends:\n file: ../other.yml\n service: base\n networks: [\"${DOCKER_NETWORK}\"]\n", "w: extends another file"}, + {"extends short form", "services:\n w:\n extends: base\n networks: [\"${DOCKER_NETWORK}\"]\n", "w: extends another file"}, + {"extends via merge key", "x-base: &b\n extends:\n file: ../other.yml\n service: base\nservices:\n w:\n <<: *b\n image: x\n networks: [\"${DOCKER_NETWORK}\"]\n", "w: extends another file"}, + {"build network host", "services:\n w:\n build:\n context: .\n network: host\n networks: [\"${DOCKER_NETWORK}\"]\n", "w: build.network host"}, + {"build network by name", "services:\n w:\n build:\n context: .\n network: HOST\n networks: [\"${DOCKER_NETWORK}\"]\n", "w: build.network host"}, + {"undeclared sibling network", "services:\n w:\n image: x\n networks:\n - ${DOCKER_NETWORK}\n - ${COMPOSE_PROJECT_NAME}_b_private\n", "network ${COMPOSE_PROJECT_NAME}_b_private is not declared"}, + {"undeclared network, mapping form", "services:\n w:\n image: x\n networks:\n b_private: {}\n", "network b_private is not declared"}, + {"core service hasura", "services:\n hasura:\n image: x\n networks: [\"${DOCKER_NETWORK}\"]\n", "hasura: defines the core service hasura"}, + {"core service postgres", "services:\n postgres:\n image: x\n networks: [\"${DOCKER_NETWORK}\"]\n", "postgres: defines the core service postgres"}, + } + for _, c := range cases { + c := c + t.Run(c.name, func(t *testing.T) { + compattest.Both(t, func(t *testing.T) { wantE129(t, c.frag, c.want) }) + }) + } +} + +func TestFragmentIsolationAllowed(t *testing.T) { + cases := map[string]string{ + "project network only": "services:\n w:\n image: x\n networks: [\"${DOCKER_NETWORK}\"]\n", + "project network default": "services:\n w:\n image: x\n networks: [\"${DOCKER_NETWORK:-x}\", default]\n", + "declared own network": "services:\n w:\n image: x\n networks: [\"${DOCKER_NETWORK}\", private]\nnetworks:\n private:\n name: \"${COMPOSE_PROJECT_NAME}_w_private\"\n", + "same-file extends": "services:\n base:\n image: x\n networks: [\"${DOCKER_NETWORK}\"]\n w:\n extends:\n service: base\n", + "build network none": "services:\n w:\n build:\n context: .\n network: none\n networks: [\"${DOCKER_NETWORK}\"]\n", + "build without network": "services:\n w:\n build: .\n networks: [\"${DOCKER_NETWORK}\"]\n", + "optional service name": "services:\n redis:\n image: redis:7\n networks: [\"${DOCKER_NETWORK}\"]\n", + } + for name, frag := range cases { + frag := frag + t.Run(name, func(t *testing.T) { + r, err := resolveFragment([]byte(frag)) + if err != nil { + t.Fatal(err) + } + if v := r.isolationViolations(); len(v) != 0 { + t.Errorf("violations %q on a fragment that stays inside its stack", v) + } + }) + } +} + +// TestFragmentIsolationFirstPartyFixtures: the recorded first-party fragments +// break none of the rules. +func TestFragmentIsolationFirstPartyFixtures(t *testing.T) { + for _, glob := range []string{"testdata/plugin-compose-fixtures/*.yml", "testdata/legacy_networks/*.yml"} { + files, err := filepath.Glob(glob) + if err != nil || len(files) == 0 { + t.Fatalf("no fixtures for %s (err=%v)", glob, err) + } + for _, p := range files { + b, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + r, err := resolveFragment(b) + if err != nil { + t.Fatalf("%s: %v", p, err) + } + if v := r.isolationViolations(); len(v) != 0 { + t.Errorf("%s: %q", p, v) + } + } + } +} diff --git a/internal/build/plugins_network_normalize.go b/internal/build/plugins_network_normalize.go index 6df871065..0c3c520dc 100644 --- a/internal/build/plugins_network_normalize.go +++ b/internal/build/plugins_network_normalize.go @@ -136,6 +136,14 @@ func normalizeComposeNetworks(content []byte, pluginName string) ([]byte, error) } warnOnce(msg) } + if bad := res.isolationViolations(); len(bad) > 0 { + msg := fmt.Sprintf("plugin %q reaches outside its own fragment (%s)", pluginName, strings.Join(bad, ", ")) + // compat.V15(D-0490): a fragment may include another file, extend a service of one, build on the host network, join a network it does not declare or redefine a core service (warning) -> E129 fails the build + if compat.V15() { + return content, policyError(msg, "include, extends of another file, build.network, a network the fragment does not declare and a core service name each pull in or merge with something outside the plugin's own stack, past the network policy.", "Keep the fragment to its own services and networks, or ask the plugin author to.") + } + warnOnce(msg) + } // compat.V15(P7-PLUG-17): fragments keep their own networks and ${DOCKER_NETWORK:-x} -> missing networks attached, ${DOCKER_NETWORK:-x} written as ${DOCKER_NETWORK} if !compat.V15() { return content, nil diff --git a/internal/build/plugins_network_resolved.go b/internal/build/plugins_network_resolved.go index 72becbb31..38b47fcd4 100644 --- a/internal/build/plugins_network_resolved.go +++ b/internal/build/plugins_network_resolved.go @@ -32,12 +32,15 @@ type resolvedNetwork struct { type resolvedService struct { Networks any `yaml:"networks"` NetworkMode any `yaml:"network_mode"` + Extends any `yaml:"extends"` + Build any `yaml:"build"` } // resolvedFragment is a fragment with aliases and merge keys expanded. type resolvedFragment struct { Services map[string]resolvedService `yaml:"services"` Networks map[string]resolvedNetwork `yaml:"networks"` + Include any `yaml:"include"` } // resolveFragment decodes content; an error means it cannot be decoded.