Repository navigation
Run each command with only the env vars it may have - #191
Open
ndbroadbent wants to merge 12 commits into
Open
ndbroadbent wants to merge 12 commits into
ndbroadbent wants to merge 12 commits into
Conversation
|
Warning Review limit reached
This review includes 88 billable files and costs up to $22.00.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 46 minutes for your next included review. View limit details
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Oct 26, 2026 1:47a.m. | Review ↗ |
| Go | |
Oct 26, 2026 1:47a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
… it may have The rack splits a command into words and runs them without a shell, in a process holding every env var of the app's release, RACK_URL (the rack's API password) included. cmdenv builds the plan of what a command gets (every env var less RACK_URL, or only the listed ones, with read-only substitutes such as DATABASE_URL from READ_ONLY_DATABASE_URL) and wraps the command in a fixed sh script that runs it with exactly that environment. Parse reads a wrapper back, for the mock rack. The tests run the wrapper with sh, dash, bash and busybox.
Every exec and process start reaches the rack wrapped by cmdenv, so the command runs without RACK_URL (the rack's
API password, which Convox gives every process) and, in an app that sets command_env_vars, with only those env vars
and the ones it asks for (X-Command-Env, or X-Command-All-Env-Vars for every one). command_env_read_only gives
read-only vars their substitute's value (DATABASE_URL from READ_ONLY_DATABASE_URL, say) unless the command asks to
write (X-Command-Write). The audit log records the command as sent, with the env vars it got.
approved_deploy_commands entries are objects now ({"command", "env", "all_env_vars", "write"}): under a deploy
approval the entry decides the command's env vars, and a command asking for any itself is refused. A migration
turns each stored command string into an object.
Also refused before anything runs: a command the rack can't split into words, an empty one (a process start
without a command runs the service's own command, which the gateway can't wrap), and one starting with NAME=value.
`convox run`'s placeholder gets no env vars at all. Slack messages say which env vars a command got, and warn when
it ran with --write.
…heir env vars The mock rack reads the env wrapper back (cmdenv.Parse) and prints the command and the env vars it gets. A process start or exec the gateway did not wrap fails, so the CLI E2E tests catch a command reaching the rack with every env var.
… --write exec and run send what the flags ask for in the X-Command-Env, X-Command-All-Env-Vars and X-Command-Write headers, which the gateway reads to decide the command's env vars. The CLI checks the names and the command (cmdenv.CheckCommand) before anything is sent. The PIN dialog lists the env vars the command gets, read from the app's settings, and shows warnings in red: --write gives the real value of a read-only env var, --all-env-vars gives every env var where the app limits them, and an exec can read its process's environment. The touch prompt says when the command can change data. The script review and --dry-run show the same warnings. The PIN and script dialogs move from AppleScript to a JXA NSAlert (internal/cli/macdialog) so they can show the warnings with a red icon.
… without the gateway cmdenv used envutil.ValidName, which pulled the gateway's config and database packages into everything that imports cmdenv, and broke the mock rack's image (it copies only the packages it needs). ValidName moves to cmdenv, and envutil and the settings checks call it there.
The mock rack prints the env vars the gateway's wrapper gives each command. The stages check the default (every env var except RACK_URL), an app's command_env_vars and command_env_read_only with --env and --write, and the refusal of --env RACK_URL.
Nothing used UpdateProtectedEnvVarsRequest, UpdateApprovedCommandsRequest, UpdateAppImagePatternsRequest, UpdateAllowDestructiveActionsRequest or UpdateMFASettingsRequest, and the OpenAPI spec no longer had them. Their generated web schemas were left over from an older spec.
…ings, and show a command's env vars Approved deploy commands are objects now, so the card edits them as JSON, with the card the service image patterns used (now JsonSettingCard). New cards edit command_env_vars (a list) and command_env_read_only (a JSON object). The audit log shows the env vars each command got, each read-only one with its source, and says when a command ran with --write.
…1.25 upgrade The commands guide explains command_env_vars, command_env_read_only, --env, --all-env-vars and --write, the wrapper and what an exec can still read. The deploy approval, settings, audit logging, permissions and checklist pages cover the new settings and the approved command objects, and the upgrade notes say what to change in the same release.
…teway does The hardening guide covers command_env_vars, command_env_read_only and RACK_URL, which Convox gives every app process unless the rack's system-env-disable feature gate is on. The upgrade notes say the old gateway approves no commands between the migration and the promote, and that run --env KEY=VALUE never set anything.
ndbroadbent
force-pushed
the
nathan/command-env
branch
from
October 26, 2026 01:47
6c84d46 to
9d2771d
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every
runandexeccommand now gets only the env vars it may have, instead of every env var of the app's release.What changes
internal/cmdenv). Before forwarding an exec or a process start, it replaces theCommandheader with a shortshwrapper that starts the command withenv -iand only the env vars it gets. An env var the release doesn't set stays unset. A command given every env var hasRACK_URLand the read-only env vars unset instead. The audit log, the deploy approval checks andexec_commandsall read the command as it was sent.RACK_URL. Convox puts the rack's API password in every app process unless the rack'ssystem-env-disablefeature gate is on.command_env_vars: the env vars every command gets. When it's empty, commands get every env var exceptRACK_URL.command_env_read_only: gives an env var another one's value, e.g.{"DATABASE_URL": "READ_ONLY_DATABASE_URL"}.runandexec:--env NAME: another env var.--all-env-vars: every env var exceptRACK_URL.--write: the real values of the read-only env vars.X-Command-*headers, which the gateway reads and doesn't forward.--writelets it change data, when--all-env-varsgives it every env var in an app that limits them, and for everyexecin such an app (an exec can still read its process's environment). The touch prompt says when the command can change data. To show the red warnings, the PIN dialog moves from AppleScript to a JXA NSAlert, like the script review dialog (internal/cli/macdialog).approved_deploy_commandsentries are objects:{"command": ..., "env": [...], "all_env_vars": bool, "write": bool}.runalways gets no env vars, so its process doesn't hold the app's env for the command that follows to read.env_varsorall_env_vars,env_read_only,write), and the web UI shows them.JsonSettingCard. New cards edit the two env var settings.cmdenv.Parse, refuses unwrapped ones, and prints each command's env vars for the CLI E2E tests.Upgrade notes
See "Each command gets only the env vars it may have (v0.1.25)" in
operations/upgrades.mdx:APPROVED_DEPLOY_COMMANDSenv var in the same release as the new image.shandenv.Testing
cmdenvruns the wrapper under sh, dash, bash and busybox. It checks:$, newlines, empty)RACK_URLis dropped--envwithcommand_env_varsandcommand_env_read_only,--write, and the refusal of--env RACK_URL.env -iremoved, with the env headers dropped, and with the exec warning removed.