diff --git a/docs/configuration.md b/docs/configuration.md index 9a2a8ee9..f63bd921 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -64,11 +64,14 @@ omac creates this file the first time you run `omac start`. Key fields: | `network.mode` | `string` | `"filtered"` | `filtered` (prompt for unknown hosts), `blocked` (no outbound at all), `open` (unrestricted) | | `environment.allow_vars` | `string[]` | see created file | Env vars passed into the sandbox; everything else is stripped | | `filesystem.protected_paths` | `string[]` | `["~/.ssh", "~/.gnupg", ...]` | Paths that remain blocked even if a broader grant would cover them | +| `filesystem.registry_config` | `string[]` | `[]` | Ecosystems whose package-registry settings are copied into the sandbox without their credentials. Currently `"npm"`. See [Private package registries](#private-package-registries) | See [Security model → Sandbox access reference](./security.md#sandbox-access-reference) for the full list of what the agent can and cannot access. omac never rewrites this file once it exists, so upgrading omac does not add newer default grants to a profile you already have. To pick up the newer defaults, make a copy of your current file, delete the original, and run `omac start` to write a fresh one. Then copy any changes you had made back from your saved copy into the new file. +The reverse also applies. An unknown field is an error, so that a typo cannot quietly weaken the sandbox. A file using a newer field, such as `filesystem.registry_config`, is therefore rejected by an older omac. If you share this file between machines, upgrade omac on all of them before adding a new field. + ### Opening a port To let the agent reach a local service, add the port to `network.open_port` in the sandbox grants file (`~/.config/omac/sandbox-profiles/default.json`): @@ -121,6 +124,24 @@ Java (Maven/Gradle) and Node/npm do not reliably route their package downloads t Node injection requires Node ≥ 22.21.0 (22.x line) or ≥ 24.5.0; on older versions it is skipped and downloads may still fail. +### Private package registries + +If your company hosts its own npm packages, `~/.npmrc` says where to find them. A line like `@acme:registry=https://npm.acme.test` means "packages starting with `@acme/` come from that server". + +The sandbox blocks `~/.npmrc`, because the same file usually holds an access token. Without it, npm looks for `@acme/` packages on the public registry instead, does not find them, and reports a 404. The error looks like the package does not exist, so this is easy to misread. Allowing the registry's host does not help, because npm never asks it. + +To fix this, add `npm` to `filesystem.registry_config`: + +```json +{ "filesystem": { "registry_config": ["npm"] } } +``` + +omac then writes a copy of `~/.npmrc` that contains only the registry addresses, lets the sandbox read that copy, and points npm at it. The real file stays blocked, so no token is copied. If a line cannot be copied without also copying a secret, omac skips that line and tells you which one, both at startup and in `omac doctor`. + +Private registries usually also need their host added to `network.allow_domain`, or allowed once at the network prompt. + +omac cannot pass on your access token, so packages that require login still fail to install. Only the address is shared, never the credential. + ## Audit trail omac logs every security-relevant action to an append-only file: process launches, network decisions, secret injections. The file is outside the sandbox so the agent cannot tamper with it. diff --git a/docs/security.md b/docs/security.md index fb4b688d..b2eff04f 100644 --- a/docs/security.md +++ b/docs/security.md @@ -79,6 +79,7 @@ cannot access. | `/tmp`, `$TMPDIR` | read + write | Temporary files during the agent's work | | Facade socket (`$TMPDIR/omac-/bridge.sock`) | connect | The socket the agent uses to reach skill sidecars; created by the facade, not the agent | | `~/.ssh`, `~/.gnupg`, `~/.aws`, `~/.kube`, … | **blocked** | Sensitive credentials | +| `~/.npmrc` | **blocked**; registry addresses can be shared as a stripped copy | Usually holds an access token. See [Private package registries](./configuration.md#private-package-registries) | | `~/.config/omac` (approval store, sandbox profiles, global registry) | **not mounted** | The agent must not be able to forge skill approvals | | `.env` / `.envrc` files (including nested ones inside the project) | **blocked** | Often contain secrets | | `~/.cache`, `~/Library/Caches` (host cache roots) | **blocked** | Prevents cross-project cache poisoning; omac provides its own isolated cache | diff --git a/internal/cli/doctor.go b/internal/cli/doctor.go index ab7b29cf..1330dbd8 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" "github.com/tngtech/oh-my-agentic-coder/internal/skillconfig" @@ -224,6 +226,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 { @@ -252,6 +259,94 @@ 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 { + // 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, ", ") + // 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) + 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)) + } + + 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. +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..81e4601c --- /dev/null +++ b/internal/cli/doctor_registryconf_test.go @@ -0,0 +1,230 @@ +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) + } + }) +} + +// 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) + } +} + +// 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 new file mode 100644 index 00000000..51c15375 --- /dev/null +++ b/internal/registryconf/registryconf.go @@ -0,0 +1,575 @@ +// 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; 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 ( + "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 + // 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 := "" + 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 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. +// 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. +// +// 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 { + 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 && p.Warning == "" && len(p.Rejected) == 0 { + continue + } + if p.Ecosystem == "" { + p.Ecosystem = eco + } + 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 { + // 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 + } + // 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. 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 { + 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, + Rejected: res.Rejected, + NeedsAuth: res.NeedsAuth, + }, 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 + // 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 +// 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()) + } + } + // 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{ + Ecosystem: sandboxprofile.RegistryConfigNPM, + Source: src, + Hosts: hosts, + 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 + // 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 + // 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. +// "@acme:registry" or "@acme/sub:registry". +var scopedRegistryKey = regexp.MustCompile(`^@[^:\s]+:registry$`) + +// 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 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 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 + + 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 + } + 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 isCredentialKey(key) { + res.DroppedCredentials++ + if h := credentialHost(key); h != "" { + authHosts[h] = true + } else { + defaultRegistryCredential = true + } + } + continue + } + 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 + } + if stripped { + res.StrippedUserinfo++ + } + 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 + // 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 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: 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", m.host), + }) + continue + } + res.NeedsAuth = append(res.NeedsAuth, m.key) + } + res.KeptKeys = append(res.KeptKeys, m.key) + lines = append(lines, m.line) + } + 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 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] + } + } + 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: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.TrimSpace(key), "//") + if !ok { + return "" + } + host, _, _ := strings.Cut(rest, "/") + 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 +// `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 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 { + 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, 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 + stripped = true + } + // 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 new file mode 100644 index 00000000..242b3148 --- /dev/null +++ b/internal/registryconf/registryconf_test.go @@ -0,0 +1,393 @@ +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 + rejected 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 rejected, not silently dropped", + src: "registry=not-a-url\n", + wantKeys: nil, + rejected: 1, + }, + { + name: "non-http scheme is rejected", + src: "registry=file:///tmp/evil\n", + wantKeys: nil, + rejected: 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) + } + if len(got.Rejected) != tt.rejected { + t.Errorf("rejected = %d (%+v), want %d", len(got.Rejected), got.Rejected, tt.rejected) + } + }) + } +} + +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() + 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 { + 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) { + setHome(t, 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() + setHome(t, 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") + } +} + +// --- 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() + 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 { + 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() + 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) + } + 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/sandboxprofile/profile.go b/internal/sandboxprofile/profile.go index 4c4bb5d3..9e4b1357 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 @@ -273,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. @@ -328,6 +365,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..1045567f 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 { @@ -1126,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 new file mode 100644 index 00000000..621151f5 --- /dev/null +++ b/internal/sandboxrun/registryconf.go @@ -0,0 +1,98 @@ +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 + } + 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 + } + + // 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) + 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 +} diff --git a/internal/sandboxrun/registryconf_test.go b/internal/sandboxrun/registryconf_test.go new file mode 100644 index 00000000..89cd78df --- /dev/null +++ b/internal/sandboxrun/registryconf_test.go @@ -0,0 +1,208 @@ +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) + } +} + +// 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()) + } +} diff --git a/internal/sandboxrun/run.go b/internal/sandboxrun/run.go index baa17e1e..92a7d92e 100644 --- a/internal/sandboxrun/run.go +++ b/internal/sandboxrun/run.go @@ -215,6 +215,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.