Skip to content

Feat/env filtering#140

Closed
dtomasi wants to merge 6 commits intoUse-Tusk:mainfrom
dtomasi:feat/env-filtering
Closed

Feat/env filtering#140
dtomasi wants to merge 6 commits intoUse-Tusk:mainfrom
dtomasi:feat/env-filtering

Conversation

@dtomasi
Copy link
Copy Markdown
Contributor

@dtomasi dtomasi commented Apr 26, 2026

Add environment variable filtering support

I've been using Fence for my development workflows and found it really helpful. This PR adds environment variable filtering to complement the existing sandbox controls.

What's included

Main feature (commit e6db3d5):

  • New environment section in fence.json with allowedVars and deniedVars glob patterns
  • Security-first approach where deny patterns are checked before allow patterns
  • Backward compatible - existing setups continue working unchanged
  • Comprehensive test coverage with 15+ test cases

Tooling fixes (commits 1f96100, 0cdd203):

  • Fixed inconsistent golangci-lint versioning across Makefile, shell.nix, and CI
  • Updated linter configuration from v1 to v2 format
  • Applied v2 compliance fixes (import formatting, error message casing, security annotations)

Example usage

{
  "environment": {
    "deniedVars": [
      "*_SECRET*",
      "*_PASSWORD*",
      "*_TOKEN*",
      "*_APIKEY*",
      "*_API_KEY*"
    ]
  }
}

This prevents sensitive credentials from leaking into sandboxed processes while still allowing other environment variables through.

Note on commit structure

The tooling commits address version mismatches and linting issues that became apparent during development. If you prefer, I can cherry-pick the main feature commit (e6db3d5) into a separate PR and handle the tooling updates independently.

Testing

All existing tests pass, and the new environment filtering functionality includes extensive test coverage for various glob pattern scenarios.

dtomasi added 3 commits April 26, 2026 20:17
Users can now control which environment variables get passed to sandboxed
processes via configuration. This adds a new `environment` section to
fence.json with `allowedVars` and `deniedVars` using glob patterns.

- Add Environment config to fence.json with allow/deny patterns
- Implement glob pattern matching (*_TOKEN, AWS_*, etc.)
- Add FilterEnvironmentVars and GetHardenedEnvWithConfig functions
- Deny patterns are checked first (security-first approach)
- Maintain backward compatibility with GetHardenedEnv()
- Full test coverage with 15+ test cases

Signed-off-by: Dominik Tomasi <dominik.tomasi@gmail.com>
During testing, we found the linting setup was broken in multiple ways.
The Makefile said v1, shell.nix had v2, .golangci.yml was written for v1,
and things just wouldn't install properly.

This commit fixes all of that by picking v2.11.4 as our version, updating
all configs to match, and making Makefile the single source of truth.

Problems we found:
- Makefile documented v1 but was trying to use v2 import path
- shell.nix installed v2, conflicting with Makefile's old command
- .golangci.yml was v1 syntax, incompatible with v2
- Different versions in local dev vs CI made things unreproducible

What we fixed:
- Pinned golangci-lint to v2.11.4 in Makefile
- Rewrote .golangci.yml for v2 format
- Removed tools from shell.nix (rely on Makefile instead)
- Updated GitHub Actions to use make install-lint-tools

Signed-off-by: Dominik Tomasi <dominik.tomasi@gmail.com>
Now that the linter is working, v2 has stricter rules than v1.
This commit applies those rules across the codebase.

Main changes:
- Add #nosec annotations where we've validated inputs (network ops, file I/O)
- Fix import formatting (blank lines between stdlib and internal imports)
- Lowercase some error message strings for consistency

This is all mechanical/policy stuff - no logic changes.

Signed-off-by: Dominik Tomasi <dominik.tomasi@gmail.com>
@dtomasi dtomasi force-pushed the feat/env-filtering branch from 0cdd203 to d9f9122 Compare April 26, 2026 18:18
@dtomasi dtomasi marked this pull request as ready for review April 26, 2026 18:18
@dtomasi dtomasi requested a review from jy-tan as a code owner April 26, 2026 18:18
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 issues found across 25 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="cmd/fence/hooks_runtime.go">

