diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ca334d..ae9badc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,23 @@ tagged. ## Unreleased -_Nothing yet._ +### Added + +- **`env ( … )` gains aliases, defaults, and literals.** Entries now use + shell / docker-compose parameter-expansion syntax: `NAME = ${HOST}` aliases + a differently-named host variable, `${HOST:-default}` / `${HOST-default}` + supply a fallback, `${HOST:?message}` / `${HOST?message}` make a variable + required (failing sandbox creation with a message when missing), and a bare + or quoted right-hand side (`EDITOR = vim`) sets a literal constant. Bare + `NAME` passthrough is unchanged. + +### Fixed + +- **Forwarded `env ( … )` vars now reach the agent user.** The generated + `/etc/profile.d/99-clawk-env.sh` was written `0600 root:root`, so the + agent's login shells silently skipped it and the variables never arrived. + It is now `0644`, matching the working OAuth-token export + (clawkwork/clawk#4). ## v0.2.0 diff --git a/README.md b/README.md index 1b3c056..10e154c 100644 --- a/README.md +++ b/README.md @@ -255,7 +255,8 @@ sandbox my-project ( ) network ( allow api.example.com ) forwards ( 3000 ) - env ( DATABASE_URL ) # names only; values come from your shell + env ( DATABASE_URL ) # forward a host var; values come from your shell + # also: GH=${OTHER_NAME}, LOG=${LOG:-info} defaults, API=${API:?required} on create ( "go mod download" ) agent ( instructions "Ask before running destructive commands." diff --git a/docs/configuration.md b/docs/configuration.md index 2095071..3bd1c8c 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -71,7 +71,11 @@ sandbox my-project ( ) env ( - GITHUB_TOKEN + GITHUB_TOKEN # forward host $GITHUB_TOKEN as-is + GH_TOKEN = ${ACME_GH_TOKEN} # alias: read a differently-named host var + LOG_LEVEL = ${LOG_LEVEL:-info} # default when unset or empty + API_KEY = ${API_KEY:?set it} # required — fails create if missing + EDITOR = vim # literal constant ) on create ( @@ -106,10 +110,26 @@ sandbox my-project ( plus `use …` chains — see [Networking](networking.md#policies-and-use-chains). - `forwards ( … )` — port forwards (`PORT` or `HOST:GUEST`). -- `env ( … )` — host env-var **names** to forward in. Values come from your - shell at boot; the file never stores secrets. Names must be +- `env ( … )` — environment variables to export inside the VM. Secret + *values* come from your shell at boot and are never written to disk on the + host; only names, defaults, and literals live in the file. Each entry uses + shell / docker-compose parameter-expansion syntax: + - `NAME` — forward the identically-named host variable. + - `NAME = ${HOST}` — alias: forward host `$HOST` under a different guest + name. + - `NAME = ${HOST:-default}` — use `default` when `$HOST` is unset **or** + empty; `${HOST-default}` falls back only when it is unset. + - `NAME = ${HOST:?message}` — require `$HOST`; a missing value fails + sandbox creation with `message` (`${HOST?message}` treats only unset, + not empty, as missing). + - `NAME = value` / `NAME = "value with spaces"` — a literal constant, no + host lookup. + + Host variables are referenced only through `${…}`; a bare or quoted + right-hand side is always a literal. Names (both sides) must be shell-variable shaped (letters, digits, `_`; not starting with a digit) — - lowercase names like `http_proxy` are fine. + lowercase names like `http_proxy` are fine. Whitespace around `=` is + optional. - `on create ( … )` / `on up ( … )` — shell hooks. `create` runs once after the first boot; `up` runs on every boot. Each command runs inside the guest via `bash -lc` as a login shell: variable expansion, globs, and diff --git a/internal/config/types.go b/internal/config/types.go index bf78a26..a94f812 100644 --- a/internal/config/types.go +++ b/internal/config/types.go @@ -477,12 +477,14 @@ type Sandbox struct { // instead of hitting an in-guest version error mid-boot. Zero means // the record predates the field: ABI 1. GuestABI int `json:"guest_abi,omitempty"` - // RequiredEnv holds the names (not values) of host env vars this - // sandbox wants exported inside the VM. Declared in clawk.mod via - // `env ( NAME ... )`. Values are read from the host shell at - // sandbox-create time and written to /etc/profile.d/99-clawk-env.sh - // in the guest — never persisted to disk on the host alongside the - // name so we don't check secrets into the sandbox state file. + // RequiredEnv holds the env entries this sandbox wants exported inside + // the VM, declared in clawk.mod via `env ( … )`. Each string is a + // canonical envspec entry — a bare name like "GITHUB_TOKEN" (passthrough), + // an alias/default like "GH=${ACME_GH_TOKEN:-}" or a literal like + // "EDITOR=vim". Only names and defaults/literals are stored: secret + // *values* are read from the host shell at sandbox-create/up time and + // written to /etc/profile.d/99-clawk-env.sh in the guest, never persisted + // to the sandbox state file. See internal/envspec. RequiredEnv []string `json:"required_env,omitempty"` // NestedVirt opts the VM into hardware-assisted nested // virtualization so the guest can run its own VMs (Docker with KVM, diff --git a/internal/envspec/envspec.go b/internal/envspec/envspec.go new file mode 100644 index 0000000..d788fb1 --- /dev/null +++ b/internal/envspec/envspec.go @@ -0,0 +1,312 @@ +// Package envspec parses and evaluates the entries of a clawk.mod +// `env ( … )` block — the small expression language that maps a +// guest-side environment variable to a host variable, a literal, or a +// host variable with a fallback. +// +// The grammar deliberately mirrors POSIX-shell / docker-compose +// parameter expansion, so there is nothing new to learn: +// +// NAME passthrough — export NAME = host $NAME +// (empty if unset; a warning is logged) +// NAME = ${HOST} alias — export NAME = host $HOST +// NAME = ${HOST:-default} default if HOST is unset OR empty +// NAME = ${HOST-default} default if HOST is unset (empty kept) +// NAME = ${HOST:?message} hard error if HOST is unset OR empty +// NAME = ${HOST?message} hard error if HOST is unset +// NAME = literal literal constant, no host lookup +// NAME = "literal" quoted literal (spaces / special chars) +// +// A bare or quoted right-hand side is always a literal; host variables +// are referenced only through ${…}. This removes the "is that a value +// or a variable name?" ambiguity that a bare-name alias syntax hits the +// moment defaults enter the picture. +// +// Entries round-trip through their canonical String() form, which is +// what the template parser stores and what Parse reads back — so +// clawk.mod's env list stays a plain []string end to end (dedup, union, +// and JSON persistence never need to know about the structure). +package envspec + +import ( + "fmt" + "strings" +) + +// Op is how a Spec combines its host variable and its literal argument. +type Op uint8 + +const ( + // OpPassthrough exports the host variable's value, or empty (with a + // warning) when it is unset. Covers both `NAME` and `NAME = ${HOST}`. + OpPassthrough Op = iota + // OpDefaultUnset uses Arg when the host variable is unset; a set but + // empty value is kept. `${HOST-arg}`. + OpDefaultUnset + // OpDefaultEmpty uses Arg when the host variable is unset OR empty. + // `${HOST:-arg}`. + OpDefaultEmpty + // OpRequiredUnset fails resolution when the host variable is unset. + // `${HOST?arg}` (arg is the error message). + OpRequiredUnset + // OpRequiredEmpty fails resolution when the host variable is unset OR + // empty. `${HOST:?arg}`. + OpRequiredEmpty + // OpLiteral exports Arg verbatim with no host lookup. + OpLiteral +) + +// Spec is one parsed env entry. +type Spec struct { + Name string // guest variable to export (always a valid env name) + Host string // host variable to read; "" exactly when Op == OpLiteral + Arg string // default value, error message, or literal — per Op + Op Op +} + +// Parse reads one canonical entry (see the package doc) into a Spec. It +// is the single source of truth for the grammar: the template parser +// calls it to validate at parse time, and the sandbox layer calls it +// again to evaluate at manifest-build time. +func Parse(entry string) (Spec, error) { + name, rhs, hasValue := strings.Cut(entry, "=") + name = strings.TrimSpace(name) + if err := ValidateName(name); err != nil { + return Spec{}, err + } + if !hasValue { + // Bare name: passthrough of the identically-named host variable. + return Spec{Name: name, Host: name, Op: OpPassthrough}, nil + } + rhs = strings.TrimSpace(rhs) + + // Quoted right-hand side is always a literal. + if strings.HasPrefix(rhs, `"`) { + val, err := Unquote(rhs) + if err != nil { + return Spec{}, err + } + return Spec{Name: name, Op: OpLiteral, Arg: val}, nil + } + + // ${…} is a host-variable reference; anything else is a bare literal. + if strings.HasPrefix(rhs, "${") && strings.HasSuffix(rhs, "}") { + return parseRef(name, rhs[2:len(rhs)-1]) + } + return Spec{Name: name, Op: OpLiteral, Arg: rhs}, nil +} + +// parseRef parses the inside of a ${…} reference: a host variable name +// followed by an optional operator and argument. +func parseRef(name, inner string) (Spec, error) { + host, rest := scanName(inner) + if host == "" { + return Spec{}, fmt.Errorf("empty variable name in ${%s}", inner) + } + if err := ValidateName(host); err != nil { + return Spec{}, err + } + s := Spec{Name: name, Host: host} + switch { + case rest == "": + s.Op = OpPassthrough + case strings.HasPrefix(rest, ":-"): + s.Op, s.Arg = OpDefaultEmpty, rest[2:] + case strings.HasPrefix(rest, ":?"): + s.Op, s.Arg = OpRequiredEmpty, rest[2:] + case strings.HasPrefix(rest, "-"): + s.Op, s.Arg = OpDefaultUnset, rest[1:] + case strings.HasPrefix(rest, "?"): + s.Op, s.Arg = OpRequiredUnset, rest[1:] + default: + return Spec{}, fmt.Errorf( + "unsupported operator in ${%s}: %q (want :- , - , :? or ?)", inner, rest) + } + return s, nil +} + +// Resolve evaluates the spec against a host-env lookup (os.LookupEnv in +// production; a fake in tests). warnUnset reports a bare passthrough +// whose host variable is unset — the caller logs it and exports empty, +// preserving the historical behavior. A required-but-missing variable +// returns an error instead. +func (s Spec) Resolve(lookup func(string) (string, bool)) (value string, warnUnset bool, err error) { + if s.Op == OpLiteral { + return s.Arg, false, nil + } + val, ok := lookup(s.Host) + switch s.Op { + case OpPassthrough: + return val, !ok, nil + case OpDefaultUnset: + if !ok { + return s.Arg, false, nil + } + return val, false, nil + case OpDefaultEmpty: + if !ok || val == "" { + return s.Arg, false, nil + } + return val, false, nil + case OpRequiredUnset: + if !ok { + return "", false, s.missingErr() + } + return val, false, nil + case OpRequiredEmpty: + if !ok || val == "" { + return "", false, s.missingErr() + } + return val, false, nil + default: + return "", false, fmt.Errorf("envspec: unknown op %d", s.Op) + } +} + +func (s Spec) missingErr() error { + if msg := strings.TrimSpace(s.Arg); msg != "" { + return fmt.Errorf("required env %s (reads host variable %s) is unset: %s", + s.Name, s.Host, msg) + } + return fmt.Errorf("required env %s (reads host variable %s) is unset on host", + s.Name, s.Host) +} + +// String returns the canonical, re-parseable text for the spec. +// Parse(s.String()) == s for every valid Spec. +func (s Spec) String() string { + switch s.Op { + case OpPassthrough: + if s.Host == s.Name { + return s.Name // bare-name shorthand + } + return s.Name + "=${" + s.Host + "}" + case OpDefaultUnset: + return s.Name + "=${" + s.Host + "-" + s.Arg + "}" + case OpDefaultEmpty: + return s.Name + "=${" + s.Host + ":-" + s.Arg + "}" + case OpRequiredUnset: + return s.Name + "=${" + s.Host + "?" + s.Arg + "}" + case OpRequiredEmpty: + return s.Name + "=${" + s.Host + ":?" + s.Arg + "}" + case OpLiteral: + return s.Name + "=" + quoteLiteral(s.Arg) + default: + return s.Name + } +} + +// ValidateName enforces the POSIX shell variable-name shape. Lowercase +// is allowed on purpose — names like http_proxy are legitimate. +func ValidateName(s string) error { + if s == "" { + return fmt.Errorf("invalid env name: empty") + } + for i, r := range s { + switch { + case r == '_' || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z'): + case r >= '0' && r <= '9': + if i == 0 { + return fmt.Errorf("invalid env name %q: must not start with a digit", s) + } + default: + return fmt.Errorf( + "invalid env name %q: names only (letters, digits, '_')", s) + } + } + return nil +} + +// scanName splits off the leading POSIX-name-shaped run from s. Used to +// separate a host variable name from a trailing ${…} operator. +func scanName(s string) (name, rest string) { + i := 0 + for i < len(s) { + c := s[i] + if c == '_' || (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') { + i++ + continue + } + break + } + return s[:i], s[i:] +} + +// quoteLiteral returns v as a bare word when it is safe to write +// unquoted, otherwise as a double-quoted string. "Safe" means Parse +// would read the bare form back as this exact literal. +func quoteLiteral(v string) string { + if isSafeBareLiteral(v) { + return v + } + return Quote(v) +} + +func isSafeBareLiteral(v string) bool { + if v == "" || v[0] == '"' || strings.HasPrefix(v, "${") { + return false + } + for _, r := range v { + switch r { + case ' ', '\t', '\n', '\r', '(', ')', '#', '"', '=': + return false + } + } + return true +} + +// Quote renders v as a double-quoted string using the same escape set +// the clawk.mod lexer understands (\" \\ \n \t). +func Quote(v string) string { + var b strings.Builder + b.Grow(len(v) + 2) + b.WriteByte('"') + for _, r := range v { + switch r { + case '"': + b.WriteString(`\"`) + case '\\': + b.WriteString(`\\`) + case '\n': + b.WriteString(`\n`) + case '\t': + b.WriteString(`\t`) + default: + b.WriteRune(r) + } + } + b.WriteByte('"') + return b.String() +} + +// Unquote is the inverse of Quote. It requires a leading and trailing +// double quote. +func Unquote(s string) (string, error) { + if len(s) < 2 || s[0] != '"' || s[len(s)-1] != '"' { + return "", fmt.Errorf("malformed quoted value %q", s) + } + inner := s[1 : len(s)-1] + var b strings.Builder + b.Grow(len(inner)) + for i := 0; i < len(inner); i++ { + c := inner[i] + if c == '\\' && i+1 < len(inner) { + switch inner[i+1] { + case '"': + b.WriteByte('"') + case '\\': + b.WriteByte('\\') + case 'n': + b.WriteByte('\n') + case 't': + b.WriteByte('\t') + default: + return "", fmt.Errorf("unknown escape \\%c in %q", inner[i+1], s) + } + i++ + continue + } + b.WriteByte(c) + } + return b.String(), nil +} diff --git a/internal/envspec/envspec_test.go b/internal/envspec/envspec_test.go new file mode 100644 index 0000000..56db2f0 --- /dev/null +++ b/internal/envspec/envspec_test.go @@ -0,0 +1,136 @@ +package envspec + +import "testing" + +func TestParse(t *testing.T) { + tests := []struct { + entry string + want Spec + }{ + {"GITHUB_TOKEN", Spec{Name: "GITHUB_TOKEN", Host: "GITHUB_TOKEN", Op: OpPassthrough}}, + {"http_proxy", Spec{Name: "http_proxy", Host: "http_proxy", Op: OpPassthrough}}, + {"GH=${ACME_GH_TOKEN}", Spec{Name: "GH", Host: "ACME_GH_TOKEN", Op: OpPassthrough}}, + {"LOG=${LOG:-info}", Spec{Name: "LOG", Host: "LOG", Arg: "info", Op: OpDefaultEmpty}}, + {"LOG=${LOG:-}", Spec{Name: "LOG", Host: "LOG", Arg: "", Op: OpDefaultEmpty}}, + {"PORT=${PORT-8080}", Spec{Name: "PORT", Host: "PORT", Arg: "8080", Op: OpDefaultUnset}}, + {"K=${K:?must be set}", Spec{Name: "K", Host: "K", Arg: "must be set", Op: OpRequiredEmpty}}, + {"K=${K?unset}", Spec{Name: "K", Host: "K", Arg: "unset", Op: OpRequiredUnset}}, + {"EDITOR=vim", Spec{Name: "EDITOR", Arg: "vim", Op: OpLiteral}}, + {"BANNER=\"hi there\"", Spec{Name: "BANNER", Arg: "hi there", Op: OpLiteral}}, + {"EMPTY=\"\"", Spec{Name: "EMPTY", Arg: "", Op: OpLiteral}}, + // A default that itself contains '=' — Cut must split on the first '='. + {"KV=${KV:-a=b}", Spec{Name: "KV", Host: "KV", Arg: "a=b", Op: OpDefaultEmpty}}, + // A quoted literal that looks like an interpolation stays literal. + {"LIT=\"${X}\"", Spec{Name: "LIT", Arg: "${X}", Op: OpLiteral}}, + } + for _, tt := range tests { + got, err := Parse(tt.entry) + if err != nil { + t.Errorf("Parse(%q) error: %v", tt.entry, err) + continue + } + if got != tt.want { + t.Errorf("Parse(%q) = %+v, want %+v", tt.entry, got, tt.want) + } + } +} + +func TestParseErrors(t *testing.T) { + for _, entry := range []string{ + "1BAD", // leading digit + "foo-bar", // hyphen in bare name + "foo.bar", // dot + "GH=${1BAD}", // bad host name + "GH=${}", // empty host name + "GH=${HOST:x}", // unknown operator + "GH=${HOST%bad}", // unknown operator + } { + if _, err := Parse(entry); err == nil { + t.Errorf("Parse(%q) = nil error, want failure", entry) + } + } +} + +func TestStringRoundTrips(t *testing.T) { + // For every valid entry, Parse(spec.String()) must reproduce the spec. + for _, entry := range []string{ + "GITHUB_TOKEN", + "GH=${ACME_GH_TOKEN}", + "LOG=${LOG:-info}", + "LOG=${LOG:-}", + "PORT=${PORT-8080}", + "K=${K:?must be set}", + "K=${K?unset}", + "EDITOR=vim", + "BANNER=\"hi there\"", + "EMPTY=\"\"", + "KV=${KV:-a=b}", + "LIT=\"${X}\"", + } { + s1, err := Parse(entry) + if err != nil { + t.Fatalf("Parse(%q): %v", entry, err) + } + s2, err := Parse(s1.String()) + if err != nil { + t.Fatalf("re-Parse(%q from %q): %v", s1.String(), entry, err) + } + if s1 != s2 { + t.Errorf("round-trip drift for %q: %+v -> %q -> %+v", entry, s1, s1.String(), s2) + } + } +} + +func TestResolve(t *testing.T) { + env := map[string]string{ + "SET": "value", + "EMPTY": "", + } + lookup := func(k string) (string, bool) { v, ok := env[k]; return v, ok } + + tests := []struct { + name string + entry string + wantVal string + wantWarn bool + wantErr bool + }{ + {"passthrough set", "SET", "value", false, false}, + {"passthrough unset warns", "MISSING", "", true, false}, + {"passthrough empty no warn", "EMPTY", "", false, false}, + {"alias set", "G=${SET}", "value", false, false}, + {"alias unset warns", "G=${MISSING}", "", true, false}, + {":- uses default when unset", "L=${MISSING:-def}", "def", false, false}, + {":- uses default when empty", "L=${EMPTY:-def}", "def", false, false}, + {":- keeps set value", "L=${SET:-def}", "value", false, false}, + {"- uses default when unset", "L=${MISSING-def}", "def", false, false}, + {"- keeps empty value", "L=${EMPTY-def}", "", false, false}, + {":? errors when unset", "R=${MISSING:?nope}", "", false, true}, + {":? errors when empty", "R=${EMPTY:?nope}", "", false, true}, + {":? passes when set", "R=${SET:?nope}", "value", false, false}, + {"? errors when unset", "R=${MISSING?nope}", "", false, true}, + {"? passes when empty", "R=${EMPTY?nope}", "", false, false}, + {"literal", "E=vim", "vim", false, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spec, err := Parse(tt.entry) + if err != nil { + t.Fatalf("Parse(%q): %v", tt.entry, err) + } + val, warn, err := spec.Resolve(lookup) + if (err != nil) != tt.wantErr { + t.Fatalf("Resolve err = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr { + return + } + if val != tt.wantVal { + t.Errorf("value = %q, want %q", val, tt.wantVal) + } + if warn != tt.wantWarn { + t.Errorf("warnUnset = %v, want %v", warn, tt.wantWarn) + } + }) + } +} diff --git a/internal/sandbox/oci_sandbox.go b/internal/sandbox/oci_sandbox.go index a9f48cc..c0548d8 100644 --- a/internal/sandbox/oci_sandbox.go +++ b/internal/sandbox/oci_sandbox.go @@ -171,7 +171,11 @@ func OCIGuestManifest(sb *config.Sandbox, stateDir, cacheDir, rootDir string) (g token, _ := LoadOAuthToken(rootDir) files := append(DefaultHostFiles(rootDir), WorkspaceDocFile(sb)) files = append(files, ClaudeJSONMarkerFile(sb.Phases, token != "")) - if envFile, ok := EnvFile(sb); ok { + envFile, ok, err := EnvFile(sb) + if err != nil { + return guestcfg.Manifest{}, err + } + if ok { files = append(files, envFile) } for _, f := range files { diff --git a/internal/sandbox/shares.go b/internal/sandbox/shares.go index f3d1282..5139091 100644 --- a/internal/sandbox/shares.go +++ b/internal/sandbox/shares.go @@ -14,6 +14,7 @@ import ( "text/template" "github.com/clawkwork/clawk/internal/config" + "github.com/clawkwork/clawk/internal/envspec" "github.com/google/renameio/v2" ) @@ -592,11 +593,15 @@ func shellEscapeDoubleQuoted(v string) string { } // EnvFile synthesizes an /etc/profile.d script that exports every -// sandbox-required env var. Names come from sb.RequiredEnv (declared in -// clawk.mod); values come from the host's process env at this call. -// Missing host vars log a warning and are exported empty — callers -// decide whether that's fatal (e.g., an MCP server failing to auth is -// a clear signal). +// sandbox-required env var. Entries come from sb.RequiredEnv (declared in +// clawk.mod, in canonical envspec form); values are resolved against the +// host's process env at this call. +// +// Resolution follows the envspec grammar: a bare passthrough / ${HOST} +// alias exports the host value (empty + a warning when unset); ${HOST:-x} +// / ${HOST-x} fall back to a default; a bare/quoted literal is exported +// verbatim; and ${HOST:?msg} / ${HOST?msg} make a missing variable a hard +// error (returned here, failing sandbox creation with a clear message). // // Written into /etc/profile.d/99-clawk-env.sh so every login shell // (ssh, `claude ...`, interactive bash, phase setup scripts) picks up @@ -604,31 +609,49 @@ func shellEscapeDoubleQuoted(v string) string { // // Returns ok=false if the sandbox has no required env — saves the // caller from having to filter empty HostFiles. -func EnvFile(sb *config.Sandbox) (HostFile, bool) { +func EnvFile(sb *config.Sandbox) (HostFile, bool, error) { if len(sb.RequiredEnv) == 0 { - return HostFile{}, false + return HostFile{}, false, nil } var b bytes.Buffer b.WriteString("# Generated by clawk — host env vars requested in clawk.mod\n") - for _, name := range sb.RequiredEnv { - val, ok := os.LookupEnv(name) - if !ok { + for _, entry := range sb.RequiredEnv { + spec, err := envspec.Parse(entry) + if err != nil { + // The template parser already validated every entry, so this + // only fires on a hand-edited sandbox record — still worth a + // clear error rather than a malformed export line. + return HostFile{}, false, fmt.Errorf( + "sandbox %q: invalid env entry %q: %w", sb.Name, entry, err) + } + val, warnUnset, err := spec.Resolve(os.LookupEnv) + if err != nil { + return HostFile{}, false, fmt.Errorf("sandbox %q: %w", sb.Name, err) + } + if warnUnset { fmt.Fprintf(os.Stderr, "warning: %s required by clawk.mod is unset on host; "+ - "exporting empty in sandbox %q\n", name, sb.Name) + "exporting empty in sandbox %q\n", spec.Host, sb.Name) } // Double-quoted with $ and " escaped — covers secrets that // contain dollar signs or quotes. Single quotes aren't safe // because bash single-quote strings can't contain single // quotes, and API keys sometimes do. - fmt.Fprintf(&b, "export %s=\"%s\"\n", name, shellEscapeDoubleQuoted(val)) + fmt.Fprintf(&b, "export %s=\"%s\"\n", spec.Name, shellEscapeDoubleQuoted(val)) } return HostFile{ Content: b.Bytes(), GuestPath: "/etc/profile.d/99-clawk-env.sh", - Mode: 0o600, // root-owned; secrets - Owner: "root:root", - }, true + // 0644 root-owned, deliberately readable by the agent user — + // exactly like 98-clawk-claude-oauth.sh (OAuthTokenEnvFile). The + // agent's login shells run /etc/profile, which silently skips any + // profile.d script it can't read, so a 0600 file here means the + // forwarded vars never reach the shell (issue clawkwork/clawk#4). + // World-read costs nothing: the only non-root principal inside the + // VM is `agent`, which is precisely who these vars are exported for. + Mode: 0o644, + Owner: "root:root", + }, true, nil } // WorkspaceDocFile returns a CLAUDE.md that gets dropped at diff --git a/internal/sandbox/shares_test.go b/internal/sandbox/shares_test.go index 6439fef..badab21 100644 --- a/internal/sandbox/shares_test.go +++ b/internal/sandbox/shares_test.go @@ -3,8 +3,10 @@ package sandbox import ( "os" "path/filepath" + "strings" "testing" + "github.com/clawkwork/clawk/internal/config" "github.com/stretchr/testify/require" ) @@ -108,3 +110,114 @@ func TestToolchainCacheSharesUniqueTags(t *testing.T) { seen[sh.Tag] = sh.HostPath } } + +func TestEnvFileNoRequiredEnv(t *testing.T) { + if _, ok, err := EnvFile(&config.Sandbox{Name: "sb"}); ok || err != nil { + t.Fatalf("EnvFile with no RequiredEnv: ok=%v err=%v, want false/nil", ok, err) + } +} + +// TestEnvFileAgentReadable is the regression guard for +// clawkwork/clawk#4: 99-clawk-env.sh was written 0600 root:root, so the +// agent's login shells (which run /etc/profile as the non-root `agent` +// user) silently skipped it and the forwarded vars never arrived. The +// file must be readable by the agent — i.e. carry the world-read bit, +// since agent is neither root nor in root's group — exactly like the +// sibling OAuth export 98-clawk-claude-oauth.sh. +func TestEnvFileAgentReadable(t *testing.T) { + t.Setenv("CLAWK_TEST_TOKEN", "s3cr3t") + t.Setenv("CLAWK_TEST_MISSING", "") // present-but-empty is fine + os.Unsetenv("CLAWK_TEST_MISSING") + + hf, ok, err := EnvFile(&config.Sandbox{ + Name: "sb", + RequiredEnv: []string{"CLAWK_TEST_TOKEN", "CLAWK_TEST_MISSING"}, + }) + if err != nil || !ok { + t.Fatalf("EnvFile: ok=%v err=%v", ok, err) + } + if hf.GuestPath != "/etc/profile.d/99-clawk-env.sh" { + t.Errorf("GuestPath = %q", hf.GuestPath) + } + if hf.Mode != 0o644 { + t.Errorf("Mode = %o, want 0644 (agent-readable, matching the OAuth export)", hf.Mode) + } + if hf.Mode&0o004 == 0 { + t.Errorf("Mode = %o lacks the other-read bit; the non-root agent user can't source it", hf.Mode) + } + content := string(hf.Content) + if !strings.Contains(content, `export CLAWK_TEST_TOKEN="s3cr3t"`) { + t.Errorf("missing export for a set var:\n%s", content) + } + if !strings.Contains(content, `export CLAWK_TEST_MISSING=""`) { + t.Errorf("missing empty export for an unset var:\n%s", content) + } +} + +func TestEnvFileEscapesShellMetachars(t *testing.T) { + t.Setenv("CLAWK_TEST_WEIRD", `val"with$weird\meta`+"`stuff`") + hf, ok, err := EnvFile(&config.Sandbox{ + Name: "sb", + RequiredEnv: []string{"CLAWK_TEST_WEIRD"}, + }) + if err != nil || !ok { + t.Fatalf("EnvFile: ok=%v err=%v", ok, err) + } + content := string(hf.Content) + for _, want := range []string{`\"`, `\$`, `\\`, "\\`"} { + if !strings.Contains(content, want) { + t.Errorf("missing escape %q in:\n%s", want, content) + } + } +} + +// TestEnvFileComposeModel exercises the envspec grammar end to end +// through EnvFile: alias, :- default (applied and skipped), and a bare +// literal — checking the generated export lines use the guest-side name +// and the resolved value. +func TestEnvFileComposeModel(t *testing.T) { + t.Setenv("HOST_GH", "ghp_xyz") + os.Unsetenv("HOST_MISSING") + t.Setenv("HOST_SET", "present") + + hf, ok, err := EnvFile(&config.Sandbox{ + Name: "sb", + RequiredEnv: []string{ + "GH_TOKEN=${HOST_GH}", // alias + "LOG_LEVEL=${HOST_MISSING:-info}", // default applied (unset) + "MODE=${HOST_SET:-fallback}", // default skipped (set) + "EDITOR=vim", // literal + }, + }) + if err != nil || !ok { + t.Fatalf("EnvFile: ok=%v err=%v", ok, err) + } + content := string(hf.Content) + for _, want := range []string{ + `export GH_TOKEN="ghp_xyz"`, + `export LOG_LEVEL="info"`, + `export MODE="present"`, + `export EDITOR="vim"`, + } { + if !strings.Contains(content, want) { + t.Errorf("missing line %q in:\n%s", want, content) + } + } +} + +// TestEnvFileRequiredMissingErrors verifies that a ${HOST:?msg} whose +// host variable is unset fails EnvFile (and therefore sandbox creation) +// with a message that includes the author's note. +func TestEnvFileRequiredMissingErrors(t *testing.T) { + os.Unsetenv("HOST_REQUIRED") + _, _, err := EnvFile(&config.Sandbox{ + Name: "sb", + RequiredEnv: []string{"API_KEY=${HOST_REQUIRED:?set it in your shell}"}, + }) + if err == nil { + t.Fatal("expected error for required-but-missing env var") + } + if !strings.Contains(err.Error(), "set it in your shell") { + t.Errorf("error missing the author's note: %v", err) + } +} diff --git a/internal/template/lex.go b/internal/template/lex.go index e90970f..6e429a1 100644 --- a/internal/template/lex.go +++ b/internal/template/lex.go @@ -50,6 +50,7 @@ const ( TokNewline TokLParen TokRParen + TokEquals // '=', separates an env entry's name from its value TokIdent TokString // double-quoted, supports \" \\ \n \t escapes ) @@ -64,6 +65,8 @@ func (k TokenKind) String() string { return "'('" case TokRParen: return "')'" + case TokEquals: + return "'='" case TokIdent: return "identifier" case TokString: @@ -168,6 +171,14 @@ func Lex(src string) ([]Token, error) { emit(TokRParen, "", line, col) i++ col++ + case r == '=': + // Standalone '='. Used by env entries (`NAME = ${HOST}`); a '=' + // inside a quoted string never reaches here (the '"' case above + // consumes the whole string first), so existing values that + // embed '=' — shell commands in `on ( … )` etc. — are untouched. + emit(TokEquals, "", line, col) + i++ + col++ case r == '"': // Double-quoted string with simple backslash escapes. startLine, startCol := line, col @@ -209,13 +220,41 @@ func Lex(src string) ([]Token, error) { } return nil, fmt.Errorf("line %d col %d: unterminated string", startLine, startCol) default: - // Identifier: run until whitespace, a paren, or a quote. + // Identifier: run until whitespace, a paren, a quote, or '='. startLine, startCol := line, col start := i for i < n { c := src[i] + // A ${…} parameter reference scans as one unit — braces and + // all — so an env value like ${VAR:-a default} stays a single + // identifier even though it contains spaces. Nesting is + // tracked so ${FOO:-${BAR}} would balance, though shells + // rarely nest here. + if c == '$' && i+1 < n && src[i+1] == '{' { + i += 2 + col += 2 + depth := 1 + for i < n && depth > 0 { + switch src[i] { + case '{': + depth++ + case '}': + depth-- + case '\n': + return nil, fmt.Errorf( + "line %d col %d: unterminated ${…}", startLine, startCol) + } + i++ + col++ + } + if depth != 0 { + return nil, fmt.Errorf( + "line %d col %d: unterminated ${…}", startLine, startCol) + } + continue + } if c == ' ' || c == '\t' || c == '\n' || c == '\r' || - c == '(' || c == ')' || c == '#' || c == '"' { + c == '(' || c == ')' || c == '#' || c == '"' || c == '=' { break } // '/' is an ordinary identifier byte. "//" and "/*" open a diff --git a/internal/template/lex_test.go b/internal/template/lex_test.go index a07692f..89be559 100644 --- a/internal/template/lex_test.go +++ b/internal/template/lex_test.go @@ -52,6 +52,40 @@ allow ( } } +// TestLexEnvValues covers the two lexer additions for the env compose +// syntax: a standalone '=' token, and ${…} scanning as one identifier +// even when it contains spaces (so a default like ${VAR:-a b} survives). +func TestLexEnvValues(t *testing.T) { + toks, err := Lex("GH = ${ACME_GH_TOKEN:-a default}\n") + require.NoError(t, err) + + var kinds []TokenKind + var idents []string + for _, tok := range toks { + if tok.Kind == TokEOF || tok.Kind == TokNewline { + continue + } + kinds = append(kinds, tok.Kind) + if tok.Kind == TokIdent { + idents = append(idents, tok.Val) + } + } + require.Equal(t, []TokenKind{TokIdent, TokEquals, TokIdent}, kinds) + require.Equal(t, []string{"GH", "${ACME_GH_TOKEN:-a default}"}, idents) + + // A '=' with no surrounding spaces still splits into three tokens. + toks, err = Lex("GH=${X}\n") + require.NoError(t, err) + require.Equal(t, TokIdent, toks[0].Kind) + require.Equal(t, "GH", toks[0].Val) + require.Equal(t, TokEquals, toks[1].Kind) + require.Equal(t, "${X}", toks[2].Val) + + // An unterminated ${ is a lex error, not a silently truncated token. + _, err = Lex("GH = ${X\n") + require.Error(t, err) +} + func TestLexComments(t *testing.T) { // Every comment form must strip to the same two identifiers. A lone '/' // in a path is NOT a comment — only "//" and "/*" open one. diff --git a/internal/template/parse.go b/internal/template/parse.go index c8b1689..2083fff 100644 --- a/internal/template/parse.go +++ b/internal/template/parse.go @@ -7,6 +7,8 @@ import ( "slices" "strconv" "time" + + "github.com/clawkwork/clawk/internal/envspec" ) // FileSpec is one entry in a `files (...)` block: a host file copied @@ -94,7 +96,7 @@ type Template struct { // block. DenySources []string Forwards []string // port forward specs (PORT or HOST:GUEST) - Env []string // env var NAMES (not values) to pull from host and export in the VM + Env []string // env entries to export in the VM (canonical envspec form; see parseEnvBlock) // Lifecycle hooks. Each is a list of shell commands run inside the // VM at the named moment. @@ -291,12 +293,12 @@ func (p *parser) parseTemplateDirective(tmpl *Template, t Token) error { case "agent": return p.parseAgentBlock(tmpl) case "env": - // env ( FOO BAR BAZ ) or env FOO - // Values come from the host shell at `clawk run / up` time; - // the Clawkfile only names which variables the project needs. - // Never persisted as values — avoids checking secrets into repo - // config. Names must be shell-variable shaped (validateEnvName). - return p.parseValidatedIdentBlock(&tmpl.Env, "env", validateEnvName) + // env ( FOO GH=${HOST} LOG=${LOG:-info} ED=vim ) or env FOO + // Values come from the host shell at `clawk run / up` time; the + // Clawkfile only names which variables the project needs (and, + // optionally, how to map/default them). Never persisted as values + // — avoids checking secrets into repo config. See envspec. + return p.parseEnvBlock(&tmpl.Env) case "apps", "app": // Previous iteration's directives — explicitly rejected so users // migrating from an older Clawkfile get a clear message pointing @@ -381,24 +383,93 @@ func (p *parser) parseValidatedIdentBlock(dst *[]string, directive string, valid } } -// validateEnvName enforces the POSIX shell variable-name shape on `env` -// entries. Deliberately NOT uppercase-only: lowercase names like -// http_proxy are legitimate. Enforced at parse time because an invalid -// name written here would otherwise fail much later, inside the guest's -// generated profile script, where the error is far harder to trace. -func validateEnvName(s string) error { - for i, r := range s { - switch { - case r == '_' || (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z'): - case r >= '0' && r <= '9': - if i == 0 { - return fmt.Errorf("invalid env name %q: must not start with a digit", s) - } - default: - return fmt.Errorf("invalid env name %q: names only (letters, digits, '_'), values come from the host shell", s) +// parseEnvBlock parses an `env ( … )` block (or the inline `env ENTRY` +// form). Each entry is either a bare name (passthrough of the same host +// variable) or `NAME = VALUE`, where VALUE is a ${…} host reference — with +// optional :- / - / :? / ? operator — a bare literal, or a quoted literal. +// See package envspec for the full grammar. +// +// Entries are stored in their canonical envspec.String() form, so the rest +// of the pipeline keeps treating the env list as a plain []string (dedup, +// union, and JSON persistence stay oblivious to the structure). Validating +// here — rather than in the guest's generated profile script — keeps a bad +// entry's error on its own source line where it is easy to trace. +func (p *parser) parseEnvBlock(dst *[]string) error { + p.advance() // consume "env" + t := p.peek() + + // Inline form: `env ENTRY` + if t.Kind == TokIdent { + entry, err := p.parseEnvEntry() + if err != nil { + return err } + *dst = append(*dst, entry) + return p.expectNewlineOrEOF() } - return nil + + // Block form: `env ( … )` + if t.Kind != TokLParen { + return p.errorAt(t, "expected '(' or identifier after %q, got %s", "env", t) + } + p.advance() + for { + p.skipNewlines() + t := p.peek() + if t.Kind == TokRParen { + p.advance() + return p.expectNewlineOrEOF() + } + if t.Kind != TokIdent { + return p.errorAt(t, "expected env entry or ')' in %q, got %s", "env", t) + } + entry, err := p.parseEnvEntry() + if err != nil { + return err + } + *dst = append(*dst, entry) + } +} + +// parseEnvEntry consumes one env entry — `NAME` or `NAME = VALUE` — and +// returns its canonical text. The caller has confirmed the next token is +// the name identifier. +func (p *parser) parseEnvEntry() (string, error) { + nameTok := p.peek() + if isKeyword(nameTok.Val) { + return "", p.errorAt(nameTok, "unexpected keyword %q in %q", nameTok.Val, "env") + } + p.advance() + + // Bare name → passthrough of the same-named host variable. + if p.peek().Kind != TokEquals { + spec, err := envspec.Parse(nameTok.Val) + if err != nil { + return "", p.errorAt(nameTok, "%v", err) + } + return spec.String(), nil + } + p.advance() // consume '=' + + valTok := p.peek() + var rhs string + switch valTok.Kind { + case TokIdent: + rhs = valTok.Val + case TokString: + // The lexer already stripped the quotes; re-quote so envspec reads + // it back as a literal rather than a bare word or ${…} reference. + rhs = envspec.Quote(valTok.Val) + default: + return "", p.errorAt(valTok, "expected a value after '=' in %q, got %s", "env", valTok) + } + p.advance() + + spec, err := envspec.Parse(nameTok.Val + "=" + rhs) + if err != nil { + return "", p.errorAt(nameTok, "%v", err) + } + return spec.String(), nil } func (p *parser) parseProvider(tmpl *Template) error { diff --git a/internal/template/parse_test.go b/internal/template/parse_test.go index 84a548d..8bc1fb8 100644 --- a/internal/template/parse_test.go +++ b/internal/template/parse_test.go @@ -136,6 +136,50 @@ includes ~/code/app } } +// TestParseEnvComposeModel covers the full env grammar — passthrough, +// alias, :- / - defaults, :? required, and bare/quoted literals — and +// checks each entry is stored in canonical envspec form. Whitespace +// around '=' is flexible (the lexer emits a standalone '=' token), and a +// ${…} value may contain spaces because it scans as one unit. +func TestParseEnvComposeModel(t *testing.T) { + src := `env ( + NPM_TOKEN + GH_TOKEN = ${ACME_GH_TOKEN} + LOG_LEVEL = ${LOG_LEVEL:-info} + PORT = ${PORT-8080} + API_KEY = ${API_KEY:?set it in your shell} + EDITOR=vim + BANNER = "hi there" +) +` + tmpl, err := parseBody(src) + require.NoError(t, err) + require.Equal(t, []string{ + "NPM_TOKEN", + "GH_TOKEN=${ACME_GH_TOKEN}", + "LOG_LEVEL=${LOG_LEVEL:-info}", + "PORT=${PORT-8080}", + "API_KEY=${API_KEY:?set it in your shell}", + "EDITOR=vim", + `BANNER="hi there"`, + }, tmpl.Env) +} + +// TestParseEnvErrors: malformed env entries fail at parse time with a +// traceable message rather than surfacing later in the guest. +func TestParseEnvErrors(t *testing.T) { + for _, src := range []string{ + "env ( 1BAD )\n", // bad name + "env ( GH = ${1BAD} )\n", // bad host name + "env ( GH = ${HOST:x} )\n", // unknown operator + "env ( GH = )\n", // missing value + } { + if _, err := parseBody(src); err == nil { + t.Errorf("parseBody(%q) = nil error, want failure", src) + } + } +} + // TestParseNetworkAccumulates: allow/deny entries within a network block // accumulate into the domain/IP slices. func TestParseNetworkAccumulates(t *testing.T) { diff --git a/internal/template/resources.go b/internal/template/resources.go index 2d89592..3ed1420 100644 --- a/internal/template/resources.go +++ b/internal/template/resources.go @@ -374,7 +374,7 @@ func (p *parser) parseNamespaceBlock() (NamespaceDef, error) { case "shares": err = p.parseSharesBlock(def.Template) case "env": - err = p.parseValidatedIdentBlock(&def.Template.Env, "env", validateEnvName) + err = p.parseEnvBlock(&def.Template.Env) case "agent": err = p.parseAgentBlock(def.Template) case "vm", "includes", "on":