fix(desktop): use allowlist for terminal env filtering to prevent secret leakage#582
Conversation
Electron's main process runs with NODE_ENV=production in packaged builds. This was being inherited by all spawned terminal sessions, causing unexpected behavior in user commands (e.g., pnpm install skipping devDependencies). - Remove NODE_ENV from terminal environment (follows GOOGLE_API_KEY pattern) - Add defensive tests for all NODE_ENV values (production, development, test, unset)
📝 WalkthroughWalkthroughAdds an allowlist-based environment sanitizer (exports Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Desktop App (main)
participant Proc as process.env (raw)
participant Sanitizer as buildSafeEnv
participant Shell as shellEnv (user)
participant Terminal as Spawned Terminal
App->>Proc: read sanitized raw process.env
Proc->>Sanitizer: provide raw env for locale detection & filtering
Sanitizer-->>App: return allowed/sanitized base env
Shell-->>App: provide shellEnv overrides
App->>Terminal: merge(sanitized base env, shellEnv) -> final terminal env
Note right of Terminal: Locale derived from raw/sanitized process.env\nSanitization applied before merge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Expand terminal environment filtering based on Oracle consultation: - Restructure to clean baseEnv BEFORE merging with shellEnv (user env wins) - Add behavior-changing vars: NODE_OPTIONS, NODE_PATH, ELECTRON_RUN_AS_NODE - Add prefix-based cleanup: VITE_*, MAIN_VITE_*, NEXT_PUBLIC_*, TURBO_*, ELECTRON_VITE_* - Add app secrets: GOOGLE_CLIENT_ID, GH_CLIENT_ID, SENTRY_DSN_DESKTOP - Extract removeAppEnvVars() for testability - Add 42 comprehensive tests including user env preservation scenarios
…v filtering Replace the previous denylist approach with an allowlist to address PR review feedback: - P0 (security): Secrets like DATABASE_URL, CLERK_SECRET_KEY, NEON_API_KEY can no longer leak - unknown vars are excluded by default - P1 (UX): User's VITE_*/TURBO_* vars from their shell are no longer stripped (they simply won't be in Electron's process.env to begin with) - P2 (clarity): Fixed misleading shellEnv comment - it provides shell wrapper control vars (ZDOTDIR, BASH_ENV), not user's environment Allowlist includes: - Core shell vars: PATH, HOME, USER, SHELL, TERM, LANG, LC_*, TZ - SSH vars: SSH_AUTH_SOCK, SSH_AGENT_PID (critical for git) - Proxy vars: HTTP_PROXY, HTTPS_PROXY, NO_PROXY (both cases) - Language managers: NVM_DIR, PYENV_ROOT, RBENV_ROOT, GOPATH, etc. - System paths: Homebrew, XDG directories, editor preferences - Our metadata: SUPERSET_* prefix Tests updated: 53 tests covering allowlist behavior including secret exclusion.
andreasasprou
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal/env.ts (1)
66-180: Excellent allowlist approach with comprehensive coverage.The allowlist strategy is security-sound: unknown variables (including secrets) are excluded by default. The included categories cover the essential shell infrastructure, language tooling, and user preferences.
Optional: Consider adding a few more user-environment variables
If users report missing functionality, consider adding these to the allowlist:
"EDITOR", "VISUAL", "PAGER", + "MANPATH", // Custom man page paths + "PKG_CONFIG_PATH", // Custom pkg-config paths + "INFOPATH", // Custom info paths + "GPG_TTY", // GPG terminal (needed for commit signing)Note: Deliberately omitting
LD_LIBRARY_PATH/DYLD_LIBRARY_PATHis reasonable—they can introduce security and stability risks if Electron's library paths leak to user shells.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Co-locate tests with implementation files using .test.ts or .test.tsx suffix
Files:
apps/desktop/src/main/lib/terminal/env.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
apps/desktop/src/main/lib/terminal/env.test.tsapps/desktop/src/main/lib/terminal/env.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/lib/electron-router-dom.ts : Do not import Node.js modules like node:path or dotenv in electron-router-dom.ts and similar shared files - they run in both main and renderer processes
Applied to files:
apps/desktop/src/main/lib/terminal/env.test.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/renderer/**/*.{ts,tsx} : Never import Node.js modules (fs, path, os, net) in renderer process or shared code - they are externalized for browser compatibility
Applied to files:
apps/desktop/src/main/lib/terminal/env.test.ts
🧬 Code graph analysis (2)
apps/desktop/src/main/lib/terminal/env.test.ts (1)
apps/desktop/src/main/lib/terminal/env.ts (3)
buildSafeEnv(191-210)removeAppEnvVars(215-219)buildTerminalEnv(221-267)
apps/desktop/src/main/lib/terminal/env.ts (1)
apps/desktop/src/main/lib/agent-setup/index.ts (1)
getShellEnv(59-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (7)
apps/desktop/src/main/lib/terminal/env.ts (3)
191-210: Clean and correct allowlist filtering implementation.The logic properly checks exact matches before prefix matches, preserves immutability, and handles the filtering correctly.
215-219: Good backward compatibility strategy.The deprecated wrapper cleanly delegates to the new implementation while giving consumers time to migrate.
240-248: Correct integration of allowlist filtering.The use of
rawBaseEnvfor locale detection is appropriately defensive—it ensuresgetLocalehas access to all locale-related variables before the allowlist filter is applied. The subsequent filtering viabuildSafeEnvcorrectly prevents Electron's internal variables from leaking to user terminals.apps/desktop/src/main/lib/terminal/env.test.ts (4)
107-384: Exceptionally thorough test coverage for buildSafeEnv.The test suite systematically validates:
- Exclusion of dangerous runtime vars (NODE_ENV, NODE_OPTIONS, etc.)
- Exclusion of secrets (API keys, tokens, DB URLs)
- Exclusion of build-time vars (VITE_, NEXT_PUBLIC_, TURBO_*)
- Inclusion of all allowlisted categories (shell, SSH, language managers, proxies, locale, etc.)
- Prefix-based filtering (SUPERSET_, LC_)
- Non-mutation guarantee
- Edge cases (empty input)
This comprehensive coverage significantly reduces the risk of regressions.
386-393: Adequate coverage for the deprecated wrapper.The test confirms correct delegation to
buildSafeEnv, which is sufficient given the wrapper's simplicity and the comprehensive tests for the underlying function.
403-433: Excellent test hygiene with environment state restoration.The
beforeEach/afterEachhooks properly save and restore environment variables, preventing test pollution. This is especially important for tests that modifyprocess.envdirectly.
435-529: Comprehensive tests for buildTerminalEnv behavior.The tests thoroughly validate:
- Non-allowlisted variables from Electron's
process.envare excluded from terminal sessions- Terminal metadata (TERM_PROGRAM, COLORTERM) is correctly set
- Superset-specific environment variables are properly injected
- Optional parameters are handled gracefully (both present and absent)
- LANG is set to a UTF-8 locale
This ensures the allowlist filtering integrates correctly with terminal environment construction.
…ng vars
Address PR review feedback:
P0 (block): Windows env vars are case-insensitive. The system stores
'Path' not 'PATH', 'SystemRoot' not 'SYSTEMROOT'. Added case-insensitive
matching on win32 platform to prevent PATH from being dropped.
P1 (must fix): Added missing Windows vars:
- TEMP, TMP: Required by many build tools and installers
- PATHEXT: Required for command resolution on Windows
- PROGRAMFILES(X86): Common Windows program path
P2 (should fix): Fixed misleading comment in buildSafeEnv docstring.
shellEnv provides shell wrapper control vars (ZDOTDIR, BASH_ENV), not
user's actual environment.
Changes:
- buildSafeEnv now accepts optional { platform } for testability
- Added isAllowedVar() and hasAllowedPrefix() helpers with Windows support
- Added 7 new tests for Windows case-insensitivity behavior
Tests: 60 passing
Add non-secret developer config vars that users expect to pass through: SSL/TLS: - SSL_CERT_FILE, SSL_CERT_DIR, NODE_EXTRA_CA_CERTS, REQUESTS_CA_BUNDLE Git (config, not credentials): - GIT_SSH_COMMAND, GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL - GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL, GIT_EDITOR, GIT_PAGER AWS (profile selection, not credentials): - AWS_PROFILE, AWS_DEFAULT_REGION, AWS_REGION - AWS_CONFIG_FILE, AWS_SHARED_CREDENTIALS_FILE Docker/Kubernetes: - DOCKER_HOST, DOCKER_CONFIG, DOCKER_CERT_PATH, DOCKER_TLS_VERIFY - COMPOSE_PROJECT_NAME, KUBECONFIG, KUBE_CONFIG_PATH Cloud CLIs: - CLOUDSDK_CONFIG (Google Cloud), AZURE_CONFIG_DIR (Azure) SDK paths: - JAVA_HOME, ANDROID_HOME, ANDROID_SDK_ROOT, FLUTTER_ROOT, DOTNET_ROOT These are config vars that users might set in their shell before launching Superset, and would expect to carry through to terminal sessions. Tests: 65 passing (+5 new)
Update docstring to accurately describe what we're protecting against: - App secrets from .env (DATABASE_URL, API keys) are blocked - User shell config vars (proxy, tool paths) are intentionally allowed - Note that allowlisted vars like HTTP_PROXY may contain user credentials
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/terminal/env.ts (1)
38-47: Consider adding error logging for locale detection failures.While the fallback behavior is appropriate, the coding guidelines suggest never swallowing errors silently. Consider adding a console log with context when locale detection fails.
🔎 Optional improvement
try { const result = execSync("locale 2>/dev/null | grep LANG= | cut -d= -f2", { encoding: "utf-8", timeout: 1000, }).trim(); if (result?.includes("UTF-8")) { return result; } - } catch { - // Ignore - will use fallback + } catch (error) { + console.warn("[terminal/env] Locale detection failed, using fallback:", error); }Based on coding guidelines.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/main/lib/terminal/env.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/main/lib/terminal/env.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/main/lib/terminal/env.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/main/lib/terminal/env.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/main/lib/terminal/env.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
📚 Learning: 2026-01-02T06:50:28.662Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-02T06:50:28.662Z
Learning: Applies to apps/desktop/src/main/index.ts : Load environment variables from monorepo root .env in desktop app with override: true before any imports in src/main/index.ts and electron.vite.config.ts
Applied to files:
apps/desktop/src/main/lib/terminal/env.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (5)
apps/desktop/src/main/lib/terminal/env.ts (5)
66-227: Excellent allowlist design with strong security boundaries.The allowlist is comprehensive and well-documented. The categorization is clear, and the security model correctly excludes app secrets (NODE_ENV, DATABASE_URL, API keys) while including essential infrastructure variables (PATH, HOME, SSH_AUTH_SOCK) and user configuration (proxy settings, language managers). The Windows case-insensitivity documentation is particularly helpful.
238-260: LGTM: Windows case-insensitivity correctly implemented.The helper functions properly handle Windows' case-insensitive environment variables by converting keys to uppercase for matching while preserving original casing in the output. The logic is sound and well-documented.
262-303: LGTM: Clean implementation with excellent documentation.The function correctly implements the allowlist filtering with proper documentation of the threat model. The optional platform parameter is a good design choice for testability, and the implementation preserves immutability.
305-312: LGTM: Good deprecation pattern.The backward-compatible wrapper is well-documented and provides a clear migration path for existing code.
333-341: Clarify the use ofrawBaseEnvfor locale derivation.Line 341 uses
rawBaseEnvto derive the locale, butgetLocaleonly readsLANGandLC_ALL, both of which are in the allowlist and would be present inbaseEnv. UsingbaseEnvinstead would be more consistent with the security model and make the code clearer. Is there a specific reason for using the unsanitized environment here?If this was unintentional, consider this change:
🔎 Proposed simplification
const rawBaseEnv = sanitizeEnv(process.env) || {}; const baseEnv = buildSafeEnv(rawBaseEnv); const shellEnv = getShellEnv(shell); - const locale = getLocale(rawBaseEnv); + const locale = getLocale(baseEnv);
2 participants
Summary
Problem
In packaged Electron builds,
NODE_ENV=productionleaks into spawned terminal sessions. This causespnpm installto skip devDependencies, breaking developer workflows.Why / Context
The previous denylist approach had gaps - it blocked some vars but missed actual secrets.
Switched to allowlist because:
Threat Model
Prevent app secrets (DATABASE_URL, API keys from .env) from leaking to terminals. User shell config vars (proxy, tool paths) are intentionally allowed so terminals behave like the user's normal environment.
Note: Allowlisted vars like HTTP_PROXY may contain user-configured credentials - this is by design since users expect their proxy config to work.
What's Allowed (safe infrastructure vars)
Core shell:
PATH,HOME,USER,SHELL,TERM,TMPDIRLANG,LC_*,TZSSH (critical for git):
SSH_AUTH_SOCK,SSH_AGENT_PIDProxy configuration:
HTTP_PROXY,HTTPS_PROXY,NO_PROXY(both cases)ALL_PROXY,FTP_PROXYLanguage version managers:
NVM_DIR,NVM_BIN,NVM_INC,NVM_CD_FLAGS,NVM_RC_VERSIONPYENV_ROOT,PYENV_SHELL,PYENV_VERSIONRBENV_ROOT,RBENV_SHELL,RBENV_VERSIONGOPATH,GOROOT,GOBINCARGO_HOME,RUSTUP_HOMEDENO_DIR,DENO_INSTALL,BUN_INSTALLPNPM_HOME,VOLTA_HOME,ASDF_DIR,FNM_DIR,SDKMAN_DIRDeveloper tools (paths, not credentials):
SSL_CERT_FILE,SSL_CERT_DIR,NODE_EXTRA_CA_CERTSGIT_SSH_COMMAND,GIT_AUTHOR_*,GIT_COMMITTER_*,GIT_EDITORAWS_PROFILE,AWS_DEFAULT_REGION,AWS_CONFIG_FILE(not credentials)DOCKER_HOST,DOCKER_CONFIG,KUBECONFIGJAVA_HOME,ANDROID_HOME,FLUTTER_ROOT,DOTNET_ROOTSystem paths:
HOMEBREW_PREFIX,HOMEBREW_CELLAR,HOMEBREW_REPOSITORYOur metadata (prefix):
SUPERSET_*What's Excluded (everything else)
By design, anything not in the allowlist is excluded:
NODE_ENV,NODE_OPTIONS,NODE_PATH- behavior-changingDATABASE_URL,CLERK_SECRET_KEY,NEON_API_KEY- secretsVITE_*,NEXT_PUBLIC_*,TURBO_*- build-time varsGOOGLE_API_KEY,GH_CLIENT_SECRET,SENTRY_AUTH_TOKEN- credentialsManual QA
echo $NODE_ENVreturns emptypnpm installinstalls devDependencies as expectedTesting
bun run typecheck✓bun test src/main/lib/terminal/env.test.ts✓ (65 tests)