<violation number="1" location="cmd/fence/hooks_runtime.go:208">
P1: `isPureCDCommand` no longer matches standard `cd <path>` commands, so plain directory changes are misclassified and may be wrapped instead of skipped.</violation>
</file>

<file name="Makefile">

<violation number="1" location="Makefile:78">
P2: `lint-fix` was added as a command target but not declared phony, so `make lint-fix` can be skipped if a file/directory named `lint-fix` exists.</violation>
</file>

<file name="docs/schema/fence.schema.json">

<violation number="1" location="docs/schema/fence.schema.json:83">
P2: Schema documentation for `environment.allowedVars` is incorrect: empty `allowedVars` does not block all vars at runtime, causing misleading config semantics.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread cmd/fence/hooks_runtime.go Outdated
Comment thread Makefile
Comment thread docs/schema/fence.schema.json
dtomasi added 3 commits April 26, 2026 20:25
Linting changes in d9f9122 broke cd command detection logic.
Commands like 'cd ../repo' were incorrectly rejected.
The description incorrectly stated that empty allowedVars blocks all
environment variables. The actual implementation allows all variables
by default when allowedVars is empty.
Targets that don't produce files with their name should be declared
as .PHONY to prevent make from skipping them if files with those
names exist.
@jy-tan
Copy link
Copy Markdown
Contributor

jy-tan commented Apr 27, 2026

@dtomasi happy to hear that Fence has been helpful, and thanks for the contribution! Before we dig into the code, I want to be really clear on the "why", because the goal isn't quite clear to me yet and I think it changes whether this belongs in Fence at all.

Fence already strips LD_* / DYLD_* so the dynamic linker of the sandboxed process can't be hijacked by env the parent set - that's about preserving sandbox integrity

This PR points the other direction: protecting the caller's secrets from the sandboxed process. That's a real concern, especially for coding agents, but I'm not yet convinced Fence is the right layer for it, for two reasons:

  1. It's a meaningful shift in Fence's posture on env. Today env is treated as caller-controlled — PATH, HOME, SSH_AUTH_SOCK, CI vars, tool-specific tokens, etc. all flow through by design. Adding selective filtering changes that contract, and I'd want us to be deliberate about whether we want to own "Fence sanitizes env" as a guarantee, because once it's a config surface users will reasonably expect it to be one.

  2. Even if we did take that on, I think the invoker is a better-positioned layer than Fence. The shell, CI runner, direnv, op run, etc. assemble the env in the first place and have full context for what should reach the child. Fence sees the env after that, with less context. The shell-level fix (e.g., env -u MY_TOKEN fence claude, env -i PATH=$PATH HOME=$HOME fence claude) is also strictly broader, since it catches cases this PR can't, like fence -c "MY_TOKEN=1 claude" where the assignment is interpreted by the inner shell and never passes through os.Environ() in Fence at all.

Could you spell out what Fence is meant to guarantee here? Concretely: what does it guarantee that env -u / env -i at the call site doesn't, and what does it explicitly not guarantee?

A few notes for later:

  • Appreciate the linter update, but I'd like these in separate PRs so it's easier to review.
  • The schema says an empty allowedVars means no env vars pass through, but the code/struct tag say empty means all env vars pass through.
  • docs/configuration.md should be updated.

@dtomasi
Copy link
Copy Markdown
Contributor Author

dtomasi commented Apr 27, 2026

@jy-tan Thanks for the detailed feedback!

My intention was to address what I've observed - many developers don't separate secrets in their shell configurations and keep everything together in their bash/ZSH setup. The idea was to enable sharing a configuration file that ensures dangerous environment variables are filtered out by default when working with agents, rather than relying on each developer to remember proper filtering.

However, I understand your points about layer responsibility and Fence's design philosophy. I can see this doesn't align well with the project's scope and architecture.

I'd recommend closing this PR. Happy to extract the linter fixes to a separate PR if that would be helpful. Please find #144

Thanks for the clear explanation!

@jy-tan
Copy link
Copy Markdown
Contributor

jy-tan commented Apr 27, 2026

Closing for now, thanks nonetheless!

@jy-tan jy-tan closed this Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants