feat(skills): add env_passthrough allowlist to skill manifest by neo-wanderer · Pull Request #3219 · librefang/librefang

neo-wanderer · 2026-04-26T07:52:40Z

Summary

Adds optional env_passthrough = ["VAR1", "VAR2"] to skill manifests (top-level, sibling to [skill]/[runtime]).
Loader (librefang-skills/src/loader.rs) honors the allowlist by re-injecting matching host env vars after env_clear() for Python, Node, and Shell runtimes.
Default empty — full env_clear isolation preserved for skills that don't opt in.

Motivation

Today's env_clear() (correct for security) means any skill whose subprocess shells out to a tool requiring env-based credentials silently fails. Concrete blocker: gog's file-backed keyring needs GOG_KEYRING_PASSWORD to operate non-interactively, and there's no path to get that var into a Python skill subprocess.

This patch adds the same opt-in shape that [exec_policy].allowed_env_vars provides for shell_exec. Names are public (visible in the manifest TOML), values are read from the host env only at invocation time — no secret leakage into the manifest.

Implementation notes

New field SkillManifest::env_passthrough: Vec<String> with #[serde(default)].
Helper apply_env_passthrough(&mut Command, &[String]) shared by all three runtime paths.
WASM skills have a separate sandbox/capability model and are intentionally out of scope.
Builtin / PromptOnly runtimes don't spawn subprocesses, so the field is inert for them.

Tests

test_skill_manifest_env_passthrough_roundtrip — TOML parse + re-serialize preserves the field.
test_skill_manifest_env_passthrough_default_empty — absent field defaults to empty.
test_env_passthrough_allowlist_injects_var (loader) — sets two vars in the host env, declares only one in the manifest allowlist, runs a probe skill via execute_skill_tool, asserts the allowlisted var reaches the subprocess and the non-allowlisted var does NOT.

Test plan

cargo build --workspace --lib
cargo test -p librefang-skills (166 passed)
cargo test --workspace (only failure is librefang-kernel::config::tests::test_load_config_defaults, pre-existing on upstream/main and unrelated to this change)
cargo clippy --workspace --all-targets -- -D warnings (clean)

Skills can now declare an env_passthrough list in skill.toml that specifies host environment variables to forward into the skill subprocess. The default remains empty — full env_clear isolation — preserving the security posture for third-party skills. Mirrors the existing [exec_policy].allowed_env_vars mechanism for shell_exec. Concrete motivation: skills that wrap a CLI requiring credentials in env (e.g., gog with GOG_KEYRING_PASSWORD for its file-backed keyring) cannot work today because env_clear strips those vars before the subprocess sees them. The allowlist is per-skill, names only — values are not stored in the manifest. Variables not present in the host environment are silently skipped. Visibility is opt-in and the manifest is public, so users can audit what each skill imports. Plumbed through execute_python, execute_node, and execute_shell via a small apply_env_passthrough helper. WASM skills are out of scope (separate sandbox model). Includes serde round-trip tests and a loader integration test that asserts allowlisted vars reach the subprocess and non-allowlisted vars do not.

houko

Overall

Diff shape is clean: 1 new field, 1 helper, 3 runtime sites + tests. The use case is real (credential helpers like keyring backends need a way through env_clear). But the security model has a foundational issue plus two correctness gaps before this should land.

Blocking — security model

1. The `[exec_policy].allowed_env_vars` mirror is misleading; this PR's allowlist is author-controlled, not operator-controlled

PR description says "Mirrors the existing [exec_policy].allowed_env_vars mechanism for shell_exec". They are not the same shape. shell_exec's allowlist lives in config.toml: [exec_policy].allowed_env_vars (runtime/src/tool_runner.rs:629-637) — the operator decides which vars shell_exec can see.

This PR's env_passthrough lives in skill.toml — the skill author decides. There is no operator-side gate. A community skill ships with:

