diff --git a/changes/unreleased/tool-env-passthrough-names.fixed.md b/changes/unreleased/tool-env-passthrough-names.fixed.md new file mode 100644 index 000000000..a5327ae5b --- /dev/null +++ b/changes/unreleased/tool-env-passthrough-names.fixed.md @@ -0,0 +1 @@ +- **`OPENSYSML_TOOL_ENV_PASSTHROUGH` refuses malformed entries.** An entry containing `=` or a NUL byte is not an environment variable name; the tool's invocation now fails with an error naming the variable and the entry instead of forwarding it. diff --git a/docs/reference/environment.md b/docs/reference/environment.md index fdb65dc49..24296069d 100644 --- a/docs/reference/environment.md +++ b/docs/reference/environment.md @@ -23,7 +23,7 @@ run that would never finish into a reported error instead of a hang. | `OPENSYSML_ENGINES` | unset (no engines) | Directory of the **engine manifest**: one JSON file per external analysis engine — a model checker, a simulator, a solver — spoken to over its standard input by the protocol on [External engines](external-engines.md), listed by `-engines` and selected by `-engine ` like any engine of the build. Read beside `OPENSYSML_TOOLS` under the same rules; see [External engines](#external-engines) | | `OPENSYSML_TOOL_TIMEOUT` | `10s` | How long one tool process may take, as a Go duration (`5s`, `500ms`), after which the performance fails with a timeout; for an external engine, how long its `describe` and `covers` may take and the grace a `run` has to answer `cancel` before its process is ended. A value that is not a positive duration is the default | | `OPENSYSML_TOOL_MAX_OUTPUT` | `64M` | How much one external process may write before it is cut off: a tool's one reply and the whole of its standard error, an external engine's one protocol line and the whole of its standard error. Bytes, or bytes with a `K`, `M` or `G` suffix; a value that is not a positive size is the default | -| `OPENSYSML_TOOL_ENV_PASSTHROUGH` | unset | Comma-separated names of this process's environment variables handed to a tool started under a manifest [`invocation`](#external-tools) block, beside `PATH`, `HOME`, `TMPDIR` and `LANG`; nothing else of the environment reaches such a tool. A tool without the block inherits the whole environment as before | +| `OPENSYSML_TOOL_ENV_PASSTHROUGH` | unset | Comma-separated names of this process's environment variables handed to a tool started under a manifest [`invocation`](#external-tools) block, beside `PATH`, `HOME`, `TMPDIR` and `LANG`; nothing else of the environment reaches such a tool. An entry containing `=` or a NUL byte is not an environment variable name and is refused. A tool without the block inherits the whole environment as before | | `OPENSYSML_TOOL_KEEP` | unset | `1` keeps the per-invocation directory a tool's `{inputFile}` and `{outputDir}` name, for inspection; otherwise it is removed once the reply is read | | `OPENSYSML_GRPC_INDEX_POOL` | `4` | Whether `sysml-grpc` builds the one shared standard library index ahead of the requests needing it; any positive value prewarms, `0` builds it on the first request instead | | `OPENSYSML_GRPC_MAX_HELD_OBJECTS` | `10000` | The most objects `sysml-grpc` keeps for one cached model, nested objects counted: `Instantiate` creates them and document queries bind and enumerate them for as long as the model stays cached. An `Instantiate`, query or render whose objects would pass the bound fails whole with `RESOURCE_EXHAUSTED`, leaving none of them, until the model leaves the cache, which releases them together; nothing is evicted behind an id a client holds. Read at startup | diff --git a/internal/exec/analysis/invocation.go b/internal/exec/analysis/invocation.go index e0e7f9fcd..465817b6e 100644 --- a/internal/exec/analysis/invocation.go +++ b/internal/exec/analysis/invocation.go @@ -527,6 +527,10 @@ func (inv *Invocation) renderEnv(sc scope) ([]string, error) { names := append([]string(nil), baseToolEnv...) for _, name := range strings.Split(os.Getenv(ToolEnvPassthroughEnv), ",") { if name = strings.TrimSpace(name); name != "" { + if strings.ContainsAny(name, "=\x00") { + return nil, &runtime.ToolError{Tool: sc.tool, Kind: runtime.ToolProcessFailed, + Detail: fmt.Sprintf("%s lists %q, which is not an environment variable name", ToolEnvPassthroughEnv, name)} + } names = append(names, name) } } diff --git a/internal/exec/analysis/invocation_test.go b/internal/exec/analysis/invocation_test.go index 9c1223cf0..520f08f37 100644 --- a/internal/exec/analysis/invocation_test.go +++ b/internal/exec/analysis/invocation_test.go @@ -372,6 +372,29 @@ func TestInvocationEnvIsMinimal(t *testing.T) { } } +// A name OPENSYSML_TOOL_ENV_PASSTHROUGH lists that carries `=` or a NUL byte is not an +// environment variable name: the refusal names the variable and the entry. Whitespace and +// empty entries around the names are still tolerated. +func TestInvocationEnvPassthroughRefusesMalformedNames(t *testing.T) { + t.Setenv("OPENSYSML_TEST_SHOWN", "shown") + inv := checked(t, &Invocation{}, "label") + sc := scope{tool: "Solver", inputs: map[string]runtime.ToolValue{"label": {Text: "r1"}}} + + t.Setenv(ToolEnvPassthroughEnv, " OPENSYSML_TEST_SHOWN ,") + if _, err := inv.renderEnv(sc); err != nil { + t.Fatalf("a valid name: %v", err) + } + for _, bad := range []string{"BAD=VALUE", "BAD = VALUE"} { + t.Setenv(ToolEnvPassthroughEnv, " OPENSYSML_TEST_SHOWN , "+bad+" ,") + _, err := inv.renderEnv(sc) + want := fmt.Sprintf("%s lists %q, which is not an environment variable name", ToolEnvPassthroughEnv, strings.TrimSpace(bad)) + var fault *runtime.ToolError + if !errors.As(err, &fault) || fault.Tool != "Solver" || fault.Kind != runtime.ToolProcessFailed || fault.Detail != want { + t.Errorf("renderEnv: %v, want a ToolError{Tool: Solver, Kind: ToolProcessFailed, Detail: %q}", err, want) + } + } +} + func sortedPairs(env []string) bool { for i := 1; i < len(env); i++ { if env[i-1] > env[i] {