fix: allow operator env vars in MCP config substitution; warn on refused references - #811
Merged
Merged
Conversation
Operator-configured MCP servers can reference deployment env vars with
${VAR} syntax, but the lookup deliberately excludes restricted names
(_SECRET/_PRIVATE_KEY suffixes and reserved runtime names) so MCP config
cannot pull control-plane secrets. Unresolvable references were passed
through as literal ${VAR} text with no trace anywhere, which reads as
"env MCPs don't work" and is painful to diagnose.
Keep the guard, make it loud: warn once per referenced name when a
reference names a restricted variable (with a rename suggestion) or is
not defined in the task environment.
Contributor
|
No code issues found. See task
Reviewed c4773d5 |
…of name Drop the _SECRET/_PRIVATE_KEY suffix heuristic from the MCP config substitution guard. Operator-defined deployment env vars are already present in the sandbox environment, so refusing to substitute them into MCP server config protected nothing while breaking legitimate setups (e.g. REDDIT_CLIENT_SECRET for an authenticated Reddit MCP). The guard now keys off source instead of name shape: reserved Roomote runtime names (ROOMOTE_*, AUTH_TOKEN, JOB_AUTH_*, PREVIEW_AUTH_*, DATABASE_URL, REDIS_URL, BASH_ENV) remain non-injectable, while operator-defined vars always substitute — and win on collision, so an operator's own DATABASE_URL resolves to their value, never an internal one. Refused reserved-name references still warn loudly.
Review follow-up: envVars is mutated by injectEnvVars() (auth bypass
values, BASH_ENV, PREVIEW_DOMAINS, ...) before runTask reads it, so
deriving the operator overlay from it could reclassify runtime-internal
values as operator-provided and let ${ROOMOTE_AUTH_BYPASS_VALUE} resolve
past the reserved-name guard.
Thread the existing pre-injection snapshot (userEnvVars) from
executeTaskRun through runFn into runTask and build the overlay from
that. As defense in depth, Roomote-namespaced names (ROOMOTE_*,
AUTH_TOKEN, BASH_ENV, JOB_AUTH_*, PREVIEW_AUTH_*) are dropped from the
overlay even if present in the operator map, while generic reserved
names (DATABASE_URL, REDIS_URL) can still be shadowed by an operator's
own values.
Contributor
Author
|
Addressed in c4773d5: the operator overlay is now built from the pre-injection |
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.
Problem
Environment-config MCP servers can reference deployment env vars with
${VAR}syntax, but the substitution guard filtered by name shape: any variable ending in_SECRETor_PRIVATE_KEYwas silently refused, and the literal${VAR}text was passed to the MCP server with no warning at any layer. So this natural config just doesn't authenticate, with nothing to go on:The name-shape filter also protected nothing for these vars: operator-defined deployment env vars are already injected into the sandbox runtime env, readable by the agent and every process in it. Refusing to substitute them into MCP config was friction without security.
Fix
Key the guard off source instead of name shape:
DATABASE_URL), the operator's value wins — internal values are never reachable.ROOMOTE_*,AUTH_TOKEN,JOB_AUTH_*,PREVIEW_AUTH_*,DATABASE_URL,REDIS_URL,BASH_ENV) remain non-injectable. This is the boundary that matters: MCP config substitution runs worker-side and could otherwise re-expose values the sandbox-secrets scrubbing deliberately keeps away from tasks._SECRET/_PRIVATE_KEYsuffix heuristics are gone.And make any refusal loud instead of silent: a reference to a reserved name warns with guidance, and a reference to an undefined name warns as a likely typo. Unresolved references are still passed through literally (streamable-http headers rely on this for the OpenCode-level
{env:...}conversion).Also extracts the reference-scanning regex into
collectEnvVarReferencesnext tosubstituteEnvVarsso the two can't drift.Tests
Covers: operator var with
_SECRETname substitutes; non-reserved secret-like task env var substitutes; reserved name refused + warned; operator value wins reserved-name collisions; undefined reference warns; fully-resolved config does not warn. Full worker run-task + commands suites pass; lint/typecheck/knip pass.