env_passthrough = ["AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY", "OPENAI_API_KEY", "GITHUB_TOKEN"]

…and any operator who installs it without auditing the manifest leaks every credential set in their environment. The "names are public" mitigation only works if operators routinely diff every skill.toml before install, which they don't (and the skill registry sync currently auto-installs).

What's needed (in this PR or as a strict prerequisite):

An operator-side gate. Either (a) a global [skills].env_passthrough_allowed_vars allowlist that the skill's request is intersected against, or (b) a deny-by-default block for sensitive name patterns (*_KEY, *_TOKEN, *_PASSWORD, *_SECRET, AWS_*, GITHUB_*, *_API_KEY, …) that operators can override per-skill.
(b) is the better default because the long tail of credential env-var conventions is open-ended; a deny-list catches most accidents without operators having to think.

Skill authors can still document which vars they need; operators decide what to expose.

2. No denylist for vars that compromise subprocess isolation

LD_PRELOAD, LD_LIBRARY_PATH, DYLD_INSERT_LIBRARIES, DYLD_LIBRARY_PATH, PYTHONPATH, PYTHONHOME, PYTHONSTARTUP, NODE_OPTIONS, NODE_PATH — every one of these can either inject code (LD_PRELOAD, PYTHONSTARTUP) or redirect imports/modules to attacker-controlled paths if the host environment has them set.

Today this PR happily forwards any of them. A skill author who declares env_passthrough = ["LD_PRELOAD"] and convinces an operator to install gets arbitrary code execution in every Python/Node/Shell skill subprocess, including gog's and any other future skill with the same passthrough.

Hard-block these regardless of operator config:

const FORBIDDEN_PASSTHROUGH: &[&str] = &[
    "LD_PRELOAD", "LD_LIBRARY_PATH", "LD_AUDIT",
    "DYLD_INSERT_LIBRARIES", "DYLD_LIBRARY_PATH", "DYLD_FALLBACK_LIBRARY_PATH",
    "PYTHONPATH", "PYTHONHOME", "PYTHONSTARTUP", "PYTHONEXECUTABLE",
    "NODE_OPTIONS", "NODE_PATH",
    "PATH",  // kernel sets PATH explicitly; passthrough must not override
];

apply_env_passthrough should skip these with a tracing::warn! line. The list is short and the names are well-known; this is a 10-line change.

Blocking — correctness

3. `apply_env_passthrough` runs after kernel-controlled env settings, so it overrides them

In all three runtimes the order is:

cmd.env_clear();
cmd.env("PATH", host_path);          // kernel-curated
cmd.env("HOME", home);                // kernel-curated
cmd.env("PYTHONIOENCODING", "utf-8"); // Python only — kernel-curated
// …
apply_env_passthrough(&mut cmd, env_passthrough);  // ← this can override every line above

std::process::Command::env(name, value) is last-write-wins. A skill with env_passthrough = ["PATH"] clobbers the kernel's PATH with whatever the host has. On a system where the kernel's PATH was deliberately narrowed (e.g. for a sandboxed daemon), this re-expands the skill's binary-resolution surface back to the host's full PATH.

Fix is one of:

Run apply_env_passthrough before the kernel's cmd.env(…) lines, or
Skip names already set by the kernel inside apply_env_passthrough.

The first is simpler and matches the principle "kernel settings are non-negotiable."

(This issue is independent of #2; even with the dangerous-vars denylist, the order bug still lets a skill override HOME, TMPDIR, NODE_NO_WARNINGS, etc. Both fixes are needed.)

Smaller concerns

4. `std::env::set_var` in the test is process-global; risks race with other tests

test_env_passthrough_allowlist_injects_var calls std::env::set_var/remove_var on real env vars. The unique LIBREFANG_TEST_PASSTHROUGH_* prefix avoids name collisions, but set_var/remove_var mutate process-global state — Rust 1.80+ marks them as unsafe for exactly this reason. If other tests in the same crate also touch env vars, the cargo test harness's parallel runner can interleave them.

The comment "tests in this module run serially-enough" is hopeful, not enforced. Either:

Mark this test #[serial_test::serial] (the workspace already uses serial_test for similar cases — grep usage), or
Run via a child-process invocation that gets only the env vars you set explicitly, no global mutation.

CI might be green today and flaky next week.

5. No user-facing docs

New skill.toml field, no entry in docs/ or any skill-authoring guide. A skill author writing gog (or whoever next hits this) won't know the field exists. Worth a follow-up doc pass at minimum; ideally in this PR.

6. WASM out of scope — documented, fine

PR body explicitly notes WASM has a separate sandbox model. Reasonable. Worth a tracking issue for the WASM-side equivalent if there's any expected-future need.

Non-blocking nit

7. Manifest field naming

env_passthrough reads as a noun referring to a list of environment variables. Slightly clearer would be passthrough_env_vars or host_env_imports, mirroring [exec_policy].allowed_env_vars. Not worth churning the field name once code is committed; just for future shape if a v2 lands.

Bottom line

#1, #2, #3 are merge blockers — together they turn this from a "narrow opt-in" into a credential exfiltration vector. Each fix is small (10–30 lines). #4 is a flaky-CI prevention; #5 is doc hygiene; #6/#7 are nits.

After #1-#3 land this is a clean small feature that solves a real env_clear blocker without being the security regression it currently is.

Adds [skills].env_passthrough_denied_patterns and [skills].env_passthrough_per_skill to KernelConfig so the operator, not the skill author, has the final say on which host env vars cross the env_clear boundary into a skill subprocess. Defaults to deny-by-default for the long tail of credential conventions (*_KEY, *_TOKEN, *_PASSWORD, *_SECRET, *_API_KEY, AWS_*, GITHUB_*). Operators grant exceptions per-skill via env_passthrough_per_skill — e.g. [skills.env_passthrough_per_skill] gog = ["GOG_KEYRING_PASSWORD"] Introduces EnvPassthroughPolicy in librefang-types as the runtime representation. Construction wired up via from_skills_config; the loader-side resolution that consumes it lands in the next commit. Addresses review #1 on PR librefang#3219: skill manifest's env_passthrough allowlist is author-controlled, but credentials are an operator concern. Without an operator-side gate, a community skill shipping env_passthrough = ["AWS_SECRET_ACCESS_KEY"] would silently leak every secret in the host environment to anyone who installs it without auditing the manifest.

…el env Three security fixes to env_passthrough on top of the operator-side config landed in the previous commit. All three were merge blockers on PR librefang#3219. 1. Hard-block FORBIDDEN_PASSTHROUGH (review #2). Names like LD_PRELOAD, PYTHONPATH, NODE_OPTIONS, DYLD_INSERT_LIBRARIES, etc. are dropped regardless of skill manifest or operator config — these inject code or redirect imports/library lookup and would defeat the env_clear isolation entirely. Not overridable; even per-skill operator overrides cannot unblock them. 2. Hard-block KERNEL_RESERVED_ENV. Names the kernel sets explicitly per-runtime (PATH, HOME, PYTHONIOENCODING, NODE_NO_WARNINGS, …) are dropped. A skill cannot widen a deliberately narrowed kernel PATH or override HOME via env_passthrough. 3. Apply passthrough before kernel env settings (review #3). tokio::process::Command::env is last-write-wins; the original PR set PATH/HOME/PYTHONIOENCODING and *then* called apply_env_passthrough, so a skill with env_passthrough = ["PATH"] could clobber the kernel-curated PATH. Reordered so passthrough runs first; kernel settings always win. Belt-and-suspenders, the resolver also strips kernel-reserved names so the ordering is defense-in-depth, not the only line of defense. Plumbing: - librefang-types: EnvPassthroughPolicy with from_skills_config. - KernelHandle: skill_env_passthrough_policy() with default-None impl; kernel impl pulls from KernelConfig.skills. - librefang-skills::loader: resolve_effective_passthrough applies the full pipeline (forbidden → kernel-reserved → operator deny → per- skill override). execute_skill_tool now takes the policy and resolves once before dispatching to the per-runtime spawner. - tool_runner: fetches policy via the kernel handle at the dispatch site so the existing execute_tool signature is unchanged.

…docs Test coverage for the env_passthrough resolver (5 unit tests): - Forbidden vars (LD_PRELOAD, PYTHONPATH) are blocked. - Kernel-reserved vars (PATH, HOME, …) are blocked. - Forbidden match is case-insensitive. - Operator deny patterns (AWS_*, *_KEY) work. - Per-skill override unblocks denied vars only for the named skill, and cannot bypass the FORBIDDEN_PASSTHROUGH hard block. Mark the existing env-mutating integration test #[serial_test::serial(skill_env_passthrough)] (review #4): it calls std::env::set_var/remove_var which mutates process-global state and would race with any other env-touching test under the parallel default of cargo test. The serial harness is already used by hands/ extensions for the same reason. Docs (review #5): add an Environment Variable Passthrough section to docs/src/app/agent/skills/page.mdx covering the two-party opt-in model (skill author declares; operator gates), the resolution order, the FORBIDDEN list, and a worked gog example. Calls out the distinction from skill config variables — credentials should go through config, not env.

neo-wanderer · 2026-04-26T13:51:22Z

Thanks for the detailed review @houko — addressed in three follow-up commits on this branch (fe5b777, 84ed49d, 1dfb44e):

Blockers

#1 — operator-controlled allowlist (fe5b777)

Added [skills].env_passthrough_denied_patterns and [skills].env_passthrough_per_skill to KernelConfig. Took your option (b) — deny-by-default — with these defaults:

*_KEY, *_TOKEN, *_PASSWORD, *_SECRET, *_API_KEY, AWS_*, GITHUB_*

Operators grant exceptions per-skill:

[skills.env_passthrough_per_skill]
gog = ["GOG_KEYRING_PASSWORD"]

Skill author still declares intent in skill.toml (good for docs and matching the override key); operator decides what actually flows through. New EnvPassthroughPolicy type in librefang-types carries this from config to the loader.

#2 — FORBIDDEN_PASSTHROUGH hard block (84ed49d)

Exactly the const you suggested, plus LD_AUDIT and PYTHONEXECUTABLE for completeness. Filtered in resolve_effective_passthrough with a tracing::warn! line per rejection. Also added KERNEL_RESERVED_ENV (PATH, HOME, PYTHONIOENCODING, NODE_NO_WARNINGS, SHELL, TERM, …) — non-overridable even by per-skill operator config. The per-skill override cannot bypass the FORBIDDEN list, and there's a unit test asserting that.

#3 — ordering (84ed49d)

Took your preferred fix: apply_env_passthrough now runs before the kernel's cmd.env(...) lines in all three runtimes (Python, Node, Shell). Kernel settings win on last-write-wins. As belt-and-suspenders, the resolver also strips kernel-reserved names so the ordering is defense-in-depth, not the only line of defense.

Smaller

#4 — test serialization (1dfb44e). Added serial_test = \"3\" as a dev-dep and marked the env-mutating test #[serial_test::serial(skill_env_passthrough)].

#5 — docs (1dfb44e). New ## Environment Variable Passthrough section in docs/src/app/agent/skills/page.mdx covering the two-party opt-in model, resolution order, FORBIDDEN list, and a worked gog example. Calls out the distinction from skill config variables — credentials should go through config, not env.

#6 — WASM. Will file a separate tracking issue.

#7 — field name. Per your guidance, leaving as env_passthrough to avoid churn.

Coverage

5 new unit tests on resolve_effective_passthrough:

forbidden var blocked even without operator policy
kernel-reserved blocked even without operator policy
forbidden match is case-insensitive
operator deny patterns work (*_KEY, AWS_*)
per-skill override unblocks denied vars only for the named skill, and cannot bypass FORBIDDEN

172 tests pass in librefang-skills; clippy clean across affected crates; golden config schema regenerated.

houko · 2026-04-26T14:59:59Z

Nice work on the operator-side gate, FORBIDDEN block, and ordering fix — those address the bulk of the original concerns. Few new findings from this pass, ordered by severity:

1. Case-sensitivity asymmetry in the deny path (security gap)

name_matches is case-insensitive (good — LD_PRELOAD and ld_preload are both blocked), but the operator deny patterns are evaluated through librefang_types::capability::glob_matches, which is case-sensitive (raw starts_with / ends_with / ==).

That means a skill declaring:

env_passthrough = ["aws_secret_access_key"]

bypasses the default deny pattern AWS_*. Two ways this becomes exploitable:

Windows: env-var names are case-insensitive in the OS, so std::env::var("aws_secret_access_key") returns the value of AWS_SECRET_ACCESS_KEY. The skill gets the secret without ever matching the operator's deny list.
Unix: rarer, but users do set lowercase vars (e.g. database_password) and they'd flow through unblocked.

Suggested fix: lowercase both pattern and name before the glob match in resolve_effective_passthrough, or thread a case_insensitive: bool flag into glob_matches. Worth a unit test mirroring test_resolve_passthrough_forbidden_is_case_insensitive for the deny-pattern branch.

2. `librefang skill test` bypasses operator policy

cmd_skill_test (crates/librefang-cli/src/main.rs:6920) passes None for env_policy. So during dev/CLI testing only FORBIDDEN_PASSTHROUGH and KERNEL_RESERVED_ENV apply — operator deny patterns and per-skill overrides are skipped entirely.

Concrete footgun: a skill declares env_passthrough = ["AWS_SECRET_ACCESS_KEY"]. librefang skill test runs with the var present and the skill "works". Same skill in production gets stripped by the default *_KEY deny — silent behavior change between test and prod.

Suggested fix: have cmd_skill_test load ~/.librefang/config.toml's [skills] block and build EnvPassthroughPolicy::from_skills_config(...), falling back to from_skills_config(&SkillsConfig::default()) so the conservative defaults still apply when no config exists. The dev should see the same gate prod will apply.

3. Non-subprocess runtimes silently ignore the field

Plumbing only touches Python / Node / Shell. A skill author who writes env_passthrough = [...] in a Builtin, PromptOnly, or Wasm manifest gets no feedback that the field is inert — the manifest parses fine, the var just never reaches anything.

Low-cost fix: emit a WARN at registry load time when env_passthrough is non-empty for a runtime that doesn't honor it. Same shape as the existing tracing::warn! calls in the resolver.

4. Doc rot on `EnvPassthroughPolicy::from_skills_config`

/// ... Returns `None` when the operator has explicitly disabled the deny check ...
pub fn from_skills_config(cfg: &SkillsConfig) -> Self {

Comment says Returns None, signature is -> Self. The Optional-ness lives on KernelHandle::skill_env_passthrough_policy() instead. Either drop that sentence from the doc or change the return type.

Nothing blocking on its own, but #1 and #2 together are the kind of gap where the manifest's stated security posture diverges from what's actually enforced — worth tightening before this lands.

…cy, runtime warn - resolve_effective_passthrough lowercases both pattern and name before glob match, closing a bypass where 'aws_secret_access_key' would slip past the default 'AWS_*' deny pattern (Windows env vars are case-insensitive at the OS level). New unit test asserts mixed-case manifest entries are blocked. - librefang skill test now loads [skills] from ~/.librefang/config.toml and applies the same EnvPassthroughPolicy the daemon does, falling back to SkillsConfig::default() when no config exists. Dev runs see the same gate prod will apply instead of silently passing every declared var. - registry.load_skill emits a tracing::warn when env_passthrough is non-empty for Builtin / PromptOnly / Wasm runtimes, since the field only flows to subprocess-spawning runtimes (Python / Node / Shell) and was previously inert without feedback. - Drop stale 'Returns None' sentence on EnvPassthroughPolicy::from_skills_config doc; the Optional-ness lives on KernelHandle::skill_env_passthrough_policy.

neo-wanderer · 2026-04-26T16:08:03Z

Thanks again @houko — addressed all four findings in b6624ff.

#1 — case-sensitivity asymmetry (security gap)

resolve_effective_passthrough now lowercases both the deny pattern and the manifest name before calling glob_matches. The aws_secret_access_key bypass against the default AWS_* deny is closed. New unit test test_resolve_passthrough_deny_pattern_is_case_insensitive asserts mixed-case (aws_secret_access_key, Aws_Region, openai_api_key) all get blocked under the default policy.

I went with lowercase-both-sides at the call site rather than threading a case_insensitive: bool into glob_matches — keeps the change scoped to skill env-passthrough and avoids touching capability-grant matching, which is a different security domain (some grants intentionally distinguish case).

#2 — librefang skill test bypassed operator policy

Added load_skill_env_policy_from_config() in the CLI: parses [skills] from ~/.librefang/config.toml, falls back to SkillsConfig::default() (so the conservative built-in deny patterns still apply when no config file exists), and cmd_skill_test now passes Some(&policy) to execute_skill_tool. Dev test now sees the same gate prod will apply.

#3 — non-subprocess runtimes silently ignore the field

registry::load_skill now emits a tracing::warn when manifest.env_passthrough is non-empty for Builtin / PromptOnly / Wasm runtimes. Same shape as the existing resolver warns; tells authors to move credentials to [skill.config] or drop the field.

#4 — doc rot on from_skills_config

Dropped the stale "Returns None" sentence and replaced it with a note that Self is always populated and that the optional-skip lives on KernelHandle::skill_env_passthrough_policy() — that's where the "no operator gate" decision actually gets made.

Build clean, cargo test -p librefang-skills 173 pass, clippy clean across affected crates.

houko

LGTM. Defense-in-depth design is solid — operator gate plus per-skill override mirrors the existing [exec_policy].allowed_env_vars shape, and the tests cover the load-bearing invariant (override cannot bypass FORBIDDEN_PASSTHROUGH).

Three nits being addressed in a follow-up PR:

name_matches is a redundant wrapper for eq_ignore_ascii_case — drop it.
from_skills_config always returns Some(...), including for the empty-config case, which makes the Option semantically dead. Better to return Option<Self> so None actually means "no operator gate".
The end-to-end test relies on std::env::set_var, which becomes unsafe in Rust 2024 edition. The serial_test key only protects against the same test running in parallel, not against other tokio threads reading env. Tighten the serial key for now and revisit before the edition bump.

None of these block the merge.

Three follow-ups, none behavior-changing for the default config: - Drop the `name_matches` wrapper; inline `str::eq_ignore_ascii_case` at the four call sites in `loader.rs`. - `EnvPassthroughPolicy::from_skills_config` now returns `Option<Self>`, returning `None` when both `denied_patterns` and `per_skill_overrides` are empty. Previously it always returned `Some(...)`, which made the `Option` parameter on `execute_skill_tool` semantically dead. `SkillsConfig::default()` ships with a non-empty deny list, so the default config still produces `Some(...)`; only operators who have explicitly cleared both fields see `None`. - Tighten the `serial_test` key on `test_env_passthrough_allowlist_injects_var` from `skill_env_passthrough` to the conventional `env`. The test calls `std::env::set_var`, which is process-global; the old key only serialized against other passthrough tests, not against any other env-touching test in the workspace. Note the proper fix (inject an env-getter) is required before the Rust 2024 edition bump that marks `set_var` as `unsafe`.

github-actions Bot added ready-for-review PR is ready for maintainer review area/skills Skill system and FangHub marketplace size/M 50-249 lines changed and removed ready-for-review PR is ready for maintainer review labels Apr 26, 2026

github-actions Bot assigned neo-wanderer Apr 26, 2026

github-actions Bot requested a review from houko April 26, 2026 07:52

houko requested changes Apr 26, 2026

View reviewed changes

github-actions Bot and others added 5 commits April 26, 2026 12:38

Merge branch 'main' into feat/skill-env-passthrough

f4f090a

Merge branch 'main' into feat/skill-env-passthrough

fdddee2

Merge branch 'main' into feat/skill-env-passthrough

36f073a

neo-wanderer requested a review from houko April 26, 2026 13:52

github-actions Bot added area/docs Documentation and guides area/runtime Agent loop, LLM drivers, WASM sandbox area/kernel Core kernel (scheduling, RBAC, workflows) size/L 250-999 lines changed and removed size/M 50-249 lines changed labels Apr 26, 2026

neo-wanderer added 2 commits April 26, 2026 21:28

Merge branch 'main' into feat/skill-env-passthrough

9dcdc1c

Merge branch 'main' into feat/skill-env-passthrough

5da846b

houko approved these changes Apr 27, 2026

View reviewed changes

houko merged commit d31437b into librefang:main Apr 27, 2026
28 of 29 checks passed

houko mentioned this pull request Apr 27, 2026

refactor(skills): tidy env_passthrough nits from #3219 review #3273

Merged

4 tasks

houko mentioned this pull request Apr 27, 2026

release: v2026.4.28-beta7 #3436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat(skills): add env_passthrough allowlist to skill manifest#3219

feat(skills): add env_passthrough allowlist to skill manifest#3219
houko merged 10 commits into
librefang:mainfrom
neo-wanderer:feat/skill-env-passthrough

neo-wanderer commented Apr 26, 2026

Uh oh!

houko left a comment

Uh oh!

neo-wanderer commented Apr 26, 2026

Uh oh!

houko commented Apr 26, 2026

Uh oh!

neo-wanderer commented Apr 26, 2026

Uh oh!

houko left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

neo-wanderer commented Apr 26, 2026

Summary

Motivation

Implementation notes

Tests

Test plan

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Overall

Blocking — security model

1. The [exec_policy].allowed_env_vars mirror is misleading; this PR's allowlist is author-controlled, not operator-controlled

2. No denylist for vars that compromise subprocess isolation

Blocking — correctness

3. apply_env_passthrough runs after kernel-controlled env settings, so it overrides them

Smaller concerns

4. std::env::set_var in the test is process-global; risks race with other tests

5. No user-facing docs

6. WASM out of scope — documented, fine

Non-blocking nit

7. Manifest field naming

Bottom line

Uh oh!

neo-wanderer commented Apr 26, 2026

Blockers

Smaller

Coverage

Uh oh!

houko commented Apr 26, 2026

1. Case-sensitivity asymmetry in the deny path (security gap)

2. librefang skill test bypasses operator policy

3. Non-subprocess runtimes silently ignore the field

4. Doc rot on EnvPassthroughPolicy::from_skills_config

Uh oh!

neo-wanderer commented Apr 26, 2026

Uh oh!

houko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

1. The `[exec_policy].allowed_env_vars` mirror is misleading; this PR's allowlist is author-controlled, not operator-controlled

3. `apply_env_passthrough` runs after kernel-controlled env settings, so it overrides them

4. `std::env::set_var` in the test is process-global; risks race with other tests

2. `librefang skill test` bypasses operator policy

4. Doc rot on `EnvPassthroughPolicy::from_skills_config`

houko left a comment •

edited

Loading