fix(analysis): refuse malformed OPENSYSML_TOOL_ENV_PASSTHROUGH names

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.

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 <name>` 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 |

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)
 		}
 	}

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] {