feat(sandbox,cli): project a credential-free registry config into the sandbox - #259
Conversation
… sandbox 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 <noreply@anthropic.com> Signed-off-by: Ilia Zhuravok <ilia.zhuravok@tngtech.com>
…y projection 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 <noreply@anthropic.com> Signed-off-by: Ilia Zhuravok <ilia.zhuravok@tngtech.com>
| u.User = nil | ||
| stripped = true | ||
| } | ||
| return u.String(), u.Hostname(), stripped, "" |
There was a problem hiding this comment.
[MUST FIX] bug, security, missing-test
registryURL returns u.Hostname() (port stripped), but credentialHost (line 401) keeps host:port, so a port-scoped credential never correlates with its mapping:
registry=https://npm.acme.test:8443
//npm.acme.test:8443/:_authToken=T
yields kept=[registry], needsAuth=[] — the global registry to a credentialed host is projected, and the scoped mapping to that host silently loses its NeedsAuth warning. This is the regression eae1299 claims to close; its regression test only covers the portless shape.
| return u.String(), u.Hostname(), stripped, "" | |
| return u.String(), u.Host, stripped, "" |
Correlating on u.Host is contained — the host feeds only keptHosts correlation and rejection message text. Add the port pair (global refused, scoped flagged) to TestScrubNPMRCRefusesUnauthenticatedGlobalRegistry.
There was a problem hiding this comment.
Fixed, and confirmed the shape you gave was projected before the fix.
Took u.Host as suggested, but routed both sides through one normalizeHost
helper rather than only changing the return, because the same comparison had a
second break: credentialHost lowercased and u.Hostname() did not, so
registry=https://NPM.Acme.Test + //npm.acme.test/:_authToken=T also failed
to correlate. Normalization is lowercase + strip an explicit default port
(npm's nerfDart does the same), so https://host:443 now correlates with
//host/:_authToken too.
Tests: TestCorrelationMatchesPortScopedCredential (your exact pair, global
refused and the scoped variant flagged), TestCorrelationIsCaseInsensitive,
TestCorrelationNormalizesDefaultPort, and TestCorrelationIgnoresUnrelatedHost
for the flip side.
| v = v[1 : len(v)-1] | ||
| } | ||
| } | ||
| return os.ExpandEnv(v) |
There was a problem hiding this comment.
[SHOULD FIX] security, bug, docs-drift
os.ExpandEnv expands from the full host env, and an expanded secret survives into the sandbox-readable projection in a URL path position: @acme:registry=https://npm.acme.test/api/${SECRET}/npm/ with SECRET=LIVESECRET projects …/api/LIVESECRET/npm/. Query/fragment are refused for exactly this reason; path is not, so the package invariant "no credential can survive by construction" does not hold.
Refusing expanded values outright would regress the corporate ${ART_HOST} shape the expansion exists for. Suggested: when expansion actually modified the value, emit a launch/doctor warning of the same class as Rejected ("mapping was expanded from ${…}; verify it carries no secret"), and document the residual path-position risk next to the query/fragment refusal.
There was a problem hiding this comment.
Fixed, and I went stricter than your suggestion: a placeholder outside the URL's
authority is refused, not warned about.
Reasoning: a warning leaves the package's stated invariant conditional on
someone reading it, and we already refuse a query string for exactly this
"cannot tell a secret from a load-bearing value" reason — so warning here would
be inconsistent with the neighbouring rule. Your concern about regressing
${ART_HOST} is addressed by keeping expansion in the authority:
@acme:registry=https://${ART_HOST}/api/npm/npm/ -> projected
@acme:registry=https://host:${PORT}/npm/ -> projected
@acme:registry=https://${U}:${PW}@host/npm/ -> expands, then userinfo stripped
@acme:registry=https://host/api/${SECRET}/npm/ -> refused, names SECRET
Mechanism: split the raw template at the authority boundary (find "://", then
the first /, ? or #) — structural, because an unexpanded template usually
does not parse as a URL. The authority goes through os.ExpandEnv; the
remainder goes through os.Expand with a mapper that only records that a
placeholder was consumed. So the refusal decision is made by os.Expand's own
tokenizer (${NAME}, $NAME, $$ cannot drift from the expansion path), and
no expanded value is ever matched against the URL — no coincidental-substring
false positives. TestExpansionAllowedInAuthorityOnly covers each position,
including a literal $ in the path that must not trigger a refusal.
Docs and the package doc updated: the invariant now reads as "userinfo removed;
query, fragment and non-authority interpolation refused", which is what the code
actually enforces.
|
|
||
| // 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|^//)`) |
There was a problem hiding this comment.
[NICE TO HAVE] bug, docs-accuracy
The ^// alternative counts //npm.acme.test/:always-auth=true as credential material (DroppedCredentials++), so doctor prints "including the auth token it holds" for a file holding only a boolean flag. Restrict the classification to _auth|_authtoken|_password variants; treat other //host/: keys as dropped-but-not-credential.
There was a problem hiding this comment.
Fixed. Classification now matches the key's leaf (after the last :)
against _auth|_authtoken|_password, so //host/:always-auth=true is dropped
like any other non-mapping key but no longer counted, and no longer marks the
host as needing auth. username/email are also excluded — they are dropped,
but they are not tokens and counting them overstated what a projection
protects. TestHostScopedNonCredentialIsNotCountedAsCredential.
| // 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 { |
There was a problem hiding this comment.
[NICE TO HAVE] decision-conflict
Host-less credentials (_auth=…, _password=…) return "" here and never correlate with the global registry mapping — registry=https://npm.acme.test + _auth=BASE64SECRET is kept with no NeedsAuth flag. That contradicts the stated refusal rationale ("the file held a credential for that host"), and eae1299's message claims credential correlation that only holds for //host/-scoped keys. Pin the chosen semantics in a comment + test so this doesn't get re-filed.
There was a problem hiding this comment.
Fixed by choosing the semantics rather than only documenting them: npm applies
legacy host-less _auth/_password to the default registry, so they now
bear on the global registry mapping (refused, same as the //host/-scoped
case). A scoped mapping is unaffected — a host-less credential says nothing
about a scope's own registry.
Pinned by comment and by TestHostlessCredentialAppliesToGlobalRegistry, which
asserts both halves so the alternative reading cannot be re-filed silently.
| authHosts := map[string]bool{} | ||
|
|
||
| for _, raw := range strings.Split(string(src), "\n") { | ||
| line := strings.TrimSpace(raw) |
There was a problem hiding this comment.
[NICE TO HAVE] bug
A UTF-8-BOM npmrc (plausible for Windows-authored files) has its leading @scope:registry=… counted as Dropped rather than Rejected: hosts=[], Rejected=[] → InspectNPM returns nil → neither launch nor doctor says anything — the silent 404 this feature exists to prevent. Trim a leading BOM once, or report leading garbage as Rejected.
There was a problem hiding this comment.
Fixed: one leading BOM is trimmed at the top of ScrubNPMRC. Good catch on the
consequence — it was not just a mis-count, it landed in Dropped where
InspectNPM returns nil, so neither the launch path nor doctor said anything,
which is precisely the silent 404 this feature exists to prevent.
TestScrubHandlesBOM. (TestScrubHandlesCRLF pins the CRLF behaviour you
noted as correct-but-untested, including that the CR does not defeat
correlation.)
| // 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{} |
There was a problem hiding this comment.
[NICE TO HAVE] maintainability
Duplicate mapping keys project twice, and the correlation loop pairs res.KeptKeys[i] with lines[i] by index across a re-filtering pass — a future continue between the two append sites (lines 341-342 vs 368-369) desyncs them silently. Carrying mapping{key, host, line} in one slice (with explicit last-wins) removes both hazards.
There was a problem hiding this comment.
Fixed as suggested: one []mapping{key, host, line} replaces the parallel
slices, so nothing can desync, and duplicates are resolved explicitly with
last-wins (npm's ini semantics) instead of being projected twice.
TestDuplicateKeyLastWins.
| // 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 |
There was a problem hiding this comment.
[NICE TO HAVE] backwards-compat
A profile that both allowlists NPM_CONFIG_USERCONFIG and opts into the projection silently gets the projection instead — injected wins, and the user's other settings in their config are dropped. Emit a one-line warning when injected already contains NPM_CONFIG_USERCONFIG.
There was a problem hiding this comment.
Fixed: warns when the variable is already set to a different path, naming the
path whose settings the sandbox will not see. The projection still takes
precedence — the warning explains rather than defers, since silently honouring
the user's file would reintroduce the unmasked-config problem. Tests for both
the warning and the quiet ordinary path.
| } | ||
| enabled := slices.Contains(profile.Filesystem.RegistryConfig, sandboxprofile.RegistryConfigNPM) | ||
| src, err := registryconf.NPMUserConfig() | ||
| if err != nil { |
There was a problem hiding this comment.
[NICE TO HAVE] bug, consistency
The launch path turns a non-ENOENT read failure on ~/.npmrc (e.g. EACCES after a root-owned sudo npm config set) into a projection warning (registryconf.go:143-145), but doctor swallows the identical error and prints nothing. Report it as [warn] or reuse the projector's Warning path.
There was a problem hiding this comment.
Fixed: reported as [warn] with the consequence spelled out, matching the
launch path. Worth flagging that doctor is the only check that runs before a
launch, so this silence was the more costly of the two.
TestDoctorRegistryConfigReportsUnreadableConfig.
| // 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"` |
There was a problem hiding this comment.
[NICE TO HAVE] docs-drift, backwards-compat
Adding this field means an older omac binary hard-rejects any shared/base profile that gains registry_config with a bare json: unknown field "registry_config" (strict decode at profile.go:301) — no hint that a newer omac is needed, so upgrading one machine breaks every other machine's profile. Consider a known-field hint in the parse-error path, and note the forward-incompatibility in CONFIGURATION.md.
There was a problem hiding this comment.
Both parts done. CONFIGURATION.md gains a note that profiles are parsed
strictly and a profile using registry_config is rejected by an older omac, so
shared/checked-in profiles want the upgrade first. And the unknown-field branch
of Parse now suggests which direction to look ("if this profile was written
for a newer omac, upgrade omac; otherwise remove the field") while other parse
errors keep the plain message. Strictness is unchanged — a typo must still fail
loudly. TestUnknownFieldErrorHintsAtVersionSkew asserts both the hint and that
it does not leak onto unrelated parse errors.
Note this only helps from this version forward; already-released binaries will
still emit the bare message. That is why the doc note matters more than the code
change here.
|
Adversarial review — verdict: fix-then-ship, risk: medium. Findings 1-9 inline; P1/P2 confirmed empirically against the real scrubber. Test gaps (nice to have): the P1/P2 both slipped through because there is no port-scoped credential case and no post-condition pinning that env-expanded values are scrutinized after expansion. Also missing: duplicate-key, BOM, and CRLF cases (CRLF currently correct but untested), a pre-set |
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 <noreply@anthropic.com> Signed-off-by: Ilia Zhuravok <ilia.zhuravok@tngtech.com>
|
Thanks — this was a genuinely useful review. All nine findings are fixed in
On the test-gap list: added port-scoped, mixed-case, default-port and Windows staging: deliberately not added. You are right that |
NoRiceToday
left a comment
NoRiceToday
left a comment
There was a problem hiding this comment.
LGTM now, thanks for addressing the feedback :) Just take care of the conflicts then you can merge
main rewrote the documentation (3187b34 "Remove old docs" and the commits around it), so docs/CONFIGURATION.md and docs/SECURITY_MODEL.md no longer exist. Both were modified on this branch, which is the whole conflict; the Go changes merged cleanly. Resolution: accept the deletion and port this branch's two doc additions into the new structure, rewritten to match its shorter, task-oriented style rather than moved verbatim. - docs/configuration.md: a `filesystem.registry_config` row in the sandbox grants field table, and a "Private package registries" subsection next to "Java and Node dependency downloads", which is the closest existing analogue (same shape of problem, same neighbourhood). - docs/configuration.md: one sentence added to the existing paragraph about upgrades, covering the reverse direction (an older omac rejects a profile that uses a newer field). It replaces the standalone note this branch had, because that paragraph already sets up the context. - docs/security.md: a `~/.npmrc` row in the sandbox access reference table, pointing at the configuration section. Checked that both sides survived the automatic merge of the two Go files: setupRegistryConfig is still wired into sandboxrun.Run, doctorRegistryConfig into runDoctor, and main's skillstate/skillconfig rework of doctor plus its reworded allow_vars warning are intact. Verified after the merge: go build ./... and go vet ./internal/... clean; go test ./... passes except TestIntegrationWorktreeKnownLimitations and TestIntegrationWorkflowInterpretersRunnable, which fail identically on a clean origin/main worktree on this host and pass in CI. End to end with the merged binary, cold cache, knob on, no override_deny: 16 skainet models, plugin 1.0.3 installed, ~/.npmrc still unreadable inside the sandbox, and the projected file holding only the mapping line. Refs #150 Refs #241 Signed-off-by: Ilia Zhuravok <ilia.zhuravok@tngtech.com>
2 participants
Issue: Closes #150 · Refs #241
What
filesystem.registry_config: ["npm"]: omac projects a credential-free copy of~/.npmrcinto the sandbox so scoped packages resolve against their private registry.omac doctornow reports a private registry mapping the sandbox cannot see — the failure that previously had no diagnostic surface anywhere.Why
~/.npmrcis a baseline protected path (internal/sandboxprofile/baseline.go:51) because it commonly holds_authToken. But the same file carries the@scope:registry=mapping, which is load-bearing configuration, not a secret. With it masked, npm resolves a scoped package against the public registry and gets a 404 that reads like "no such package" — no denial event, nothing inomac diagnose, and (per #241) host-side cache purges and--forcereinstalls have no effect because they touch paths the sandbox never reads.The blunt existing workaround,
override_deny: ["~/.npmrc"], works but grants the whole file including the token — exactly what #150 set out to avoid.How
network.proxy_injectionmechanism: accepted-value list next toProxyInjectionTools(), rejected-if-unknown inProfile.Validate().internal/registryconfdoes the scrub. Only registry-mapping keys survive; a kept value must resolve to a credential-freehttp(s)URL. npm's own value syntax is honored first (quotes stripped,${VAR}expanded) so a mapping npm would act on is not lost.internal/sandboxrun/registryconf.gowrites the copy per launch, appends only that file toGrants.ReadPaths, and injectsNPM_CONFIG_USERCONFIGthrough the sameinjectedenv map asProxyInjectionEnv.profileaudit.Check(documented I/O-free) — it sits alongside the other doctor checks.Two things are refused rather than projected, both reported at launch and by doctor:
?apiKey=…) and omac cannot tell a secret parameter from a load-bearing one.registrykey when the file holds a credential for that host: redirecting all resolution to a registry omac cannot authenticate to would break even the public installs that work today. A scoped mapping to such a host is kept (that scope was already failing) with a 401/403 warning.Note: the file-side detector uses "host is not npm's default" rather than netproxy's
isPackageRegistryheuristic, which returns false for exactly the corporate shape this exists for (tng-artifacts.int.tngtech.comhas noregistry/npmlabel) — verified before relying on it.Verification
go build ./...,go vet ./internal/...clean.go test ./...passes exceptTestIntegrationWorktreeKnownLimitationsandTestIntegrationWorkflowInterpretersRunnable, which fail identically on cleanmainon this host (local toolchain paths absent from the default profile).End-to-end against #241's actual case, Linux/bwrap, one binary, cold cache, no
override_deny:Inside the sandbox with the knob on:
omac doctoron a profile without the knob names the cause unprompted:The second commit is review fixes; each finding has a regression test (query-string leak, npm value syntax, unauthenticated global registry, non-fatal read failure, doctor reporting both projection and override).
Follow-up
omac cache clear(the omac cache scope pins@latestat first resolution, so a stale scope survives the fix).$XDG_STATE_HOME/opencode-skainet/, which omac does not grant (internal/config/harness.go:253).🤖 Generated with Claude Code