From 01ed8ab25ecd31b3ccabdbbb9426967964c66f32 Mon Sep 17 00:00:00 2001 From: Ilia Zhuravok Date: Mon, 24 Aug 2026 11:47:18 +0100 Subject: [PATCH 1/3] feat(sandbox,cli): project a credential-free registry config into the sandbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A scoped package resolves through a mapping in ~/.npmrc (@acme:registry=...). That file is a protected path because it also commonly holds an auth token, so inside the sandbox npm falls back to the public registry and 404s — a failure that reads like "no such package", leaves no denial event, and is invisible to `omac diagnose`. filesystem.registry_config: ["npm"] opts a profile into a projection: omac derives a copy of ~/.npmrc holding only registry mappings, grants read access to that copy alone, and points npm at it via NPM_CONFIG_USERCONFIG. The host file stays masked, so unlike override_deny no credential is exposed. Unset keeps today's behavior. `omac doctor` now reports a private mapping the sandbox cannot see, and warns when override_deny is doing a job the projection does without the exposure. Verified on Linux/bwrap against the case in #241, one binary, cold cache: knob off -> 0 skainet models (only ALLOW registry.npmjs.org) knob on -> 14 skainet models, installs @tngtech/opencode-skainet 1.0.3 ALLOW tng-artifacts.int.tngtech.com, ALLOW chat.model.tngtech.com inside the sandbox, with the knob on: cat ~/.npmrc -> No such file or directory cat $NPM_CONFIG_USERCONFIG -> the mapping line only omac doctor -> "[warn] registry config: ~/.npmrc maps a scope to tng-artifacts.int.tngtech.com, but the sandbox cannot read it" go build ./... && go test ./... pass except TestIntegrationWorktreeKnownLimitations and TestIntegrationWorkflowInterpretersRunnable, which fail identically on clean main on this host (local toolchain paths absent from the default profile). Closes #150 Refs #241 Co-Authored-By: Claude Opus 5 Signed-off-by: Ilia Zhuravok --- docs/CONFIGURATION.md | 33 ++- docs/SECURITY_MODEL.md | 1 + internal/cli/doctor.go | 66 +++++ internal/cli/doctor_registryconf_test.go | 140 ++++++++++ internal/registryconf/registryconf.go | 305 +++++++++++++++++++++ internal/registryconf/registryconf_test.go | 267 ++++++++++++++++++ internal/sandboxprofile/profile.go | 33 +++ internal/sandboxprofile/profile_test.go | 29 +- internal/sandboxrun/registryconf.go | 68 +++++ internal/sandboxrun/registryconf_test.go | 158 +++++++++++ internal/sandboxrun/run.go | 10 + 11 files changed, 1105 insertions(+), 5 deletions(-) create mode 100644 internal/cli/doctor_registryconf_test.go create mode 100644 internal/registryconf/registryconf.go create mode 100644 internal/registryconf/registryconf_test.go create mode 100644 internal/sandboxrun/registryconf.go create mode 100644 internal/sandboxrun/registryconf_test.go diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index be452a39..ae2ccc9d 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -124,11 +124,42 @@ Profile fields: `workdir.access` (none/read/write/readwrite), expansion), `filesystem.deny` (mask files inside granted trees — a bare name like `.env` or `*.key` is denied in every granted directory, the working directory included), `filesystem.override_deny` (punch -holes in the built-in protected-path list), `network.mode` +holes in the built-in protected-path list), +`filesystem.registry_config` (see below), `network.mode` (filtered/blocked/open), `network.network_prompt`, `network.proxy_injection`, and `environment.allow_vars`. See the scaffolded `default.json` for the full schema. +### Private package registries (`filesystem.registry_config`) + +A scoped package resolves through a mapping in your package manager's +user config — `@acme:registry=https://npm.acme.test` in `~/.npmrc`. That +file is a protected path, because it also commonly holds an auth token. +Masked, it makes npm fall back to the public registry, where the package +does not exist: the install fails with a **404 that reads like "no such +package"** rather than "your registry configuration is invisible". +Allowlisting the registry host does not help — the tool never asks it. + +Opting in projects a credential-free copy instead: + +```json +{ "filesystem": { "registry_config": ["npm"] } } +``` + +At launch omac derives a copy of `~/.npmrc` holding **only** registry +mappings, grants read access to that copy alone, and points npm at it via +`NPM_CONFIG_USERCONFIG`. Every other entry is dropped, and inline URL +credentials (`https://user:pass@host`) are stripped, so no secret can +reach the sandbox. The host file stays masked. + +Private registries usually also need their host in +`network.allow_domain` (or an allow at the network prompt). + +The blunt alternative — `override_deny: ["~/.npmrc"]` — also works, but +grants the whole file including any token. `omac doctor` flags a private +mapping the sandbox cannot see, and warns when `override_deny` is doing +a job the projection would do without the exposure. + When a host is neither allowed nor denied by the profile, the network prompt dialog offers eleven choices (`Deny once` is preselected): diff --git a/docs/SECURITY_MODEL.md b/docs/SECURITY_MODEL.md index 9c1e5eb7..37a8777e 100644 --- a/docs/SECURITY_MODEL.md +++ b/docs/SECURITY_MODEL.md @@ -210,6 +210,7 @@ sandbox: | Bridge socket (`$TMPDIR/omac-/bridge.sock`) | read+write | `--allow-file` / `--read` flags | | Dynamic socket dir (e.g. Agent View `/tmp/cc-daemon-`) | read+write + AF_UNIX connect | `--allow-unix-dir` flag / `filesystem.allow_unix_dir` | | Paths in `~/.ssh`, `~/.gnupg`, `~/.aws`, `~/.kube`, … | **denied** | protected paths (override with `filesystem.override_deny`) | +| `~/.npmrc` | **denied**; registry mappings available as a scrubbed copy | protected path; opt in with `filesystem.registry_config: ["npm"]` — projects mappings only, drops every credential, so `override_deny` is not needed ([configuration](CONFIGURATION.md#private-package-registries-filesystemregistry_config)) | | `~/.config/omac` (skill approval store, sandbox profiles, global registry) | **not mounted** | never granted — the host-only anchor for [skill spawn approval](#self-authored-skills) | | Workdir and granted-tree `.env` / `.envrc` (incl. nested) | **denied** | baseline workdir-protected set (override with `filesystem.override_deny: [".env"]`) | | Files matching `filesystem.deny` (e.g. `*.key`) inside granted trees | **denied** | user deny list (`filesystem.deny` / `--deny`) | diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index b32e554f..cae12e7c 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "slices" "strings" "github.com/tngtech/oh-my-agentic-coder/internal/builtinskills" @@ -15,6 +16,7 @@ import ( "github.com/tngtech/oh-my-agentic-coder/internal/osinfo" "github.com/tngtech/oh-my-agentic-coder/internal/profileaudit" "github.com/tngtech/oh-my-agentic-coder/internal/registry" + "github.com/tngtech/oh-my-agentic-coder/internal/registryconf" "github.com/tngtech/oh-my-agentic-coder/internal/sandboxprofile" "github.com/tngtech/oh-my-agentic-coder/internal/sandboxrun" ) @@ -171,6 +173,11 @@ func runDoctor(args []string, env *Env) int { // that policy linted rather than an unused "default". doctorProfileLint(env, defaultPolicyRef(lc)) + // Advisory: a private-registry mapping the sandbox cannot see makes + // scoped installs 404 with no denial anywhere to point at, so nothing + // else in doctor or diagnose would mention it. + doctorRegistryConfig(env, defaultPolicyRef(lc)) + fmt.Fprintln(env.Stdout, "\nWhen a run fails, `omac diagnose` shows what the sandbox blocked and why.") if failures > 0 { @@ -199,6 +206,65 @@ func doctorProfileLint(env *Env, profileRef string) { } } +// doctorRegistryConfig reports whether ~/.npmrc maps a scope to a private +// registry that the sandbox cannot see. That combination fails in a way no +// other check catches: the masked file yields no denial event, and npm's +// fallback to the public registry returns a plain 404 that reads like "no +// such package" (see #150, #241). +// +// Advisory only — it never affects doctor's exit code. +func doctorRegistryConfig(env *Env, profileRef string) { + profile, _, err := sandboxprofile.Resolve(profileRef) + if err != nil { + return // profile problems are already reported by the sandbox section + } + enabled := slices.Contains(profile.Filesystem.RegistryConfig, sandboxprofile.RegistryConfigNPM) + src, err := registryconf.NPMUserConfig() + if err != nil { + return + } + overridden := sandboxprofile.BuildOverrideLookup(profile.Filesystem.OverrideDeny)[src] + + notice, err := registryconf.InspectNPM(enabled, overridden) + if err != nil || notice == nil { + return + } + hosts := strings.Join(notice.Hosts, ", ") + switch { + case notice.Enabled: + fmt.Fprintf(env.Stdout, "[ok] registry config: %s mappings (%s) are projected into the sandbox\n", + notice.Ecosystem, hosts) + case notice.Overridden: + fmt.Fprintf(env.Stdout, "[warn] registry config: %s is exposed to the sandbox via filesystem.override_deny\n", + notice.Source) + fmt.Fprintf(env.Stdout, " That grants the whole file%s. Prefer filesystem.registry_config: [%q],\n", + credentialSuffix(notice.Credentialed), notice.Ecosystem) + fmt.Fprintf(env.Stdout, " which projects only the registry mappings (%s) and drops every credential.\n", hosts) + default: + fmt.Fprintf(env.Stdout, "[warn] registry config: %s maps a scope to %s, but the sandbox cannot read it\n", + notice.Source, hosts) + fmt.Fprintf(env.Stdout, " Scoped installs will fail with a 404 against the public registry. Fix:\n") + fmt.Fprintf(env.Stdout, " add filesystem.registry_config: [%q] to the sandbox profile%s.\n", + notice.Ecosystem, credentialNote(notice.Credentialed)) + } +} + +// credentialSuffix describes what an override_deny grant exposes. +func credentialSuffix(credentialed bool) string { + if credentialed { + return ", including the auth token it holds" + } + return "" +} + +// credentialNote explains why the projection beats the blunt alternative. +func credentialNote(credentialed bool) string { + if credentialed { + return " (the file also holds an auth token, so override_deny would expose it)" + } + return "" +} + // doctorBuiltinSkills reports whether omac's built-in skills (provisioned by // `omac setup`) are present and current in each installed harness's native // skills dir. It is advisory: a missing/stale/foreign bundle is a warning, not diff --git a/internal/cli/doctor_registryconf_test.go b/internal/cli/doctor_registryconf_test.go new file mode 100644 index 00000000..6398ba60 --- /dev/null +++ b/internal/cli/doctor_registryconf_test.go @@ -0,0 +1,140 @@ +package cli + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// stageDoctorNpmrc stages a HOME + workdir + default profile and writes the +// given npmrc body, then runs doctor and returns its output. profileJSON is +// the sandbox profile the launcher's default template points at. +func stageDoctorNpmrc(t *testing.T, npmrc, profileJSON string) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + workdir := t.TempDir() + + writeWorkdirConfig(t, workdir, "builtin", []string{ + "{{self}}", "sandbox", "run", + "--profile", "default", + "--", "{{inner_cmd}}", "{{inner_args}}", + }) + stageProfile(t, home, profileJSON) + + if npmrc != "" { + if err := os.WriteFile(filepath.Join(home, ".npmrc"), []byte(npmrc), 0o600); err != nil { + t.Fatal(err) + } + } + + env, outBuf, _, drain := newPipeEnv(t, "") + env.Workdir = workdir + if code := runDoctor([]string{}, env); code != ExitOK { + t.Errorf("doctor exit = %d, want ExitOK (advisory)", code) + } + drain() + return outBuf.String() +} + +// TestDoctorRegistryConfigWarnsOnInvisibleMapping is the #241 shape: a scope +// mapped to a private registry that the sandbox cannot read. +func TestDoctorRegistryConfigWarnsOnInvisibleMapping(t *testing.T) { + out := stageDoctorNpmrc(t, + "@acme:registry=https://npm.acme.test\n", + `{"meta": {"name": "default"}, "environment": {"allow_vars": ["HOME"]}}`) + + if !strings.Contains(out, "registry config") || !strings.Contains(out, "npm.acme.test") { + t.Errorf("doctor did not report the invisible mapping; got:\n%s", out) + } + if !strings.Contains(out, "404") { + t.Errorf("doctor did not explain the 404 symptom; got:\n%s", out) + } + if !strings.Contains(out, `registry_config: ["npm"]`) { + t.Errorf("doctor did not name the fix; got:\n%s", out) + } +} + +// TestDoctorRegistryConfigQuietWhenEnabled asserts the check confirms rather +// than nags once the profile opts in. +func TestDoctorRegistryConfigQuietWhenEnabled(t *testing.T) { + out := stageDoctorNpmrc(t, + "@acme:registry=https://npm.acme.test\n", + `{"meta": {"name": "default"}, "filesystem": {"registry_config": ["npm"]}, "environment": {"allow_vars": ["HOME"]}}`) + + if !strings.Contains(out, "[ok] registry config") { + t.Errorf("doctor did not confirm the projection; got:\n%s", out) + } + if strings.Contains(out, "404") { + t.Errorf("doctor still warned with registry_config enabled; got:\n%s", out) + } +} + +// TestDoctorRegistryConfigFlagsOverrideDenyExposure asserts the blunt remedy +// is called out as exposing the token the projection would have dropped. +func TestDoctorRegistryConfigFlagsOverrideDenyExposure(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + workdir := t.TempDir() + writeWorkdirConfig(t, workdir, "builtin", []string{ + "{{self}}", "sandbox", "run", + "--profile", "default", + "--", "{{inner_cmd}}", "{{inner_args}}", + }) + // override_deny is matched on the expanded path, so "~/.npmrc" resolves + // to the staged HOME. + stageProfile(t, home, `{ + "meta": {"name": "default"}, + "filesystem": {"override_deny": ["~/.npmrc"]}, + "environment": {"allow_vars": ["HOME"]} + }`) + body := "@acme:registry=https://npm.acme.test\n//npm.acme.test/:_authToken=SECRET\n" + if err := os.WriteFile(filepath.Join(home, ".npmrc"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + env, outBuf, _, drain := newPipeEnv(t, "") + env.Workdir = workdir + runDoctor([]string{}, env) + drain() + out := outBuf.String() + + if !strings.Contains(out, "override_deny") { + t.Errorf("doctor did not mention the override_deny exposure; got:\n%s", out) + } + if !strings.Contains(out, "auth token") { + t.Errorf("doctor did not warn that the token is exposed; got:\n%s", out) + } + if strings.Contains(out, "SECRET") { + t.Fatalf("doctor echoed secret material; got:\n%s", out) + } +} + +// TestDoctorRegistryConfigSilentWithoutPrivateMapping keeps the check from +// becoming noise: no npmrc, or one that only points at npm's own registry, +// must produce no registry-config line at all. +func TestDoctorRegistryConfigSilentWithoutPrivateMapping(t *testing.T) { + profile := `{"meta": {"name": "default"}, "environment": {"allow_vars": ["HOME"]}}` + + t.Run("no npmrc", func(t *testing.T) { + out := stageDoctorNpmrc(t, "", profile) + if strings.Contains(out, "registry config") { + t.Errorf("reported a registry-config finding with no npmrc; got:\n%s", out) + } + }) + + t.Run("default registry only", func(t *testing.T) { + out := stageDoctorNpmrc(t, "registry=https://registry.npmjs.org\n", profile) + if strings.Contains(out, "registry config") { + t.Errorf("reported a finding for the default registry; got:\n%s", out) + } + }) + + t.Run("credentials only", func(t *testing.T) { + out := stageDoctorNpmrc(t, "//registry.npmjs.org/:_authToken=SECRET\n", profile) + if strings.Contains(out, "registry config") { + t.Errorf("reported a finding with no mapping to project; got:\n%s", out) + } + }) +} diff --git a/internal/registryconf/registryconf.go b/internal/registryconf/registryconf.go new file mode 100644 index 00000000..b65b958e --- /dev/null +++ b/internal/registryconf/registryconf.go @@ -0,0 +1,305 @@ +// Package registryconf derives a credential-free projection of a package +// manager's user config file, so a sandboxed toolchain can resolve +// scope→registry mappings without the host file ever being readable. +// +// The problem it solves: a scoped package like @acme/foo resolves through +// the `@acme:registry=` line in ~/.npmrc. That file is a protected path +// (internal/sandboxprofile/baseline.go, protectedCommon) because it also +// commonly holds `_authToken`. Masking it entirely makes npm fall back to +// the public registry, where the package does not exist — the install +// fails with a 404 that reads like "no such package" rather than "your +// registry configuration is invisible". See #150 / #241. +// +// No credential can survive by construction: only registry-mapping keys +// are kept, a kept value must parse as an http(s) URL, and any userinfo +// in that URL is removed. +package registryconf + +import ( + "fmt" + "net/url" + "os" + "path/filepath" + "regexp" + "slices" + "strings" + + "github.com/tngtech/oh-my-agentic-coder/internal/sandboxprofile" +) + +// Projection is one scrubbed config file written for the sandboxed child. +type Projection struct { + // Ecosystem is the sandboxprofile.RegistryConfigTools identifier. + Ecosystem string + // Source is the host file the projection was derived from. + Source string + // Path is the scrubbed file. Grant read access to exactly this path. + Path string + // EnvVar points the tool at Path (e.g. NPM_CONFIG_USERCONFIG). + EnvVar string + // KeptKeys are the registry-mapping keys that survived the scrub. + KeptKeys []string + // Dropped counts removed non-comment entries, credentials included. + Dropped int + // StrippedUserinfo counts kept mappings whose URL carried inline + // credentials (https://user:pass@host) that were removed. + StrippedUserinfo int +} + +// Summary renders a one-line, secret-free description for the launch log. +func (p Projection) Summary() string { + extra := "" + if p.StrippedUserinfo > 0 { + extra = fmt.Sprintf(", %d inline credential(s) stripped", p.StrippedUserinfo) + } + return fmt.Sprintf("%s: projected %d registry mapping(s) from %s (%d other entr(ies) dropped%s)", + p.Ecosystem, len(p.KeptKeys), p.Source, p.Dropped, extra) +} + +// projector derives one ecosystem's projection into dir. A missing host +// config is not an error: it returns ok=false and no projection. +type projector func(dir string) (Projection, bool, error) + +// projectors maps each registry_config ecosystem to its implementation. +// Adding an ecosystem (.pypirc, cargo credentials) is one entry here plus +// one in sandboxprofile.RegistryConfigTools; TestProjectorsCoverProfileTools +// asserts the two stay in sync. +var projectors = map[string]projector{ + sandboxprofile.RegistryConfigNPM: projectNPM, +} + +// Project writes a scrubbed config for every requested ecosystem into dir +// and returns what it produced. Ecosystems are pre-validated by +// sandboxprofile.Profile.Validate, so an unknown one is a programming +// error rather than user input. +func Project(ecosystems []string, dir string) ([]Projection, error) { + var out []Projection + for _, eco := range ecosystems { + proj, ok := projectors[eco] + if !ok { + return nil, fmt.Errorf("registry_config: no projector for %q", eco) + } + p, present, err := proj(dir) + if err != nil { + return nil, err + } + if !present { + continue + } + out = append(out, p) + } + return out, nil +} + +// NPMUserConfig returns the host file npm reads its user config from. +// Exported so the doctor-side detector inspects exactly the same path the +// projector would read. +func NPMUserConfig() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", err + } + return filepath.Join(home, ".npmrc"), nil +} + +func projectNPM(dir string) (Projection, bool, error) { + src, err := NPMUserConfig() + if err != nil { + return Projection{}, false, fmt.Errorf("registry_config npm: %w", err) + } + raw, err := os.ReadFile(src) + if err != nil { + if os.IsNotExist(err) { + return Projection{}, false, nil + } + return Projection{}, false, fmt.Errorf("registry_config npm: read %s: %w", src, err) + } + res := ScrubNPMRC(raw) + if len(res.KeptKeys) == 0 { + // No mapping means npm's default resolution is already correct; + // writing an empty file would add a grant for no reason. + return Projection{}, false, nil + } + dest := filepath.Join(dir, "npmrc") + if err := os.WriteFile(dest, res.Content, 0o600); err != nil { + return Projection{}, false, fmt.Errorf("registry_config npm: write projection: %w", err) + } + return Projection{ + Ecosystem: sandboxprofile.RegistryConfigNPM, + Source: src, + Path: dest, + EnvVar: "NPM_CONFIG_USERCONFIG", + KeptKeys: res.KeptKeys, + Dropped: res.Dropped, + StrippedUserinfo: res.StrippedUserinfo, + }, true, nil +} + +// defaultNPMRegistryHost is npm's built-in registry. A mapping that points +// here needs no projection: it is what npm would do anyway. +const defaultNPMRegistryHost = "registry.npmjs.org" + +// Notice reports that a projection would change the outcome of a launch. +// It is advisory, produced by inspection rather than at launch time, so +// `omac doctor` can explain a 404 the user has not hit yet. +type Notice struct { + // Ecosystem is the registry_config identifier that would apply. + Ecosystem string + // Source is the host config file inspected. + Source string + // Hosts are the non-default registry hosts the file maps to. + Hosts []string + // Credentialed is true when the file also holds auth entries, which + // is why override_deny is the wrong remedy. + Credentialed bool + // Enabled reflects whether the profile already opts in. + Enabled bool + // Overridden reflects whether override_deny already exposes Source. + Overridden bool +} + +// InspectNPM reports whether ~/.npmrc maps any scope to a non-default +// registry, and how the given profile currently handles it. It returns nil +// when there is nothing to say: no file, no mapping, or every mapping +// already points at npm's default registry. +// +// The predicate is deliberately "host is not npm's default" rather than +// netproxy's isPackageRegistry heuristic: that heuristic answers a +// different question (does a *host* look like a registry?) and returns +// false for exactly the corporate shape this detector exists for — +// e.g. tng-artifacts.int.tngtech.com has no "registry"/"npm" label. +func InspectNPM(enabled, overridden bool) (*Notice, error) { + src, err := NPMUserConfig() + if err != nil { + return nil, err + } + raw, err := os.ReadFile(src) + if err != nil { + if os.IsNotExist(err) { + return nil, nil + } + return nil, err + } + res := ScrubNPMRC(raw) + var hosts []string + for _, line := range strings.Split(strings.TrimSpace(string(res.Content)), "\n") { + _, value, ok := strings.Cut(line, "=") + if !ok { + continue + } + u, err := url.Parse(strings.TrimSpace(value)) + if err != nil || u.Hostname() == "" || u.Hostname() == defaultNPMRegistryHost { + continue + } + if !slices.Contains(hosts, u.Hostname()) { + hosts = append(hosts, u.Hostname()) + } + } + if len(hosts) == 0 { + return nil, nil + } + return &Notice{ + Ecosystem: sandboxprofile.RegistryConfigNPM, + Source: src, + Hosts: hosts, + Credentialed: res.DroppedCredentials > 0, + Enabled: enabled, + Overridden: overridden, + }, nil +} + +// ScrubResult is the outcome of scrubbing one config file. +type ScrubResult struct { + // Content is the projected file body (mapping lines only), empty when + // nothing survived. + Content []byte + // KeptKeys are the config keys that survived. + KeptKeys []string + // Dropped counts removed non-comment entries. + Dropped int + // DroppedCredentials counts the subset of Dropped whose key carries + // authentication material. Reported separately because "this file + // holds a token" changes the advice: override_deny would expose it, + // a projection does not. + DroppedCredentials int + // StrippedUserinfo counts kept URLs that carried inline credentials. + StrippedUserinfo int +} + +// scopedRegistryKey matches npm's scoped-registry form, e.g. +// "@acme:registry" or "@acme/sub:registry". +var scopedRegistryKey = regexp.MustCompile(`^@[^:\s]+:registry$`) + +// credentialKey matches npmrc keys that carry authentication material, +// including the per-registry form "//host/path/:_authToken". +var credentialKey = regexp.MustCompile(`(?i)(_auth|_authtoken|_password|username|email|^//)`) + +// ScrubNPMRC keeps only registry mappings from an npmrc body. Everything +// else — credentials, comments, unrelated knobs — is dropped. +func ScrubNPMRC(src []byte) ScrubResult { + var res ScrubResult + var lines []string + for _, raw := range strings.Split(string(src), "\n") { + line := strings.TrimSpace(raw) + if line == "" || strings.HasPrefix(line, ";") || strings.HasPrefix(line, "#") { + continue + } + key, value, ok := strings.Cut(line, "=") + if !ok { + res.Dropped++ + continue + } + key, value = strings.TrimSpace(key), strings.TrimSpace(value) + if !isRegistryMapping(key) { + res.Dropped++ + if credentialKey.MatchString(key) { + res.DroppedCredentials++ + } + continue + } + clean, stripped, ok := registryURL(value) + if !ok { + // A mapping whose value is not an http(s) URL is not something + // we can vouch for; dropping is the safe direction. + res.Dropped++ + continue + } + if stripped { + res.StrippedUserinfo++ + } + lines = append(lines, key+"="+clean) + res.KeptKeys = append(res.KeptKeys, key) + } + if len(lines) > 0 { + res.Content = []byte(strings.Join(lines, "\n") + "\n") + } + return res +} + +// isRegistryMapping reports whether key is a registry mapping: the global +// `registry` or a scoped `@scope:registry`. Comparison is case-insensitive +// because npm lowercases config keys. +func isRegistryMapping(key string) bool { + k := strings.ToLower(key) + return k == "registry" || scopedRegistryKey.MatchString(k) +} + +// registryURL validates a mapping value and removes any credentials +// embedded in it. ok=false means the value is not an absolute http(s) URL +// and must not be projected. Note that a hostname merely *containing* +// "token" (api.trustedtokens.eu) is a legitimate registry and is kept — +// the credential check is structural (userinfo), not textual. +func registryURL(value string) (clean string, stripped bool, ok bool) { + u, err := url.Parse(value) + if err != nil || u.Host == "" { + return "", false, false + } + if u.Scheme != "http" && u.Scheme != "https" { + return "", false, false + } + if u.User != nil { + u.User = nil + return u.String(), true, true + } + return u.String(), false, true +} diff --git a/internal/registryconf/registryconf_test.go b/internal/registryconf/registryconf_test.go new file mode 100644 index 00000000..bd292f54 --- /dev/null +++ b/internal/registryconf/registryconf_test.go @@ -0,0 +1,267 @@ +package registryconf + +import ( + "net/url" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/tngtech/oh-my-agentic-coder/internal/sandboxprofile" +) + +// TestProjectorsCoverProfileTools guards against drift: every ecosystem +// the profile accepts must have a projector, and vice versa. +func TestProjectorsCoverProfileTools(t *testing.T) { + tools := sandboxprofile.RegistryConfigTools() + if len(tools) != len(projectors) { + t.Fatalf("registry_config: %d profile tool(s) vs %d projector(s)", len(tools), len(projectors)) + } + for _, tool := range tools { + if _, ok := projectors[tool]; !ok { + t.Errorf("no projector for profile tool %q", tool) + } + } +} + +func TestScrubNPMRCKeepsOnlyRegistryMappings(t *testing.T) { + tests := []struct { + name string + src string + wantKeys []string + wantBody string + dropped int + }{ + { + name: "the #241 shape: bare scope mapping", + src: "@tngtech:registry=https://tng-artifacts.int.tngtech.com\n", + wantKeys: []string{"@tngtech:registry"}, + wantBody: "@tngtech:registry=https://tng-artifacts.int.tngtech.com\n", + }, + { + name: "token lines are dropped, mapping survives", + src: "@acme:registry=https://npm.acme.test\n" + + "//npm.acme.test/:_authToken=SECRET\n" + + "_auth=BASE64SECRET\n" + + "_password=hunter2\n" + + "email=dev@acme.test\n", + wantKeys: []string{"@acme:registry"}, + wantBody: "@acme:registry=https://npm.acme.test\n", + dropped: 4, + }, + { + name: "global registry mapping is kept", + src: "registry=https://npm.acme.test\n", + wantKeys: []string{"registry"}, + wantBody: "registry=https://npm.acme.test\n", + }, + { + name: "comments and blanks are ignored, not counted", + src: "; a comment\n# another\n\nregistry=https://npm.acme.test\n", + wantKeys: []string{"registry"}, + wantBody: "registry=https://npm.acme.test\n", + }, + { + name: "unrelated knobs are dropped", + src: "strict-ssl=false\ncafile=/etc/ca.pem\nregistry=https://npm.acme.test\n", + wantKeys: []string{"registry"}, + wantBody: "registry=https://npm.acme.test\n", + dropped: 2, + }, + { + name: "a host containing \"token\" is a legitimate registry", + src: "@tt:registry=https://api.trustedtokens.eu\n", + wantKeys: []string{"@tt:registry"}, + wantBody: "@tt:registry=https://api.trustedtokens.eu\n", + }, + { + name: "non-URL mapping value is dropped", + src: "registry=not-a-url\n", + wantKeys: nil, + dropped: 1, + }, + { + name: "non-http scheme is dropped", + src: "registry=file:///tmp/evil\n", + wantKeys: nil, + dropped: 1, + }, + { + name: "line without = is dropped", + src: "garbage\n", + wantKeys: nil, + dropped: 1, + }, + { + name: "keys are matched case-insensitively", + src: "@Acme:Registry=https://npm.acme.test\n", + wantKeys: []string{"@Acme:Registry"}, + wantBody: "@Acme:Registry=https://npm.acme.test\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ScrubNPMRC([]byte(tt.src)) + if strings.Join(got.KeptKeys, ",") != strings.Join(tt.wantKeys, ",") { + t.Errorf("kept keys = %v, want %v", got.KeptKeys, tt.wantKeys) + } + if string(got.Content) != tt.wantBody { + t.Errorf("content = %q, want %q", got.Content, tt.wantBody) + } + if got.Dropped != tt.dropped { + t.Errorf("dropped = %d, want %d", got.Dropped, tt.dropped) + } + }) + } +} + +func TestScrubNPMRCStripsInlineCredentials(t *testing.T) { + got := ScrubNPMRC([]byte("@acme:registry=https://user:hunter2@npm.acme.test/path\n")) + if got.StrippedUserinfo != 1 { + t.Errorf("StrippedUserinfo = %d, want 1", got.StrippedUserinfo) + } + if strings.Contains(string(got.Content), "hunter2") || strings.Contains(string(got.Content), "user") { + t.Fatalf("projection leaked inline credentials: %q", got.Content) + } + if want := "@acme:registry=https://npm.acme.test/path\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } +} + +// TestScrubNPMRCNeverLeaksSecretMaterial is the invariant that justifies +// projecting a protected file at all: whatever the input, every output +// line must be a registry mapping whose URL carries no userinfo. The check +// is structural rather than textual because a legitimate key (@acme:registry) +// and a legitimate host (api.trustedtokens.eu) both look credential-ish. +func TestScrubNPMRCNeverLeaksSecretMaterial(t *testing.T) { + credentialKey := regexp.MustCompile(`(?i)(_auth|authtoken|_password|passwd|username|email|^//)`) + inputs := []string{ + "//registry.npmjs.org/:_authToken=npm_LIVETOKEN\n@acme:registry=https://npm.acme.test\n", + "_auth=aGVsbG86d29ybGQ=\n_password=pw\nusername=dev\nregistry=https://npm.acme.test\n", + "@a:registry=https://u:p@a.test\n@b:registry=https://b.test\n//a.test/:_password=x\n", + "registry=https://npm.acme.test\n//npm.acme.test/:_authToken=${NPM_TOKEN}\n", + } + for _, in := range inputs { + out := string(ScrubNPMRC([]byte(in)).Content) + for _, line := range strings.Split(strings.TrimSpace(out), "\n") { + if line == "" { + continue + } + key, value, ok := strings.Cut(line, "=") + if !ok { + t.Errorf("projected line is not key=value: %q (input %q)", line, in) + continue + } + if credentialKey.MatchString(key) { + t.Errorf("projected a credential-bearing key %q (input %q)", key, in) + } + if !isRegistryMapping(key) { + t.Errorf("projected a non-mapping key %q (input %q)", key, in) + } + u, err := url.Parse(value) + if err != nil { + t.Errorf("projected an unparseable URL %q (input %q)", value, in) + continue + } + if u.User != nil { + t.Errorf("projected inline userinfo in %q (input %q)", value, in) + } + } + for _, marker := range []string{"LIVETOKEN", "hunter2", "aGVsbG86d29ybGQ=", "NPM_TOKEN", "u:p"} { + if strings.Contains(out, marker) { + t.Errorf("scrub leaked %q\n input: %q\noutput: %q", marker, in, out) + } + } + } +} + +func TestProjectNPMWritesScrubbedFile(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + npmrc := filepath.Join(home, ".npmrc") + body := "@acme:registry=https://npm.acme.test\n//npm.acme.test/:_authToken=SECRET\n" + if err := os.WriteFile(npmrc, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + dir := t.TempDir() + projs, err := Project([]string{sandboxprofile.RegistryConfigNPM}, dir) + if err != nil { + t.Fatal(err) + } + if len(projs) != 1 { + t.Fatalf("got %d projection(s), want 1", len(projs)) + } + p := projs[0] + if p.EnvVar != "NPM_CONFIG_USERCONFIG" { + t.Errorf("EnvVar = %q", p.EnvVar) + } + if p.Source != npmrc { + t.Errorf("Source = %q, want %q", p.Source, npmrc) + } + got, err := os.ReadFile(p.Path) + if err != nil { + t.Fatal(err) + } + if want := "@acme:registry=https://npm.acme.test\n"; string(got) != want { + t.Errorf("projected file = %q, want %q", got, want) + } + if strings.Contains(string(got), "SECRET") { + t.Fatal("projected file leaked the auth token") + } + info, err := os.Stat(p.Path) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("projection mode = %o, want 600", perm) + } + // The summary goes into the launch log; it must not echo the file body. + if strings.Contains(p.Summary(), "SECRET") { + t.Errorf("summary leaked secret material: %q", p.Summary()) + } +} + +func TestProjectNPMAbsentOrMappinglessIsNoop(t *testing.T) { + t.Run("missing npmrc", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + projs, err := Project([]string{sandboxprofile.RegistryConfigNPM}, t.TempDir()) + if err != nil { + t.Fatal(err) + } + if len(projs) != 0 { + t.Fatalf("got %d projection(s), want none", len(projs)) + } + }) + + t.Run("npmrc with no mapping", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if err := os.WriteFile(filepath.Join(home, ".npmrc"), []byte("_auth=x\n"), 0o600); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + projs, err := Project([]string{sandboxprofile.RegistryConfigNPM}, dir) + if err != nil { + t.Fatal(err) + } + if len(projs) != 0 { + t.Fatalf("got %d projection(s), want none", len(projs)) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Errorf("wrote %d file(s) for a mappingless npmrc, want none", len(entries)) + } + }) +} + +func TestProjectUnknownEcosystem(t *testing.T) { + if _, err := Project([]string{"nope"}, t.TempDir()); err == nil { + t.Fatal("want error for unknown ecosystem") + } +} diff --git a/internal/sandboxprofile/profile.go b/internal/sandboxprofile/profile.go index 4c4bb5d3..ed7baa64 100644 --- a/internal/sandboxprofile/profile.go +++ b/internal/sandboxprofile/profile.go @@ -114,6 +114,22 @@ type Filesystem struct { // deny set (Baseline.ProtectedPaths). It does not grant access by // itself; a matching allow/read/write grant is still required. OverrideDeny []string `json:"override_deny,omitempty"` + // RegistryConfig opts a package-manager ecosystem into a scrubbed + // projection of its user config file. The host file stays protected + // and unreadable; omac derives a copy holding only the registry + // mappings (no credentials), grants read access to that copy alone, + // and points the tool at it through its native config env var. + // + // This exists because a scope→registry mapping is load-bearing + // *configuration* that happens to live in the same file as a + // credential. Without it, a scoped package resolves against the + // public registry and 404s (see #150, #241). Values: + // - "npm": derive from ~/.npmrc, inject NPM_CONFIG_USERCONFIG. + // + // Unset means the historical behavior: the file stays fully masked. + // The blunt alternative — override_deny on ~/.npmrc — grants the + // whole file including any auth token; this does not. + RegistryConfig []string `json:"registry_config,omitempty"` } // Network configures isolation, filtering and port openings. @@ -174,6 +190,18 @@ func ProxyInjectionTools() []string { return []string{ProxyInjectJVM, ProxyInjectNode} } +// registry_config ecosystem identifiers. +const ( + RegistryConfigNPM = "npm" +) + +// RegistryConfigTools lists the accepted filesystem.registry_config +// ecosystem identifiers. Single source of truth for validation; the +// registryconf projector registry must supply one entry per name here. +func RegistryConfigTools() []string { + return []string{RegistryConfigNPM} +} + // NetworkPrompt mirrors nono's network_prompt block. type NetworkPrompt struct { // Enabled defaults to true when the network_prompt object is @@ -328,6 +356,11 @@ func (p *Profile) Validate() error { return fmt.Errorf("sandbox profile: invalid network.proxy_injection %q (want one of %s)", tool, strings.Join(ProxyInjectionTools(), ", ")) } } + for _, eco := range p.Filesystem.RegistryConfig { + if !slices.Contains(RegistryConfigTools(), eco) { + return fmt.Errorf("sandbox profile: invalid filesystem.registry_config %q (want one of %s)", eco, strings.Join(RegistryConfigTools(), ", ")) + } + } for _, group := range []struct { name string ports []int diff --git a/internal/sandboxprofile/profile_test.go b/internal/sandboxprofile/profile_test.go index c3ba8d24..9ebb3610 100644 --- a/internal/sandboxprofile/profile_test.go +++ b/internal/sandboxprofile/profile_test.go @@ -96,10 +96,12 @@ func TestParseValidationErrors(t *testing.T) { `{"network": {"listen_port": [70000]}}`, `{"network": {"network_prompt": {"on_unavailable": "ask"}}}`, `{"environment": {"allow_vars": [" "]}}`, - `{"environment": {"deny_vars": [" "]}}`, // empty deny_vars entry - `{"filesystem": {"deny": [" "]}}`, // empty deny entry - `{"filesystem": {"deny": ["[a-"]}}`, // malformed basename glob - `{"network": {"proxy_injection": ["python"]}}`, // unsupported tool + `{"environment": {"deny_vars": [" "]}}`, // empty deny_vars entry + `{"filesystem": {"deny": [" "]}}`, // empty deny entry + `{"filesystem": {"deny": ["[a-"]}}`, // malformed basename glob + `{"network": {"proxy_injection": ["python"]}}`, // unsupported tool + `{"filesystem": {"registry_config": ["pypi"]}}`, // unsupported ecosystem + `{"filesystem": {"registry_config": ["npmrc"]}}`, // near-miss name } for _, c := range cases { if _, err := Parse([]byte(c)); err == nil { @@ -119,6 +121,25 @@ func TestProxyInjection(t *testing.T) { } } +func TestRegistryConfig(t *testing.T) { + p, err := Parse([]byte(`{"filesystem": {"registry_config": ["npm"]}}`)) + if err != nil { + t.Fatalf("valid registry_config rejected: %v", err) + } + if want := []string{RegistryConfigNPM}; !slices.Equal(p.Filesystem.RegistryConfig, want) { + t.Errorf("registry_config = %v, want %v", p.Filesystem.RegistryConfig, want) + } + // Unset is the historical behavior and must stay the zero value, so + // nothing is projected unless the profile opts in. + bare, err := Parse([]byte(`{}`)) + if err != nil { + t.Fatal(err) + } + if len(bare.Filesystem.RegistryConfig) != 0 { + t.Errorf("registry_config defaulted to %v, want empty", bare.Filesystem.RegistryConfig) + } +} + func TestParseDenyVars(t *testing.T) { p, err := Parse([]byte(`{"environment": {"allow_vars": ["SKAINET_TOKEN"], "deny_vars": ["XDG_*", "HTTP_PROXY"]}}`)) if err != nil { diff --git a/internal/sandboxrun/registryconf.go b/internal/sandboxrun/registryconf.go new file mode 100644 index 00000000..efdca3f4 --- /dev/null +++ b/internal/sandboxrun/registryconf.go @@ -0,0 +1,68 @@ +package sandboxrun + +import ( + "fmt" + "io" + "os" + "strings" + + "github.com/tngtech/oh-my-agentic-coder/internal/registryconf" + "github.com/tngtech/oh-my-agentic-coder/internal/sandboxprofile" +) + +// setupRegistryConfig honors filesystem.registry_config: it derives a +// credential-free copy of each requested package-manager config, grants +// read access to that copy alone, and points the tool at it through its +// native config env var. The protected host file stays masked. +// +// grants and injected are mutated in place, so this must run after grants +// are resolved and before BuildChildArgv turns them into backend rules. +// The returned cleanup removes the projection directory; it is safe to +// call even on the error paths. +func setupRegistryConfig(merged *sandboxprofile.Profile, grants *Grants, injected map[string]string, stderr io.Writer) (func(), error) { + noop := func() {} + ecosystems := merged.Filesystem.RegistryConfig + if len(ecosystems) == 0 { + return noop, nil + } + + dir, err := os.MkdirTemp("", "omac-registryconf-") + if err != nil { + return noop, fmt.Errorf("registry_config: create projection dir: %w", err) + } + cleanup := func() { _ = os.RemoveAll(dir) } + if err := os.Chmod(dir, 0o700); err != nil { + cleanup() + return noop, fmt.Errorf("registry_config: secure projection dir: %w", err) + } + + projections, err := registryconf.Project(ecosystems, dir) + if err != nil { + cleanup() + return noop, err + } + if len(projections) == 0 { + // Nothing to project (no config file, or no registry mapping in + // it). Say so: the user asked for a projection and got none, and + // silence here reads as "it worked". + fmt.Fprintf(stderr, "omac sandbox: registry_config (%s): no registry mapping found to project; "+ + "scoped packages will resolve against the default registry\n", strings.Join(ecosystems, ", ")) + cleanup() + return noop, nil + } + + overrides := sandboxprofile.BuildOverrideLookup(merged.Filesystem.OverrideDeny) + for _, p := range projections { + // Grant exactly the projected file, read-only. The host file is + // untouched and stays protected. + grants.ReadPaths = append(grants.ReadPaths, p.Path) + injected[p.EnvVar] = p.Path + fmt.Fprintf(stderr, "omac sandbox: registry_config: %s\n", p.Summary()) + if overrides[p.Source] { + fmt.Fprintf(stderr, "omac sandbox: WARNING: %s is also in filesystem.override_deny, so the sandbox can read the "+ + "real file including any auth token it holds. The projection makes that grant unnecessary — "+ + "drop the override_deny entry to keep the credential protected.\n", p.Source) + } + } + return cleanup, nil +} diff --git a/internal/sandboxrun/registryconf_test.go b/internal/sandboxrun/registryconf_test.go new file mode 100644 index 00000000..ad664948 --- /dev/null +++ b/internal/sandboxrun/registryconf_test.go @@ -0,0 +1,158 @@ +package sandboxrun + +import ( + "bytes" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/tngtech/oh-my-agentic-coder/internal/sandboxprofile" +) + +// writeNpmrc points HOME at a temp dir holding the given npmrc body. +func writeNpmrc(t *testing.T, body string) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + path := filepath.Join(home, ".npmrc") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + return path +} + +func TestSetupRegistryConfigNoopWhenUnset(t *testing.T) { + writeNpmrc(t, "@acme:registry=https://npm.acme.test\n") + grants := &Grants{} + injected := map[string]string{} + var stderr bytes.Buffer + + cleanup, err := setupRegistryConfig(&sandboxprofile.Profile{}, grants, injected, &stderr) + if err != nil { + t.Fatal(err) + } + defer cleanup() + + if len(grants.ReadPaths) != 0 { + t.Errorf("granted %v with registry_config unset", grants.ReadPaths) + } + if len(injected) != 0 { + t.Errorf("injected %v with registry_config unset", injected) + } + if stderr.Len() != 0 { + t.Errorf("unexpected output: %q", stderr.String()) + } +} + +func TestSetupRegistryConfigGrantsProjectionOnly(t *testing.T) { + npmrc := writeNpmrc(t, "@acme:registry=https://npm.acme.test\n//npm.acme.test/:_authToken=SECRET\n") + grants := &Grants{} + injected := map[string]string{} + var stderr bytes.Buffer + + profile := &sandboxprofile.Profile{ + Filesystem: sandboxprofile.Filesystem{RegistryConfig: []string{sandboxprofile.RegistryConfigNPM}}, + } + cleanup, err := setupRegistryConfig(profile, grants, injected, &stderr) + if err != nil { + t.Fatal(err) + } + + projected := injected["NPM_CONFIG_USERCONFIG"] + if projected == "" { + t.Fatal("NPM_CONFIG_USERCONFIG was not injected") + } + if !slices.Contains(grants.ReadPaths, projected) { + t.Errorf("ReadPaths %v does not include the projection %q", grants.ReadPaths, projected) + } + if slices.Contains(grants.ReadPaths, npmrc) { + t.Error("granted the protected host npmrc; only the projection may be granted") + } + if len(grants.ReadPaths) != 1 { + t.Errorf("ReadPaths = %v, want exactly the projection", grants.ReadPaths) + } + body, err := os.ReadFile(projected) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(body), "SECRET") { + t.Fatalf("projection leaked the token: %q", body) + } + if !strings.Contains(stderr.String(), "registry_config") { + t.Errorf("launch log did not mention the projection: %q", stderr.String()) + } + + // The projection must not outlive the run. + cleanup() + if _, err := os.Stat(projected); !os.IsNotExist(err) { + t.Errorf("projection survived cleanup: %v", err) + } +} + +func TestSetupRegistryConfigWarnsWhenOverrideDenyAlsoGrantsHostFile(t *testing.T) { + npmrc := writeNpmrc(t, "@acme:registry=https://npm.acme.test\n") + grants := &Grants{} + var stderr bytes.Buffer + + profile := &sandboxprofile.Profile{ + Filesystem: sandboxprofile.Filesystem{ + RegistryConfig: []string{sandboxprofile.RegistryConfigNPM}, + OverrideDeny: []string{npmrc}, + }, + } + cleanup, err := setupRegistryConfig(profile, grants, map[string]string{}, &stderr) + if err != nil { + t.Fatal(err) + } + defer cleanup() + + out := stderr.String() + if !strings.Contains(out, "override_deny") || !strings.Contains(out, "WARNING") { + t.Errorf("expected a warning that override_deny exposes the real file, got: %q", out) + } +} + +func TestSetupRegistryConfigReportsNothingToProject(t *testing.T) { + writeNpmrc(t, "_auth=SECRET\n") // credentials only, no mapping + grants := &Grants{} + injected := map[string]string{} + var stderr bytes.Buffer + + profile := &sandboxprofile.Profile{ + Filesystem: sandboxprofile.Filesystem{RegistryConfig: []string{sandboxprofile.RegistryConfigNPM}}, + } + cleanup, err := setupRegistryConfig(profile, grants, injected, &stderr) + if err != nil { + t.Fatal(err) + } + defer cleanup() + + if len(grants.ReadPaths) != 0 || len(injected) != 0 { + t.Errorf("projected nothing but still granted %v / injected %v", grants.ReadPaths, injected) + } + if !strings.Contains(stderr.String(), "no registry mapping") { + t.Errorf("silent no-op; want an explicit notice, got: %q", stderr.String()) + } +} + +func TestSetupRegistryConfigMissingHostFileIsQuietNoop(t *testing.T) { + t.Setenv("HOME", t.TempDir()) // no .npmrc at all + grants := &Grants{} + injected := map[string]string{} + var stderr bytes.Buffer + + profile := &sandboxprofile.Profile{ + Filesystem: sandboxprofile.Filesystem{RegistryConfig: []string{sandboxprofile.RegistryConfigNPM}}, + } + cleanup, err := setupRegistryConfig(profile, grants, injected, &stderr) + if err != nil { + t.Fatal(err) + } + defer cleanup() + + if len(grants.ReadPaths) != 0 || len(injected) != 0 { + t.Errorf("granted %v / injected %v for a missing npmrc", grants.ReadPaths, injected) + } +} diff --git a/internal/sandboxrun/run.go b/internal/sandboxrun/run.go index a2971bfe..f7dc1620 100644 --- a/internal/sandboxrun/run.go +++ b/internal/sandboxrun/run.go @@ -213,6 +213,16 @@ func Run(opts Options) int { } } + // registry_config: derive credential-free package-manager configs and + // grant read access to the copies. Must run before BuildChildArgv, which + // freezes grants into backend rules, and the projection dir must outlive + // the child (bwrap binds it at launch), so cleanup is deferred. + registryCleanup, err := setupRegistryConfig(merged, grants, injected, stderr) + if err != nil { + return fail("%v", err) + } + defer registryCleanup() + // Denial markers must outlive argv construction: bwrap reads the // bind sources at launch, so cleanup is deferred until after the // child exits (below), not when BuildChildArgv returns. From eae1299aeda59000142a2184f7bc49df7e7d6269 Mon Sep 17 00:00:00 2001 From: Ilia Zhuravok Date: Mon, 24 Aug 2026 12:24:13 +0100 Subject: [PATCH 2/3] fix(sandbox,cli): close credential and regression gaps in the registry projection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on this branch. Two of them broke stated guarantees; both were confirmed empirically before fixing. Secret leak. registryURL stripped only URL userinfo, so a secret carried anywhere else survived into the sandbox-readable file, contradicting the package's "no credential can survive by construction" invariant and the docs. Verified: `@acme:registry=https://npm.acme.test/api/?apiKey=SECRET` was projected verbatim (Dropped=0, StrippedUserinfo=0), and the never-leaks test passed because it only asserted u.User == nil. A URL with a query string or fragment is now refused outright rather than stripped: that is where an API key usually hides, and omac cannot tell a secret parameter from a load-bearing one. Regression risk. The global `registry` key was projected even when the file held a credential for that host, pointing npm at a private mirror it cannot authenticate to — which breaks every install, including the public dependencies that work today with the file fully masked. Credential keys (`//host/:_authToken`) are now correlated with mapping hosts: a global mapping needing auth is refused, a scoped one is kept (that scope was already failing) with a warning that installs may 401. Also fixed: - Values npm accepts but url.Parse rejects were silently skipped, and because InspectNPM reads the scrubbed output, doctor went silent too — leaving exactly the unexplained 404 this feature exists to prevent. npm's value syntax is now honored first (surrounding quotes removed, ${VAR} expanded), and anything still unusable is reported as a rejection at launch and by doctor instead of vanishing. - A non-ENOENT failure reading ~/.npmrc aborted the launch (unresolvable HOME, or EACCES after a root-owned `sudo npm config set`). An opt-in convenience that only ever adds a mapping now degrades to a warning. - doctor checked Enabled before Overridden, so a profile with both registry_config and override_deny got only "[ok] … projected" and was never told the token-bearing file is still readable. Each condition is now reported independently. - docs: document the refusals, npm value syntax, and the scoped-vs-global auth distinction. Verified the #241 acceptance test is unaffected by the stricter scrub: cold cache, knob on -> 14 skainet models, plugin 1.0.3, and doctor still names the cause on a profile without the knob. Refs #150 Refs #241 Co-Authored-By: Claude Opus 5 Signed-off-by: Ilia Zhuravok --- docs/CONFIGURATION.md | 24 ++- internal/cli/doctor.go | 32 +++- internal/cli/doctor_registryconf_test.go | 55 ++++++ internal/registryconf/registryconf.go | 191 ++++++++++++++++++--- internal/registryconf/registryconf_test.go | 134 ++++++++++++++- internal/sandboxrun/registryconf.go | 40 +++-- 6 files changed, 428 insertions(+), 48 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index ae2ccc9d..dd46a316 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -148,9 +148,27 @@ Opting in projects a credential-free copy instead: At launch omac derives a copy of `~/.npmrc` holding **only** registry mappings, grants read access to that copy alone, and points npm at it via -`NPM_CONFIG_USERCONFIG`. Every other entry is dropped, and inline URL -credentials (`https://user:pass@host`) are stripped, so no secret can -reach the sandbox. The host file stays masked. +`NPM_CONFIG_USERCONFIG`. The host file stays masked. No secret can reach +the sandbox: every non-mapping entry is dropped, inline URL credentials +(`https://user:pass@host`) are stripped, and a mapping whose URL carries a +query string or fragment is **refused** rather than copied — that is where +an API key usually hides (`?apiKey=…`), and omac cannot tell a secret +parameter from a load-bearing one. + +npm's own value syntax is honored first, so a mapping npm would act on is +not lost: surrounding quotes are removed and `${VAR}` is expanded from the +environment. + +Two cases are deliberately **not** projected, and both are reported at +launch and by `omac doctor` rather than skipped quietly: + +- A mapping omac cannot turn into a credential-free `http(s)` URL. +- The **global** `registry` key when the file also holds a credential for + that host. omac cannot supply the token, and redirecting all resolution + to a registry it cannot authenticate to would break even the public + installs that work today. A *scoped* mapping to such a host is still + projected — that scope was already failing — with a warning that installs + may return 401/403. Private registries usually also need their host in `network.allow_domain` (or an allow at the network prompt). diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index cae12e7c..b733e624 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -230,16 +230,16 @@ func doctorRegistryConfig(env *Env, profileRef string) { return } hosts := strings.Join(notice.Hosts, ", ") + // Each condition is reported on its own: a profile can have BOTH + // registry_config and override_deny, and reporting only the former + // ("[ok] … projected") would reassure the user while the real + // token-bearing file stays readable by the sandbox. switch { + case len(notice.Hosts) == 0: + // Only rejections to report; the mapping list is empty. case notice.Enabled: fmt.Fprintf(env.Stdout, "[ok] registry config: %s mappings (%s) are projected into the sandbox\n", notice.Ecosystem, hosts) - case notice.Overridden: - fmt.Fprintf(env.Stdout, "[warn] registry config: %s is exposed to the sandbox via filesystem.override_deny\n", - notice.Source) - fmt.Fprintf(env.Stdout, " That grants the whole file%s. Prefer filesystem.registry_config: [%q],\n", - credentialSuffix(notice.Credentialed), notice.Ecosystem) - fmt.Fprintf(env.Stdout, " which projects only the registry mappings (%s) and drops every credential.\n", hosts) default: fmt.Fprintf(env.Stdout, "[warn] registry config: %s maps a scope to %s, but the sandbox cannot read it\n", notice.Source, hosts) @@ -247,6 +247,26 @@ func doctorRegistryConfig(env *Env, profileRef string) { fmt.Fprintf(env.Stdout, " add filesystem.registry_config: [%q] to the sandbox profile%s.\n", notice.Ecosystem, credentialNote(notice.Credentialed)) } + + if notice.Overridden { + fmt.Fprintf(env.Stdout, "[warn] registry config: %s is exposed to the sandbox via filesystem.override_deny\n", + notice.Source) + fmt.Fprintf(env.Stdout, " That grants the whole file%s.\n", credentialSuffix(notice.Credentialed)) + if notice.Enabled { + fmt.Fprintf(env.Stdout, " filesystem.registry_config is already projecting the mappings, so this grant\n") + fmt.Fprintf(env.Stdout, " is redundant — drop it to keep the credential protected.\n") + } else { + fmt.Fprintf(env.Stdout, " Prefer filesystem.registry_config: [%q], which projects only the registry\n", notice.Ecosystem) + fmt.Fprintf(env.Stdout, " mappings and drops every credential.\n") + } + } + + // Rejections are the silent-failure case: config exists, omac will not + // use it, and nothing else would say so. + for _, r := range notice.Rejected { + fmt.Fprintf(env.Stdout, "[warn] registry config: %s cannot be projected from %s\n", r.Key, notice.Source) + fmt.Fprintf(env.Stdout, " %s\n", r.Reason) + } } // credentialSuffix describes what an override_deny grant exposes. diff --git a/internal/cli/doctor_registryconf_test.go b/internal/cli/doctor_registryconf_test.go index 6398ba60..bfa5df37 100644 --- a/internal/cli/doctor_registryconf_test.go +++ b/internal/cli/doctor_registryconf_test.go @@ -138,3 +138,58 @@ func TestDoctorRegistryConfigSilentWithoutPrivateMapping(t *testing.T) { } }) } + +// TestDoctorRegistryConfigReportsBothProjectionAndOverride is the review +// finding: with registry_config AND override_deny set, doctor printed only +// the reassuring "[ok] … projected" line and never mentioned that the real +// token-bearing file is still readable by the sandbox. +func TestDoctorRegistryConfigReportsBothProjectionAndOverride(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + workdir := t.TempDir() + writeWorkdirConfig(t, workdir, "builtin", []string{ + "{{self}}", "sandbox", "run", + "--profile", "default", + "--", "{{inner_cmd}}", "{{inner_args}}", + }) + stageProfile(t, home, `{ + "meta": {"name": "default"}, + "filesystem": {"registry_config": ["npm"], "override_deny": ["~/.npmrc"]}, + "environment": {"allow_vars": ["HOME"]} + }`) + body := "@acme:registry=https://npm.acme.test\n//npm.acme.test/:_authToken=SECRET\n" + if err := os.WriteFile(filepath.Join(home, ".npmrc"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + + env, outBuf, _, drain := newPipeEnv(t, "") + env.Workdir = workdir + runDoctor([]string{}, env) + drain() + out := outBuf.String() + + if !strings.Contains(out, "[ok] registry config") { + t.Errorf("did not confirm the projection; got:\n%s", out) + } + if !strings.Contains(out, "override_deny") || !strings.Contains(out, "redundant") { + t.Errorf("did not flag the redundant, token-exposing override; got:\n%s", out) + } + if strings.Contains(out, "SECRET") { + t.Fatalf("doctor echoed secret material; got:\n%s", out) + } +} + +// TestDoctorRegistryConfigReportsUnusableMapping keeps doctor from staying +// silent when the npmrc has private-registry config omac cannot project. +func TestDoctorRegistryConfigReportsUnusableMapping(t *testing.T) { + out := stageDoctorNpmrc(t, + "@acme:registry=https://npm.acme.test/api/?apiKey=SECRET\n", + `{"meta": {"name": "default"}, "environment": {"allow_vars": ["HOME"]}}`) + + if !strings.Contains(out, "cannot be projected") { + t.Errorf("did not report the unusable mapping; got:\n%s", out) + } + if strings.Contains(out, "SECRET") { + t.Fatalf("doctor echoed the secret; got:\n%s", out) + } +} diff --git a/internal/registryconf/registryconf.go b/internal/registryconf/registryconf.go index b65b958e..d1f79c0e 100644 --- a/internal/registryconf/registryconf.go +++ b/internal/registryconf/registryconf.go @@ -44,8 +44,22 @@ type Projection struct { // StrippedUserinfo counts kept mappings whose URL carried inline // credentials (https://user:pass@host) that were removed. StrippedUserinfo int + // Rejected lists mappings omac recognized but refused to project. + Rejected []Rejected + // NeedsAuth lists projected mapping keys whose host has a credential + // entry omac deliberately did not copy. + NeedsAuth []string + // Warning explains why no projection was produced, when the reason is + // worth telling the user about (an unreadable config rather than an + // absent one). Path is empty in that case. + Warning string } +// Projected reports whether a usable projection was written. A Projection +// with Projected()==false may still carry a Warning or Rejected entries the +// caller should surface. +func (p Projection) Projected() bool { return p.Path != "" } + // Summary renders a one-line, secret-free description for the launch log. func (p Projection) Summary() string { extra := "" @@ -57,7 +71,9 @@ func (p Projection) Summary() string { } // projector derives one ecosystem's projection into dir. A missing host -// config is not an error: it returns ok=false and no projection. +// config is not an error: it returns present=false. A returned Projection +// may still carry a Warning or Rejected entries when present=false, so the +// caller can explain why nothing was projected. type projector func(dir string) (Projection, bool, error) // projectors maps each registry_config ecosystem to its implementation. @@ -72,6 +88,11 @@ var projectors = map[string]projector{ // and returns what it produced. Ecosystems are pre-validated by // sandboxprofile.Profile.Validate, so an unknown one is a programming // error rather than user input. +// +// Entries with Projected()==false are still returned when they carry a +// Warning or Rejected mappings: the caller must be able to say why a +// requested projection produced nothing, since silence there reproduces +// the very 404 this package prevents. func Project(ecosystems []string, dir string) ([]Projection, error) { var out []Projection for _, eco := range ecosystems { @@ -83,9 +104,12 @@ func Project(ecosystems []string, dir string) ([]Projection, error) { if err != nil { return nil, err } - if !present { + if !present && p.Warning == "" && len(p.Rejected) == 0 { continue } + if p.Ecosystem == "" { + p.Ecosystem = eco + } out = append(out, p) } return out, nil @@ -105,20 +129,27 @@ func NPMUserConfig() (string, error) { func projectNPM(dir string) (Projection, bool, error) { src, err := NPMUserConfig() if err != nil { - return Projection{}, false, fmt.Errorf("registry_config npm: %w", err) + // Nothing to project and nothing omac can do about it. This is an + // opt-in convenience that only ever ADDS a mapping, so it must not + // take the whole launch down (a systemd unit or minimal container + // with no resolvable home would otherwise never start). + return Projection{Warning: fmt.Sprintf("cannot locate the npm user config: %v", err)}, false, nil } raw, err := os.ReadFile(src) if err != nil { if os.IsNotExist(err) { return Projection{}, false, nil } - return Projection{}, false, fmt.Errorf("registry_config npm: read %s: %w", src, err) + // Same reasoning as above: EACCES on ~/.npmrc (e.g. root-owned + // after a sudo npm config set) is a warning, not a fatal error. + return Projection{Warning: fmt.Sprintf("cannot read %s: %v", src, err)}, false, nil } res := ScrubNPMRC(raw) if len(res.KeptKeys) == 0 { // No mapping means npm's default resolution is already correct; - // writing an empty file would add a grant for no reason. - return Projection{}, false, nil + // writing an empty file would add a grant for no reason. Rejections + // still travel back so the caller can explain the silence. + return Projection{Source: src, Rejected: res.Rejected}, false, nil } dest := filepath.Join(dir, "npmrc") if err := os.WriteFile(dest, res.Content, 0o600); err != nil { @@ -132,6 +163,8 @@ func projectNPM(dir string) (Projection, bool, error) { KeptKeys: res.KeptKeys, Dropped: res.Dropped, StrippedUserinfo: res.StrippedUserinfo, + Rejected: res.Rejected, + NeedsAuth: res.NeedsAuth, }, true, nil } @@ -156,6 +189,10 @@ type Notice struct { Enabled bool // Overridden reflects whether override_deny already exposes Source. Overridden bool + // Rejected lists mappings that exist in the file but cannot be + // projected. Carried so doctor reports them instead of staying silent + // while scoped installs keep 404ing. + Rejected []Rejected } // InspectNPM reports whether ~/.npmrc maps any scope to a non-default @@ -195,7 +232,10 @@ func InspectNPM(enabled, overridden bool) (*Notice, error) { hosts = append(hosts, u.Hostname()) } } - if len(hosts) == 0 { + // A rejected mapping is exactly the case that must not be silent: the + // file has private-registry config, the sandbox cannot use it, and the + // user would otherwise get an unexplained 404. + if len(hosts) == 0 && len(res.Rejected) == 0 { return nil, nil } return &Notice{ @@ -205,9 +245,19 @@ func InspectNPM(enabled, overridden bool) (*Notice, error) { Credentialed: res.DroppedCredentials > 0, Enabled: enabled, Overridden: overridden, + Rejected: res.Rejected, }, nil } +// Rejected is a registry mapping omac recognized but refused to project, +// with the reason. These are reported rather than silently skipped: a +// mapping that does not reach the sandbox leaves the exact 404 this package +// exists to prevent, so silence would recreate the original bug. +type Rejected struct { + Key string + Reason string +} + // ScrubResult is the outcome of scrubbing one config file. type ScrubResult struct { // Content is the projected file body (mapping lines only), empty when @@ -224,6 +274,13 @@ type ScrubResult struct { DroppedCredentials int // StrippedUserinfo counts kept URLs that carried inline credentials. StrippedUserinfo int + // Rejected lists recognized mappings that could not be projected. + Rejected []Rejected + // NeedsAuth lists kept mapping keys whose registry host has a + // credential entry in the source file. omac cannot supply that + // credential (it is exactly what the projection drops), so installs + // against those hosts may fail authentication. + NeedsAuth []string } // scopedRegistryKey matches npm's scoped-registry form, e.g. @@ -236,9 +293,22 @@ var credentialKey = regexp.MustCompile(`(?i)(_auth|_authtoken|_password|username // ScrubNPMRC keeps only registry mappings from an npmrc body. Everything // else — credentials, comments, unrelated knobs — is dropped. +// +// A mapping is kept only if its value resolves to a credential-free +// http(s) URL. npm's own value syntax is honored first (surrounding quotes +// are stripped, ${VAR} is expanded from the environment), so a mapping npm +// would act on is not silently lost. Anything still unusable — or carrying +// a secret outside the userinfo, e.g. ?apiKey= — is recorded in Rejected +// rather than dropped quietly. func ScrubNPMRC(src []byte) ScrubResult { var res ScrubResult var lines []string + // keptHosts maps a kept mapping key to its registry host, so the + // credential correlation below can run after the whole file is read + // (auth lines may appear before or after the mapping they apply to). + keptHosts := map[string]string{} + authHosts := map[string]bool{} + for _, raw := range strings.Split(string(src), "\n") { line := strings.TrimSpace(raw) if line == "" || strings.HasPrefix(line, ";") || strings.HasPrefix(line, "#") { @@ -254,14 +324,15 @@ func ScrubNPMRC(src []byte) ScrubResult { res.Dropped++ if credentialKey.MatchString(key) { res.DroppedCredentials++ + if h := credentialHost(key); h != "" { + authHosts[h] = true + } } continue } - clean, stripped, ok := registryURL(value) - if !ok { - // A mapping whose value is not an http(s) URL is not something - // we can vouch for; dropping is the safe direction. - res.Dropped++ + clean, host, stripped, reason := registryURL(npmValue(value)) + if reason != "" { + res.Rejected = append(res.Rejected, Rejected{Key: key, Reason: reason}) continue } if stripped { @@ -269,13 +340,68 @@ func ScrubNPMRC(src []byte) ScrubResult { } lines = append(lines, key+"="+clean) res.KeptKeys = append(res.KeptKeys, key) + keptHosts[key] = host + } + + // Correlate kept mappings with the credentials that were dropped. The + // global `registry` key is special: pointing npm at a private mirror it + // cannot authenticate to breaks *every* install, including the public + // dependencies that work today with the file masked. That is a + // regression, so it is refused rather than projected. A scoped mapping + // only affects its own scope, which was already failing, so it is kept + // with a warning. + var keptLines []string + var keptKeys []string + for i, key := range res.KeptKeys { + host := keptHosts[key] + if authHosts[host] { + if strings.EqualFold(key, "registry") { + res.Rejected = append(res.Rejected, Rejected{ + Key: key, + Reason: fmt.Sprintf("%s requires authentication that omac cannot supply; projecting the global registry "+ + "would redirect every install there and break the public ones that work today", host), + }) + continue + } + res.NeedsAuth = append(res.NeedsAuth, key) + } + keptKeys = append(keptKeys, key) + keptLines = append(keptLines, lines[i]) } - if len(lines) > 0 { - res.Content = []byte(strings.Join(lines, "\n") + "\n") + res.KeptKeys = keptKeys + if len(keptLines) > 0 { + res.Content = []byte(strings.Join(keptLines, "\n") + "\n") } return res } +// npmValue applies npm's ini value syntax before the URL is validated: +// surrounding quotes are removed and ${VAR}/$VAR are expanded from the +// environment, both of which npm does itself. Without this a perfectly +// good mapping (`@acme:registry="https://npm.acme.test"`) would look +// unparseable and be silently skipped. +func npmValue(value string) string { + v := strings.TrimSpace(value) + if len(v) >= 2 { + if (v[0] == '"' && v[len(v)-1] == '"') || (v[0] == '\'' && v[len(v)-1] == '\'') { + v = v[1 : len(v)-1] + } + } + return os.ExpandEnv(v) +} + +// credentialHost extracts the registry host from a per-registry auth key +// such as "//npm.acme.test/:_authToken" or "//npm.acme.test/api/:_password". +// Returns "" for keys that are not host-scoped (e.g. a bare "_auth"). +func credentialHost(key string) string { + rest, ok := strings.CutPrefix(strings.ToLower(strings.TrimSpace(key)), "//") + if !ok { + return "" + } + host, _, _ := strings.Cut(rest, "/") + return host +} + // isRegistryMapping reports whether key is a registry mapping: the global // `registry` or a scoped `@scope:registry`. Comparison is case-insensitive // because npm lowercases config keys. @@ -284,22 +410,37 @@ func isRegistryMapping(key string) bool { return k == "registry" || scopedRegistryKey.MatchString(k) } -// registryURL validates a mapping value and removes any credentials -// embedded in it. ok=false means the value is not an absolute http(s) URL -// and must not be projected. Note that a hostname merely *containing* -// "token" (api.trustedtokens.eu) is a legitimate registry and is kept — -// the credential check is structural (userinfo), not textual. -func registryURL(value string) (clean string, stripped bool, ok bool) { +// registryURL validates a mapping value and removes credentials embedded in +// it. A non-empty reason means the value must not be projected. Note that a +// hostname merely *containing* "token" (api.trustedtokens.eu) is a +// legitimate registry and is kept — the credential checks are structural, +// not textual. +// +// A query string or fragment is refused outright rather than stripped: it +// is a common place to carry an API key (`?apiKey=…`), and omac cannot tell +// a secret query parameter from a load-bearing one. Stripping it might +// silently change resolution; keeping it would leak the secret into a +// sandbox-readable file. Refusing says so instead. +func registryURL(value string) (clean, host string, stripped bool, reason string) { u, err := url.Parse(value) - if err != nil || u.Host == "" { - return "", false, false + if err != nil { + return "", "", false, fmt.Sprintf("value %q is not a URL", value) + } + if u.Host == "" { + return "", "", false, fmt.Sprintf("value %q has no host", value) } if u.Scheme != "http" && u.Scheme != "https" { - return "", false, false + return "", "", false, fmt.Sprintf("scheme %q is not http(s)", u.Scheme) + } + if u.RawQuery != "" || u.ForceQuery { + return "", "", false, "URL carries a query string, which commonly holds an API key omac must not copy into the sandbox" + } + if u.Fragment != "" { + return "", "", false, "URL carries a fragment" } if u.User != nil { u.User = nil - return u.String(), true, true + stripped = true } - return u.String(), false, true + return u.String(), u.Hostname(), stripped, "" } diff --git a/internal/registryconf/registryconf_test.go b/internal/registryconf/registryconf_test.go index bd292f54..cc77df7f 100644 --- a/internal/registryconf/registryconf_test.go +++ b/internal/registryconf/registryconf_test.go @@ -32,6 +32,7 @@ func TestScrubNPMRCKeepsOnlyRegistryMappings(t *testing.T) { wantKeys []string wantBody string dropped int + rejected int }{ { name: "the #241 shape: bare scope mapping", @@ -76,16 +77,16 @@ func TestScrubNPMRCKeepsOnlyRegistryMappings(t *testing.T) { wantBody: "@tt:registry=https://api.trustedtokens.eu\n", }, { - name: "non-URL mapping value is dropped", + name: "non-URL mapping value is rejected, not silently dropped", src: "registry=not-a-url\n", wantKeys: nil, - dropped: 1, + rejected: 1, }, { - name: "non-http scheme is dropped", + name: "non-http scheme is rejected", src: "registry=file:///tmp/evil\n", wantKeys: nil, - dropped: 1, + rejected: 1, }, { name: "line without = is dropped", @@ -113,6 +114,9 @@ func TestScrubNPMRCKeepsOnlyRegistryMappings(t *testing.T) { if got.Dropped != tt.dropped { t.Errorf("dropped = %d, want %d", got.Dropped, tt.dropped) } + if len(got.Rejected) != tt.rejected { + t.Errorf("rejected = %d (%+v), want %d", len(got.Rejected), got.Rejected, tt.rejected) + } }) } } @@ -265,3 +269,125 @@ func TestProjectUnknownEcosystem(t *testing.T) { t.Fatal("want error for unknown ecosystem") } } + +// --- review findings on the registry_config projection --- + +// TestScrubNPMRCRefusesQueryStringSecret is the leak the review found: a +// secret outside the userinfo (?apiKey=…) was projected verbatim, despite +// the package's "no credential can survive" invariant. +func TestScrubNPMRCRefusesQueryStringSecret(t *testing.T) { + for _, src := range []string{ + "@acme:registry=https://npm.acme.test/api/npm/?apiKey=SUPERSECRET\n", + "@acme:registry=https://user:pw@npm.acme.test/api/?tok=SUPERSECRET\n", + "@acme:registry=https://npm.acme.test/api#SUPERSECRET\n", + } { + got := ScrubNPMRC([]byte(src)) + if strings.Contains(string(got.Content), "SUPERSECRET") { + t.Errorf("projected a secret from the URL: %q (input %q)", got.Content, src) + } + if len(got.Rejected) != 1 { + t.Errorf("rejected = %+v, want 1 entry explaining the refusal (input %q)", got.Rejected, src) + } + } +} + +// TestScrubNPMRCHonorsNpmValueSyntax covers mappings npm acts on but Go's +// url.Parse rejects verbatim. Silently skipping these left the user with the +// exact unexplained 404 this feature exists to prevent. +func TestScrubNPMRCHonorsNpmValueSyntax(t *testing.T) { + t.Run("quoted value", func(t *testing.T) { + got := ScrubNPMRC([]byte("@acme:registry=\"https://npm.acme.test\"\n")) + if want := "@acme:registry=https://npm.acme.test\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } + }) + t.Run("env var interpolation", func(t *testing.T) { + t.Setenv("ART_HOST", "npm.acme.test") + got := ScrubNPMRC([]byte("@acme:registry=https://${ART_HOST}/api/npm/npm/\n")) + if want := "@acme:registry=https://npm.acme.test/api/npm/npm/\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } + }) + t.Run("unset env var is reported, not silently skipped", func(t *testing.T) { + got := ScrubNPMRC([]byte("@acme:registry=https://${OMAC_TEST_UNSET_HOST}/api/\n")) + if len(got.KeptKeys) != 0 { + t.Errorf("kept %v for an unresolvable value", got.KeptKeys) + } + if len(got.Rejected) != 1 { + t.Errorf("rejected = %+v, want 1 entry", got.Rejected) + } + }) +} + +// TestScrubNPMRCRefusesUnauthenticatedGlobalRegistry is the regression the +// review identified: projecting a global `registry` whose token was dropped +// points npm at a private mirror it cannot authenticate to, breaking even +// the public installs that work today with the file fully masked. +func TestScrubNPMRCRefusesUnauthenticatedGlobalRegistry(t *testing.T) { + got := ScrubNPMRC([]byte("registry=https://npm.acme.test\n//npm.acme.test/:_authToken=T\n")) + if len(got.KeptKeys) != 0 { + t.Errorf("projected %v; the global registry needs auth omac cannot supply", got.KeptKeys) + } + if len(got.Rejected) != 1 || !strings.Contains(got.Rejected[0].Reason, "authentication") { + t.Fatalf("rejected = %+v, want one entry explaining the auth problem", got.Rejected) + } + + // A *scoped* mapping to the same host is kept: that scope was already + // failing, so there is no regression — but it must be flagged. + scoped := ScrubNPMRC([]byte("@acme:registry=https://npm.acme.test\n//npm.acme.test/:_authToken=T\n")) + if len(scoped.KeptKeys) != 1 { + t.Errorf("kept = %v, want the scoped mapping", scoped.KeptKeys) + } + if len(scoped.NeedsAuth) != 1 { + t.Errorf("NeedsAuth = %v, want the scoped mapping flagged", scoped.NeedsAuth) + } + + // Without a credential entry the global mapping is fine to project. + plain := ScrubNPMRC([]byte("registry=https://npm.acme.test\n")) + if len(plain.KeptKeys) != 1 || len(plain.NeedsAuth) != 0 { + t.Errorf("kept = %v, needsAuth = %v; want the mapping projected cleanly", plain.KeptKeys, plain.NeedsAuth) + } +} + +// TestProjectNPMUnreadableConfigIsNotFatal keeps an opt-in convenience from +// taking the whole launch down. +func TestProjectNPMUnreadableConfigIsNotFatal(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + // A directory where the file is expected makes the read fail with + // something other than IsNotExist. + if err := os.Mkdir(filepath.Join(home, ".npmrc"), 0o755); err != nil { + t.Fatal(err) + } + projs, err := Project([]string{sandboxprofile.RegistryConfigNPM}, t.TempDir()) + if err != nil { + t.Fatalf("read failure must not be fatal, got: %v", err) + } + if len(projs) != 1 || projs[0].Projected() { + t.Fatalf("projections = %+v, want one non-projected entry", projs) + } + if projs[0].Warning == "" { + t.Error("no warning explaining why nothing was projected") + } +} + +// TestInspectNPMReportsRejections keeps doctor from going silent on config +// it cannot use. +func TestInspectNPMReportsRejections(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + body := "@acme:registry=https://npm.acme.test/api/?apiKey=SECRET\n" + if err := os.WriteFile(filepath.Join(home, ".npmrc"), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + notice, err := InspectNPM(false, false) + if err != nil { + t.Fatal(err) + } + if notice == nil { + t.Fatal("no notice for an npmrc whose mapping cannot be projected") + } + if len(notice.Rejected) != 1 { + t.Errorf("notice.Rejected = %+v, want 1 entry", notice.Rejected) + } +} diff --git a/internal/sandboxrun/registryconf.go b/internal/sandboxrun/registryconf.go index efdca3f4..669e4ed1 100644 --- a/internal/sandboxrun/registryconf.go +++ b/internal/sandboxrun/registryconf.go @@ -41,28 +41,48 @@ func setupRegistryConfig(merged *sandboxprofile.Profile, grants *Grants, injecte cleanup() return noop, err } - if len(projections) == 0 { - // Nothing to project (no config file, or no registry mapping in - // it). Say so: the user asked for a projection and got none, and - // silence here reads as "it worked". - fmt.Fprintf(stderr, "omac sandbox: registry_config (%s): no registry mapping found to project; "+ - "scoped packages will resolve against the default registry\n", strings.Join(ecosystems, ", ")) - cleanup() - return noop, nil - } - overrides := sandboxprofile.BuildOverrideLookup(merged.Filesystem.OverrideDeny) + granted := 0 for _, p := range projections { + // Every rejection is reported: a mapping that does not reach the + // sandbox produces exactly the silent 404 this feature exists to + // prevent, so it must never be dropped quietly. + for _, r := range p.Rejected { + fmt.Fprintf(stderr, "omac sandbox: WARNING: registry_config %s: not projecting %q — %s\n", + p.Ecosystem, r.Key, r.Reason) + } + if p.Warning != "" { + fmt.Fprintf(stderr, "omac sandbox: WARNING: registry_config %s: %s\n", p.Ecosystem, p.Warning) + } + if !p.Projected() { + continue + } + // Grant exactly the projected file, read-only. The host file is // untouched and stays protected. grants.ReadPaths = append(grants.ReadPaths, p.Path) injected[p.EnvVar] = p.Path + granted++ fmt.Fprintf(stderr, "omac sandbox: registry_config: %s\n", p.Summary()) + if len(p.NeedsAuth) > 0 { + fmt.Fprintf(stderr, "omac sandbox: WARNING: registry_config %s: %s point at a registry that needs authentication, "+ + "and the credential is deliberately not copied into the sandbox — installs from it may fail with 401/403. "+ + "Supply the token to the registry another way, or expect those packages to be unavailable.\n", + p.Ecosystem, strings.Join(p.NeedsAuth, ", ")) + } if overrides[p.Source] { fmt.Fprintf(stderr, "omac sandbox: WARNING: %s is also in filesystem.override_deny, so the sandbox can read the "+ "real file including any auth token it holds. The projection makes that grant unnecessary — "+ "drop the override_deny entry to keep the credential protected.\n", p.Source) } } + if granted == 0 { + // The user asked for a projection and got none; silence here reads + // as "it worked". + fmt.Fprintf(stderr, "omac sandbox: registry_config (%s): no registry mapping was projected; "+ + "scoped packages will resolve against the default registry\n", strings.Join(ecosystems, ", ")) + cleanup() + return noop, nil + } return cleanup, nil } From c8047789847c586d42a38c8154bf60b92b69a1a4 Mon Sep 17 00:00:00 2001 From: Ilia Zhuravok Date: Thu, 27 Aug 2026 15:10:02 +0100 Subject: [PATCH 3/3] fix(sandbox,cli): harden the registry projection against review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of #259 returned nine findings. All are addressed here; the two substantive ones broke guarantees the PR states. P1 (MUST). registryURL returned u.Hostname() — port stripped — while credentialHost returned a lowercased host:port, so a port-scoped credential never correlated with its mapping: registry=https://npm.acme.test:8443 //npm.acme.test:8443/:_authToken=T projected the global registry without the token it needs — exactly the regression eae1299 claimed to close. A second facet the review did not mention: credentialHost lowercased and u.Hostname() did not, so a mixed-case mapping host escaped correlation too. Both sides now normalize through one helper (lowercase, strip an explicit default port, matching npm's own nerfDart keying). Ports are stripped regardless of scheme deliberately: a credential key carries no scheme, and any asymmetry there risks under-correlating, which silently reopens this hole. P2 (SHOULD). npmValue called os.ExpandEnv on the whole value, so a secret could be interpolated into a URL path — a position neither stripped like userinfo nor refused like a query string — making the package's "no credential can survive by construction" claim false. Expansion is now split at the authority boundary: the authority expands normally (the corporate ${ART_HOST} shape keeps working), and if the remainder consumes any placeholder the mapping is refused with the variable named. The refusal decision is made by os.Expand itself, so the placeholder syntax cannot drift from the syntax the authority expansion uses, and no expanded *value* is ever matched against the URL — so there are no coincidental-substring refusals. Also fixed: - `//host/:always-auth=true` was classified as credential material via a bare `^//`, making doctor claim the file "holds an auth token" for a boolean. Classification now matches on the key's leaf against _auth/_authtoken/ _password only. - Host-less legacy credentials (`_auth`, `_password`) never correlated, so `registry=…` + `_auth=…` was projected unflagged, contradicting the stated refusal rationale. npm applies those to the default registry, so they now bear on the global mapping; scoped mappings are unaffected. Semantics pinned by comment and test. - A UTF-8 BOM glued itself to the first key, landing the mapping in Dropped where neither the launch path nor doctor reports anything — the silent 404 this feature exists to prevent. Trimmed once. - KeptKeys and lines were parallel slices paired by index across a re-filtering pass, which any future `continue` would desync silently, and duplicate keys projected twice. Replaced by one []mapping with explicit last-wins (npm ini semantics). - A pre-set NPM_CONFIG_USERCONFIG was silently overridden by the projection, dropping the user's own config. Now warned, naming the overridden path. - doctor swallowed a non-ENOENT read failure on ~/.npmrc that the launch path warns about, leaving the one check that runs *before* a launch silent. - Strict profile decoding rejects a newer profile with a bare "unknown field", giving no clue about version skew. The unknown-field case now says which direction to look; other parse errors are unchanged. Forward incompatibility documented in CONFIGURATION.md. - Package doc and CONFIGURATION.md restated to match what the code guarantees (authority-only expansion, query/fragment refused). Tests: port-scoped, mixed-case, default-port and unrelated-host correlation; expansion in host/port/userinfo/path positions plus a literal `$`; boolean host-scoped key; host-less credential for global vs scoped; BOM; CRLF; duplicate keys; symlinked ~/.npmrc; pre-set env var; doctor read failure; unknown-field hint. The HOME test helper also sets USERPROFILE — not for Windows support (.goreleaser.yaml builds linux and darwin only), just so the helper does not lie about what it stages. Verified the acceptance test is unaffected by the stricter scrubber: cold cache, knob on, no override_deny -> 17 skainet models, plugin 1.0.3 inside the sandbox: cat ~/.npmrc -> No such file or directory cat $NPM_CONFIG_USERCONFIG -> the mapping line only grep -c "_auth|_password|apiKey" -> 0 go build ./..., go vet ./internal/... clean; go test ./... passes except the two integration tests that fail identically on clean main on this host and pass in CI. Refs #150 Refs #241 Co-Authored-By: Claude Opus 5 Signed-off-by: Ilia Zhuravok --- docs/CONFIGURATION.md | 14 +- internal/cli/doctor.go | 11 +- internal/cli/doctor_registryconf_test.go | 35 +++ internal/registryconf/registryconf.go | 221 ++++++++++++---- .../registryconf_hardening_test.go | 248 ++++++++++++++++++ internal/registryconf/registryconf_test.go | 10 +- internal/sandboxprofile/profile.go | 9 + internal/sandboxprofile/profile_test.go | 26 ++ internal/sandboxrun/registryconf.go | 10 + internal/sandboxrun/registryconf_test.go | 50 ++++ 10 files changed, 580 insertions(+), 54 deletions(-) create mode 100644 internal/registryconf/registryconf_hardening_test.go diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index dd46a316..ac63b63d 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -156,8 +156,12 @@ an API key usually hides (`?apiKey=…`), and omac cannot tell a secret parameter from a load-bearing one. npm's own value syntax is honored first, so a mapping npm would act on is -not lost: surrounding quotes are removed and `${VAR}` is expanded from the -environment. +not lost: surrounding quotes are removed, and `${VAR}` is expanded — but +**only inside the URL's authority**, so the corporate +`https://${ART_HOST}/api/npm/` shape works while +`https://host/api/${SECRET}/npm/` is refused. A path segment is not +strippable the way userinfo is, and omac cannot tell an interpolated secret +from an interpolated path. Two cases are deliberately **not** projected, and both are reported at launch and by `omac doctor` rather than skipped quietly: @@ -173,6 +177,12 @@ launch and by `omac doctor` rather than skipped quietly: Private registries usually also need their host in `network.allow_domain` (or an allow at the network prompt). +> **Sharing a profile across machines.** Profiles are parsed strictly — +> an unknown field is an error, so a typo cannot silently weaken the +> sandbox. The trade-off is that a profile using `registry_config` is +> rejected by an omac older than the release that added it. If you share or +> check in a profile, upgrade omac everywhere before adding the field. + The blunt alternative — `override_deny: ["~/.npmrc"]` — also works, but grants the whole file including any token. `omac doctor` flags a private mapping the sandbox cannot see, and warns when `override_deny` is doing diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index b733e624..9cc57652 100644 --- a/internal/cli/doctor.go +++ b/internal/cli/doctor.go @@ -226,7 +226,16 @@ func doctorRegistryConfig(env *Env, profileRef string) { overridden := sandboxprofile.BuildOverrideLookup(profile.Filesystem.OverrideDeny)[src] notice, err := registryconf.InspectNPM(enabled, overridden) - if err != nil || notice == nil { + if err != nil { + // The launch path turns the same failure into a projection warning + // (registryconf.projectNPM), so staying silent here would mean the + // only place that can warn *before* a run does not. + fmt.Fprintf(env.Stdout, "[warn] registry config: cannot inspect %s: %v\n", src, err) + fmt.Fprintf(env.Stdout, " A private-registry mapping in that file cannot be projected, so scoped\n") + fmt.Fprintf(env.Stdout, " installs may fail with a 404 against the public registry.\n") + return + } + if notice == nil { return } hosts := strings.Join(notice.Hosts, ", ") diff --git a/internal/cli/doctor_registryconf_test.go b/internal/cli/doctor_registryconf_test.go index bfa5df37..81e4601c 100644 --- a/internal/cli/doctor_registryconf_test.go +++ b/internal/cli/doctor_registryconf_test.go @@ -193,3 +193,38 @@ func TestDoctorRegistryConfigReportsUnusableMapping(t *testing.T) { t.Fatalf("doctor echoed the secret; got:\n%s", out) } } + +// TestDoctorRegistryConfigReportsUnreadableConfig covers the review finding +// that the launch path warns about an unreadable ~/.npmrc while doctor — the +// one place that can warn *before* a run — printed nothing. +func TestDoctorRegistryConfigReportsUnreadableConfig(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + workdir := t.TempDir() + writeWorkdirConfig(t, workdir, "builtin", []string{ + "{{self}}", "sandbox", "run", + "--profile", "default", + "--", "{{inner_cmd}}", "{{inner_args}}", + }) + stageProfile(t, home, `{"meta": {"name": "default"}, "environment": {"allow_vars": ["HOME"]}}`) + // A directory where the file belongs makes the read fail with something + // other than IsNotExist. + if err := os.Mkdir(filepath.Join(home, ".npmrc"), 0o755); err != nil { + t.Fatal(err) + } + + env, outBuf, _, drain := newPipeEnv(t, "") + env.Workdir = workdir + if code := runDoctor([]string{}, env); code != ExitOK { + t.Errorf("doctor exit = %d, want ExitOK (advisory)", code) + } + drain() + out := outBuf.String() + + if !strings.Contains(out, "cannot inspect") { + t.Errorf("doctor stayed silent on an unreadable npmrc; got:\n%s", out) + } + if !strings.Contains(out, "404") { + t.Errorf("doctor did not explain the consequence; got:\n%s", out) + } +} diff --git a/internal/registryconf/registryconf.go b/internal/registryconf/registryconf.go index d1f79c0e..51c15375 100644 --- a/internal/registryconf/registryconf.go +++ b/internal/registryconf/registryconf.go @@ -10,9 +10,12 @@ // fails with a 404 that reads like "no such package" rather than "your // registry configuration is invisible". See #150 / #241. // -// No credential can survive by construction: only registry-mapping keys -// are kept, a kept value must parse as an http(s) URL, and any userinfo -// in that URL is removed. +// No credential can survive by construction. Only registry-mapping keys are +// kept; a kept value must parse as an http(s) URL; userinfo in that URL is +// removed; and every remaining position where a secret could hide is refused +// rather than copied — a query string or fragment (`?apiKey=…`), and a +// ${VAR} interpolated anywhere but the URL's authority. What omac cannot +// distinguish from a load-bearing value, it declines to project and reports. package registryconf import ( @@ -287,29 +290,69 @@ type ScrubResult struct { // "@acme:registry" or "@acme/sub:registry". var scopedRegistryKey = regexp.MustCompile(`^@[^:\s]+:registry$`) -// credentialKey matches npmrc keys that carry authentication material, -// including the per-registry form "//host/path/:_authToken". -var credentialKey = regexp.MustCompile(`(?i)(_auth|_authtoken|_password|username|email|^//)`) +// credentialLeafKey matches the npmrc keys that actually carry +// authentication material. It is applied to the key's *leaf* — the part +// after the last ":" — so the per-registry form "//host/path/:_authToken" +// matches while "//host/:always-auth" (a boolean) does not. Matching the +// whole key on a bare "^//" would classify every host-scoped setting as a +// credential and make doctor claim the file "holds an auth token" for a +// flag. +var credentialLeafKey = regexp.MustCompile(`(?i)^(_auth|_authtoken|_password)$`) + +// isCredentialKey reports whether an npmrc key carries authentication +// material. Note that `username`/`email` are deliberately excluded: they are +// dropped like every other non-mapping key, but they are not tokens, and +// counting them would overstate what a projection protects. +func isCredentialKey(key string) bool { + k := strings.ToLower(strings.TrimSpace(key)) + leaf := k + if i := strings.LastIndex(k, ":"); i >= 0 { + leaf = k[i+1:] + } + return credentialLeafKey.MatchString(leaf) +} + +// bom is the UTF-8 byte-order mark. A Windows-authored npmrc can start with +// one, which would otherwise glue itself to the first key and make that +// mapping unrecognizable — landing it in Dropped, where nothing reports it. +const bom = "\uFEFF" + +// mapping is one projected registry line, carried as a unit so the key, its +// correlation host and its rendered line cannot drift apart. The previous +// shape kept these in parallel slices paired by index across a re-filtering +// pass, which any future `continue` would have desynced silently. +type mapping struct { + key string + host string // normalized host[:port], for credential correlation + line string // "key=value" as projected +} // ScrubNPMRC keeps only registry mappings from an npmrc body. Everything // else — credentials, comments, unrelated knobs — is dropped. // // A mapping is kept only if its value resolves to a credential-free // http(s) URL. npm's own value syntax is honored first (surrounding quotes -// are stripped, ${VAR} is expanded from the environment), so a mapping npm -// would act on is not silently lost. Anything still unusable — or carrying -// a secret outside the userinfo, e.g. ?apiKey= — is recorded in Rejected -// rather than dropped quietly. +// are stripped, ${VAR} is expanded in the URL's authority), so a mapping npm +// would act on is not silently lost. Anything still unusable — or carrying a +// secret anywhere but the userinfo, e.g. `?apiKey=` or an interpolated path +// segment — is recorded in Rejected rather than dropped quietly. +// +// Duplicate keys follow npm's ini semantics: last one wins, and only that +// one is projected. func ScrubNPMRC(src []byte) ScrubResult { var res ScrubResult - var lines []string - // keptHosts maps a kept mapping key to its registry host, so the - // credential correlation below can run after the whole file is read - // (auth lines may appear before or after the mapping they apply to). - keptHosts := map[string]string{} + var mappings []mapping + // authHosts holds the normalized hosts that have a credential entry. + // Correlation runs after the whole file is read because an auth line may + // appear before or after the mapping it applies to. authHosts := map[string]bool{} + // defaultRegistryCredential records a host-less legacy credential + // (`_auth`, `_password`). npm applies those to the *default* registry, so + // they bear on the global `registry` mapping even though they name no host. + defaultRegistryCredential := false - for _, raw := range strings.Split(string(src), "\n") { + body := strings.TrimPrefix(string(src), bom) + for _, raw := range strings.Split(body, "\n") { line := strings.TrimSpace(raw) if line == "" || strings.HasPrefix(line, ";") || strings.HasPrefix(line, "#") { continue @@ -322,15 +365,22 @@ func ScrubNPMRC(src []byte) ScrubResult { key, value = strings.TrimSpace(key), strings.TrimSpace(value) if !isRegistryMapping(key) { res.Dropped++ - if credentialKey.MatchString(key) { + if isCredentialKey(key) { res.DroppedCredentials++ if h := credentialHost(key); h != "" { authHosts[h] = true + } else { + defaultRegistryCredential = true } } continue } - clean, host, stripped, reason := registryURL(npmValue(value)) + expanded, reason := npmValue(value) + if reason != "" { + res.Rejected = append(res.Rejected, Rejected{Key: key, Reason: reason}) + continue + } + clean, host, stripped, reason := registryURL(expanded) if reason != "" { res.Rejected = append(res.Rejected, Rejected{Key: key, Reason: reason}) continue @@ -338,9 +388,14 @@ func ScrubNPMRC(src []byte) ScrubResult { if stripped { res.StrippedUserinfo++ } - lines = append(lines, key+"="+clean) - res.KeptKeys = append(res.KeptKeys, key) - keptHosts[key] = host + m := mapping{key: key, host: host, line: key + "=" + clean} + // Last one wins, matching npm's ini parser, so a duplicated key is + // projected once with the effective value. + if i := indexOfKey(mappings, key); i >= 0 { + mappings[i] = m + continue + } + mappings = append(mappings, m) } // Correlate kept mappings with the credentials that were dropped. The @@ -350,56 +405,126 @@ func ScrubNPMRC(src []byte) ScrubResult { // regression, so it is refused rather than projected. A scoped mapping // only affects its own scope, which was already failing, so it is kept // with a warning. - var keptLines []string - var keptKeys []string - for i, key := range res.KeptKeys { - host := keptHosts[key] - if authHosts[host] { - if strings.EqualFold(key, "registry") { + var lines []string + for _, m := range mappings { + isGlobal := strings.EqualFold(m.key, "registry") + needsAuth := authHosts[m.host] || (isGlobal && defaultRegistryCredential) + if needsAuth { + if isGlobal { res.Rejected = append(res.Rejected, Rejected{ - Key: key, + Key: m.key, Reason: fmt.Sprintf("%s requires authentication that omac cannot supply; projecting the global registry "+ - "would redirect every install there and break the public ones that work today", host), + "would redirect every install there and break the public ones that work today", m.host), }) continue } - res.NeedsAuth = append(res.NeedsAuth, key) + res.NeedsAuth = append(res.NeedsAuth, m.key) } - keptKeys = append(keptKeys, key) - keptLines = append(keptLines, lines[i]) + res.KeptKeys = append(res.KeptKeys, m.key) + lines = append(lines, m.line) } - res.KeptKeys = keptKeys - if len(keptLines) > 0 { - res.Content = []byte(strings.Join(keptLines, "\n") + "\n") + if len(lines) > 0 { + res.Content = []byte(strings.Join(lines, "\n") + "\n") } return res } +// indexOfKey finds a mapping by key, case-insensitively (npm lowercases +// config keys, so `Registry` and `registry` are the same setting). +func indexOfKey(mappings []mapping, key string) int { + for i, m := range mappings { + if strings.EqualFold(m.key, key) { + return i + } + } + return -1 +} + // npmValue applies npm's ini value syntax before the URL is validated: -// surrounding quotes are removed and ${VAR}/$VAR are expanded from the -// environment, both of which npm does itself. Without this a perfectly -// good mapping (`@acme:registry="https://npm.acme.test"`) would look -// unparseable and be silently skipped. -func npmValue(value string) string { +// surrounding quotes are removed, and ${VAR}/$VAR is expanded — but only +// inside the URL's authority. +// +// The restriction is what keeps "no credential can survive" true. Expansion +// exists for the corporate `https://${ART_HOST}/api/npm/` shape, but +// os.ExpandEnv applied to the whole value would happily interpolate a secret +// into a path segment (`https://host/api/${SECRET}/npm/`), which — unlike +// userinfo — is not stripped and unlike a query string was not refused. So +// the value is split at the authority boundary: the authority is expanded +// normally, and if the remainder consumes any placeholder the mapping is +// refused with the variable named. +// +// The split is structural (find "://", then the first "/", "?" or "#") rather +// than a URL parse, because an unexpanded template frequently does not parse. +// The remainder's refusal decision is made by os.Expand itself, so the +// placeholder syntax it recognizes — ${NAME}, $NAME, $$ escaping — cannot +// drift from the syntax the authority expansion uses. +func npmValue(value string) (string, string) { v := strings.TrimSpace(value) if len(v) >= 2 { if (v[0] == '"' && v[len(v)-1] == '"') || (v[0] == '\'' && v[len(v)-1] == '\'') { v = v[1 : len(v)-1] } } - return os.ExpandEnv(v) + authority, remainder := splitAuthority(v) + + var interpolated []string + remainder = os.Expand(remainder, func(name string) string { + interpolated = append(interpolated, name) + return os.Getenv(name) + }) + if len(interpolated) > 0 { + return "", fmt.Sprintf("${%s} is interpolated into the URL path, where omac cannot tell a secret from a "+ + "path segment; only host-position ${VAR} is projected", interpolated[0]) + } + return os.ExpandEnv(authority) + remainder, "" +} + +// splitAuthority divides a URL template into its authority (scheme plus +// "://" plus host[:port], including any userinfo) and everything after it. +// A value with no "://" has no authority to expand; it is returned entirely +// as the remainder, and the http(s) validation in registryURL rejects it. +func splitAuthority(v string) (authority, remainder string) { + i := strings.Index(v, "://") + if i < 0 { + return "", v + } + rest := v[i+len("://"):] + if j := strings.IndexAny(rest, "/?#"); j >= 0 { + return v[:i+len("://")+j], rest[j:] + } + return v, "" } // credentialHost extracts the registry host from a per-registry auth key -// such as "//npm.acme.test/:_authToken" or "//npm.acme.test/api/:_password". -// Returns "" for keys that are not host-scoped (e.g. a bare "_auth"). +// such as "//npm.acme.test/:_authToken" or "//npm.acme.test:8443/api/:_password", +// normalized the same way registryURL normalizes a mapping's host so the two +// correlate. Returns "" for keys that are not host-scoped (e.g. a bare +// "_auth"), which npm applies to the default registry instead. func credentialHost(key string) string { - rest, ok := strings.CutPrefix(strings.ToLower(strings.TrimSpace(key)), "//") + rest, ok := strings.CutPrefix(strings.TrimSpace(key), "//") if !ok { return "" } host, _, _ := strings.Cut(rest, "/") - return host + return normalizeHost(host) +} + +// normalizeHost lowercases a host[:port] and strips an explicit default port, +// so `https://host:443` and `//host/:_authToken` describe the same registry. +// npm's own credential keying (nerfDart) normalizes the same way; without it a +// port-scoped or mixed-case credential would silently fail to correlate with +// its mapping, and the global-registry refusal this feature relies on would +// not fire. +// +// Both ports are stripped regardless of scheme, deliberately. A credential key +// carries no scheme (`//host:port/:_authToken`), so a scheme-aware rule could +// only be applied to one side, and any asymmetry there risks *under*- +// correlating — which silently reopens the regression this guards. The one +// inaccuracy this accepts, `http://host:443` matching a `//host/` credential, +// fails in the safe direction: it refuses a mapping and says why. +func normalizeHost(hostPort string) string { + h := strings.ToLower(strings.TrimSpace(hostPort)) + return strings.TrimSuffix(strings.TrimSuffix(h, ":443"), ":80") } // isRegistryMapping reports whether key is a registry mapping: the global @@ -442,5 +567,9 @@ func registryURL(value string) (clean, host string, stripped bool, reason string u.User = nil stripped = true } - return u.String(), u.Hostname(), stripped, "" + // The returned host keeps its port and is normalized, so a port-scoped + // credential (`//host:8443/:_authToken`) correlates with the mapping it + // applies to. Returning u.Hostname() here dropped the port and silently + // defeated that correlation. + return u.String(), normalizeHost(u.Host), stripped, "" } diff --git a/internal/registryconf/registryconf_hardening_test.go b/internal/registryconf/registryconf_hardening_test.go new file mode 100644 index 00000000..14454fd5 --- /dev/null +++ b/internal/registryconf/registryconf_hardening_test.go @@ -0,0 +1,248 @@ +package registryconf + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tngtech/oh-my-agentic-coder/internal/sandboxprofile" +) + +// Tests for the PR #259 review findings. Each case is a shape that slipped +// through the first round: the credential correlation was keyed on a host +// spelled differently on each side, and the value expansion could smuggle a +// secret into a position nothing stripped or refused. + +// setHome points os.UserHomeDir at dir. USERPROFILE is set alongside HOME +// because os.UserHomeDir reads that variable on Windows; omac ships linux and +// darwin only (.goreleaser.yaml), so this costs nothing today and keeps the +// helper honest if that ever changes. +func setHome(t *testing.T, dir string) { + t.Helper() + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) +} + +// --- P1: credential correlation must survive port and case differences --- + +func TestCorrelationMatchesPortScopedCredential(t *testing.T) { + // The reported gap: registryURL returned a portless host while a + // credential key kept its port, so this pair never correlated and the + // global registry was projected without the token it needs. + got := ScrubNPMRC([]byte( + "registry=https://npm.acme.test:8443\n" + + "//npm.acme.test:8443/:_authToken=T\n")) + if len(got.KeptKeys) != 0 { + t.Errorf("projected %v; a global registry needing auth must be refused", got.KeptKeys) + } + if len(got.Rejected) != 1 || !strings.Contains(got.Rejected[0].Reason, "authentication") { + t.Fatalf("rejected = %+v, want one entry naming the auth problem", got.Rejected) + } + + // Same host:port, but scoped — kept, and flagged. + scoped := ScrubNPMRC([]byte( + "@acme:registry=https://npm.acme.test:8443\n" + + "//npm.acme.test:8443/:_authToken=T\n")) + if len(scoped.KeptKeys) != 1 { + t.Errorf("kept = %v, want the scoped mapping projected", scoped.KeptKeys) + } + if len(scoped.NeedsAuth) != 1 { + t.Errorf("NeedsAuth = %v, want the scoped mapping flagged", scoped.NeedsAuth) + } +} + +func TestCorrelationIsCaseInsensitive(t *testing.T) { + // url.Parse preserves host case while credential keys were lowercased, + // so a mixed-case mapping used to escape correlation too. + got := ScrubNPMRC([]byte( + "registry=https://NPM.Acme.Test\n" + + "//npm.acme.test/:_authToken=T\n")) + if len(got.KeptKeys) != 0 || len(got.Rejected) != 1 { + t.Errorf("kept=%v rejected=%+v; want the global mapping refused", got.KeptKeys, got.Rejected) + } +} + +func TestCorrelationNormalizesDefaultPort(t *testing.T) { + // npm's own credential keying strips a default port; ours must agree, or + // an explicit :443 in the mapping hides the credential. + got := ScrubNPMRC([]byte( + "registry=https://npm.acme.test:443\n" + + "//npm.acme.test/:_authToken=T\n")) + if len(got.KeptKeys) != 0 || len(got.Rejected) != 1 { + t.Errorf("kept=%v rejected=%+v; want the global mapping refused", got.KeptKeys, got.Rejected) + } +} + +func TestCorrelationIgnoresUnrelatedHost(t *testing.T) { + // The flip side: a credential for a different host must not suppress a + // perfectly good mapping. + got := ScrubNPMRC([]byte( + "registry=https://npm.acme.test\n" + + "//other.example/:_authToken=T\n")) + if len(got.KeptKeys) != 1 || len(got.NeedsAuth) != 0 { + t.Errorf("kept=%v needsAuth=%v; want the mapping projected unflagged", got.KeptKeys, got.NeedsAuth) + } +} + +// --- P2: ${VAR} may only supply the authority --- + +func TestExpansionAllowedInAuthorityOnly(t *testing.T) { + t.Setenv("OMAC_TEST_ART_HOST", "npm.acme.test") + t.Setenv("OMAC_TEST_SECRET", "LIVESECRET") + + t.Run("host position is projected", func(t *testing.T) { + got := ScrubNPMRC([]byte("@acme:registry=https://${OMAC_TEST_ART_HOST}/api/npm/npm/\n")) + if want := "@acme:registry=https://npm.acme.test/api/npm/npm/\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } + }) + + t.Run("path position is refused and names the variable", func(t *testing.T) { + got := ScrubNPMRC([]byte("@acme:registry=https://npm.acme.test/api/${OMAC_TEST_SECRET}/npm/\n")) + if strings.Contains(string(got.Content), "LIVESECRET") { + t.Fatalf("projected an expanded secret: %q", got.Content) + } + if len(got.KeptKeys) != 0 { + t.Errorf("kept = %v, want nothing projected", got.KeptKeys) + } + if len(got.Rejected) != 1 { + t.Fatalf("rejected = %+v, want 1 entry", got.Rejected) + } + if !strings.Contains(got.Rejected[0].Reason, "OMAC_TEST_SECRET") { + t.Errorf("reason does not name the variable: %q", got.Rejected[0].Reason) + } + }) + + t.Run("port position is projected", func(t *testing.T) { + t.Setenv("OMAC_TEST_PORT", "8443") + got := ScrubNPMRC([]byte("@acme:registry=https://npm.acme.test:${OMAC_TEST_PORT}/npm/\n")) + if want := "@acme:registry=https://npm.acme.test:8443/npm/\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } + }) + + t.Run("interpolated userinfo is stripped, not projected", func(t *testing.T) { + t.Setenv("OMAC_TEST_USER", "dev") + t.Setenv("OMAC_TEST_PW", "hunter2") + got := ScrubNPMRC([]byte("@acme:registry=https://${OMAC_TEST_USER}:${OMAC_TEST_PW}@npm.acme.test/npm/\n")) + if strings.Contains(string(got.Content), "hunter2") { + t.Fatalf("projected an interpolated credential: %q", got.Content) + } + if got.StrippedUserinfo != 1 { + t.Errorf("StrippedUserinfo = %d, want 1", got.StrippedUserinfo) + } + }) + + t.Run("a literal dollar in the path is not a placeholder", func(t *testing.T) { + // Guards against over-refusing: os.Expand leaves a `$` that is not + // followed by a name untouched, so this must still project. + got := ScrubNPMRC([]byte("@acme:registry=https://npm.acme.test/api/$/npm/\n")) + if len(got.KeptKeys) != 1 { + t.Errorf("kept = %v (rejected %+v), want the mapping projected", got.KeptKeys, got.Rejected) + } + }) +} + +// --- P3 / P4: what counts as a credential, and where it applies --- + +func TestHostScopedNonCredentialIsNotCountedAsCredential(t *testing.T) { + // `always-auth` is a boolean, not a token: counting it made doctor claim + // the file "holds an auth token". + got := ScrubNPMRC([]byte( + "@acme:registry=https://npm.acme.test\n" + + "//npm.acme.test/:always-auth=true\n")) + if got.DroppedCredentials != 0 { + t.Errorf("DroppedCredentials = %d, want 0 for a boolean flag", got.DroppedCredentials) + } + if len(got.NeedsAuth) != 0 { + t.Errorf("NeedsAuth = %v; a boolean flag is not a credential", got.NeedsAuth) + } + if len(got.KeptKeys) != 1 { + t.Errorf("kept = %v, want the mapping projected", got.KeptKeys) + } +} + +func TestHostlessCredentialAppliesToGlobalRegistry(t *testing.T) { + // npm applies legacy host-less `_auth`/`_password` to the default + // registry, so they bear on the global mapping even though they name no + // host. Pinned here because the alternative reading (correlate only + // //host/-scoped keys) leaves the stated refusal rationale untrue. + got := ScrubNPMRC([]byte( + "registry=https://npm.acme.test\n" + + "_auth=BASE64SECRET\n")) + if len(got.KeptKeys) != 0 { + t.Errorf("projected %v; the global registry has a credential omac cannot supply", got.KeptKeys) + } + if len(got.Rejected) != 1 { + t.Fatalf("rejected = %+v, want 1 entry", got.Rejected) + } + + // A scoped mapping is unaffected: a host-less credential says nothing + // about a scope's own registry. + scoped := ScrubNPMRC([]byte( + "@acme:registry=https://npm.acme.test\n" + + "_auth=BASE64SECRET\n")) + if len(scoped.KeptKeys) != 1 || len(scoped.NeedsAuth) != 0 { + t.Errorf("kept=%v needsAuth=%v; want the scoped mapping projected unflagged", + scoped.KeptKeys, scoped.NeedsAuth) + } +} + +// --- P5 / P6 and file-shape edge cases --- + +func TestScrubHandlesBOM(t *testing.T) { + // A Windows-authored npmrc starts with a BOM, which used to glue itself + // to the first key — landing the mapping in Dropped, where neither the + // launch path nor doctor reports anything. + got := ScrubNPMRC([]byte("\uFEFF@acme:registry=https://npm.acme.test\n")) + if len(got.KeptKeys) != 1 { + t.Fatalf("kept = %v (dropped %d, rejected %+v), want the mapping projected", + got.KeptKeys, got.Dropped, got.Rejected) + } + if want := "@acme:registry=https://npm.acme.test\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } +} + +func TestScrubHandlesCRLF(t *testing.T) { + got := ScrubNPMRC([]byte("@acme:registry=https://npm.acme.test\r\n//npm.acme.test/:_authToken=T\r\n")) + if want := "@acme:registry=https://npm.acme.test\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } + if len(got.NeedsAuth) != 1 { + t.Errorf("NeedsAuth = %v; the CR must not defeat correlation", got.NeedsAuth) + } +} + +func TestDuplicateKeyLastWins(t *testing.T) { + got := ScrubNPMRC([]byte( + "@acme:registry=https://first.example\n" + + "@acme:registry=https://second.example\n")) + if len(got.KeptKeys) != 1 { + t.Errorf("kept = %v, want one entry (npm ini semantics: last wins)", got.KeptKeys) + } + if want := "@acme:registry=https://second.example\n"; string(got.Content) != want { + t.Errorf("content = %q, want %q", got.Content, want) + } +} + +func TestProjectNPMFollowsSymlinkedConfig(t *testing.T) { + home := t.TempDir() + setHome(t, home) + real := filepath.Join(t.TempDir(), "npmrc.real") + if err := os.WriteFile(real, []byte("@acme:registry=https://npm.acme.test\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, filepath.Join(home, ".npmrc")); err != nil { + t.Skipf("symlink unsupported here: %v", err) + } + + projs, err := Project([]string{sandboxprofile.RegistryConfigNPM}, t.TempDir()) + if err != nil { + t.Fatal(err) + } + if len(projs) != 1 || !projs[0].Projected() { + t.Fatalf("projections = %+v, want one usable projection through the symlink", projs) + } +} diff --git a/internal/registryconf/registryconf_test.go b/internal/registryconf/registryconf_test.go index cc77df7f..242b3148 100644 --- a/internal/registryconf/registryconf_test.go +++ b/internal/registryconf/registryconf_test.go @@ -183,7 +183,7 @@ func TestScrubNPMRCNeverLeaksSecretMaterial(t *testing.T) { func TestProjectNPMWritesScrubbedFile(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setHome(t, home) npmrc := filepath.Join(home, ".npmrc") body := "@acme:registry=https://npm.acme.test\n//npm.acme.test/:_authToken=SECRET\n" if err := os.WriteFile(npmrc, []byte(body), 0o600); err != nil { @@ -230,7 +230,7 @@ func TestProjectNPMWritesScrubbedFile(t *testing.T) { func TestProjectNPMAbsentOrMappinglessIsNoop(t *testing.T) { t.Run("missing npmrc", func(t *testing.T) { - t.Setenv("HOME", t.TempDir()) + setHome(t, t.TempDir()) projs, err := Project([]string{sandboxprofile.RegistryConfigNPM}, t.TempDir()) if err != nil { t.Fatal(err) @@ -242,7 +242,7 @@ func TestProjectNPMAbsentOrMappinglessIsNoop(t *testing.T) { t.Run("npmrc with no mapping", func(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setHome(t, home) if err := os.WriteFile(filepath.Join(home, ".npmrc"), []byte("_auth=x\n"), 0o600); err != nil { t.Fatal(err) } @@ -353,7 +353,7 @@ func TestScrubNPMRCRefusesUnauthenticatedGlobalRegistry(t *testing.T) { // taking the whole launch down. func TestProjectNPMUnreadableConfigIsNotFatal(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setHome(t, home) // A directory where the file is expected makes the read fail with // something other than IsNotExist. if err := os.Mkdir(filepath.Join(home, ".npmrc"), 0o755); err != nil { @@ -375,7 +375,7 @@ func TestProjectNPMUnreadableConfigIsNotFatal(t *testing.T) { // it cannot use. func TestInspectNPMReportsRejections(t *testing.T) { home := t.TempDir() - t.Setenv("HOME", home) + setHome(t, home) body := "@acme:registry=https://npm.acme.test/api/?apiKey=SECRET\n" if err := os.WriteFile(filepath.Join(home, ".npmrc"), []byte(body), 0o600); err != nil { t.Fatal(err) diff --git a/internal/sandboxprofile/profile.go b/internal/sandboxprofile/profile.go index ed7baa64..9e4b1357 100644 --- a/internal/sandboxprofile/profile.go +++ b/internal/sandboxprofile/profile.go @@ -301,6 +301,15 @@ func Parse(data []byte) (*Profile, error) { dec.DisallowUnknownFields() var p Profile if err := dec.Decode(&p); err != nil { + // Strict decoding is what makes a typo fail loudly instead of + // silently weakening the sandbox, but it also means an OLDER omac + // rejects a profile that a newer one has added a field to. That is + // easy to hit with a shared or checked-in profile, so the unknown-field + // case says which direction to look rather than just naming the field. + if strings.Contains(err.Error(), "unknown field") { + return nil, fmt.Errorf("parse sandbox profile: %w "+ + "(if this profile was written for a newer omac, upgrade omac; otherwise remove the field)", err) + } return nil, fmt.Errorf("parse sandbox profile: %w", err) } // A second JSON value in the stream is a malformed profile. diff --git a/internal/sandboxprofile/profile_test.go b/internal/sandboxprofile/profile_test.go index 9ebb3610..1045567f 100644 --- a/internal/sandboxprofile/profile_test.go +++ b/internal/sandboxprofile/profile_test.go @@ -1147,3 +1147,29 @@ func containsPath(paths []string, want string) bool { } return false } + +// TestUnknownFieldErrorHintsAtVersionSkew covers the review finding that a +// profile written for a newer omac is rejected by an older binary with a bare +// "unknown field", giving no clue which direction to look. Strictness is kept +// (a typo must not silently weaken the sandbox); only the message improves. +func TestUnknownFieldErrorHintsAtVersionSkew(t *testing.T) { + _, err := Parse([]byte(`{"filesystem": {"some_future_field": ["x"]}}`)) + if err == nil { + t.Fatal("unknown field must still be rejected") + } + if !strings.Contains(err.Error(), "some_future_field") { + t.Errorf("error no longer names the field: %v", err) + } + if !strings.Contains(err.Error(), "newer omac") { + t.Errorf("error gives no version-skew hint: %v", err) + } + // A malformed profile that is not an unknown-field problem keeps the + // plain message. + _, err = Parse([]byte(`{"workdir": {"access": 5}}`)) + if err == nil { + t.Fatal("type error must still be rejected") + } + if strings.Contains(err.Error(), "newer omac") { + t.Errorf("version hint leaked onto an unrelated parse error: %v", err) + } +} diff --git a/internal/sandboxrun/registryconf.go b/internal/sandboxrun/registryconf.go index 669e4ed1..621151f5 100644 --- a/internal/sandboxrun/registryconf.go +++ b/internal/sandboxrun/registryconf.go @@ -58,6 +58,16 @@ func setupRegistryConfig(merged *sandboxprofile.Profile, grants *Grants, injecte continue } + // An injected var wins over the profile's env allowlist (see + // FilterEnv), so a user who both forwards the var and opts into the + // projection silently loses their own config. Say so rather than + // swapping it out quietly. + if prev := os.Getenv(p.EnvVar); prev != "" && prev != p.Path { + fmt.Fprintf(stderr, "omac sandbox: WARNING: registry_config %s: %s was already set to %s; "+ + "the projection takes precedence, so settings in that file are not visible to the sandbox\n", + p.Ecosystem, p.EnvVar, prev) + } + // Grant exactly the projected file, read-only. The host file is // untouched and stays protected. grants.ReadPaths = append(grants.ReadPaths, p.Path) diff --git a/internal/sandboxrun/registryconf_test.go b/internal/sandboxrun/registryconf_test.go index ad664948..89cd78df 100644 --- a/internal/sandboxrun/registryconf_test.go +++ b/internal/sandboxrun/registryconf_test.go @@ -156,3 +156,53 @@ func TestSetupRegistryConfigMissingHostFileIsQuietNoop(t *testing.T) { t.Errorf("granted %v / injected %v for a missing npmrc", grants.ReadPaths, injected) } } + +// TestSetupRegistryConfigWarnsWhenEnvVarAlreadySet covers the review finding +// that an injected var silently wins over a value the user forwarded +// themselves, dropping their own config with no notice. +func TestSetupRegistryConfigWarnsWhenEnvVarAlreadySet(t *testing.T) { + writeNpmrc(t, "@acme:registry=https://npm.acme.test\n") + t.Setenv("NPM_CONFIG_USERCONFIG", "/home/dev/custom-npmrc") + + grants := &Grants{} + injected := map[string]string{} + var stderr bytes.Buffer + profile := &sandboxprofile.Profile{ + Filesystem: sandboxprofile.Filesystem{RegistryConfig: []string{sandboxprofile.RegistryConfigNPM}}, + } + cleanup, err := setupRegistryConfig(profile, grants, injected, &stderr) + if err != nil { + t.Fatal(err) + } + defer cleanup() + + out := stderr.String() + if !strings.Contains(out, "already set") || !strings.Contains(out, "/home/dev/custom-npmrc") { + t.Errorf("no warning naming the overridden path; got:\n%s", out) + } + // The projection still wins — the warning explains, it does not defer. + if injected["NPM_CONFIG_USERCONFIG"] == "/home/dev/custom-npmrc" { + t.Error("projection did not take precedence") + } +} + +// TestSetupRegistryConfigQuietWhenEnvVarUnset keeps the warning from firing on +// the ordinary path. +func TestSetupRegistryConfigQuietWhenEnvVarUnset(t *testing.T) { + writeNpmrc(t, "@acme:registry=https://npm.acme.test\n") + t.Setenv("NPM_CONFIG_USERCONFIG", "") + + var stderr bytes.Buffer + profile := &sandboxprofile.Profile{ + Filesystem: sandboxprofile.Filesystem{RegistryConfig: []string{sandboxprofile.RegistryConfigNPM}}, + } + cleanup, err := setupRegistryConfig(profile, &Grants{}, map[string]string{}, &stderr) + if err != nil { + t.Fatal(err) + } + defer cleanup() + + if strings.Contains(stderr.String(), "already set") { + t.Errorf("spurious override warning; got:\n%s", stderr.String()) + } +}