fix(#333): unify env var blocklist for sandbox and credentials - #401
fix(#333): unify env var blocklist for sandbox and credentials#401fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
Extract a single shared ReservedEnvKeys set in internal/sandbox/reserved.go referenced by both the provider credential validation (EnsureProvider) and the sandbox environment validation (bootstrapEnv). This eliminates drift between the two paths — a variable that is unsafe as a provider credential key is equally unsafe when injected via runner_env. The unified blocklist covers: - Infrastructure (PATH, HOME, SHELL) - Dynamic linker injection (LD_PRELOAD, LD_LIBRARY_PATH) - Shell injection (BASH_ENV, ENV, PROMPT_COMMAND) - Proxy/network (HTTP(S)_PROXY, ALL_PROXY, etc.) - TLS trust chain (SSL_CERT_*, *_CA_BUNDLE, etc.) - Git config (GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM, etc.) - Runtime injection (NODE_OPTIONS, PYTHONPATH, etc.) - FULLSEND_* prefix EnsureProvider now rejects reserved credential key names before building openshell args. bootstrapEnv rejects reserved runner_env key names before writing the sandbox .env file. Note: pre-commit could not run in-sandbox (shellcheck-py download blocked by network policy, exit 3). The post-script runs authoritative pre-commit on the runner. Closes #333
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
|
🤖 Finished Review · ✅ Success · Started 7:30 AM UTC · Completed 7:41 AM UTC |
Review — request-changesPR: fix(#333): unify env var blocklist for sandbox and credentials OverviewThis PR adds a unified The security intent is sound — blocking dangerous env vars from entering the sandbox process is defense-in-depth. However, there is a critical correctness bug that would break all production harness configurations. Critical
The # internal/scaffold/fullsend-repo/harness/code.yaml:55-58
runner_env:
FULLSEND_OUTPUT_SCHEMA: ${FULLSEND_DIR}/schemas/code-result.schema.json
FULLSEND_OUTPUT_FILE: code-result.jsonThe same pattern appears in Remediation: Exempt specific platform-managed MediumMutable exported blocklist map (
sandbox.ReservedEnvKeys["LD_PRELOAD"] = false
// or
delete(sandbox.ReservedEnvKeys, "PATH")
// or
sandbox.ReservedEnvKeys = nilFor a security-critical blocklist, the map should be unexported with access only through Documentation gaps for new validation behavior ( ADR-0024 documents LowNon-deterministic error reporting (
Missing regression test for platform keys ( The test suite covers rejection of Positive observations
Labels: PR modifies sandbox security validation in internal/sandbox/ and is labeled security on the linked issue |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // name. It checks both the exact key against [ReservedEnvKeys] and the | ||
| // FULLSEND_ prefix. | ||
| func IsReservedEnvKey(key string) bool { | ||
| if ReservedEnvKeys[key] { |
There was a problem hiding this comment.
[critical] logic-error
The FULLSEND_ prefix reservation in IsReservedEnvKey breaks all existing harness configurations. Production harness templates (code.yaml, review.yaml, triage.yaml, fix.yaml, retro.yaml, prioritize.yaml) set FULLSEND_OUTPUT_SCHEMA and FULLSEND_OUTPUT_FILE in runner_env. The new ValidateEnvKeys(h.RunnerEnv) call at the top of bootstrapEnv rejects these keys before bootstrapEnv can read them (lines 1163-1179), causing every agent run to fail.
Suggested fix: Exempt platform-managed FULLSEND_ keys (FULLSEND_OUTPUT_SCHEMA, FULLSEND_OUTPUT_FILE) from the blocklist, or validate only the keys that are directly written into the sandbox .env file rather than the full h.RunnerEnv map.
| // - Git config: git configuration injection vectors | ||
| // - Runtime injection: language-specific startup/path injection | ||
| // | ||
| // The FULLSEND_ prefix is also reserved (checked by [IsReservedEnvKey]). |
There was a problem hiding this comment.
[medium] mutable-security-control
ReservedEnvKeys is declared as an exported var map[string]bool. Any code in-process can mutate or nil-out the map to bypass the blocklist (e.g., sandbox.ReservedEnvKeys["LD_PRELOAD"] = false). For a security-critical blocklist, this should be unexported with access only through IsReservedEnvKey().
Suggested fix: Change to unexported var reservedEnvKeys and remove external direct map access. If read access is needed, expose via a function returning a copy.
| func ValidateCredentialKeys(credentials map[string]string) error { | ||
| for key := range credentials { | ||
| if IsReservedEnvKey(key) { | ||
| return fmt.Errorf("credential key %q is a reserved environment variable and cannot be used", key) |
There was a problem hiding this comment.
[low] error-handling
ValidateCredentialKeys and ValidateEnvKeys iterate a map and return on the first reserved key found. Go map iteration is non-deterministic, so when multiple reserved keys are present, the error message names a different key on each run.
Suggested fix: Sort keys before iterating to produce deterministic error messages, or collect all reserved keys into a single error.
|
/fs-fix |
|
🤖 Finished Fix · ❌ Failure · Started 7:50 AM UTC · Completed 8:05 AM UTC |
1 participant
Extract a single shared ReservedEnvKeys set in internal/sandbox/reserved.go referenced by both the provider credential validation (EnsureProvider) and the sandbox environment validation (bootstrapEnv). This eliminates drift between the two paths — a variable that is unsafe as a provider credential key is equally unsafe when injected via runner_env.
The unified blocklist covers:
EnsureProvider now rejects reserved credential key names before building openshell args. bootstrapEnv rejects reserved runner_env key names before writing the sandbox .env file.
Note: pre-commit could not run in-sandbox (shellcheck-py download blocked by network policy, exit 3). The post-script runs authoritative pre-commit on the runner.
Closes #333
Post-script verification
agent/333-unify-env-blocklist)a2afa8b179a351845733c680b839a39a82304db6..HEAD)