From 8a303ec06ba8b10843ef9e3b95f3df44eaa13b65 Mon Sep 17 00:00:00 2001 From: Thando Mini Date: Thu, 11 Jun 2026 13:11:58 +0200 Subject: [PATCH] fix(web,update): PathEscape ollama model names; respect NXD_NO_BROWSER MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two low-severity hardening fixes from the 2026-06-11 security audit, bundled because they live across two tiny files. - ollamaRemoteDigest interpolated the model name and tag directly into the /v2/library//manifests/ URL. An operator-supplied model name with a slash (e.g. accidental "library/gemma4") would traverse into a different registry API subtree; a tag with spaces produced a malformed URL and a confusing 400. Wrap both segments with url.PathEscape so the URL structure is invariant under any config value. - openBrowser fired `open` / `xdg-open` with the full token-bearing URL as a process argument. On most systems `ps` is world-readable by every local user — multi-tenant CI runners and shared dev hosts could leak the auth token to other users on the machine. Add NXD_NO_BROWSER=1 opt-out so operators in headless / SSH / CI environments can suppress the launch. The URL is still printed via log.Printf for operator discovery. Surfaced by the 2026-06-11 security audit (SEC-L1, SEC-L2). New tests: - TestOllamaRemoteDigest_PathEscapesModelName captures r.URL.EscapedPath() and asserts a slashed model name comes through as %2F instead of segment-splitting. - TestOpenBrowser_RespectsNxdNoBrowser sets NXD_NO_BROWSER and runs the function (no panic, no leaked process). --- internal/update/checker.go | 8 +++++- internal/update/checker_test.go | 42 ++++++++++++++++++++++++++++++++ internal/web/openbrowser_test.go | 24 ++++++++++++++++++ internal/web/server.go | 10 ++++++++ 4 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 internal/web/openbrowser_test.go diff --git a/internal/update/checker.go b/internal/update/checker.go index 257fe5d..fb426d6 100644 --- a/internal/update/checker.go +++ b/internal/update/checker.go @@ -8,6 +8,7 @@ import ( "io" "log" "net/http" + urlpkg "net/url" "strings" "time" ) @@ -141,7 +142,12 @@ func (c *Checker) ollamaLocalDigest(ctx context.Context, model string) (string, func (c *Checker) ollamaRemoteDigest(ctx context.Context, model string) (string, error) { name, tag := parseOllamaModel(model) - url := fmt.Sprintf("%s/v2/library/%s/manifests/%s", c.ollamaRegistryURL, name, tag) + // PathEscape so a model name containing `/` (e.g. accidental + // "library/gemma4") can't traverse into a different URL subtree, and so + // any operator-supplied tag with special chars produces a well-formed + // URL instead of an obscure 404 / 400. + url := fmt.Sprintf("%s/v2/library/%s/manifests/%s", + c.ollamaRegistryURL, urlpkg.PathEscape(name), urlpkg.PathEscape(tag)) req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { diff --git a/internal/update/checker_test.go b/internal/update/checker_test.go index 4cc19ac..2aa2635 100644 --- a/internal/update/checker_test.go +++ b/internal/update/checker_test.go @@ -35,6 +35,48 @@ func newOllamaMocks(t *testing.T, localDigest, remoteDigest string) (local *http return local, remote } +// TestOllamaRemoteDigest_PathEscapesModelName guards SEC-L1: the model name +// and tag are now url.PathEscape'd before being interpolated into the +// /v2/library//manifests/ URL. A model name with `/` no longer +// traverses into a different registry subtree; a tag with special chars +// produces a well-formed URL instead of a 400. +func TestOllamaRemoteDigest_PathEscapesModelName(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + var seenPath string + local, _ := newOllamaMocks(t, "sha256:local", "sha256:remote") + remote := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Use EscapedPath so percent-encoded segments stay visible — + // r.URL.Path is the decoded form which would hide the escaping. + seenPath = r.URL.EscapedPath() + resp := map[string]string{"digest": "sha256:remote"} + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) + })) + t.Cleanup(remote.Close) + + c := NewChecker( + WithOllamaLocalURL(local.URL), + WithOllamaRegistryURL(remote.URL), + ) + + // Operator-controlled config could include slashes — confirm they don't + // rewrite the URL path structure. + if _, err := c.CheckOllama(ctx, []string{"weird/name:tag with space"}); err != nil { + t.Fatalf("CheckOllama: %v", err) + } + // `weird/name` becomes `weird%2Fname`; `tag with space` becomes + // `tag%20with%20space`. The crucial invariant: no second "/library/" + // segment appears, and the URL has exactly the expected hop count. + if strings.Contains(seenPath, "weird/name") { + t.Errorf("model name slash not escaped, path = %q", seenPath) + } + if !strings.Contains(seenPath, "weird%2Fname") { + t.Errorf("expected percent-encoded slash, path = %q", seenPath) + } +} + func TestCheckOllama_UpdateAvailable(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() diff --git a/internal/web/openbrowser_test.go b/internal/web/openbrowser_test.go new file mode 100644 index 0000000..b92b386 --- /dev/null +++ b/internal/web/openbrowser_test.go @@ -0,0 +1,24 @@ +package web + +import ( + "testing" +) + +// TestOpenBrowser_RespectsNxdNoBrowser guards SEC-L2: when the operator sets +// NXD_NO_BROWSER, openBrowser must NOT spawn `open` / `xdg-open`. The auth +// token is embedded in the URL, and process args are world-readable via +// `ps` on most multi-tenant systems — operators in headless / CI / SSH +// environments should be able to suppress the launch entirely. +// +// The function returns void so we assert on side effects: a non-empty +// envvar must short-circuit the exec.Command call. With NXD_NO_BROWSER +// unset on the CI runner the actual exec would attempt to fork — and that +// would noise up other tests via stderr. Asserting the no-op path is +// sufficient: the post-condition is "no crash, no fork." +func TestOpenBrowser_RespectsNxdNoBrowser(t *testing.T) { + t.Setenv("NXD_NO_BROWSER", "1") + // Must not panic, must not block, must not fork. A regression that + // removed the short-circuit would surface here as a leaked process or + // a stderr spew from the missing `open` binary on CI. + openBrowser("http://127.0.0.1:0/?token=test") +} diff --git a/internal/web/server.go b/internal/web/server.go index 686bbb3..adcf386 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -12,6 +12,7 @@ import ( "log" "net" "net/http" + "os" "os/exec" "runtime" "sync" @@ -193,6 +194,15 @@ func (s *Server) Start(ctx context.Context) error { } func openBrowser(url string) { + // CI / headless / SSH sessions don't have a browser, and the URL we + // pass to `open` / `xdg-open` contains the auth token. Process args + // are world-readable via `ps` on most systems, so leaking the token + // to every local user is a real concern in multi-tenant environments. + // Opt-out: set NXD_NO_BROWSER=1 to suppress the launch entirely. The + // URL is still printed via log.Printf for operator discovery. + if os.Getenv("NXD_NO_BROWSER") != "" { + return + } var cmd *exec.Cmd switch runtime.GOOS { case "darwin":